afr: thin-arbiter read txn fixes

- Fixes afr_ta_read_txn() to handle inode refresh failures.
code-path.
- Fixes a double free issue of dict.

Note: This patch address post-merge review comments for commit
69532c141be160b3fea03c1579ae4ac13018dcdf

fixes: bz#1686398
Change-Id: Id5299b45b68569d47df6b73755918237a1592cb4
Signed-off-by: Ravishankar N <ravishankar@redhat.com>

4 files changed, 77 insertions, 22 deletions

diff --git a/tests/bugs/replicate/ta-inode-refresh-read.t b/tests/bugs/replicate/ta-inode-refresh-read.t
new file mode 100644
index 00000000000..6dd6ff7f163
--- /dev/null
+++ b/tests/bugs/replicate/ta-inode-refresh-read.t
@@ -0,0 +1,40 @@
+#!/bin/bash
+
+# Test read transaction inode refresh logic for thin-arbiter.
+
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+. $(dirname $0)/../../thin-arbiter.rc
+cleanup;
+TEST ta_create_brick_and_volfile brick0
+TEST ta_create_brick_and_volfile brick1
+TEST ta_create_ta_and_volfile ta
+TEST ta_start_brick_process brick0
+TEST ta_start_brick_process brick1
+TEST ta_start_ta_process ta
+
+TEST ta_create_mount_volfile brick0 brick1 ta
+# Set afr xlator options to choose brick0 as read-subvol.
+sed -i '/iam-self-heal-daemon/a \     option read-subvolume-index 0' $B0/mount.vol
+TEST [ $? -eq 0 ]
+sed -i '/iam-self-heal-daemon/a \     option choose-local false' $B0/mount.vol
+TEST [ $? -eq 0 ]
+
+TEST ta_start_mount_process $M0
+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "1" ta_up_status $V0 $M0 0
+EXPECT_WITHIN $PROCESS_UP_TIMEOUT "trusted.afr.patchy-ta-2" ls $B0/ta
+
+TEST touch $M0/FILE
+TEST ls $B0/brick0/FILE
+TEST ls $B0/brick1/FILE
+TEST ! ls $B0/ta/FILE
+TEST setfattr -n user.name -v ravi $M0/FILE
+
+# Remove gfid hardlink from brick0 which is the read-subvol for FILE.
+# This triggers inode refresh up on a getfattr and eventually calls
+# afr_ta_read_txn(). Without this patch, afr_ta_read_txn() will again query
+# brick0 causing getfattr to fail.
+TEST rm -f $(gf_get_gfid_backend_file_path $B0/brick0 FILE)
+TEST getfattr -n user.name $M0/FILE
+
+cleanup;
diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index 47854445716..002f97276f9 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -1269,20 +1269,25 @@ afr_inode_refresh_done(call_frame_t *frame, xlator_t *this, int error)
     success_replies = alloca0(priv->child_count);
     afr_fill_success_replies(local, priv, success_replies);
 
-    if (!afr_has_quorum(success_replies, this, frame)) {
-        error = afr_final_errno(frame->local, this->private);
-        if (!error)
-            error = afr_quorum_errno(priv);
-        goto refresh_done;
-    }
-
     if (priv->thin_arbiter_count && local->is_read_txn &&
         AFR_COUNT(success_replies, priv->child_count) != priv->child_count) {
         /* We need to query the good bricks and/or thin-arbiter.*/
+        if (success_replies[0]) {
+            local->read_txn_query_child = AFR_CHILD_ZERO;
+        } else if (success_replies[1]) {
+            local->read_txn_query_child = AFR_CHILD_ONE;
+        }
         error = EINVAL;
         goto refresh_done;
     }
 
