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, 13 insertions, 5 deletions

diff --git a/xlators/mount/fuse/src/fuse-bridge.c b/xlators/mount/fuse/src/fuse-bridge.c
index ee12d869c..315259ece 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,