From 5579f616c2c21a2a2cd2ef70b58149df85550db7 Mon Sep 17 00:00:00 2001

From: karthik-us <ksubrahm@redhat.com>

Date: Mon, 27 Nov 2017 12:51:16 +0530

Subject: [PATCH 084/128] cluster/afr: Fix for arbiter becoming source

Problem:

When eager-lock is on, and two writes happen in parallel on a FD

we were observing the following behaviour:

- First write fails on one data brick

- Since the post-op is not yet happened, the inode refresh will get

  both the data bricks as readable and set it in the inode context

- In flight split brain check see both the data bricks as readable

  and allows the second write

- Second write fails on the other data brick

- Now the post-op happens and marks both the data bricks as bad and

  arbiter will become source for healing

Fix:

Adding one more variable called write_suvol in inode context and it

will have the in memory representation of the writable subvols. Inode

refresh will not update this value and its lifetime is pre-op through

unlock in the afr transaction. Initially the pre-op will set this

value same as read_subvol in inode context and then in the in flight

split brain check we will use this value instead of read_subvol.

After all the checks we will update the value of this and set the

read_subvol same as this to avoid having incorrect value in that.

Upstream patch: https://review.gluster.org/#/c/18049/

> Change-Id: I2ef6904524ab91af861d59690974bbc529ab1af3

> BUG: 1482064

> Signed-off-by: karthik-us <ksubrahm@redhat.com>

Change-Id: I91cd21e378a7ae3757c2209fcb91a613d73e09ee

BUG: 1401969

Signed-off-by: karthik-us <ksubrahm@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/124292

Tested-by: RHGS Build Bot <nigelb@redhat.com>

Reviewed-by: Atin Mukherjee <amukherj@redhat.com>

---

 xlators/cluster/afr/src/afr-common.c      | 76 ++++++++++++++++++++++++++++++-

 xlators/cluster/afr/src/afr-lk-common.c   | 18 ++++++--

 xlators/cluster/afr/src/afr-transaction.c |  4 ++

 xlators/cluster/afr/src/afr.h             | 10 ++++

 4 files changed, 102 insertions(+), 6 deletions(-)

diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c

index 9c96056..a8ba5a0 100644

--- a/xlators/cluster/afr/src/afr-common.c

+++ b/xlators/cluster/afr/src/afr-common.c

@@ -149,6 +149,7 @@ __afr_inode_ctx_get (xlator_t *this, inode_t *inode, afr_inode_ctx_t **ctx)

                 }

                 tmp_ctx->spb_choice = -1;

                 tmp_ctx->read_subvol = 0;

+                tmp_ctx->write_subvol = 0;

         } else {

                 tmp_ctx = (afr_inode_ctx_t *) ctx_int;

         }

