cluster/afr: Perform gfid heal inside locks.

Problem:
Allowing lookup with 'gfid-req' will lead to assigning gfid at posix layer.
When two mounts perform lookup in parallel that can lead to both bricks getting
different gfids leading to gfid-mismatch/EIO for the lookup.

Fix:
Perform gfid heal inside lock.

BUG: 1129529
Change-Id: I20c6c5e25ee27eeb906bff2f4c8ad0da18d00090
Signed-off-by: Pranith Kumar K <pkarampu@redhat.com>
Reviewed-on: http://review.gluster.org/8512
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Krutika Dhananjay <kdhananj@redhat.com>

6 files changed, 69 insertions, 27 deletions

diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index 8fc202512f1..cc7df9a3ea6 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -1443,7 +1443,8 @@ afr_lookup_selfheal_wrap (void *opaque)
 	local = frame->local;
 	this = frame->this;
 
-	afr_selfheal_name (frame->this, local->loc.pargfid, local->loc.name);
+	afr_selfheal_name (frame->this, local->loc.pargfid, local->loc.name,
+                           &local->cont.lookup.gfid_req);
 
 	afr_replies_wipe (local, this->private);
 
@@ -1477,6 +1478,10 @@ afr_lookup_entry_heal (call_frame_t *frame, xlator_t *this)
 		if (!replies[i].valid)
 			continue;
 
