summaryrefslogtreecommitdiffstats
path: root/xlators/cluster/dht/src/dht-helper.c
diff options
context:
space:
mode:
authorPranith Kumar K <pkarampu@redhat.com>2013-12-11 09:33:53 +0530
committerAnand Avati <avati@redhat.com>2014-01-19 23:05:15 -0800
commit8d55c25f158921b508bff0e7f25158991913f922 (patch)
tree258500ed1f6b15d6f2f6848a5cbd883b8dc40ea2 /xlators/cluster/dht/src/dht-helper.c
parentc2b09dc87e0763dfdff1e93a1dc6cc4c05f091bf (diff)
syncop: Change return value of syncop
Problem: We found a day-1 bug when syncop_xxx() infra is used inside a synctask with compilation optimization (CFLAGS -O2). Detailed explanation of the Root cause: We found the bug in 'gf_defrag_migrate_data' in rebalance operation: Lets look at interesting parts of the function: int gf_defrag_migrate_data (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc, dict_t *migrate_data) { ..... code section - [ Loop ] while ((ret = syncop_readdirp (this, fd, 131072, offset, NULL, &entries)) != 0) { ..... code section - [ ERRNO-1 ] (errno of readdirp is stored in readdir_operrno by a thread) /* Need to keep track of ENOENT errno, that means, there is no need to send more readdirp() */ readdir_operrno = errno; ..... code section - [ SYNCOP-1 ] (syncop_getxattr is called by a thread) ret = syncop_getxattr (this, &entry_loc, &dict, GF_XATTR_LINKINFO_KEY); code section - [ ERRNO-2] (checking for failures of syncop_getxattr(). This may not always be executed in same thread which executed [SYNCOP-1]) if (ret < 0) { if (errno != ENODATA) { loglevel = GF_LOG_ERROR; defrag->total_failures += 1; ..... } the function above could be executed by thread(t1) till [SYNCOP-1] and code from [ERRNO-2] can be executed by a different thread(t2) because of the way syncop-infra schedules the tasks. when the code is compiled with -O2 optimization this is the assembly code that is generated: [ERRNO-1] 1165 readdir_operrno = errno; <<---- errno gets expanded as *(__errno_location()) 0x00007fd149d48b60 <+496>: callq 0x7fd149d410c0 <address@hidden> 0x00007fd149d48b72 <+514>: mov %rax,0x50(%rsp) <<------ Address returned by __errno_location() is stored in a special location in stack for later use. 0x00007fd149d48b77 <+519>: mov (%rax),%eax 0x00007fd149d48b79 <+521>: mov %eax,0x78(%rsp) .... [ERRNO-2] 1281 if (errno != ENODATA) { 0x00007fd149d492ae <+2366>: mov 0x50(%rsp),%rax <<----- Because it already stored the address returned by __errno_location(), it just dereferences the address to get the errno value. BUT THIS CODE NEED NOT BE EXECUTED BY SAME THREAD!!! 0x00007fd149d492b3 <+2371>: mov $0x9,%ebp 0x00007fd149d492b8 <+2376>: mov (%rax),%edi 0x00007fd149d492ba <+2378>: cmp $0x3d,%edi The problem is that __errno_location() value of t1 and t2 are different. So [ERRNO-2] ends up reading errno of t1 instead of errno of t2 even though t2 is executing [ERRNO-2] code section. When code is compiled without any optimization for [ERRNO-2]: 1281 if (errno != ENODATA) { 0x00007fd58e7a326f <+2237>: callq 0x7fd58e797300 <address@hidden><<--- As it is calling __errno_location() again it gets the location from t2 so it works as intended. 0x00007fd58e7a3274 <+2242>: mov (%rax),%eax 0x00007fd58e7a3276 <+2244>: cmp $0x3d,%eax 0x00007fd58e7a3279 <+2247>: je 0x7fd58e7a32a1 <gf_defrag_migrate_data+2287> Fix: Make syncop_xxx() return (-errno) value as the return value in case of errors and all the functions which make syncop_xxx() will need to use (-ret) to figure out the reason for failure in case of syncop_xxx() failures. Change-Id: I314d20dabe55d3e62ff66f3b4adb1cac2eaebb57 BUG: 1040356 Signed-off-by: Pranith Kumar K <pkarampu@redhat.com> Reviewed-on: http://review.gluster.org/6475 Tested-by: Gluster Build System <jenkins@build.gluster.com> Reviewed-by: Anand Avati <avati@redhat.com>
Diffstat (limited to 'xlators/cluster/dht/src/dht-helper.c')
-rw-r--r--xlators/cluster/dht/src/dht-helper.c24
1 files changed, 16 insertions, 8 deletions
diff --git a/xlators/cluster/dht/src/dht-helper.c b/xlators/cluster/dht/src/dht-helper.c
index 00a98f1cfa5..76bfbaedb77 100644
--- a/xlators/cluster/dht/src/dht-helper.c
+++ b/xlators/cluster/dht/src/dht-helper.c
@@ -769,17 +769,20 @@ dht_migration_complete_check_task (void *data)
dst_node = dht_linkfile_subvol (this, NULL, NULL, dict);
if (ret) {
- if (!dht_inode_missing(errno) || (!local->loc.inode)) {
+ if (!dht_inode_missing(-ret) || (!local->loc.inode)) {
gf_log (this->name, GF_LOG_ERROR,
"%s: failed to get the 'linkto' xattr %s",
- local->loc.path, strerror (errno));
+ local->loc.path, strerror (-ret));
+ ret = -1;
goto out;
}
/* Need to do lookup on hashed subvol, then get the file */
ret = syncop_lookup (this, &local->loc, NULL, &stbuf, NULL,
NULL);
- if (ret)
+ if (ret) {
+ ret = -1;
goto out;
+ }
dst_node = dht_subvol_get_cached (this, local->loc.inode);
}
@@ -799,6 +802,7 @@ dht_migration_complete_check_task (void *data)
gf_log (this->name, GF_LOG_ERROR,
"%s: failed to lookup the file on %s",
local->loc.path, dst_node->name);
+ ret = -1;
goto out;
}
@@ -869,10 +873,11 @@ dht_migration_complete_check_task (void *data)
ret = syncop_open (dst_node, &tmp_loc,
iter_fd->flags, iter_fd);
- if (ret == -1) {
+ if (ret < 0) {
gf_log (this->name, GF_LOG_ERROR, "failed to open "
"the fd (%p, flags=0%o) on file %s @ %s",
iter_fd, iter_fd->flags, path, dst_node->name);
+ ret = -1;
open_failed = 1;
}
}
@@ -955,11 +960,12 @@ dht_rebalance_inprogress_task (void *data)
conf->link_xattr_name);
}
- if (ret) {
+ if (ret < 0) {
gf_log (this->name, GF_LOG_ERROR,
"%s: failed to get the 'linkto' xattr %s",
- local->loc.path, strerror (errno));
- goto out;
+ local->loc.path, strerror (-ret));
+ ret = -1;
+ goto out;
}
dst_node = dht_linkfile_subvol (this, NULL, NULL, dict);
@@ -981,6 +987,7 @@ dht_rebalance_inprogress_task (void *data)
gf_log (this->name, GF_LOG_ERROR,
"%s: failed to lookup the file on %s",
local->loc.path, dst_node->name);
+ ret = -1;
goto out;
}
@@ -1014,10 +1021,11 @@ dht_rebalance_inprogress_task (void *data)
ret = syncop_open (dst_node, &tmp_loc,
iter_fd->flags, iter_fd);
- if (ret == -1) {
+ if (ret < 0) {
gf_log (this->name, GF_LOG_ERROR, "failed to send open "
"the fd (%p, flags=0%o) on file %s @ %s",
iter_fd, iter_fd->flags, path, dst_node->name);
+ ret = -1;
open_failed = 1;
}
}