@@ -216,7 +217,7 @@ __afr_set_in_flight_sb_status (xlator_t *this, afr_local_t *local,

         if (ret < 0)

                 return ret;

-        val = ctx->read_subvol;

+        val = ctx->write_subvol;

         metadatamap_old = metadatamap = (val & 0x000000000000ffff);

         datamap_old = datamap = (val & 0x00000000ffff0000) >> 16;

@@ -276,6 +277,7 @@ __afr_set_in_flight_sb_status (xlator_t *this, afr_local_t *local,

                 (((uint64_t) datamap) << 16) |

                 (((uint64_t) event) << 32);

+        ctx->write_subvol = val;

         ctx->read_subvol = val;

         return ret;

@@ -6421,3 +6423,75 @@ afr_serialize_xattrs_with_delimiter (call_frame_t *frame, xlator_t *this,

 out:

         return ret;

 }

+

+int

+afr_write_subvol_set (call_frame_t *frame, xlator_t *this)

+{

+        afr_local_t      *local = NULL;

+        afr_inode_ctx_t  *ctx   = NULL;

+        uint64_t          val   = 0;

+        uint64_t          val1  = 0;

+        int               ret   = -1;

+

+        local = frame->local;

+        LOCK(&local->inode->lock);

+        {

+                ret = __afr_inode_ctx_get (this, local->inode, &ctx;;

+                if (ret < 0) {

+                        gf_msg (this->name, GF_LOG_ERROR, 0,

+                                AFR_MSG_DICT_GET_FAILED,

+                                "ERROR GETTING INODE CTX");

+                        UNLOCK(&local->inode->lock);

+                        return ret;

+                }

+

+                val = ctx->write_subvol;

+                /*

+                 * We need to set the value of write_subvol to read_subvol in 2

+                 * cases:

+                 * 1. Initially when the value is 0. i.e., it's the first lock

+                 * request.

+                 * 2. If it's a metadata transaction. If metadata transactions

+                 * comes in between data transactions and we have a brick

+                 * disconnect, the next metadata transaction won't get the

+                 * latest value of readables, since we do resetting of

+                 * write_subvol in unlock code path only if it's a data

+                 * transaction. To handle those scenarios we need to set the

+                 * value of write_subvol to read_subvol in case of metadata

+                 * transactions.

+                */

+                if (val == 0 ||

+                    local->transaction.type == AFR_METADATA_TRANSACTION) {

+                        val1 = ctx->read_subvol;

+                        ctx->write_subvol = val1;

+                }

+        }

+        UNLOCK (&local->inode->lock);

+

+        return 0;

+}

+

+int

+afr_write_subvol_reset (call_frame_t *frame, xlator_t *this)

+{

+        afr_local_t      *local = NULL;

+        afr_inode_ctx_t  *ctx   = NULL;

+        int               ret   = -1;

+

+        local = frame->local;

+        LOCK(&local->inode->lock);

+        {

+                ret = __afr_inode_ctx_get (this, local->inode, &ctx;;

+                if (ret < 0) {

+                        gf_msg (this->name, GF_LOG_ERROR, 0,

+                                AFR_MSG_DICT_GET_FAILED,

+                                "ERROR GETTING INODE CTX");

+                        UNLOCK(&local->inode->lock);

+                        return ret;

+                }

+                ctx->write_subvol = 0;

+        }

+        UNLOCK(&local->inode->lock);

+

+        return 0;

+}

diff --git a/xlators/cluster/afr/src/afr-lk-common.c b/xlators/cluster/afr/src/afr-lk-common.c

index 1f2a117..c17f60f 100644

--- a/xlators/cluster/afr/src/afr-lk-common.c

+++ b/xlators/cluster/afr/src/afr-lk-common.c

@@ -613,12 +613,16 @@ static int32_t

 afr_unlock_common_cbk (call_frame_t *frame, void *cookie, xlator_t *this,

                        int32_t op_ret, int32_t op_errno, dict_t *xdata)

 {

-        afr_local_t         *local    = NULL;

-        afr_internal_lock_t *int_lock = NULL;

-        int call_count = 0;

+        afr_local_t             *local          = NULL;

+        afr_internal_lock_t     *int_lock       = NULL;

+        afr_fd_ctx_t            *fd_ctx         = NULL;

+        afr_private_t           *priv           = NULL;

+        int                      call_count     = 0;

+        int                      ret            = 0;

         local    = frame->local;

         int_lock = &local->internal_lock;

+        priv = this->private;

         LOCK (&frame->lock);

         {

@@ -629,11 +633,15 @@ afr_unlock_common_cbk (call_frame_t *frame, void *cookie, xlator_t *this,

         if (call_count == 0) {

                 gf_msg_trace (this->name, 0,

                               "All internal locks unlocked");

-

+                if (local->fd) {

+                        fd_ctx = afr_fd_ctx_get (local->fd, this);

+                        if (0 == AFR_COUNT (fd_ctx->lock_acquired, priv->child_count))

+                                ret = afr_write_subvol_reset (frame, this);

+                }

                 int_lock->lock_cbk (frame, this);

         }

-        return 0;

+        return ret;

 }

 void

diff --git a/xlators/cluster/afr/src/afr-transaction.c b/xlators/cluster/afr/src/afr-transaction.c

index 35621d9..91c4f78 100644

--- a/xlators/cluster/afr/src/afr-transaction.c

+++ b/xlators/cluster/afr/src/afr-transaction.c

@@ -1791,6 +1791,10 @@ afr_changelog_pre_op (call_frame_t *frame, xlator_t *this)

 	if (pre_nop)

 		goto next;

+        ret = afr_write_subvol_set (frame, this);

+        if (ret)

+                goto err;

+

 	if (!local->pre_op_compat) {

 		dict_copy (xdata_req, local->xdata_req);

 		goto next;

diff --git a/xlators/cluster/afr/src/afr.h b/xlators/cluster/afr/src/afr.h

index c4ceb66..672d053 100644

--- a/xlators/cluster/afr/src/afr.h

+++ b/xlators/cluster/afr/src/afr.h

@@ -837,6 +837,7 @@ typedef struct _afr_local {

 typedef struct _afr_inode_ctx {

         uint64_t        read_subvol;

+        uint64_t        write_subvol;

         int             spb_choice;

         gf_timer_t      *timer;

         gf_boolean_t    need_refresh;

@@ -1262,4 +1263,13 @@ int

 afr_serialize_xattrs_with_delimiter (call_frame_t *frame, xlator_t *this,

                                      char *buf, const char *default_str,

                                      int32_t *serz_len, char delimiter);

+

+int

+__afr_inode_ctx_get (xlator_t *this, inode_t *inode, afr_inode_ctx_t **ctx);

+

+int

+afr_write_subvol_set (call_frame_t *frame, xlator_t *this);

+

+int

+afr_write_subvol_reset (call_frame_t *frame, xlator_t *this);

 #endif /* __AFR_H__ */

-- 

1.8.3.1