+    if (!afr_has_quorum(success_replies, this, frame)) {
+        error = afr_final_errno(frame->local, this->private);
+        if (!error)
+            error = afr_quorum_errno(priv);
+        goto refresh_done;
+    }
+
     ret = afr_replies_interpret(frame, this, local->refreshinode, &start_heal);
 
     if (ret && afr_selfheal_enabled(this) && start_heal) {
@@ -5696,6 +5701,7 @@ afr_local_init(afr_local_t *local, afr_private_t *priv, int32_t *op_errno)
     if (priv->thin_arbiter_count) {
         local->ta_child_up = priv->ta_child_up;
         local->ta_failed_subvol = AFR_CHILD_UNKNOWN;
+        local->read_txn_query_child = AFR_CHILD_UNKNOWN;
     }
     local->is_new_entry = _gf_false;
 
diff --git a/xlators/cluster/afr/src/afr-read-txn.c b/xlators/cluster/afr/src/afr-read-txn.c
index 9a91f2e56fc..7e258049005 100644
--- a/xlators/cluster/afr/src/afr-read-txn.c
+++ b/xlators/cluster/afr/src/afr-read-txn.c
@@ -114,7 +114,7 @@ afr_ta_read_txn(void *opaque)
     call_frame_t *frame = NULL;
     xlator_t *this = NULL;
     int read_subvol = -1;
-    int up_child = AFR_CHILD_UNKNOWN;
+    int query_child = AFR_CHILD_UNKNOWN;
     int possible_bad_child = AFR_CHILD_UNKNOWN;
     int ret = 0;
     int op_errno = ENOMEM;
@@ -134,18 +134,18 @@ afr_ta_read_txn(void *opaque)
     this = frame->this;
     local = frame->local;
     priv = this->private;
+    query_child = local->read_txn_query_child;
 
-    if (local->child_up[AFR_CHILD_ZERO]) {
-        up_child = AFR_CHILD_ZERO;
+    if (query_child == AFR_CHILD_ZERO) {
         possible_bad_child = AFR_CHILD_ONE;
-    } else if (local->child_up[AFR_CHILD_ONE]) {
-        up_child = AFR_CHILD_ONE;
+    } else if (query_child == AFR_CHILD_ONE) {
         possible_bad_child = AFR_CHILD_ZERO;
+    } else {
+        /*read_txn_query_child is AFR_CHILD_UNKNOWN*/
+        goto out;
     }
 
-    GF_ASSERT(up_child != AFR_CHILD_UNKNOWN);
-
-    /* Query the up_child to see if it blames the down one. */
+    /* Ask the query_child to see if it blames the possibly bad one. */
     xdata_req = dict_new();
     if (!xdata_req)
         goto out;
@@ -159,29 +159,32 @@ afr_ta_read_txn(void *opaque)
         goto out;
 
     if (local->fd) {
-        ret = syncop_fxattrop(priv->children[up_child], local->fd,
+        ret = syncop_fxattrop(priv->children[query_child], local->fd,
                               GF_XATTROP_ADD_ARRAY, xdata_req, NULL, &xdata_rsp,
                               NULL);
     } else {
-        ret = syncop_xattrop(priv->children[up_child], &local->loc,
+        ret = syncop_xattrop(priv->children[query_child], &local->loc,
                              GF_XATTROP_ADD_ARRAY, xdata_req, NULL, &xdata_rsp,
                              NULL);
     }
     if (ret || !xdata_rsp) {
         gf_msg(this->name, GF_LOG_ERROR, -ret, AFR_MSG_THIN_ARB,
                "Failed xattrop for gfid %s on %s",
-               uuid_utoa(local->inode->gfid), priv->children[up_child]->name);
+               uuid_utoa(local->inode->gfid),
+               priv->children[query_child]->name);
         op_errno = -ret;
         goto out;
     }
 
     if (afr_ta_dict_contains_pending_xattr(xdata_rsp, priv,
                                            possible_bad_child)) {
-        read_subvol = up_child;
+        read_subvol = query_child;
         goto out;
     }
     dict_unref(xdata_rsp);
-    /* Query thin-arbiter to see if it blames any data brick. */
+    xdata_rsp = NULL;
+
+    /* It doesn't. So query thin-arbiter to see if it blames any data brick. */
     ret = afr_fill_ta_loc(this, &loc);
     if (ret) {
         gf_msg(this->name, GF_LOG_ERROR, -ret, AFR_MSG_THIN_ARB,
@@ -211,8 +214,8 @@ afr_ta_read_txn(void *opaque)
         goto unlock;
     }
 
-    if (!afr_ta_dict_contains_pending_xattr(xdata_rsp, priv, up_child)) {
-        read_subvol = up_child;
+    if (!afr_ta_dict_contains_pending_xattr(xdata_rsp, priv, query_child)) {
+        read_subvol = query_child;
     } else {
         gf_msg(this->name, GF_LOG_ERROR, EIO, AFR_MSG_THIN_ARB,
                "Failing read for gfid %s since good brick %s is down",
@@ -450,6 +453,11 @@ afr_read_txn(call_frame_t *frame, xlator_t *this, inode_t *inode,
 
     if (priv->thin_arbiter_count &&
         AFR_COUNT(local->child_up, priv->child_count) != priv->child_count) {
+        if (local->child_up[0]) {
+            local->read_txn_query_child = AFR_CHILD_ZERO;
+        } else if (local->child_up[1]) {
+            local->read_txn_query_child = AFR_CHILD_ONE;
+        }
         afr_ta_read_txn_synctask(frame, this);
         return 0;
     }
diff --git a/xlators/cluster/afr/src/afr.h b/xlators/cluster/afr/src/afr.h
index 8c0fe5efd8b..a044b2f0dc3 100644
--- a/xlators/cluster/afr/src/afr.h
+++ b/xlators/cluster/afr/src/afr.h
@@ -878,6 +878,7 @@ typedef struct _afr_local {
     afr_inode_ctx_t *inode_ctx;
 
     /*For thin-arbiter transactions.*/
+    unsigned char read_txn_query_child;
     unsigned char ta_child_up;
     struct list_head ta_waitq;
     struct list_head ta_onwireq;