afr: check validity of afr_reply

...in various self-heal code paths.

Originally found by Pranith in __afr_selfheal_name_impunge ()

Also change __afr_selfheal_assign_gfid() to send lookup only on those
bricks that don't have a gfid matching that of the source.

Change-Id: I70a2ccd750a2af92c5fc36e0eefb2b6125404b4a
BUG: 1482923
Signed-off-by: Pranith Kumar K <pkarampu@redhat.com>
Signed-off-by: Ravishankar N <ravishankar@redhat.com>
Reviewed-on: https://review.gluster.org/18065
Smoke: Gluster Build System <jenkins@build.gluster.org>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>

5 files changed, 71 insertions, 47 deletions

diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index a255f9d14ad..c6a110b2288 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -1699,6 +1699,19 @@ afr_local_transaction_cleanup (afr_local_t *local, xlator_t *this)
 
 }
 
+void
+afr_reply_wipe (struct afr_reply *reply)
+{
+        if (reply->xdata) {
+                dict_unref (reply->xdata);
+                reply->xdata = NULL;
+        }
+
+        if (reply->xattr) {
+                dict_unref (reply->xattr);
+                reply->xattr = NULL;
+        }
+}
 
 void
 afr_replies_wipe (struct afr_reply *replies, int count)
@@ -1706,15 +1719,7 @@ afr_replies_wipe (struct afr_reply *replies, int count)
         int i = 0;
 
         for (i = 0; i < count; i++) {
-                if (replies[i].xdata) {
-                        dict_unref (replies[i].xdata);
-                        replies[i].xdata = NULL;
-                }
-
-                if (replies[i].xattr) {
-                        dict_unref (replies[i].xattr);
-                        replies[i].xattr = NULL;
-                }
+                afr_reply_wipe (&replies[i]);
         }
 }
 
diff --git a/xlators/cluster/afr/src/afr-self-heal-common.c b/xlators/cluster/afr/src/afr-self-heal-common.c
index 9ecd63ce10c..998289711df 100644
--- a/xlators/cluster/afr/src/afr-self-heal-common.c
+++ b/xlators/cluster/afr/src/afr-self-heal-common.c
@@ -549,39 +549,43 @@ afr_selfheal_undo_pending (call_frame_t *frame, xlator_t *this, inode_t *inode,
 	return 0;
 }
 
+void
+afr_reply_copy (struct afr_reply *dst, struct afr_reply *src)
+{
+	dict_t *xdata = NULL;
+
+        dst->valid = src->valid;
+	dst->op_ret = src->op_ret;
+	dst->op_errno = src->op_errno;
+	dst->prestat = src->prestat;
+	dst->poststat = src->poststat;
+	dst->preparent = src->preparent;
+	dst->postparent = src->postparent;
+	dst->preparent2 = src->preparent2;
+	dst->postparent2 = src->postparent2;
+	if (src->xdata)
+		xdata = dict_ref (src->xdata);
+	else
+		xdata = NULL;
+	if (dst->xdata)
+		dict_unref (dst->xdata);
+	dst->xdata = xdata;
+	memcpy (dst->checksum, src->checksum, MD5_DIGEST_LENGTH);
+}
 
 void
 afr_replies_copy (struct afr_reply *dst, struct afr_reply *src, int count)
 {
 	int i = 0;
-	dict_t *xdata = NULL;
 
 	if (dst == src)
 		return;
 
 	for (i = 0; i < count; i++) {
-		dst[i].valid = src[i].valid;
-		dst[i].op_ret = src[i].op_ret;
-		dst[i].op_errno = src[i].op_errno;
-		dst[i].prestat = src[i].prestat;
-		dst[i].poststat = src[i].poststat;
-		dst[i].preparent = src[i].preparent;
-		dst[i].postparent = src[i].postparent;
-		dst[i].preparent2 = src[i].preparent2;
-		dst[i].postparent2 = src[i].postparent2;
-		if (src[i].xdata)
-			xdata = dict_ref (src[i].xdata);
-		else
-			xdata = NULL;
-		if (dst[i].xdata)
-			dict_unref (dst[i].xdata);
-		dst[i].xdata = xdata;
-		memcpy (dst[i].checksum, src[i].checksum,
-			MD5_DIGEST_LENGTH);
+                afr_reply_copy (&dst[i], &src[i]);
 	}
 }
 
-
 int
 afr_selfheal_fill_dirty (xlator_t *this, int *dirty, int subvol,
 			 int idx, dict_t *xdata)
