cluster/afr: Fix race in lockinfo (f)getxattr (#2162)

* cluster/afr: Fix race in lockinfo (f)getxattr

A shared dictionary was updated outside the lock after having updated
the number of remaining answers. This means that one thread may be
processing the last answer and unwinding the request before another
thread completes updating the dict.

    Thread 1                           Thread 2

    LOCK()
    call_cnt-- (=1)
    UNLOCK()
                                       LOCK()
                                       call_cnt-- (=0)
                                       UNLOCK()
                                       update_dict(dict)
                                       if (call_cnt == 0) {
                                           STACK_UNWIND(dict);
                                       }
    update_dict(dict)
    if (call_cnt == 0) {
        STACK_UNWIND(dict);
    }

The updates from thread 1 are lost.

This patch also reduces the work done inside the locked region and
reduces code duplication.

Fixes: #2161
Change-Id: Idc0d34ab19ea6031de0641f7b05c624d90fac8fa
Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

1 files changed, 112 insertions, 142 deletions

diff --git a/xlators/cluster/afr/src/afr-inode-read.c b/xlators/cluster/afr/src/afr-inode-read.c
index 5baf9c42ee..8999792e3b 100644
--- a/xlators/cluster/afr/src/afr-inode-read.c
+++ b/xlators/cluster/afr/src/afr-inode-read.c
@@ -15,6 +15,8 @@
 #include <stdlib.h>
 #include <signal.h>
 
+#include <urcu/uatomic.h>
+
 #include <glusterfs/glusterfs.h>
 #include "afr.h"
 #include <glusterfs/dict.h>
@@ -866,188 +868,121 @@ afr_getxattr_quota_size_cbk(call_frame_t *frame, void *cookie, xlator_t *this,
     return 0;
 }
 
-int32_t
-afr_getxattr_lockinfo_cbk(call_frame_t *frame, void *cookie, xlator_t *this,
-                          int32_t op_ret, int32_t op_errno, dict_t *dict,
-                          dict_t *xdata)
+static int32_t
+afr_update_local_dicts(call_frame_t *frame, dict_t *dict, dict_t *xdata)
 {
-    int call_cnt = 0, len = 0;
-    char *lockinfo_buf = NULL;
-    dict_t *lockinfo = NULL, *newdict = NULL;
-    afr_local_t *local = NULL;
+    afr_local_t *local;
+    dict_t *local_dict;
+    dict_t *local_xdata;
+    int32_t ret;
 
-    LOCK(&frame->lock);
-    {
-        local = frame->local;
+    local = frame->local;
+    local_dict = NULL;
+    local_xdata = NULL;
 
-        call_cnt = --local->call_count;
+    ret = -ENOMEM;
 
-        if ((op_ret < 0) || (!dict && !xdata)) {
-            goto unlock;
-        }
-
-        if (xdata) {
-            if (!local->xdata_rsp) {
-                local->xdata_rsp = dict_new();
-                if (!local->xdata_rsp) {
-                    local->op_ret = -1;
-                    local->op_errno = ENOMEM;
-                    goto unlock;
-                }
-            }
+    if ((dict != NULL) && (local->dict == NULL)) {
+        local_dict = dict_new();
+        if (local_dict == NULL) {
+            goto done;
         }
+    }
 
-        if (!dict) {
-            goto unlock;
+    if ((xdata != NULL) && (local->xdata_rsp == NULL)) {
+        local_xdata = dict_new();
+        if (local_xdata == NULL) {
+            goto done;
         }
+    }
 
-        op_ret = dict_get_ptr_and_len(dict, GF_XATTR_LOCKINFO_KEY,
-                                      (void **)&lockinfo_buf, &len);
+    if ((local_dict != NULL) || (local_xdata != NULL)) {
+        /* TODO: Maybe it would be better to preallocate both dicts before
+         *       sending the requests. This way we don't need to use a LOCK()
+         *       here. */
+        LOCK(&frame->lock);
 
-        if (!lockinfo_buf) {
-            goto unlock;
+        if ((local_dict != NULL) && (local->dict == NULL)) {
+            local->dict = local_dict;
+            local_dict = NULL;
         }
 
-        if (!local->dict) {
-            local->dict = dict_new();
-            if (!local->dict) {
-                local->op_ret = -1;
-                local->op_errno = ENOMEM;
-                goto unlock;
-            }
+        if ((local_xdata != NULL) && (local->xdata_rsp == NULL)) {
+            local->xdata_rsp = local_xdata;
+            local_xdata = NULL;
         }
-    }
-unlock:
-    UNLOCK(&frame->lock);
 
-    if (lockinfo_buf != NULL) {
-        lockinfo = dict_new();
-        if (lockinfo == NULL) {
-            local->op_ret = -1;
-            local->op_errno = ENOMEM;
-        } else {
-            op_ret = dict_unserialize(lockinfo_buf, len, &lockinfo);
-
-            if (lockinfo && local->dict) {
-                dict_copy(lockinfo, local->dict);
-            }
-        }
-    }
-
-    if (xdata && local->xdata_rsp) {
-        dict_copy(xdata, local->xdata_rsp);
+        UNLOCK(&frame->lock);
     }
 
-    if (!call_cnt) {
-        newdict = dict_new();
-        if (!newdict) {
-            local->op_ret = -1;
-            local->op_errno = ENOMEM;
-            goto unwind;
+    if (dict != NULL) {
+        if (dict_copy(dict, local->dict) < 0) {
+            goto done;
         }
+    }
 
-        op_ret = dict_allocate_and_serialize(
-            local->dict, (char **)&lockinfo_buf, (unsigned int *)&len);
-        if (op_ret != 0) {
-            local->op_ret = -1;
-            goto unwind;
+    if (xdata != NULL) {
+        if (dict_copy(xdata, local->xdata_rsp) < 0) {
+            goto done;
         }
+    }
 
-        op_ret = dict_set_dynptr(newdict, GF_XATTR_LOCKINFO_KEY,
-                                 (void *)lockinfo_buf, len);
-        if (op_ret < 0) {
-            local->op_ret = -1;
-            local->op_errno = -op_ret;
-            goto unwind;
-        }
+    ret = 0;
 
-    unwind:
-        AFR_STACK_UNWIND(getxattr, frame, op_ret, op_errno, newdict,
-                         local->xdata_rsp);
+done:
+    if (local_dict != NULL) {
+        dict_unref(local_dict);
     }
 
-    dict_unref(lockinfo);
+    if (local_xdata != NULL) {
+        dict_unref(local_xdata);
+    }
 
-    return 0;
+    return ret;
 }
 
-int32_t
-afr_fgetxattr_lockinfo_cbk(call_frame_t *frame, void *cookie, xlator_t *this,
-                           int32_t op_ret, int32_t op_errno, dict_t *dict,
-                           dict_t *xdata)
+static void
+afr_getxattr_lockinfo_cbk_common(call_frame_t *frame, int32_t op_ret,
+                                 int32_t op_errno, dict_t *dict, dict_t *xdata,
+                                 bool is_fgetxattr)
 {
-    int call_cnt = 0, len = 0;
+    int len = 0;
     char *lockinfo_buf = NULL;
     dict_t *lockinfo = NULL, *newdict = NULL;
     afr_local_t *local = NULL;
 
-    LOCK(&frame->lock);
-    {
-        local = frame->local;
-
-        call_cnt = --local->call_count;
-
-        if ((op_ret < 0) || (!dict && !xdata)) {
-            goto unlock;
-        }
-
-        if (xdata) {
-            if (!local->xdata_rsp) {
-                local->xdata_rsp = dict_new();
-                if (!local->xdata_rsp) {
-                    local->op_ret = -1;
-                    local->op_errno = ENOMEM;
-                    goto unlock;
-                }
-            }
-        }
-
-        if (!dict) {
-            goto unlock;
-        }
+    local = frame->local;
 
+    if ((op_ret >= 0) && (dict != NULL)) {
         op_ret = dict_get_ptr_and_len(dict, GF_XATTR_LOCKINFO_KEY,
                                       (void **)&lockinfo_buf, &len);
-
-        if (!lockinfo_buf) {
-            goto unlock;
-        }
-
-        if (!local->dict) {
-            local->dict = dict_new();
-            if (!local->dict) {
-                local->op_ret = -1;
-                local->op_errno = ENOMEM;
-                goto unlock;
+        if (lockinfo_buf != NULL) {
+            lockinfo = dict_new();
+            if (lockinfo == NULL) {
+                op_ret = -1;
+            } else {
+                op_ret = dict_unserialize(lockinfo_buf, len, &lockinfo);
             }
         }
     }
-unlock:
-    UNLOCK(&frame->lock);
 
-    if (lockinfo_buf != NULL) {
-        lockinfo = dict_new();
-        if (lockinfo == NULL) {
-            local->op_ret = -1;
-            local->op_errno = ENOMEM;
-        } else {
-            op_ret = dict_unserialize(lockinfo_buf, len, &lockinfo);
-
-            if (lockinfo && local->dict) {
-                dict_copy(lockinfo, local->dict);
-            }
+    if ((op_ret >= 0) && ((lockinfo != NULL) || (xdata != NULL))) {
+        op_ret = afr_update_local_dicts(frame, lockinfo, xdata);
+        if (lockinfo != NULL) {
+            dict_unref(lockinfo);
         }
     }
 
-    if (xdata && local->xdata_rsp) {
-        dict_copy(xdata, local->xdata_rsp);
+    if (op_ret < 0) {
+        local->op_ret = -1;
+        local->op_errno = ENOMEM;
     }
 
-    if (!call_cnt) {
+    if (uatomic_sub_return(&local->call_count, 1) == 0) {
         newdict = dict_new();
         if (!newdict) {
             local->op_ret = -1;
-            local->op_errno = ENOMEM;
+            local->op_errno = op_errno = ENOMEM;
             goto unwind;
         }
 
@@ -1055,23 +990,58 @@ unlock:
             local->dict, (char **)&lockinfo_buf, (unsigned int *)&len);
         if (op_ret != 0) {
             local->op_ret = -1;
+            local->op_errno = op_errno = ENOMEM;
             goto unwind;
         }
 
         op_ret = dict_set_dynptr(newdict, GF_XATTR_LOCKINFO_KEY,
                                  (void *)lockinfo_buf, len);
         if (op_ret < 0) {
-            local->op_ret = -1;
-            local->op_errno = -op_ret;
+            GF_FREE(lockinfo_buf);
+            local->op_ret = op_ret = -1;
+            local->op_errno = op_errno = -op_ret;
             goto unwind;
         }
 
     unwind:
-        AFR_STACK_UNWIND(fgetxattr, frame, op_ret, op_errno, newdict,
-                         local->xdata_rsp);
+        /* TODO: These unwinds use op_ret and op_errno instead of local->op_ret
+         *       and local->op_errno. This doesn't seem right because any
+         *       failure during processing of each answer could be silently
+         *       ignored. This is kept this was the old behavior and because
+         *       local->op_ret is initialized as -1 and local->op_errno is
+         *       initialized as EUCLEAN, which makes these values useless. */
+        if (is_fgetxattr) {
+            AFR_STACK_UNWIND(fgetxattr, frame, op_ret, op_errno, newdict,
+                             local->xdata_rsp);
+        } else {
+            AFR_STACK_UNWIND(getxattr, frame, op_ret, op_errno, newdict,
+                             local->xdata_rsp);
+        }
+
+        if (newdict != NULL) {
+            dict_unref(newdict);
+        }
     }
+}
+
+static int32_t
+afr_getxattr_lockinfo_cbk(call_frame_t *frame, void *cookie, xlator_t *this,
+                          int32_t op_ret, int32_t op_errno, dict_t *dict,
+                          dict_t *xdata)
+{
+    afr_getxattr_lockinfo_cbk_common(frame, op_ret, op_errno, dict, xdata,
+                                     false);
 
-    dict_unref(lockinfo);
+    return 0;
+}
+
+static int32_t
+afr_fgetxattr_lockinfo_cbk(call_frame_t *frame, void *cookie, xlator_t *this,
+                           int32_t op_ret, int32_t op_errno, dict_t *dict,
+                           dict_t *xdata)
+{
+    afr_getxattr_lockinfo_cbk_common(frame, op_ret, op_errno, dict, xdata,
+                                     true);
 
     return 0;
 }