From c1fc1fc9cb5a13e6ddf8c9270deb0c7609333540 Mon Sep 17 00:00:00 2001 From: Pranith Kumar K Date: Wed, 25 Jan 2017 15:31:44 +0530 Subject: cluster/ec: Don't trigger data/metadata heal on Lookups Problem-1 If Lookup which doesn't take any locks observes version mismatch it can't be trusted. If we launch a heal based on this information it will lead to self-heals which will affect I/O performance in the cases where Lookup is wrong. Considering self-heal-daemon and operations on the inode from client which take locks can still trigger heal we can choose to not attempt a heal on Lookup. Problem-2: Fixed spurious failure of tests/bitrot/bug-1373520.t For the issues above, what was happening was that ec_heal_inspect() is preventing 'name' heal to happen Problem-3: tests/basic/ec/ec-background-heals.t To be honest I don't know what the problem was, while fixing the 2 problems above, I made some changes to ec_heal_inspect() and ec_need_heal() after which when I tried to recreate the spurious failure it just didn't happen even after a long time. BUG: 1414287 Signed-off-by: Pranith Kumar K Change-Id: Ife2535e1d0b267712973673f6d474e288f3c6834 Reviewed-on: https://review.gluster.org/16468 Smoke: Gluster Build System NetBSD-regression: NetBSD Build System Reviewed-by: Xavier Hernandez CentOS-regression: Gluster Build System Reviewed-by: Ashish Pandey --- xlators/cluster/ec/src/ec-heal.c | 321 +++++++++++++++++++++++++++++---------- 1 file changed, 239 insertions(+), 82 deletions(-) (limited to 'xlators/cluster/ec/src/ec-heal.c') diff --git a/xlators/cluster/ec/src/ec-heal.c b/xlators/cluster/ec/src/ec-heal.c index 06a4dbb5841..19ba5787006 100644 --- a/xlators/cluster/ec/src/ec-heal.c +++ b/xlators/cluster/ec/src/ec-heal.c @@ -345,7 +345,7 @@ ec_heal_entry_find_direction (ec_t *ec, default_args_cbk_t *replies, if (source == -1) source = i; - ret = ec_dict_del_array (replies[i].xdata, EC_XATTR_VERSION, + ret = ec_dict_get_array (replies[i].xdata, EC_XATTR_VERSION, xattr, EC_VERSION_SIZE); if (ret == 0) { versions[i] = xattr[EC_DATA_TXN]; @@ -356,7 +356,7 @@ ec_heal_entry_find_direction (ec_t *ec, default_args_cbk_t *replies, } memset (xattr, 0, sizeof(xattr)); - ret = ec_dict_del_array (replies[i].xdata, EC_XATTR_DIRTY, + ret = ec_dict_get_array (replies[i].xdata, EC_XATTR_DIRTY, xattr, EC_VERSION_SIZE); if (ret == 0) { dirty[i] = xattr[EC_DATA_TXN]; @@ -453,6 +453,7 @@ out: loc_wipe (&loc); return op_ret; } + int ec_heal_metadata_find_direction (ec_t *ec, default_args_cbk_t *replies, uint64_t *versions, uint64_t *dirty, @@ -479,14 +480,14 @@ ec_heal_metadata_find_direction (ec_t *ec, default_args_cbk_t *replies, continue; if (replies[i].op_ret < 0) continue; - ret = ec_dict_del_array (replies[i].xdata, EC_XATTR_VERSION, + ret = ec_dict_get_array (replies[i].xdata, EC_XATTR_VERSION, xattr, EC_VERSION_SIZE); if (ret == 0) { versions[i] = xattr[EC_METADATA_TXN]; } memset (xattr, 0, sizeof (xattr)); - ret = ec_dict_del_array (replies[i].xdata, EC_XATTR_DIRTY, + ret = ec_dict_get_array (replies[i].xdata, EC_XATTR_DIRTY, xattr, EC_VERSION_SIZE); if (ret == 0) { dirty[i] = xattr[EC_METADATA_TXN]; @@ -1496,26 +1497,22 @@ unlock: /*Find direction for data heal and heal info*/ int ec_heal_data_find_direction (ec_t *ec, default_args_cbk_t *replies, - uint64_t *data_versions, uint64_t *meta_versions, + uint64_t *data_versions, uint64_t *dirty, uint64_t *size, unsigned char *sources, - unsigned char *healed_sinks, int which) + unsigned char *healed_sinks, + gf_boolean_t check_ondisksize, int which) { uint64_t xattr[EC_VERSION_SIZE] = {0}; char version_size[128] = {0}; dict_t *version_size_db = NULL; - uint64_t *m_versions = NULL; unsigned char *same = NULL; int max_same_count = 0; int source = 0; int i = 0; int ret = 0; dict_t *dict = NULL; + uint64_t source_size = 0; - if (!meta_versions) { - m_versions = alloca0 (ec->nodes * sizeof (*m_versions)); - } else { - m_versions = meta_versions; - } version_size_db = dict_new (); if (!version_size_db) { ret = -ENOMEM; @@ -1530,17 +1527,14 @@ ec_heal_data_find_direction (ec_t *ec, default_args_cbk_t *replies, dict = (which == EC_COMBINE_XDATA) ? replies[i].xdata : replies[i].xattr; - ret = ec_dict_del_array (dict, EC_XATTR_VERSION, + ret = ec_dict_get_array (dict, EC_XATTR_VERSION, xattr, EC_VERSION_SIZE); if (ret == 0) { data_versions[i] = xattr[EC_DATA_TXN]; - if (meta_versions) { - m_versions[i] = xattr[EC_METADATA_TXN]; - } } memset (xattr, 0, sizeof (xattr)); - ret = ec_dict_del_array (dict, EC_XATTR_DIRTY, + ret = ec_dict_get_array (dict, EC_XATTR_DIRTY, xattr, EC_VERSION_SIZE); if (ret == 0) { dirty[i] = xattr[EC_DATA_TXN]; @@ -1549,8 +1543,7 @@ ec_heal_data_find_direction (ec_t *ec, default_args_cbk_t *replies, &size[i]); /*Build a db of same metadata and data version and size*/ snprintf (version_size, sizeof (version_size), - "%"PRIu64"-%"PRIu64"-%"PRIu64, data_versions[i], - m_versions[i], size[i]); + "%"PRIu64"-%"PRIu64, data_versions[i], size[i]); ret = dict_get_bin (version_size_db, version_size, (void **)&same); @@ -1581,9 +1574,7 @@ ec_heal_data_find_direction (ec_t *ec, default_args_cbk_t *replies, goto out; } else { snprintf (version_size, sizeof (version_size), - "%"PRIu64"-%"PRIu64"-%"PRIu64, - data_versions[source], - m_versions[source], + "%"PRIu64"-%"PRIu64, data_versions[source], size[source]); ret = dict_get_bin (version_size_db, version_size, @@ -1598,6 +1589,30 @@ ec_heal_data_find_direction (ec_t *ec, default_args_cbk_t *replies, } } + /* There could be files with versions, size same but on disk ia_size + * could be different because of disk crashes, mark them as sinks as + * well*/ + + if (check_ondisksize) { + source_size = ec_adjust_size (ec, size[source], 1); + + for (i = 0; i < ec->nodes; i++) { + if (sources[i]) { + if (replies[i].stat.ia_size != source_size) { + sources[i] = 0; + healed_sinks[i] = 1; + max_same_count--; + } else { + source = i; + } + } + } + if (max_same_count < ec->fragments) { + ret = -EIO; + goto out; + } + } + ret = source; out: if (version_size_db) @@ -1613,17 +1628,20 @@ __ec_heal_data_prepare (call_frame_t *frame, ec_t *ec, fd_t *fd, struct iatt *stbuf) { default_args_cbk_t *replies = NULL; + default_args_cbk_t *fstat_replies = NULL; unsigned char *output = NULL; + unsigned char *fstat_output = NULL; dict_t *xattrs = NULL; uint64_t zero_array[2] = {0}; int source = 0; int ret = 0; uint64_t zero_value = 0; - uint64_t source_size = 0; int i = 0; EC_REPLIES_ALLOC (replies, ec->nodes); + EC_REPLIES_ALLOC (fstat_replies, ec->nodes); output = alloca0(ec->nodes); + fstat_output = alloca0(ec->nodes); xattrs = dict_new (); if (!xattrs || dict_set_static_bin (xattrs, EC_XATTR_VERSION, zero_array, @@ -1639,43 +1657,34 @@ __ec_heal_data_prepare (call_frame_t *frame, ec_t *ec, fd_t *fd, ret = cluster_fxattrop (ec->xl_list, locked_on, ec->nodes, replies, output, frame, ec->xl, fd, GF_XATTROP_ADD_ARRAY64, xattrs, NULL); + + ret = cluster_fstat (ec->xl_list, locked_on, ec->nodes, fstat_replies, + fstat_output, frame, ec->xl, fd, NULL); + + for (i = 0; i < ec->nodes; i++) { + output[i] = output[i] && fstat_output[i]; + replies[i].valid = output[i]; + if (output[i]) + replies[i].stat = fstat_replies[i].stat; + } + if (EC_COUNT (output, ec->nodes) <= ec->fragments) { ret = -ENOTCONN; goto out; } - source = ec_heal_data_find_direction (ec, replies, versions, NULL, + source = ec_heal_data_find_direction (ec, replies, versions, dirty, size, sources, - healed_sinks, EC_COMBINE_DICT); + healed_sinks, _gf_true, + EC_COMBINE_DICT); ret = source; if (ret < 0) goto out; - /* There could be files with versions, size same but on disk ia_size - * could be different because of disk crashes, mark them as sinks as - * well*/ - ret = cluster_fstat (ec->xl_list, locked_on, ec->nodes, replies, - output, frame, ec->xl, fd, NULL); - EC_INTERSECT (sources, sources, output, ec->nodes); - EC_INTERSECT (healed_sinks, healed_sinks, output, ec->nodes); - if (EC_COUNT (sources, ec->nodes) < ec->fragments) { - ret = -ENOTCONN; - goto out; - } - - source_size = ec_adjust_size (ec, size[source], 1); + if (stbuf) + *stbuf = replies[source].stat; for (i = 0; i < ec->nodes; i++) { - if (sources[i]) { - if (replies[i].stat.ia_size != source_size) { - sources[i] = 0; - healed_sinks[i] = 1; - } else if (stbuf) { - source = i; - *stbuf = replies[i].stat; - } - } - if (healed_sinks[i]) { if (replies[i].stat.ia_size) trim[i] = 1; @@ -1692,6 +1701,7 @@ out: if (xattrs) dict_unref (xattrs); cluster_replies_wipe (replies, ec->nodes); + cluster_replies_wipe (fstat_replies, ec->nodes); if (ret < 0) { gf_msg_debug (ec->xl->name, 0, "%s: heal failed %s", uuid_utoa (fd->inode->gfid), strerror (-ret)); @@ -2346,7 +2356,7 @@ ec_heal_do (xlator_t *this, void *data, loc_t *loc, int32_t partial) frame = create_frame (this, this->ctx->pool); if (!frame) - return; + goto out; ec_owner_set(frame, frame->root); /*Do heal as root*/ @@ -2360,15 +2370,6 @@ ec_heal_do (xlator_t *this, void *data, loc_t *loc, int32_t partial) up_subvols = alloca0(ec->nodes); ec_mask_to_char_array (ec->xl_up, up_subvols, ec->nodes); - ec_heal_inspect (frame, ec, loc->inode, up_subvols, - &need_heal); - if (!need_heal) { - gf_msg (ec->xl->name, GF_LOG_DEBUG, 0, - EC_MSG_HEAL_FAIL, "Heal is not required for : %s ", - uuid_utoa(loc->gfid)); - goto out; - } - if (loc->name && strlen (loc->name)) { ret = ec_heal_name (frame, ec, loc->parent, (char *)loc->name, participants); @@ -2386,6 +2387,17 @@ ec_heal_do (xlator_t *this, void *data, loc_t *loc, int32_t partial) } } + /* Mount triggers heal only when it detects that it must need heal, shd + * triggers heals periodically which need not be thorough*/ + ec_heal_inspect (frame, ec, loc->inode, up_subvols, _gf_false, + !ec->shd.iamshd, &need_heal); + if (!need_heal) { + gf_msg (ec->xl->name, GF_LOG_DEBUG, 0, + EC_MSG_HEAL_FAIL, "Heal is not required for : %s ", + uuid_utoa(loc->gfid)); + goto out; + } + msources = alloca0(ec->nodes); mhealed_sinks = alloca0(ec->nodes); ret = ec_heal_metadata (frame, ec, loc->inode, msources, mhealed_sinks); @@ -2425,7 +2437,8 @@ out: ec->nodes), mgood & good, mbad & bad, NULL); } - STACK_DESTROY (frame->root); + if (frame) + STACK_DESTROY (frame->root); return; } @@ -2681,40 +2694,170 @@ out: return ret; } -int32_t -ec_need_heal (ec_t *ec, default_args_cbk_t *replies, - gf_boolean_t *need_heal, int32_t lock_count) +static int32_t +_need_heal_calculate (ec_t *ec, uint64_t *dirty, unsigned char *sources, + gf_boolean_t self_locked, int32_t lock_count, + gf_boolean_t *need_heal) +{ + int i = 0; + int source_count = 0; + + source_count = EC_COUNT (sources, ec->nodes); + if (source_count == ec->nodes) { + *need_heal = _gf_false; + if (self_locked || lock_count == 0) { + for (i = 0; i < ec->nodes; i++) { + if (dirty[i]) { + *need_heal = _gf_true; + goto out; + } + } + } else { + for (i = 0; i < ec->nodes; i++) { + /* Since each lock can only increment the dirty + * count once, if dirty is > 1 it means that + * another operation has left the dirty count + * set and this indicates a problem in the + * inode.*/ + if (dirty[i] > 1) { + *need_heal = _gf_true; + goto out; + } + } + } + } else { + *need_heal = _gf_true; + } + +out: + return source_count; +} + +static int32_t +ec_need_metadata_heal (ec_t *ec, inode_t *inode, default_args_cbk_t *replies, + int32_t lock_count, gf_boolean_t self_locked, + gf_boolean_t thorough, gf_boolean_t *need_heal) { uint64_t *dirty = NULL; unsigned char *sources = NULL; unsigned char *healed_sinks = NULL; - uint64_t *data_versions = NULL; uint64_t *meta_versions = NULL; + int ret = 0; + int i = 0; + + sources = alloca0(ec->nodes); + healed_sinks = alloca0(ec->nodes); + dirty = alloca0 (ec->nodes * sizeof (*dirty)); + meta_versions = alloca0 (ec->nodes * sizeof (*meta_versions)); + ret = ec_heal_metadata_find_direction (ec, replies, meta_versions, + dirty, sources, healed_sinks); + if (ret < 0 && ret != -EIO) { + goto out; + } + + ret = _need_heal_calculate (ec, dirty, sources, self_locked, lock_count, + need_heal); + if (ret == ec->nodes && !(*need_heal)) { + for (i = 1; i < ec->nodes; i++) { + if (meta_versions[i] != meta_versions[0]) { + *need_heal = _gf_true; + goto out; + } + } + } +out: + return ret; +} + +static int32_t +ec_need_data_heal (ec_t *ec, inode_t *inode, default_args_cbk_t *replies, + int32_t lock_count, gf_boolean_t self_locked, + gf_boolean_t thorough, gf_boolean_t *need_heal) +{ + uint64_t *dirty = NULL; + unsigned char *sources = NULL; + unsigned char *healed_sinks = NULL; + uint64_t *data_versions = NULL; uint64_t *size = NULL; int ret = 0; - int source_count = 0; sources = alloca0(ec->nodes); healed_sinks = alloca0(ec->nodes); dirty = alloca0 (ec->nodes * sizeof (*dirty)); - size = alloca0 (ec->nodes * sizeof (*size)); data_versions = alloca0 (ec->nodes * sizeof (*data_versions)); - meta_versions = alloca0 (ec->nodes * sizeof (*meta_versions)); + size = alloca0 (ec->nodes * sizeof (*size)); + /* When dd is going on and heal info is called there is a very good + * chance for on disk sizes to mismatch eventhough nothing is wrong + * we don't need ondisk size check there. But if the file is either + * self-locked or the caller wants a thorough check then make sure to + * perform on disk check also. */ ret = ec_heal_data_find_direction (ec, replies, data_versions, - meta_versions, dirty, size, - sources, healed_sinks, + dirty, size, sources, healed_sinks, + self_locked || thorough, EC_COMBINE_XDATA); if (ret < 0 && ret != -EIO) { goto out; } - source_count = EC_COUNT (sources, ec->nodes); - if (source_count == ec->nodes && lock_count > 0) { - *need_heal = _gf_false; - } else { - *need_heal = _gf_true; + + ret = _need_heal_calculate (ec, dirty, sources, self_locked, lock_count, + need_heal); +out: + return ret; +} + +static int32_t +ec_need_entry_heal (ec_t *ec, inode_t *inode, default_args_cbk_t *replies, + int32_t lock_count, gf_boolean_t self_locked, + gf_boolean_t thorough, gf_boolean_t *need_heal) +{ + uint64_t *dirty = NULL; + unsigned char *sources = NULL; + unsigned char *healed_sinks = NULL; + uint64_t *data_versions = NULL; + int ret = 0; + + sources = alloca0(ec->nodes); + healed_sinks = alloca0(ec->nodes); + dirty = alloca0 (ec->nodes * sizeof (*dirty)); + data_versions = alloca0 (ec->nodes * sizeof (*data_versions)); + + ret = ec_heal_entry_find_direction (ec, replies, data_versions, + dirty, sources, healed_sinks); + if (ret < 0 && ret != -EIO) { + goto out; + } + + ret = _need_heal_calculate (ec, dirty, sources, self_locked, lock_count, + need_heal); +out: + return ret; +} + +static int32_t +ec_need_heal (ec_t *ec, inode_t *inode, default_args_cbk_t *replies, + int32_t lock_count, gf_boolean_t self_locked, + gf_boolean_t thorough, gf_boolean_t *need_heal) +{ + int ret = 0; + + + ret = ec_need_metadata_heal (ec, inode, replies, lock_count, + self_locked, thorough, need_heal); + if (ret < 0) + goto out; + + if (*need_heal) + goto out; + + if (inode->ia_type == IA_IFREG) { + ret = ec_need_data_heal (ec, inode, replies, lock_count, + self_locked, thorough, need_heal); + } else if (inode->ia_type == IA_IFDIR) { + ret = ec_need_entry_heal (ec, inode, replies, lock_count, + self_locked, thorough, need_heal); } - ret = source_count; + out: return ret; } @@ -2722,6 +2865,7 @@ out: int32_t ec_heal_inspect (call_frame_t *frame, ec_t *ec, inode_t *inode, unsigned char *locked_on, + gf_boolean_t self_locked, gf_boolean_t thorough, gf_boolean_t *need_heal) { loc_t loc = {0}; @@ -2742,8 +2886,6 @@ ec_heal_inspect (call_frame_t *frame, ec_t *ec, xdata = dict_new (); if (!xdata || - dict_set_str(xdata, GLUSTERFS_INODELK_DOM_COUNT, - ec->xl->name) || dict_set_static_bin (xdata, EC_XATTR_VERSION, zero_array, sizeof (zero_array)) || dict_set_static_bin (xdata, EC_XATTR_DIRTY, zero_array, @@ -2753,6 +2895,16 @@ ec_heal_inspect (call_frame_t *frame, ec_t *ec, ret = -ENOMEM; goto out; } + + if (!self_locked) { + ret = dict_set_str(xdata, GLUSTERFS_INODELK_DOM_COUNT, + ec->xl->name); + if (ret) { + ret = -ENOMEM; + goto out; + } + } + ret = cluster_lookup (ec->xl_list, locked_on, ec->nodes, replies, output, frame, ec->xl, &loc, xdata); @@ -2762,6 +2914,9 @@ ec_heal_inspect (call_frame_t *frame, ec_t *ec, goto out; } + if (self_locked) + goto need_heal; + for (i = 0; i < ec->nodes; i++) { if (!output[i] || !replies[i].xdata) { continue; @@ -2771,7 +2926,9 @@ ec_heal_inspect (call_frame_t *frame, ec_t *ec, break; } } - ret = ec_need_heal (ec, replies, need_heal, lock_count); +need_heal: + ret = ec_need_heal (ec, inode, replies, lock_count, + self_locked, thorough, need_heal); out: cluster_replies_wipe (replies, ec->nodes); @@ -2805,8 +2962,8 @@ ec_heal_locked_inspect (call_frame_t *frame, ec_t *ec, inode_t *inode, *need_heal = _gf_true; goto unlock; } - ret = ec_heal_inspect (frame, ec, inode, - locked_on, need_heal); + ret = ec_heal_inspect (frame, ec, inode, locked_on, _gf_true, _gf_true, + need_heal); unlock: cluster_uninodelk (ec->xl_list, locked_on, ec->nodes, replies, output, frame, ec->xl, @@ -2854,9 +3011,9 @@ ec_get_heal_info (xlator_t *this, loc_t *entry_loc, dict_t **dict_rsp) goto out; } - ret = ec_heal_inspect (frame, ec, loc.inode, up_subvols, - &need_heal); - if (ret == ec->nodes) { + ret = ec_heal_inspect (frame, ec, loc.inode, up_subvols, _gf_false, + _gf_false, &need_heal); + if (ret == ec->nodes && !need_heal) { goto set_heal; } need_heal = _gf_false; -- cgit