@@ -650,6 +654,9 @@ afr_selfheal_extract_xattr (xlator_t *this, struct afr_reply *replies,
 	priv = this->private;
 
 	for (i = 0; i < priv->child_count; i++) {
+                if (!replies[i].valid || replies[i].op_ret != 0)
+                        continue;
+
 		if (!replies[i].xdata)
 			continue;
 
diff --git a/xlators/cluster/afr/src/afr-self-heal-name.c b/xlators/cluster/afr/src/afr-self-heal-name.c
index 1d198a8883e..2aad4c75bee 100644
--- a/xlators/cluster/afr/src/afr-self-heal-name.c
+++ b/xlators/cluster/afr/src/afr-self-heal-name.c
@@ -24,13 +24,16 @@ __afr_selfheal_assign_gfid (xlator_t *this, inode_t *parent, uuid_t pargfid,
 	int             ret          = 0;
         int             up_count     = 0;
         int             locked_count = 0;
+        int             i            = 0;
 	afr_private_t  *priv         = NULL;
 	dict_t         *xdata        = NULL;
 	loc_t           loc          = {0, };
         call_frame_t   *new_frame    = NULL;
         afr_local_t    *new_local    = NULL;
+        unsigned char  *wind_on      = NULL;
 
 	priv = this->private;
+        wind_on = alloca0 (priv->child_count);
 
         new_frame = afr_frame_create (this);
         if (!new_frame) {
@@ -76,20 +79,26 @@ __afr_selfheal_assign_gfid (xlator_t *this, inode_t *parent, uuid_t pargfid,
                 }
         }
 
-        /* Clear out old replies here and wind lookup on all locked
-         * subvolumes to achieve two things:
-         *   a. gfid heal on those subvolumes that do not have gfid associated
-         *      with the inode, and
-         *   b. refresh replies, which can be consumed by
-         *      __afr_selfheal_name_impunge().
+        /* gfid heal on those subvolumes that do not have gfid associated
+         * with the inode and update those replies.
          */
+        for (i = 0; i < priv->child_count; i++) {
+                if (replies[i].valid && replies[i].op_ret == 0 &&
+                    !gf_uuid_is_null (replies[i].poststat.ia_gfid) &&
+                    !gf_uuid_compare (replies[i].poststat.ia_gfid, gfid))
+                        continue;
+                wind_on[i] = 1;
+        }
 
-        AFR_ONLIST (locked_on, new_frame, afr_selfheal_discover_cbk, lookup,
+        AFR_ONLIST (wind_on, new_frame, afr_selfheal_discover_cbk, lookup,
                     &loc, xdata);
 
-        afr_replies_wipe (replies, priv->child_count);
-
-        afr_replies_copy (replies, new_local->replies, priv->child_count);
+        for (i = 0; i < priv->child_count; i++) {
+                if (!wind_on[i])
+                        continue;
+                afr_reply_wipe (&replies[i]);
+                afr_reply_copy (&replies[i], &new_local->replies[i]);
+        }
 
 out:
 	loc_wipe (&loc);
@@ -119,7 +128,7 @@ __afr_selfheal_name_impunge (call_frame_t *frame, xlator_t *this,
 	gf_uuid_copy (parent->gfid, pargfid);
 
 	for (i = 0; i < priv->child_count; i++) {
-		if (!replies[i].valid)
+                if (!replies[i].valid || replies[i].op_ret != 0)
 			continue;
 
 		if (gf_uuid_compare (replies[i].poststat.ia_gfid,
@@ -213,7 +222,7 @@ afr_selfheal_gfid_idx_get (xlator_t *this, struct afr_reply *replies,
         priv = this->private;
 
         for (i = 0; i < priv->child_count; i++) {
-                if (!replies[i].valid)
+                if (!replies[i].valid || replies[i].op_ret != 0)
                         continue;
 
                 if (!sources[i])
@@ -281,7 +290,7 @@ afr_selfheal_name_type_mismatch_check (xlator_t *this, struct afr_reply *replies
         priv = this->private;
 
         for (i = 0; i < priv->child_count; i++) {
-                if (!replies[i].valid)
+                if (!replies[i].valid || replies[i].op_ret != 0)
                         continue;
 
                 if (replies[i].poststat.ia_type == IA_INVAL)
@@ -343,8 +352,8 @@ afr_selfheal_name_gfid_mismatch_check (xlator_t *this, struct afr_reply *replies
         priv = this->private;
 
 	for (i = 0; i < priv->child_count; i++) {
-		if (!replies[i].valid)
-			continue;
+                if (!replies[i].valid || replies[i].op_ret != 0)
+                        continue;
 
 		if (gf_uuid_is_null (replies[i].poststat.ia_gfid))
 			continue;
@@ -496,7 +505,6 @@ int
 __afr_selfheal_name_finalize_source (xlator_t *this, unsigned char *sources,
 				     unsigned char *healed_sinks,
 				     unsigned char *locked_on,
-				     struct afr_reply *replies,
                                      uint64_t *witness)
 {
 	int i = 0;
@@ -565,8 +573,7 @@ __afr_selfheal_name_prepare (call_frame_t *frame, xlator_t *this, inode_t *paren
 
 	source = __afr_selfheal_name_finalize_source (this, sources,
                                                       healed_sinks,
-						      locked_on, replies,
-                                                      witness);
+						      locked_on, witness);
 	if (source < 0) {
 		/* If source is < 0 (typically split-brain), we perform a
 		   conservative merge of entries rather than erroring out */
diff --git a/xlators/cluster/afr/src/afr-self-heal.h b/xlators/cluster/afr/src/afr-self-heal.h
index 82608d261d5..15b47f66b4c 100644
--- a/xlators/cluster/afr/src/afr-self-heal.h
+++ b/xlators/cluster/afr/src/afr-self-heal.h
@@ -215,6 +215,8 @@ afr_selfheal_discover_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
 			   int op_ret, int op_errno, inode_t *inode,
 			   struct iatt *buf, dict_t *xdata,
                            struct iatt *parbuf);
+void
+afr_reply_copy (struct afr_reply *dst, struct afr_reply *src);
 
 void
 afr_replies_copy (struct afr_reply *dst, struct afr_reply *src, int count);
diff --git a/xlators/cluster/afr/src/afr.h b/xlators/cluster/afr/src/afr.h
index cf736ed3d2e..c4ceb66914f 100644
--- a/xlators/cluster/afr/src/afr.h
+++ b/xlators/cluster/afr/src/afr.h
@@ -1159,6 +1159,9 @@ void
 afr_remove_eager_lock_stub (afr_local_t *local);
 
 void
+afr_reply_wipe (struct afr_reply *reply);
+
+void
 afr_replies_wipe (struct afr_reply *replies, int count);
 
 gf_boolean_t