From 3b70b160a46b22b77a8ad1897440ec1346795a0f Mon Sep 17 00:00:00 2001 From: Pranith Kumar K Date: Wed, 13 Aug 2014 11:11:17 +0530 Subject: 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 Reviewed-on: http://review.gluster.org/8512 Tested-by: Gluster Build System Reviewed-by: Krutika Dhananjay --- xlators/cluster/afr/src/afr-common.c | 21 +++++++-- xlators/cluster/afr/src/afr-self-heal-name.c | 66 +++++++++++++++++++--------- xlators/cluster/afr/src/afr-self-heal.h | 3 +- xlators/cluster/afr/src/afr-self-heald.c | 2 +- xlators/cluster/afr/src/afr.h | 1 + xlators/cluster/afr/src/pump.c | 3 +- 6 files changed, 69 insertions(+), 27 deletions(-) (limited to 'xlators/cluster') 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)", -- cgit