From 145185454464c1c45af64c13919e6fe5bf559769 Mon Sep 17 00:00:00 2001 From: Mohit Agrawal Date: Tue, 29 Nov 2016 10:50:04 +0530 Subject: cluster/dht: A hard link is lost during rebalance + lookup Problem: A hard link is lost during rebalance + lookup.Rebalance skip files if file has hardlink.In dht_migrate_file __is_file_migratable () function checks if a file has hardlink, if yes file is not migrated but if link is created after call this function then link will lost. Solution: Call __check_file_has_hardlink to check hardlink existence after (S+T) bits in migration process ,if file has hardlink then skip the file for migrate rebalance process. > BUG: 1396048 > Change-Id: Ia53c07ef42f1128c2eedf959a757e8df517b9d12 > Signed-off-by: Mohit Agrawal > Reviewed-on: http://review.gluster.org/15866 > Reviewed-by: Susant Palai > Smoke: Gluster Build System > NetBSD-regression: NetBSD Build System > CentOS-regression: Gluster Build System > Reviewed-by: N Balachandran > Reviewed-by: Raghavendra G > (cherry picked from commit 71dd2e914d4a537bf74e1ec3a24512fc83bacb1d) BUG: 1399432 Change-Id: I30e21efd5a054d8a3e640ab3ed8aa7955d083926 Signed-off-by: Mohit Agrawal Reviewed-on: http://review.gluster.org/15954 CentOS-regression: Gluster Build System Smoke: Gluster Build System NetBSD-regression: NetBSD Build System Reviewed-by: N Balachandran Reviewed-by: Raghavendra G --- xlators/cluster/dht/src/dht-rebalance.c | 97 +++++++++++++++++++++------------ 1 file changed, 62 insertions(+), 35 deletions(-) (limited to 'xlators/cluster/dht/src') diff --git a/xlators/cluster/dht/src/dht-rebalance.c b/xlators/cluster/dht/src/dht-rebalance.c index 59168d2fab9..5fd9c86d363 100644 --- a/xlators/cluster/dht/src/dht-rebalance.c +++ b/xlators/cluster/dht/src/dht-rebalance.c @@ -376,6 +376,50 @@ out: return ret; } + +static int +__check_file_has_hardlink (xlator_t *this, loc_t *loc, + struct iatt *stbuf, dict_t *xattrs, int flags, + gf_defrag_info_t *defrag) +{ + int ret = 0; + + if (flags == GF_DHT_MIGRATE_HARDLINK_IN_PROGRESS) { + ret = 0; + return ret; + } + if (stbuf->ia_nlink > 1) { + /* support for decomission */ + if (flags == GF_DHT_MIGRATE_HARDLINK) { + synclock_lock (&defrag->link_lock); + ret = gf_defrag_handle_hardlink + (this, loc, xattrs, stbuf); + synclock_unlock (&defrag->link_lock); + /* + Returning zero will force the file to be remigrated. + Checkout gf_defrag_handle_hardlink for more information. + */ + if (ret && ret != -2) { + gf_msg (this->name, GF_LOG_WARNING, 0, + DHT_MSG_MIGRATE_FILE_FAILED, + "Migrate file failed:" + "%s: failed to migrate file with link", + loc->path); + } + } else { + gf_msg (this->name, GF_LOG_WARNING, 0, + DHT_MSG_MIGRATE_FILE_FAILED, + "Migrate file failed:" + "%s: file has hardlinks", loc->path); + ret = -ENOTSUP; + } + } + + return ret; +} + + + /* return values 0 : File will be migrated @@ -424,40 +468,9 @@ __is_file_migratable (xlator_t *this, loc_t *loc, } } - if (flags == GF_DHT_MIGRATE_HARDLINK_IN_PROGRESS) { - ret = 0; - goto out; - } - - if (stbuf->ia_nlink > 1) { - /* support for decomission */ - if (flags == GF_DHT_MIGRATE_HARDLINK) { - synclock_lock (&defrag->link_lock); - ret = gf_defrag_handle_hardlink - (this, loc, xattrs, stbuf); - synclock_unlock (&defrag->link_lock); - /* - Returning zero will force the file to be remigrated. - Checkout gf_defrag_handle_hardlink for more information. - */ - if (ret && ret != -2) { - gf_msg (this->name, GF_LOG_WARNING, 0, - DHT_MSG_MIGRATE_FILE_FAILED, - "Migrate file failed:" - "%s: failed to migrate file with link", - loc->path); - } - } else { - gf_msg (this->name, GF_LOG_WARNING, 0, - DHT_MSG_MIGRATE_FILE_FAILED, - "Migrate file failed:" - "%s: file has hardlinks", loc->path); - ret = -ENOTSUP; - } - goto out; - } - - ret = 0; + /* Check if file has hardlink*/ + ret = __check_file_has_hardlink (this, loc, stbuf, xattrs, + flags, defrag); out: return ret; @@ -1337,8 +1350,13 @@ dht_migrate_file (xlator_t *this, loc_t *loc, xlator_t *from, xlator_t *to, goto out; } + if (xattr_rsp) { + /* we no more require this key */ + dict_del (dict, conf->link_xattr_name); + dict_unref (xattr_rsp); + } - ret = syncop_fstat (from, src_fd, &stbuf, NULL, NULL); + ret = syncop_fstat (from, src_fd, &stbuf, dict, &xattr_rsp); if (ret) { gf_msg (this->name, GF_LOG_ERROR, -ret, DHT_MSG_MIGRATE_FILE_FAILED, @@ -1348,6 +1366,15 @@ dht_migrate_file (xlator_t *this, loc_t *loc, xlator_t *from, xlator_t *to, goto out; } + /* Check again if file has hardlink */ + ret = __check_file_has_hardlink (this, loc, &stbuf, xattr_rsp, + flag, defrag); + if (ret) { + if (ret == -2) + ret = 0; + goto out; + } + /* Try to preserve 'holes' while migrating data */ if (stbuf.ia_size > (stbuf.ia_blocks * GF_DISK_SECTOR_SIZE)) file_has_holes = 1; -- cgit From 58deefde3c279fd867304118d60970bdfa72cd93 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 Change-Id: I62c7fa93c0b9b7e764065ad1574b97acf51b5996 BUG: 1408414 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 (cherry picked from commit c20febcb1ffaef3fa29563987e7a3b554aea27b3) Signed-off-by: Susant Palai Reviewed-on: http://review.gluster.org/16278 --- 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 5fd9c86d363..1d145855ed7 100644 --- a/xlators/cluster/dht/src/dht-rebalance.c +++ b/xlators/cluster/dht/src/dht-rebalance.c @@ -2362,6 +2362,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; @@ -2385,11 +2386,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; } @@ -2521,9 +2522,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) { @@ -2644,6 +2645,9 @@ out: if (xattr_rsp) dict_unref (xattr_rsp); + + + errno = fop_errno; return ret; } @@ -2669,6 +2673,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); @@ -2691,10 +2696,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; } @@ -2841,9 +2847,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; } @@ -2901,6 +2910,7 @@ out: ret = 2; } + errno = fop_errno; return ret; } int @@ -3070,7 +3080,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) @@ -3094,14 +3103,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) && @@ -3109,18 +3137,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; } @@ -3137,8 +3171,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; } @@ -3150,8 +3190,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; } @@ -3243,43 +3290,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*/ @@ -3299,8 +3366,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 From d5ccb525d3c865ea5eba9f7b79b76bc1db76906f Mon Sep 17 00:00:00 2001 From: N Balachandran Date: Wed, 16 Nov 2016 10:09:09 +0530 Subject: cluster/dht: Check for null inode Check for NULL inode before attempting to set dht inode ctx. > Change-Id: I7693c18445f138221d8417df5e95b118cedb818a > BUG: 1395261 > Signed-off-by: N Balachandran > Reviewed-on: http://review.gluster.org/15847 > Smoke: Gluster Build System > Reviewed-by: Shyamsundar Ranganathan > NetBSD-regression: NetBSD Build System > CentOS-regression: Gluster Build System > Reviewed-by: Atin Mukherjee (cherry picked from commit 8313d53accaa22feb14d284fb91245be0a32e16e) Change-Id: I7607d32d38d707dd5d71b98efffd1a458ffe90d7 BUG: 1395510 Signed-off-by: N Balachandran Reviewed-on: http://review.gluster.org/15850 Smoke: Gluster Build System CentOS-regression: Gluster Build System NetBSD-regression: NetBSD Build System Reviewed-by: Shyamsundar Ranganathan Reviewed-by: Niels de Vos --- xlators/cluster/dht/src/dht-common.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'xlators/cluster/dht/src') diff --git a/xlators/cluster/dht/src/dht-common.c b/xlators/cluster/dht/src/dht-common.c index 90db73f1f72..0ce93e3d112 100644 --- a/xlators/cluster/dht/src/dht-common.c +++ b/xlators/cluster/dht/src/dht-common.c @@ -898,8 +898,11 @@ unlock: dht_layout_set (this, local->inode, layout); } - dht_inode_ctx_time_update (local->inode, this, - &local->stbuf, 1); + if (local->inode) { + dht_inode_ctx_time_update (local->inode, this, + &local->stbuf, 1); + } + if (local->loc.parent) { dht_inode_ctx_time_update (local->loc.parent, this, &local->postparent, 1); -- cgit From aaf2a2897080754aceb42245e92b0269556d6f38 Mon Sep 17 00:00:00 2001 From: Jiffin Tony Thottan Date: Mon, 21 Nov 2016 18:08:14 +0530 Subject: dht/rename : Incase of failure remove linkto file properly Generally linkto file is created using root user. Consider following case, a user is trying to rename a file which he is not permitted. So the rename fails with EACESS and when rename tries to cleanup the linkto file, it fails. The above issue happens when rename/00.t test executed on nfs-ganesha clients : Steps executed in script * create a file "abc" using root * rename the file "abc" to "xyz" using a non root user, it fails with EACESS * delete "abc" * create directory "abc" using root * again try ot rename "abc" to "xyz" using non root user, test hungs here which slowly leds to OOM kill of ganesha process RCA put forwarded by Du for OOM kill of ganesha Note that when we hit this bug, we've a scenario of a dentry being present as: * a linkto file on one subvol * a directory on rest of subvols When a lookup happens on the dentry in such a scenario, the control flow goes into an infinite loop of: dht_lookup_everywhere dht_lookup_everywhere_cbk dht_lookup_unlink_cbk dht_lookup_everywhere_done dht_lookup_directory (as local->dir_count > 0) dht_lookup_dir_cbk (sets to local->need_selfheal = 1 as the entry is a linkto file on one of the subvol) dht_lookup_everywhere (as need_selfheal = 1). This infinite loop can cause increased consumption of memory due to: 1) dht_lookup_directory assigns a new layout to local->layout unconditionally 2) Most of the functions in this loop do a stack_wind of various fops. This results in growing of call stack (note that call-stack is destroyed only after lookup response is received by fuse - which never happens in this case) Thanks Du for root causing the oom kill and Sushant for suggesting the fix Upstream reference : >Change-Id: I1e16bc14aa685542afbd21188426ecb61fd2689d >BUG: 1397052 >Signed-off-by: Jiffin Tony Thottan >Reviewed-on: http://review.gluster.org/15894 >NetBSD-regression: NetBSD Build System >CentOS-regression: Gluster Build System >Smoke: Gluster Build System >Reviewed-by: Raghavendra G >(cherry picked from commit 57d59f4be205ae0c7888758366dc0049bdcfe449) Change-Id: I1e16bc14aa685542afbd21188426ecb61fd2689d BUG: 1401029 Signed-off-by: Jiffin Tony Thottan Reviewed-on: http://review.gluster.org/16015 Smoke: Gluster Build System NetBSD-regression: NetBSD Build System CentOS-regression: Gluster Build System Reviewed-by: Niels de Vos --- xlators/cluster/dht/src/dht-common.c | 8 +++++++- xlators/cluster/dht/src/dht-rename.c | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) (limited to 'xlators/cluster/dht/src') diff --git a/xlators/cluster/dht/src/dht-common.c b/xlators/cluster/dht/src/dht-common.c index 0ce93e3d112..a9714b02b79 100644 --- a/xlators/cluster/dht/src/dht-common.c +++ b/xlators/cluster/dht/src/dht-common.c @@ -1316,6 +1316,7 @@ dht_lookup_unlink_cbk (call_frame_t *frame, void *cookie, xlator_t *this, local = (dht_local_t*)frame->local; path = local->loc.path; + FRAME_SU_UNDO (frame, dht_local_t); gf_msg (this->name, GF_LOG_INFO, 0, DHT_MSG_UNLINK_LOOKUP_INFO, "lookup_unlink returned with " @@ -2009,7 +2010,12 @@ unlock: loc->path, subvol->name, (local->hashed_subvol? local->hashed_subvol->name : "")); - + /* * + * These stale files may be created using root + * user. Hence deletion will work only with + * root. + */ + FRAME_SU_DO (frame, dht_local_t); STACK_WIND (frame, dht_lookup_unlink_cbk, subvol, subvol->fops->unlink, loc, 0, dict_req); diff --git a/xlators/cluster/dht/src/dht-rename.c b/xlators/cluster/dht/src/dht-rename.c index 777c63de685..a9ffd1d9fb5 100644 --- a/xlators/cluster/dht/src/dht-rename.c +++ b/xlators/cluster/dht/src/dht-rename.c @@ -637,6 +637,7 @@ dht_rename_unlink_cbk (call_frame_t *frame, void *cookie, xlator_t *this, local = frame->local; prev = cookie; + FRAME_SU_UNDO (frame, dht_local_t); if (!local) { gf_msg (this->name, GF_LOG_ERROR, 0, DHT_MSG_INVALID_VALUE, @@ -745,7 +746,12 @@ dht_rename_cleanup (call_frame_t *frame) local->loc2.pargfid) == 0) { DHT_MARKER_DONT_ACCOUNT(xattr_new); } - + /* * + * The link to file is created using root permission. + * Hence deletion should happen using root. Otherwise + * it will fail. + */ + FRAME_SU_DO (frame, dht_local_t); STACK_WIND (frame, dht_rename_unlink_cbk, src_cached, src_cached->fops->unlink, &local->loc2, 0, xattr_new); -- cgit From 2b34adff73897ff6086b640fe7f7c592a9ed54b7 Mon Sep 17 00:00:00 2001 From: Raghavendra G Date: Tue, 8 Nov 2016 12:09:57 +0530 Subject: cluster/dht: Fix memory corruption while accessing regex stored in private If reconfigure is executed parallely (or concurrently with dht_init), there are races that can corrupt memory. One such race is modification of regexes stored in conf (conf->rsync_regex_valid and conf->extra_regex_valid) through dht_init_regex. With change [1], reconfigure codepath can get executed parallely (with itself or with dht_init) and this fix is needed. Also, a reconfigure can race with any thread doing dht_layout_search, resulting in dht_layout_search accessing regex freed up by reconfigure (like in bz 1399134). [1] http://review.gluster.org/15046 >Change-Id: I039422a65374cf0ccbe0073441f0e8c442ebf830 >BUG: 1399134 >Signed-off-by: Raghavendra G >Reviewed-on: http://review.gluster.org/15945 >Smoke: Gluster Build System >NetBSD-regression: NetBSD Build System >Reviewed-by: N Balachandran >CentOS-regression: Gluster Build System >Reviewed-by: Shyamsundar Ranganathan Change-Id: I039422a65374cf0ccbe0073441f0e8c442ebf830 BUG: 1399423 Signed-off-by: Raghavendra G (cherry picked from commit 64451d0f25e7cc7aafc1b6589122648281e4310a) Reviewed-on: http://review.gluster.org/15793 Smoke: Gluster Build System CentOS-regression: Gluster Build System NetBSD-regression: NetBSD Build System --- xlators/cluster/dht/src/dht-common.h | 1 + xlators/cluster/dht/src/dht-hashfn.c | 54 ++++++++++++++++++++++-------------- xlators/cluster/dht/src/dht-shared.c | 54 ++++++++++++++++++++---------------- 3 files changed, 64 insertions(+), 45 deletions(-) (limited to 'xlators/cluster/dht/src') diff --git a/xlators/cluster/dht/src/dht-common.h b/xlators/cluster/dht/src/dht-common.h index 29ac798b92b..9e9ca712417 100644 --- a/xlators/cluster/dht/src/dht-common.h +++ b/xlators/cluster/dht/src/dht-common.h @@ -548,6 +548,7 @@ struct dht_conf { /* lock migration */ gf_boolean_t lock_migration_enabled; + gf_lock_t lock; }; typedef struct dht_conf dht_conf_t; diff --git a/xlators/cluster/dht/src/dht-hashfn.c b/xlators/cluster/dht/src/dht-hashfn.c index 66e3ede736b..f8e614a40aa 100644 --- a/xlators/cluster/dht/src/dht-hashfn.c +++ b/xlators/cluster/dht/src/dht-hashfn.c @@ -41,12 +41,16 @@ dht_hash_compute_internal (int type, const char *name, uint32_t *hash_p) static gf_boolean_t -dht_munge_name (const char *original, char *modified, size_t len, regex_t *re) +dht_munge_name (const char *original, char *modified, + size_t len, regex_t *re) { - regmatch_t matches[2]; - size_t new_len; + regmatch_t matches[2] = {{0}, }; + size_t new_len = 0; + int ret = 0; - if (regexec(re,original,2,matches,0) != REG_NOMATCH) { + ret = regexec(re, original, 2, matches, 0); + + if (ret != REG_NOMATCH) { if (matches[1].rm_so != -1) { new_len = matches[1].rm_eo - matches[1].rm_so; /* Equal would fail due to the NUL at the end. */ @@ -60,7 +64,7 @@ dht_munge_name (const char *original, char *modified, size_t len, regex_t *re) } /* This is guaranteed safe because of how the dest was allocated. */ - strcpy(modified,original); + strcpy(modified, original); return _gf_false; } @@ -68,28 +72,36 @@ int dht_hash_compute (xlator_t *this, int type, const char *name, uint32_t *hash_p) { char *rsync_friendly_name = NULL; - dht_conf_t *priv = this->private; + dht_conf_t *priv = NULL; size_t len = 0; gf_boolean_t munged = _gf_false; - if (priv->extra_regex_valid) { - len = strlen(name) + 1; - rsync_friendly_name = alloca(len); - munged = dht_munge_name (name, rsync_friendly_name, len, - &priv->extra_regex); - } + priv = this->private; - if (!munged && priv->rsync_regex_valid) { - len = strlen(name) + 1; - rsync_friendly_name = alloca(len); - gf_msg_trace (this->name, 0, "trying regex for %s", name); - munged = dht_munge_name (name, rsync_friendly_name, len, - &priv->rsync_regex); - if (munged) { - gf_msg_debug (this->name, 0, - "munged down to %s", rsync_friendly_name); + LOCK (&priv->lock); + { + if (priv->extra_regex_valid) { + len = strlen(name) + 1; + rsync_friendly_name = alloca(len); + munged = dht_munge_name (name, rsync_friendly_name, len, + &priv->extra_regex); + } + + if (!munged && priv->rsync_regex_valid) { + len = strlen(name) + 1; + rsync_friendly_name = alloca(len); + gf_msg_trace (this->name, 0, "trying regex for %s", + name); + munged = dht_munge_name (name, rsync_friendly_name, len, + &priv->rsync_regex); + if (munged) { + gf_msg_debug (this->name, 0, + "munged down to %s", + rsync_friendly_name); + } } } + UNLOCK (&priv->lock); if (!munged) { rsync_friendly_name = (char *)name; diff --git a/xlators/cluster/dht/src/dht-shared.c b/xlators/cluster/dht/src/dht-shared.c index 0fea1d58e58..5c810f0dc77 100644 --- a/xlators/cluster/dht/src/dht-shared.c +++ b/xlators/cluster/dht/src/dht-shared.c @@ -336,9 +336,9 @@ out: } void dht_init_regex (xlator_t *this, dict_t *odict, char *name, - regex_t *re, gf_boolean_t *re_valid) + regex_t *re, gf_boolean_t *re_valid, dht_conf_t *conf) { - char *temp_str; + char *temp_str = NULL; if (dict_get_str (odict, name, &temp_str) != 0) { if (strcmp(name,"rsync-hash-regex")) { @@ -347,25 +347,29 @@ dht_init_regex (xlator_t *this, dict_t *odict, char *name, temp_str = "^\\.(.+)\\.[^.]+$"; } - if (*re_valid) { - regfree(re); - *re_valid = _gf_false; - } + LOCK (&conf->lock); + { + if (*re_valid) { + regfree(re); + *re_valid = _gf_false; + } - if (!strcmp(temp_str,"none")) { - return; - } + if (!strcmp(temp_str, "none")) { + goto unlock; + } - if (regcomp(re,temp_str,REG_EXTENDED) == 0) { - gf_msg_debug (this->name, 0, - "using regex %s = %s", name, temp_str); - *re_valid = _gf_true; - } - else { - gf_msg (this->name, GF_LOG_WARNING, 0, - DHT_MSG_REGEX_INFO, - "compiling regex %s failed", temp_str); + if (regcomp(re, temp_str, REG_EXTENDED) == 0) { + gf_msg_debug (this->name, 0, + "using regex %s = %s", name, temp_str); + *re_valid = _gf_true; + } else { + gf_msg (this->name, GF_LOG_WARNING, 0, + DHT_MSG_REGEX_INFO, + "compiling regex %s failed", temp_str); + } } +unlock: + UNLOCK (&conf->lock); } int @@ -486,9 +490,9 @@ dht_reconfigure (xlator_t *this, dict_t *options) } dht_init_regex (this, options, "rsync-hash-regex", - &conf->rsync_regex, &conf->rsync_regex_valid); + &conf->rsync_regex, &conf->rsync_regex_valid, conf); dht_init_regex (this, options, "extra-hash-regex", - &conf->extra_regex, &conf->extra_regex_valid); + &conf->extra_regex, &conf->extra_regex_valid, conf); GF_OPTION_RECONF ("weighted-rebalance", conf->do_weighting, options, bool, out); @@ -627,6 +631,10 @@ dht_init (xlator_t *this) goto err; } + LOCK_INIT (&conf->subvolume_lock); + LOCK_INIT (&conf->layout_lock); + LOCK_INIT (&conf->lock); + /* We get the commit-hash to set only for rebalance process */ if (dict_get_uint32 (this->options, "commit-hash", &commit_hash) == 0) { @@ -776,17 +784,15 @@ dht_init (xlator_t *this) } dht_init_regex (this, this->options, "rsync-hash-regex", - &conf->rsync_regex, &conf->rsync_regex_valid); + &conf->rsync_regex, &conf->rsync_regex_valid, conf); dht_init_regex (this, this->options, "extra-hash-regex", - &conf->extra_regex, &conf->extra_regex_valid); + &conf->extra_regex, &conf->extra_regex_valid, conf); ret = dht_layouts_init (this, conf); if (ret == -1) { goto err; } - LOCK_INIT (&conf->subvolume_lock); - LOCK_INIT (&conf->layout_lock); conf->gen = 1; -- cgit