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>

1 files changed, 36 insertions, 0 deletions

diff --git a/api/src/glfs-fops.c b/api/src/glfs-fops.c
index 5ea54567d74..67d5616fe21 100644
--- a/api/src/glfs-fops.c
+++ b/api/src/glfs-fops.c
@@ -124,6 +124,7 @@ retry:
 	}
 
 	ret = syncop_open (subvol, &loc, flags, glfd->fd);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 out:
@@ -169,6 +170,7 @@ glfs_close (struct glfs_fd *glfd)
 	}
 
 	ret = syncop_flush (subvol, fd);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	fs = glfd->fs;
 	glfs_fd_destroy (glfd);
@@ -273,6 +275,7 @@ glfs_fstat (struct glfs_fd *glfd, struct stat *stat)
 	}
 
 	ret = syncop_fstat (subvol, fd, &iatt);
+        DECODE_SYNCOP_ERR (ret);
 
 	if (ret == 0 && stat)
 		glfs_iatt_to_stat (glfd->fs, &iatt, stat);
@@ -392,9 +395,11 @@ retry:
 
 	if (ret == 0) {
 		ret = syncop_open (subvol, &loc, flags, glfd->fd);
+                DECODE_SYNCOP_ERR (ret);
 	} else {
 		ret = syncop_create (subvol, &loc, flags, mode, glfd->fd,
 				     xattr_req, &iatt);
+                DECODE_SYNCOP_ERR (ret);
 	}
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
@@ -484,6 +489,7 @@ glfs_preadv (struct glfs_fd *glfd, const struct iovec *iovec, int iovcnt,
 	size = iov_length (iovec, iovcnt);
 
 	ret = syncop_readv (subvol, fd, size, offset, 0, &iov, &cnt, &iobref);
+        DECODE_SYNCOP_ERR (ret);
 	if (ret <= 0)
 		goto out;
 
@@ -833,6 +839,7 @@ glfs_pwritev (struct glfs_fd *glfd, const struct iovec *iovec, int iovcnt,
 	iov.iov_len = size;
 
 	ret = syncop_writev (subvol, fd, &iov, 1, offset, iobref, flags);
+        DECODE_SYNCOP_ERR (ret);
 
 	iobuf_unref (iobuf);
 	iobref_unref (iobref);
@@ -1005,6 +1012,7 @@ glfs_fsync (struct glfs_fd *glfd)
 	}
 
 	ret = syncop_fsync (subvol, fd, 0);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	if (fd)
 		fd_unref (fd);
@@ -1079,6 +1087,7 @@ glfs_fdatasync (struct glfs_fd *glfd)
 	}
 
 	ret = syncop_fsync (subvol, fd, 1);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	if (fd)
 		fd_unref (fd);
@@ -1120,6 +1129,7 @@ glfs_ftruncate (struct glfs_fd *glfd, off_t offset)
 	}
 
 	ret = syncop_ftruncate (subvol, fd, offset);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	if (fd)
 		fd_unref (fd);
@@ -1188,6 +1198,7 @@ retry:
 		goto out;
 
 	ret = syncop_access (subvol, &loc, mode);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 out:
@@ -1263,6 +1274,7 @@ retry:
 	}
 
 	ret = syncop_symlink (subvol, &loc, data, xattr_req, &iatt);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -1313,6 +1325,7 @@ retry:
 	}
 
 	ret = syncop_readlink (subvol, &loc, &linkval, bufsiz);
+        DECODE_SYNCOP_ERR (ret);
 	if (ret > 0) {
 		memcpy (buf, linkval, ret);
 		GF_FREE (linkval);
@@ -1392,6 +1405,7 @@ retry:
 	}
 
 	ret = syncop_mknod (subvol, &loc, mode, dev, xattr_req, &iatt);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -1473,6 +1487,7 @@ retry:
 	}
 
 	ret = syncop_mkdir (subvol, &loc, mode, xattr_req, &iatt);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -1522,6 +1537,7 @@ retry:
 	}
 
 	ret = syncop_unlink (subvol, &loc);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -1568,6 +1584,7 @@ retry:
 	}
 
 	ret = syncop_rmdir (subvol, &loc, 0);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -1631,6 +1648,7 @@ retrynew:
 	/* TODO: check if new or old is a prefix of the other, and fail EINVAL */
 
 	ret = syncop_rename (subvol, &oldloc, &newloc);
