summaryrefslogtreecommitdiffstats
path: root/xlators/mount
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/mount
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/mount')
-rw-r--r--xlators/mount/fuse/src/fuse-bridge.c18
1 files changed, 13 insertions, 5 deletions
diff --git a/xlators/mount/fuse/src/fuse-bridge.c b/xlators/mount/fuse/src/fuse-bridge.c
index ee12d86..315259e 100644
--- a/xlators/mount/fuse/src/fuse-bridge.c
+++ b/xlators/mount/fuse/src/fuse-bridge.c
@@ -4026,12 +4026,14 @@ fuse_nameless_lookup (xlator_t *xl, uuid_t gfid, loc_t *loc)
inode_t *linked_inode = NULL;
if ((loc == NULL) || (xl == NULL)) {
+ ret = -EINVAL;
goto out;
}
if (loc->inode == NULL) {
loc->inode = inode_new (xl->itable);
if (loc->inode == NULL) {
+ ret = -ENOMEM;
goto out;
}
}
@@ -4040,13 +4042,13 @@ fuse_nameless_lookup (xlator_t *xl, uuid_t gfid, loc_t *loc)
xattr_req = dict_new ();
if (xattr_req == NULL) {
+ ret = -ENOMEM;
goto out;
}
ret = syncop_lookup (xl, loc, xattr_req, &iatt, NULL, NULL);
- if (ret < 0) {
+ if (ret < 0)
goto out;
- }
linked_inode = inode_link (loc->inode, NULL, NULL, &iatt);
inode_unref (loc->inode);
@@ -4095,9 +4097,10 @@ fuse_migrate_fd_open (xlator_t *this, fd_t *basefd, fd_t *oldfd,
"name-less lookup of gfid (%s) failed (%s)"
"(old-subvolume:%s-%d new-subvolume:%s-%d)",
uuid_utoa (basefd->inode->gfid),
- strerror (errno),
+ strerror (-ret),
old_subvol->name, old_subvol->graph->id,
new_subvol->name, new_subvol->graph->id);
+ ret = -1;
goto out;
}
@@ -4115,6 +4118,7 @@ fuse_migrate_fd_open (xlator_t *this, fd_t *basefd, fd_t *oldfd,
uuid_utoa (loc.inode->gfid),
old_subvol->name, old_subvol->graph->id,
new_subvol->name, new_subvol->graph->id);
+ ret = -1;
goto out;
}
@@ -4138,9 +4142,10 @@ fuse_migrate_fd_open (xlator_t *this, fd_t *basefd, fd_t *oldfd,
gf_log ("glusterfs-fuse", GF_LOG_WARNING,
"open on basefd (ptr:%p inode-gfid:%s) failed (%s)"
"(old-subvolume:%s-%d new-subvolume:%s-%d)", basefd,
- uuid_utoa (basefd->inode->gfid), strerror (errno),
+ uuid_utoa (basefd->inode->gfid), strerror (-ret),
old_subvol->name, old_subvol->graph->id,
new_subvol->name, new_subvol->graph->id);
+ ret = -1;
goto out;
}
@@ -4208,6 +4213,7 @@ fuse_migrate_locks (xlator_t *this, fd_t *basefd, fd_t *oldfd,
oldfd, newfd, uuid_utoa (newfd->inode->gfid),
old_subvol->name, old_subvol->graph->id,
new_subvol->name, new_subvol->graph->id);
+ ret = -1;
goto out;
}
@@ -4233,6 +4239,7 @@ fuse_migrate_locks (xlator_t *this, fd_t *basefd, fd_t *oldfd,
oldfd, newfd, uuid_utoa (newfd->inode->gfid),
old_subvol->name, old_subvol->graph->id,
new_subvol->name, new_subvol->graph->id);
+ ret = -1;
goto out;
}
@@ -4300,10 +4307,11 @@ fuse_migrate_fd (xlator_t *this, fd_t *basefd, xlator_t *old_subvol,
"syncop_fsync failed (%s) on fd (%p)"
"(basefd:%p basefd-inode.gfid:%s) "
"(old-subvolume:%s-%d new-subvolume:%s-%d)",
- strerror (errno), oldfd, basefd,
+ strerror (-ret), oldfd, basefd,
uuid_utoa (basefd->inode->gfid),
old_subvol->name, old_subvol->graph->id,
new_subvol->name, new_subvol->graph->id);
+ ret = -1;
}
} else {
gf_log ("glusterfs-fuse", GF_LOG_WARNING,