summaryrefslogtreecommitdiffstats
path: root/libglusterfs
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 /libglusterfs
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 'libglusterfs')
-rw-r--r--libglusterfs/src/syncop.c117
1 files changed, 78 insertions, 39 deletions
diff --git a/libglusterfs/src/syncop.c b/libglusterfs/src/syncop.c
index 1f36e57766c..efcd9c5f3a9 100644
--- a/libglusterfs/src/syncop.c
+++ b/libglusterfs/src/syncop.c
@@ -983,7 +983,8 @@ syncop_lookup (xlator_t *subvol, loc_t *loc, dict_t *xdata_req,
else if (args.xdata)
dict_unref (args.xdata);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1059,7 +1060,8 @@ syncop_readdirp (xlator_t *subvol,
list_splice_init (&args.entries.list, &entries->list);
/* TODO: need to free all the 'args.entries' in 'else' case */
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1118,7 +1120,8 @@ syncop_readdir (xlator_t *subvol,
list_splice_init (&args.entries.list, &entries->list);
/* TODO: need to free all the 'args.entries' in 'else' case */
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1153,7 +1156,8 @@ syncop_opendir (xlator_t *subvol,
SYNCOP (subvol, (&args), syncop_opendir_cbk, subvol->fops->opendir,
loc, fd, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1182,7 +1186,8 @@ syncop_fsyncdir (xlator_t *subvol, fd_t *fd, int datasync)
SYNCOP (subvol, (&args), syncop_fsyncdir_cbk, subvol->fops->fsyncdir,
fd, datasync, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1210,7 +1215,8 @@ syncop_removexattr (xlator_t *subvol, loc_t *loc, const char *name)
SYNCOP (subvol, (&args), syncop_removexattr_cbk, subvol->fops->removexattr,
loc, name, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1238,7 +1244,8 @@ syncop_fremovexattr (xlator_t *subvol, fd_t *fd, const char *name)
SYNCOP (subvol, (&args), syncop_fremovexattr_cbk,
subvol->fops->fremovexattr, fd, name, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1267,7 +1274,8 @@ syncop_setxattr (xlator_t *subvol, loc_t *loc, dict_t *dict, int32_t flags)
SYNCOP (subvol, (&args), syncop_setxattr_cbk, subvol->fops->setxattr,
loc, dict, flags, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1296,7 +1304,8 @@ syncop_fsetxattr (xlator_t *subvol, fd_t *fd, dict_t *dict, int32_t flags)
SYNCOP (subvol, (&args), syncop_fsetxattr_cbk, subvol->fops->fsetxattr,
fd, dict, flags, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1331,7 +1340,8 @@ syncop_listxattr (xlator_t *subvol, loc_t *loc, dict_t **dict)
else if (args.xattr)
dict_unref (args.xattr);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1348,7 +1358,8 @@ syncop_getxattr (xlator_t *subvol, loc_t *loc, dict_t **dict, const char *key)
else if (args.xattr)
dict_unref (args.xattr);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1365,7 +1376,8 @@ syncop_fgetxattr (xlator_t *subvol, fd_t *fd, dict_t **dict, const char *key)
else if (args.xattr)
dict_unref (args.xattr);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1404,7 +1416,8 @@ syncop_statfs (xlator_t *subvol, loc_t *loc, struct statvfs *buf)
if (buf)
*buf = args.statvfs_buf;
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1445,7 +1458,8 @@ syncop_setattr (xlator_t *subvol, loc_t *loc, struct iatt *iatt, int valid,
if (postop)
*postop = args.iatt2;
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1464,7 +1478,8 @@ syncop_fsetattr (xlator_t *subvol, fd_t *fd, struct iatt *iatt, int valid,
if (postop)
*postop = args.iatt2;
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1493,7 +1508,8 @@ syncop_open (xlator_t *subvol, loc_t *loc, int32_t flags, fd_t *fd)
SYNCOP (subvol, (&args), syncop_open_cbk, subvol->fops->open,
loc, flags, fd, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1555,7 +1571,8 @@ syncop_readv (xlator_t *subvol, fd_t *fd, size_t size, off_t off,
iobref_unref (args.iobref);
out:
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1588,7 +1605,8 @@ syncop_writev (xlator_t *subvol, fd_t *fd, const struct iovec *vector,
fd, (struct iovec *) vector, count, offset, flags, iobref,
NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1604,7 +1622,8 @@ int syncop_write (xlator_t *subvol, fd_t *fd, const char *buf, int size,
SYNCOP (subvol, (&args), syncop_writev_cbk, subvol->fops->writev,
fd, &vec, 1, offset, flags, iobref, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1647,10 +1666,11 @@ syncop_create (xlator_t *subvol, loc_t *loc, int32_t flags, mode_t mode,
SYNCOP (subvol, (&args), syncop_create_cbk, subvol->fops->create,
loc, flags, mode, 0, fd, xdata);
- errno = args.op_errno;
if (iatt)
*iatt = args.iatt1;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1680,7 +1700,8 @@ syncop_unlink (xlator_t *subvol, loc_t *loc)
SYNCOP (subvol, (&args), syncop_unlink_cbk, subvol->fops->unlink, loc,
0, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1709,7 +1730,8 @@ syncop_rmdir (xlator_t *subvol, loc_t *loc, int flags)
SYNCOP (subvol, (&args), syncop_rmdir_cbk, subvol->fops->rmdir, loc,
flags, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1741,7 +1763,8 @@ syncop_link (xlator_t *subvol, loc_t *oldloc, loc_t *newloc)
SYNCOP (subvol, (&args), syncop_link_cbk, subvol->fops->link,
oldloc, newloc, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1775,7 +1798,8 @@ syncop_rename (xlator_t *subvol, loc_t *oldloc, loc_t *newloc)
SYNCOP (subvol, (&args), syncop_rename_cbk, subvol->fops->rename,
oldloc, newloc, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1806,7 +1830,8 @@ syncop_ftruncate (xlator_t *subvol, fd_t *fd, off_t offset)
SYNCOP (subvol, (&args), syncop_ftruncate_cbk, subvol->fops->ftruncate,
fd, offset, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1818,7 +1843,8 @@ syncop_truncate (xlator_t *subvol, loc_t *loc, off_t offset)
SYNCOP (subvol, (&args), syncop_ftruncate_cbk, subvol->fops->truncate,
loc, offset, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1848,7 +1874,8 @@ syncop_fsync (xlator_t *subvol, fd_t *fd, int dataonly)
SYNCOP (subvol, (&args), syncop_fsync_cbk, subvol->fops->fsync,
fd, dataonly, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1879,7 +1906,8 @@ syncop_flush (xlator_t *subvol, fd_t *fd)
SYNCOP (subvol, (&args), syncop_flush_cbk, subvol->fops->flush,
fd, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1914,7 +1942,8 @@ syncop_fstat (xlator_t *subvol, fd_t *fd, struct iatt *stbuf)
if (stbuf)
*stbuf = args.iatt1;
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1930,7 +1959,8 @@ syncop_stat (xlator_t *subvol, loc_t *loc, struct iatt *stbuf)
if (stbuf)
*stbuf = args.iatt1;
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -1964,10 +1994,11 @@ syncop_symlink (xlator_t *subvol, loc_t *loc, const char *newpath, dict_t *dict,
SYNCOP (subvol, (&args), syncop_symlink_cbk, subvol->fops->symlink,
newpath, loc, 0, dict);
- errno = args.op_errno;
if (iatt)
*iatt = args.iatt1;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -2004,7 +2035,8 @@ syncop_readlink (xlator_t *subvol, loc_t *loc, char **buffer, size_t size)
*buffer = args.buffer;
else GF_FREE (args.buffer);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -2038,10 +2070,11 @@ syncop_mknod (xlator_t *subvol, loc_t *loc, mode_t mode, dev_t rdev,
SYNCOP (subvol, (&args), syncop_mknod_cbk, subvol->fops->mknod,
loc, mode, rdev, 0, dict);
- errno = args.op_errno;
if (iatt)
*iatt = args.iatt1;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -2077,10 +2110,11 @@ syncop_mkdir (xlator_t *subvol, loc_t *loc, mode_t mode, dict_t *dict,
SYNCOP (subvol, (&args), syncop_mkdir_cbk, subvol->fops->mkdir,
loc, mode, 0, dict);
- errno = args.op_errno;
if (iatt)
*iatt = args.iatt1;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -2108,7 +2142,8 @@ syncop_access (xlator_t *subvol, loc_t *loc, int32_t mask)
SYNCOP (subvol, (&args), syncop_access_cbk, subvol->fops->access,
loc, mask, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -2139,7 +2174,8 @@ syncop_fallocate(xlator_t *subvol, fd_t *fd, int32_t keep_size, off_t offset,
SYNCOP (subvol, (&args), syncop_fallocate_cbk, subvol->fops->fallocate,
fd, keep_size, offset, len, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -2169,7 +2205,8 @@ syncop_discard(xlator_t *subvol, fd_t *fd, off_t offset, size_t len)
SYNCOP (subvol, (&args), syncop_discard_cbk, subvol->fops->discard,
fd, offset, len, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -2198,7 +2235,8 @@ syncop_zerofill(xlator_t *subvol, fd_t *fd, off_t offset, off_t len)
SYNCOP (subvol, (&args), syncop_zerofill_cbk, subvol->fops->zerofill,
fd, offset, len, NULL);
- errno = args.op_errno;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}
@@ -2230,8 +2268,9 @@ syncop_lk (xlator_t *subvol, fd_t *fd, int cmd, struct gf_flock *flock)
SYNCOP (subvol, (&args), syncop_lk_cbk, subvol->fops->lk,
fd, cmd, flock, NULL);
- errno = args.op_errno;
*flock = args.flock;
+ if (args.op_ret < 0)
+ return -args.op_errno;
return args.op_ret;
}