+                if ((replies[i].op_ret == -1) &&
+                    (replies[i].op_errno == ENODATA))
+                        need_heal = _gf_true;
+
 		if (first == -1) {
 			first = i;
 			continue;
@@ -1842,8 +1847,12 @@ afr_lookup (call_frame_t *frame, xlator_t *this, loc_t *loc, dict_t *xattr_req)
         afr_local_t   *local = NULL;
         int32_t        op_errno = 0;
 	int            event = 0;
+        void          *gfid_req = NULL;
+        int            ret = 0;
 
-	if (!loc->parent) {
+	if (!loc->parent && uuid_is_null (loc->pargfid)) {
+                if (xattr_req)
+                        dict_del (xattr_req, "gfid-req");
 		afr_discover (frame, this, loc, xattr_req);
 		return 0;
 	}
@@ -1870,10 +1879,16 @@ afr_lookup (call_frame_t *frame, xlator_t *this, loc_t *loc, dict_t *xattr_req)
 
 	local->inode = inode_ref (loc->inode);
 
-	if (xattr_req)
+	if (xattr_req) {
 		/* If xattr_req was null, afr_lookup_xattr_req_prepare() will
 		   allocate one for us */
+                ret = dict_get_ptr (xattr_req, "gfid-req", &gfid_req);
+                if (ret == 0) {
+                        uuid_copy (local->cont.lookup.gfid_req, gfid_req);
+                        dict_del (xattr_req, "gfid-req");
+                }
 		local->xattr_req = dict_ref (xattr_req);
+        }
 
 	afr_read_subvol_get (loc->parent, this, NULL, &event,
 			     AFR_DATA_TRANSACTION);
diff --git a/xlators/cluster/afr/src/afr-self-heal-name.c b/xlators/cluster/afr/src/afr-self-heal-name.c
index e3ff705e315..151c401e3b1 100644
--- a/xlators/cluster/afr/src/afr-self-heal-name.c
+++ b/xlators/cluster/afr/src/afr-self-heal-name.c
@@ -21,7 +21,7 @@
 int
 __afr_selfheal_assign_gfid (call_frame_t *frame, xlator_t *this, inode_t *parent,
 			    uuid_t pargfid, const char *bname, inode_t *inode,
-			    struct afr_reply *replies, int gfid_idx)
+			    struct afr_reply *replies, void *gfid)
 {
 	int i = 0;
 	afr_private_t *priv = NULL;
@@ -38,8 +38,7 @@ __afr_selfheal_assign_gfid (call_frame_t *frame, xlator_t *this, inode_t *parent
 		return -ENOMEM;
 	}
 
-	ret = dict_set_static_bin (xdata, "gfid-req",
-				   replies[gfid_idx].poststat.ia_gfid, 16);
+	ret = dict_set_static_bin (xdata, "gfid-req", gfid, 16);
 	if (ret) {
 		dict_destroy (xdata);
 		return -ENOMEM;
@@ -150,11 +149,12 @@ __afr_selfheal_name_do (call_frame_t *frame, xlator_t *this, inode_t *parent,
 			uuid_t pargfid, const char *bname, inode_t *inode,
 			unsigned char *sources, unsigned char *sinks,
 			unsigned char *healed_sinks, int source,
-			unsigned char *locked_on, struct afr_reply *replies)
+			unsigned char *locked_on, struct afr_reply *replies,
+                        void *gfid_req)
 {
 	int i = 0;
 	afr_private_t *priv = NULL;
-	uuid_t gfid = {0, };
+	void* gfid = NULL;
 	int gfid_idx = -1;
 	gf_boolean_t source_is_empty = _gf_true;
 	gf_boolean_t need_heal = _gf_false;
@@ -167,6 +167,10 @@ __afr_selfheal_name_do (call_frame_t *frame, xlator_t *this, inode_t *parent,
 		if (!replies[i].valid)
 			continue;
 
+                if ((replies[i].op_ret == -1) &&
+                    (replies[i].op_errno == ENODATA))
+                        need_heal = _gf_true;
+
 		if (first_idx == -1) {
 			first_idx = i;
 			continue;
@@ -184,15 +188,19 @@ __afr_selfheal_name_do (call_frame_t *frame, xlator_t *this, inode_t *parent,
 		return 0;
 
 	for (i = 0; i < priv->child_count; i++) {
-		if (!replies[i].valid)
-			continue;
+                if (!sources[i])
+                        continue;
 
-		if (!replies[i].op_ret && (source == -1 || sources[i])) {
-			source_is_empty = _gf_false;
-			break;
-		}
+                if (replies[i].op_ret == -1 && replies[i].op_errno == ENOENT)
+                        continue;
+
+                source_is_empty = _gf_false;
+                break;
 	}
 
+        if (source == -1)
+                source_is_empty = _gf_false;
+
 	if (source_is_empty) {
 		return __afr_selfheal_name_expunge (frame, this, parent, pargfid,
 						    bname, inode, replies);
@@ -205,8 +213,8 @@ __afr_selfheal_name_do (call_frame_t *frame, xlator_t *this, inode_t *parent,
 		if (uuid_is_null (replies[i].poststat.ia_gfid))
 			continue;
 
-		if (uuid_is_null (gfid)) {
-			uuid_copy (gfid, replies[i].poststat.ia_gfid);
+		if (!gfid) {
+			gfid = &replies[i].poststat.ia_gfid;
 			gfid_idx = i;
 			continue;
 		}
@@ -227,17 +235,26 @@ __afr_selfheal_name_do (call_frame_t *frame, xlator_t *this, inode_t *parent,
 				return -1;
 			}
 
-			uuid_copy (gfid, replies[i].poststat.ia_gfid);
+                        gfid = &replies[i].poststat.ia_gfid;
 			gfid_idx = i;
 			continue;
 		}
 	}
 
-	if (gfid_idx == -1)
-		return -1;
+	if (gfid_idx == -1) {
+                if (!gfid_req || uuid_is_null (gfid_req))
+                        return -1;
+                gfid = gfid_req;
+        }
 
 	__afr_selfheal_assign_gfid (frame, this, parent, pargfid, bname, inode,
-				    replies, gfid_idx);
+				    replies, gfid);
+        /*TODO:
+         * once the gfid is assigned refresh the replies and carry on with
+         * impunge. i.e. gfid_idx won't be -1.
+         */
+        if (gfid_idx == -1)
+                return -1;
 
 	return __afr_selfheal_name_impunge (frame, this, parent, pargfid,
 					    bname, inode, replies, gfid_idx);
@@ -326,7 +343,7 @@ __afr_selfheal_name_prepare (call_frame_t *frame, xlator_t *this, inode_t *paren
 
 int
 afr_selfheal_name_do (call_frame_t *frame, xlator_t *this, inode_t *parent,
-		      uuid_t pargfid, const char *bname)
+		      uuid_t pargfid, const char *bname, void *gfid_req)
 {
 	afr_private_t *priv = NULL;
 	unsigned char *sources = NULL;
@@ -371,7 +388,8 @@ afr_selfheal_name_do (call_frame_t *frame, xlator_t *this, inode_t *parent,
 
 		ret = __afr_selfheal_name_do (frame, this, parent, pargfid, bname,
 					      inode, sources, sinks, healed_sinks,
-					      source, locked_on, replies);
+					      source, locked_on, replies,
+                                              gfid_req);
 	}
 unlock:
 	afr_selfheal_unentrylk (frame, this, parent, this->name, bname,
@@ -407,6 +425,10 @@ afr_selfheal_name_unlocked_inspect (call_frame_t *frame, xlator_t *this,
 		if (!replies[i].valid)
 			continue;
 
+                if ((replies[i].op_ret == -1) &&
+                    (replies[i].op_errno == ENODATA))
+                        *need_heal = _gf_true;
+
 		if (first_idx == -1) {
 			first_idx = i;
 			continue;
@@ -426,7 +448,8 @@ afr_selfheal_name_unlocked_inspect (call_frame_t *frame, xlator_t *this,
 }
 
 int
-afr_selfheal_name (xlator_t *this, uuid_t pargfid, const char *bname)
+afr_selfheal_name (xlator_t *this, uuid_t pargfid, const char *bname,
+                   void *gfid_req)
 {
 	inode_t *parent = NULL;
 	call_frame_t *frame = NULL;
@@ -447,7 +470,8 @@ afr_selfheal_name (xlator_t *this, uuid_t pargfid, const char *bname)
 		goto out;
 
 	if (need_heal)
-		afr_selfheal_name_do (frame, this, parent, pargfid, bname);
+		afr_selfheal_name_do (frame, this, parent, pargfid, bname,
+                                      gfid_req);
 out:
 	if (parent)
 		inode_unref (parent);
diff --git a/xlators/cluster/afr/src/afr-self-heal.h b/xlators/cluster/afr/src/afr-self-heal.h
index a1b972ac35d..8556b2bb335 100644
--- a/xlators/cluster/afr/src/afr-self-heal.h
+++ b/xlators/cluster/afr/src/afr-self-heal.h
@@ -85,7 +85,8 @@ int
 afr_selfheal (xlator_t *this, uuid_t gfid);
 
 int
-afr_selfheal_name (xlator_t *this, uuid_t gfid, const char *name);
+afr_selfheal_name (xlator_t *this, uuid_t gfid, const char *name,
+                   void *gfid_req);
 
 int
 afr_selfheal_data (call_frame_t *frame, xlator_t *this, inode_t *inode);
diff --git a/xlators/cluster/afr/src/afr-self-heald.c b/xlators/cluster/afr/src/afr-self-heald.c
index 25ae530913d..9dc785c967c 100644
--- a/xlators/cluster/afr/src/afr-self-heald.c
+++ b/xlators/cluster/afr/src/afr-self-heald.c
@@ -276,7 +276,7 @@ afr_shd_selfheal_name (struct subvol_healer *healer, int child, uuid_t parent,
 {
 	int ret = -1;
 
-	ret = afr_selfheal_name (THIS, parent, bname);
+	ret = afr_selfheal_name (THIS, parent, bname, NULL);
 
 	return ret;
 }
diff --git a/xlators/cluster/afr/src/afr.h b/xlators/cluster/afr/src/afr.h
index b02ecaf3050..91eef2bf1b7 100644
--- a/xlators/cluster/afr/src/afr.h
+++ b/xlators/cluster/afr/src/afr.h
@@ -419,6 +419,7 @@ typedef struct _afr_local {
         struct {
                 struct {
                         gf_boolean_t needs_fresh_lookup;
+                        uuid_t gfid_req;
                 } lookup;
 
                 struct {
diff --git a/xlators/cluster/afr/src/pump.c b/xlators/cluster/afr/src/pump.c
index eed5099563b..c6923fa45ac 100644
--- a/xlators/cluster/afr/src/pump.c
+++ b/xlators/cluster/afr/src/pump.c
@@ -520,7 +520,8 @@ gf_pump_traverse_directory (loc_t *loc)
 				continue;
 			}
 
-			ret = afr_selfheal_name (this, loc->gfid, entry->d_name);
+			ret = afr_selfheal_name (this, loc->gfid, entry->d_name,
+                                                 NULL);
 			if (ret) {
 				gf_log (this->name, GF_LOG_ERROR,
 					"%s: name self-heal failed (%s/%s)",