From 8d55c25f158921b508bff0e7f25158991913f922 Mon Sep 17 00:00:00 2001 From: Pranith Kumar K Date: Wed, 11 Dec 2013 09:33:53 +0530 Subject: 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 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 <<--- 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 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 Reviewed-on: http://review.gluster.org/6475 Tested-by: Gluster Build System Reviewed-by: Anand Avati --- api/src/glfs-resolve.c | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'api/src/glfs-resolve.c') diff --git a/api/src/glfs-resolve.c b/api/src/glfs-resolve.c index 4ca2eb6fc..c88330aa4 100644 --- a/api/src/glfs-resolve.c +++ b/api/src/glfs-resolve.c @@ -48,6 +48,7 @@ glfs_first_lookup_safe (xlator_t *subvol) loc.name = ""; ret = syncop_lookup (subvol, &loc, 0, 0, 0, 0); + DECODE_SYNCOP_ERR (ret); gf_log (subvol->name, GF_LOG_DEBUG, "first lookup complete %d", ret); @@ -98,6 +99,7 @@ glfs_refresh_inode_safe (xlator_t *subvol, inode_t *oldinode) return NULL; ret = syncop_lookup (subvol, &loc, 0, &iatt, 0, 0); + DECODE_SYNCOP_ERR (ret); if (ret) { gf_log (subvol->name, GF_LOG_WARNING, @@ -181,6 +183,7 @@ glfs_resolve_symlink (struct glfs *fs, xlator_t *subvol, inode_t *inode, loc.path = rpath; ret = syncop_readlink (subvol, &loc, &path, 4096); + DECODE_SYNCOP_ERR (ret); if (ret < 0) goto out; @@ -210,6 +213,7 @@ glfs_resolve_base (struct glfs *fs, xlator_t *subvol, inode_t *inode, goto out; ret = syncop_lookup (subvol, &loc, NULL, iatt, NULL, NULL); + DECODE_SYNCOP_ERR (ret); out: loc_wipe (&loc); @@ -268,6 +272,7 @@ glfs_resolve_component (struct glfs *fs, xlator_t *subvol, inode_t *parent, } ret = syncop_lookup (subvol, &loc, NULL, &ciatt, NULL, NULL); + DECODE_SYNCOP_ERR (ret); if (ret && reval) { inode_unref (loc.inode); loc.inode = inode_new (parent->table); @@ -292,6 +297,7 @@ glfs_resolve_component (struct glfs *fs, xlator_t *subvol, inode_t *parent, ret = syncop_lookup (subvol, &loc, xattr_req, &ciatt, NULL, NULL); + DECODE_SYNCOP_ERR (ret); } if (ret) goto out; @@ -515,6 +521,7 @@ glfs_migrate_fd_locks_safe (struct glfs *fs, xlator_t *oldsubvol, fd_t *oldfd, ret = syncop_fgetxattr (oldsubvol, oldfd, &lockinfo, GF_XATTR_LOCKINFO_KEY); + DECODE_SYNCOP_ERR (ret); if (ret < 0) { gf_log (fs->volname, GF_LOG_WARNING, "fgetxattr (%s) failed (%s) on graph %s (%d)", @@ -533,6 +540,7 @@ glfs_migrate_fd_locks_safe (struct glfs *fs, xlator_t *oldsubvol, fd_t *oldfd, } ret = syncop_fsetxattr (newsubvol, newfd, lockinfo, 0); + DECODE_SYNCOP_ERR (ret); if (ret < 0) { gf_log (fs->volname, GF_LOG_WARNING, "fsetxattr (%s) failed (%s) on graph %s (%d)", @@ -568,6 +576,7 @@ glfs_migrate_fd_safe (struct glfs *fs, xlator_t *newsubvol, fd_t *oldfd) if (!oldsubvol->switched) { ret = syncop_fsync (oldsubvol, oldfd, 0); + DECODE_SYNCOP_ERR (ret); if (ret) { gf_log (fs->volname, GF_LOG_WARNING, "fsync() failed (%s) on %s graph %s (%d)", @@ -614,6 +623,7 @@ glfs_migrate_fd_safe (struct glfs *fs, xlator_t *newsubvol, fd_t *oldfd) ret = syncop_open (newsubvol, &loc, oldfd->flags & ~(O_TRUNC|O_EXCL|O_CREAT), newfd); + DECODE_SYNCOP_ERR (ret); loc_wipe (&loc); if (ret) { -- cgit