+        DECODE_SYNCOP_ERR (ret);
 
 	if (ret == -1 && errno == ESTALE) {
 		if (reval < DEFAULT_REVAL_COUNT) {
@@ -1708,6 +1726,7 @@ retrynew:
         newloc.inode = inode_ref (oldloc.inode);
 
 	ret = syncop_link (subvol, &oldloc, &newloc);
+        DECODE_SYNCOP_ERR (ret);
 
 	if (ret == -1 && errno == ESTALE) {
 		loc_wipe (&oldloc);
@@ -1782,6 +1801,7 @@ retry:
 	}
 
 	ret = syncop_opendir (subvol, &loc, glfd->fd);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 out:
@@ -1972,6 +1992,7 @@ glfd_entry_refresh (struct glfs_fd *glfd, int plus)
 	else
 		ret = syncop_readdir (subvol, fd, 131072, glfd->offset,
 				      &entries);
+        DECODE_SYNCOP_ERR (ret);
 	if (ret >= 0) {
 		if (plus)
 			gf_link_inodes_from_dirent (THIS, fd->inode, &entries);
@@ -2150,6 +2171,7 @@ retry:
 		goto out;
 
 	ret = syncop_statfs (subvol, &loc, buf);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 out:
@@ -2191,6 +2213,7 @@ retry:
 		goto out;
 
 	ret = syncop_setattr (subvol, &loc, iatt, valid, 0, 0);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 out:
@@ -2226,6 +2249,7 @@ glfs_fsetattr (struct glfs_fd *glfd, struct iatt *iatt, int valid)
 	}
 
 	ret = syncop_fsetattr (subvol, fd, iatt, valid, 0, 0);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	if (fd)
 		fd_unref (fd);
@@ -2442,6 +2466,7 @@ retry:
 		goto out;
 
 	ret = syncop_getxattr (subvol, &loc, &xattr, name);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -2500,6 +2525,7 @@ glfs_fgetxattr (struct glfs_fd *glfd, const char *name, void *value,
 	}
 
 	ret = syncop_fgetxattr (subvol, fd, &xattr, name);
+        DECODE_SYNCOP_ERR (ret);
 	if (ret)
 		goto out;
 
@@ -2570,6 +2596,7 @@ retry:
 		goto out;
 
 	ret = syncop_getxattr (subvol, &loc, &xattr, NULL);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -2625,6 +2652,7 @@ glfs_flistxattr (struct glfs_fd *glfd, void *value, size_t size)
 	}
 
 	ret = syncop_fgetxattr (subvol, fd, &xattr, NULL);
+        DECODE_SYNCOP_ERR (ret);
 	if (ret)
 		goto out;
 
@@ -2697,6 +2725,7 @@ retry:
 	}
 
 	ret = syncop_setxattr (subvol, &loc, xattr, flags);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -2760,6 +2789,7 @@ glfs_fsetxattr (struct glfs_fd *glfd, const char *name, const void *value,
 	}
 
 	ret = syncop_fsetxattr (subvol, fd, xattr, flags);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	if (xattr)
 		dict_unref (xattr);
@@ -2803,6 +2833,7 @@ retry:
 		goto out;
 
 	ret = syncop_removexattr (subvol, &loc, name);
+        DECODE_SYNCOP_ERR (ret);
 
 	ESTALE_RETRY (ret, errno, reval, &loc, retry);
 
@@ -2853,6 +2884,7 @@ glfs_fremovexattr (struct glfs_fd *glfd, const char *name)
 	}
 
 	ret = syncop_fremovexattr (subvol, fd, name);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	if (fd)
 		fd_unref (fd);
@@ -2887,6 +2919,7 @@ glfs_fallocate (struct glfs_fd *glfd, int keep_size, off_t offset, size_t len)
 	}
 
 	ret = syncop_fallocate (subvol, fd, keep_size, offset, len);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	if (fd)
 		fd_unref(fd);
@@ -2921,6 +2954,7 @@ glfs_discard (struct glfs_fd *glfd, off_t offset, size_t len)
 	}
 
 	ret = syncop_discard (subvol, fd, offset, len);
+        DECODE_SYNCOP_ERR (ret);
 out:
 	if (fd)
 		fd_unref(fd);
@@ -2952,6 +2986,7 @@ glfs_zerofill (struct glfs_fd *glfd, off_t offset, off_t len)
         }
 
         ret = syncop_zerofill (subvol, fd, offset, len);
+        DECODE_SYNCOP_ERR (ret);
 out:
         if (fd)
                 fd_unref(fd);
@@ -3200,6 +3235,7 @@ glfs_posix_lock (struct glfs_fd *glfd, int cmd, struct flock *flock)
 	gf_flock_from_flock (&gf_flock, flock);
 	gf_flock_from_flock (&saved_flock, flock);
 	ret = syncop_lk (subvol, fd, cmd, &gf_flock);
+        DECODE_SYNCOP_ERR (ret);
 	gf_flock_to_flock (&gf_flock, flock);
 
 	if (ret == 0 && (cmd == F_SETLK || cmd == F_SETLKW))