From c20febcb1ffaef3fa29563987e7a3b554aea27b3 Mon Sep 17 00:00:00 2001 From: Susant Palai Date: Tue, 13 Dec 2016 14:26:53 +0530 Subject: dht/rebalance: reverify lookup failures race: readdirp has read one entry, and doing a lookup on that entry, but user might have renamed/removed that entry just after readdirp but before lookup. Since remove-brick is a costly opertaion,will ingore any ENOENT/ESTALE failures and move on. Change-Id: I62c7fa93c0b9b7e764065ad1574b97acf51b5996 BUG: 1408115 Signed-off-by: Susant Palai Reviewed-on: http://review.gluster.org/15846 Reviewed-by: Raghavendra G Smoke: Gluster Build System CentOS-regression: Gluster Build System NetBSD-regression: NetBSD Build System --- xlators/cluster/dht/src/dht-messages.h | 11 +- xlators/cluster/dht/src/dht-rebalance.c | 177 ++++++++++++++++++++++---------- 2 files changed, 130 insertions(+), 58 deletions(-) (limited to 'xlators/cluster/dht/src') diff --git a/xlators/cluster/dht/src/dht-messages.h b/xlators/cluster/dht/src/dht-messages.h index 153f4de0458..30b64eb5711 100644 --- a/xlators/cluster/dht/src/dht-messages.h +++ b/xlators/cluster/dht/src/dht-messages.h @@ -40,7 +40,7 @@ */ #define GLFS_DHT_BASE GLFS_MSGID_COMP_DHT -#define GLFS_DHT_NUM_MESSAGES 117 +#define GLFS_DHT_NUM_MESSAGES 118 #define GLFS_MSGID_END (GLFS_DHT_BASE + GLFS_DHT_NUM_MESSAGES + 1) /* Messages with message IDs */ @@ -1072,11 +1072,18 @@ #define DHT_MSG_LOCK_INODE_UNREF_FAILED (GLFS_DHT_BASE + 116) /* - * @messageid 109116 + * @messageid 109117 * @diagnosis * @recommendedaction None */ #define DHT_MSG_ASPRINTF_FAILED (GLFS_DHT_BASE + 117) +/* + * @messageid 109118 + * @diagnosis + * @recommendedaction None + */ +#define DHT_MSG_DIR_LOOKUP_FAILED (GLFS_DHT_BASE + 118) + #define glfs_msg_end_x GLFS_MSGID_END, "Invalid: End of messages" #endif /* _DHT_MESSAGES_H_ */ diff --git a/xlators/cluster/dht/src/dht-rebalance.c b/xlators/cluster/dht/src/dht-rebalance.c index c92bb6ad4d7..dc28908b9ad 100644 --- a/xlators/cluster/dht/src/dht-rebalance.c +++ b/xlators/cluster/dht/src/dht-rebalance.c @@ -2452,6 +2452,7 @@ gf_defrag_get_entry (xlator_t *this, int i, struct dht_container **container, struct dht_container *tmp_container = NULL; xlator_t *hashed_subvol = NULL; xlator_t *cached_subvol = NULL; + int fop_errno = 0; if (defrag->defrag_status != GF_DEFRAG_STATUS_STARTED) { ret = -1; @@ -2475,11 +2476,11 @@ gf_defrag_get_entry (xlator_t *this, int i, struct dht_container **container, } if (ret < 0) { - gf_msg (this->name, GF_LOG_ERROR, 0, + gf_msg (this->name, GF_LOG_WARNING, -ret, DHT_MSG_MIGRATE_DATA_FAILED, - "%s: Migrate data failed: Readdir returned" - " %s. Aborting migrate-data", loc->path, - strerror(-ret)); + "Readdirp failed. Aborting data migration for " + "directory: %s", loc->path); + fop_errno = -ret; ret = -1; goto out; } @@ -2611,9 +2612,9 @@ gf_defrag_get_entry (xlator_t *this, int i, struct dht_container **container, ret = syncop_lookup (this, &entry_loc, NULL, NULL, NULL, NULL); if (ret) { - gf_msg (this->name, GF_LOG_ERROR, 0, + gf_msg (this->name, GF_LOG_WARNING, -ret, DHT_MSG_MIGRATE_FILE_FAILED, - "Migrate file failed:%s lookup failed", + "lookup failed for file:%s", entry_loc.path); if (-ret != ENOENT && -ret != ESTALE) { @@ -2734,6 +2735,9 @@ out: if (xattr_rsp) dict_unref (xattr_rsp); + + + errno = fop_errno; return ret; } @@ -2759,6 +2763,7 @@ gf_defrag_process_dir (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, int throttle_up = 0; struct dir_dfmeta *dir_dfmeta = NULL; int should_commit_hash = 1; + int fop_errno = 0; gf_log (this->name, GF_LOG_INFO, "migrate data called on %s", loc->path); @@ -2781,10 +2786,11 @@ gf_defrag_process_dir (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, ret = syncop_opendir (this, loc, fd, NULL, NULL); if (ret) { - gf_msg (this->name, GF_LOG_ERROR, 0, + gf_msg (this->name, GF_LOG_WARNING, 0, DHT_MSG_MIGRATE_DATA_FAILED, "Migrate data failed: Failed to open dir %s", loc->path); + fop_errno = -ret; ret = -1; goto out; } @@ -2931,9 +2937,12 @@ gf_defrag_process_dir (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, migrate_data, dir_dfmeta, xattr_req, &should_commit_hash); + if (ret) { - gf_log ("DHT", GF_LOG_INFO, "Found critical " + fop_errno = errno; + gf_log ("this->name", GF_LOG_WARNING, "Found " "error from gf_defrag_get_entry"); + ret = -1; goto out; } @@ -2991,6 +3000,7 @@ out: ret = 2; } + errno = fop_errno; return ret; } int @@ -3160,7 +3170,6 @@ out: return ret; } - int gf_defrag_fix_layout (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, dict_t *fix_layout, dict_t *migrate_data) @@ -3184,14 +3193,33 @@ gf_defrag_fix_layout (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, goto out; } - - ret = syncop_lookup (this, loc, &iatt, NULL, NULL, NULL); if (ret) { - gf_log (this->name, GF_LOG_ERROR, "Lookup failed on %s", - loc->path); - ret = -1; - goto out; + if (strcmp (loc->path, "/") == 0) { + gf_msg (this->name, GF_LOG_ERROR, -ret, + DHT_MSG_DIR_LOOKUP_FAILED, + "lookup failed for:%s", loc->path); + + defrag->total_failures++; + ret = -1; + goto out; + } + + if (-ret == ENOENT || -ret == ESTALE) { + gf_msg (this->name, GF_LOG_INFO, errno, + DHT_MSG_DIR_LOOKUP_FAILED, + "Dir:%s renamed or removed. Skipping", + loc->path); + ret = 0; + goto out; + } else { + gf_msg (this->name, GF_LOG_ERROR, -ret, + DHT_MSG_DIR_LOOKUP_FAILED, + "lookup failed for:%s", loc->path); + + defrag->total_failures++; + goto out; + } } if ((defrag->cmd != GF_DEFRAG_CMD_START_TIER) && @@ -3199,18 +3227,24 @@ gf_defrag_fix_layout (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, ret = gf_defrag_process_dir (this, defrag, loc, migrate_data); if (ret && ret != 2) { - defrag->total_failures++; + if (errno == ENOENT || errno == ESTALE) { + ret = 0; + goto out; + } else { - gf_msg (this->name, GF_LOG_ERROR, 0, - DHT_MSG_DEFRAG_PROCESS_DIR_FAILED, - "gf_defrag_process_dir failed for directory: %s" - , loc->path); + defrag->total_failures++; - if (conf->decommission_in_progress) { - goto out; - } + gf_msg (this->name, GF_LOG_ERROR, 0, + DHT_MSG_DEFRAG_PROCESS_DIR_FAILED, + "gf_defrag_process_dir failed for " + "directory: %s", loc->path); - should_commit_hash = 0; + if (conf->decommission_in_progress) { + goto out; + } + + should_commit_hash = 0; + } } else if (ret == 2) { should_commit_hash = 0; } @@ -3227,8 +3261,14 @@ gf_defrag_fix_layout (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, ret = syncop_opendir (this, loc, fd, NULL, NULL); if (ret) { - gf_log (this->name, GF_LOG_ERROR, "Failed to open dir %s", - loc->path); + if (-ret == ENOENT || -ret == ESTALE) { + ret = 0; + goto out; + } + + gf_log (this->name, GF_LOG_ERROR, "Failed to open dir %s, " + "err:%d", loc->path, -ret); + ret = -1; goto out; } @@ -3240,8 +3280,15 @@ gf_defrag_fix_layout (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, { if (ret < 0) { - gf_log (this->name, GF_LOG_ERROR, "Readdir returned %s" - ". Aborting fix-layout",strerror(-ret)); + if (-ret == ENOENT || -ret == ESTALE) { + ret = 0; + goto out; + } + + gf_msg (this->name, GF_LOG_ERROR, -ret, + DHT_MSG_READDIR_ERROR, "readdirp failed for " + "path %s. Aborting fix-layout", loc->path); + ret = -1; goto out; } @@ -3333,43 +3380,63 @@ gf_defrag_fix_layout (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, ret = syncop_lookup (this, &entry_loc, &iatt, NULL, NULL, NULL); - /*Check whether it is ENOENT or ESTALE*/ if (ret) { - gf_log (this->name, GF_LOG_ERROR, "%s" - " lookup failed with %d", - entry_loc.path, -ret); - - if (!conf->decommission_in_progress && - -ret != ENOENT && -ret != ESTALE) { - should_commit_hash = 0; + if (-ret == ENOENT || -ret == ESTALE) { + gf_msg (this->name, GF_LOG_INFO, errno, + DHT_MSG_DIR_LOOKUP_FAILED, + "Dir:%s renamed or removed. " + "Skipping", loc->path); + ret = 0; + continue; + } else { + gf_msg (this->name, GF_LOG_ERROR, -ret, + DHT_MSG_DIR_LOOKUP_FAILED, + "lookup failed for:%s", + entry_loc.path); + defrag->total_failures++; + if (conf->decommission_in_progress) { + defrag->defrag_status = + GF_DEFRAG_STATUS_FAILED; + ret = -1; + goto out; + } else { + should_commit_hash = 0; + continue; + } } - - continue; } ret = syncop_setxattr (this, &entry_loc, fix_layout, 0, NULL, NULL); if (ret) { - gf_log (this->name, GF_LOG_ERROR, "Setxattr " - "failed for %s", entry_loc.path); - - defrag->total_failures++; - - /*Don't go for fix-layout of child subtree if" - fix-layout failed*/ - if (conf->decommission_in_progress) { - defrag->defrag_status = - GF_DEFRAG_STATUS_FAILED; - - ret = -1; - - goto out; - } else { + if (-ret == ENOENT || -ret == ESTALE) { + gf_msg (this->name, GF_LOG_INFO, -ret, + DHT_MSG_LAYOUT_FIX_FAILED, + "Setxattr failed. Dir %s " + "renamed or removed", + entry_loc.path); + ret = 0; continue; + } else { + + gf_msg (this->name, GF_LOG_ERROR, -ret, + DHT_MSG_LAYOUT_FIX_FAILED, + "Setxattr failed for %s", + entry_loc.path); + + defrag->total_failures++; + + if (conf->decommission_in_progress) { + defrag->defrag_status = + GF_DEFRAG_STATUS_FAILED; + ret = -1; + goto out; + } else { + continue; + } } } - /* A return value of 2 means, either process_dir or * lookup of a dir failed. Hence, don't commit hash * for the current directory*/ @@ -3389,8 +3456,6 @@ gf_defrag_fix_layout (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, defrag->defrag_status = GF_DEFRAG_STATUS_FAILED; - ret = -1; - goto out; } else { /* Let's not commit-hash if -- cgit