From 18093998f1e5178e707055904b32149c52acc774 Mon Sep 17 00:00:00 2001

From: Pranith Kumar K <pkarampu@redhat.com>

Date: Fri, 2 Mar 2018 10:13:20 +0530

Subject: [PATCH 192/201] cluster/afr: Remove compound-fops usage in afr

We are not seeing much improvement with this change. So removing the

feature so that it doesn't need to be maintained anymore.

 > Fixes: #414

Upstream-patch: https://review.gluster.org/19655

BUG: 1559788

Change-Id: Ic7969b151544daf2547bd262a9fa03f575626411

Signed-off-by: Pranith Kumar K <pkarampu@redhat.com>

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

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

---

 tests/basic/afr/compounded-write-txns.t   |  37 ----

 xlators/cluster/afr/src/afr-common.c      |  43 ----

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

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

 xlators/cluster/afr/src/afr.c             |  10 +-

 xlators/cluster/afr/src/afr.h             |  13 --

 6 files changed, 7 insertions(+), 433 deletions(-)

 delete mode 100644 tests/basic/afr/compounded-write-txns.t

diff --git a/tests/basic/afr/compounded-write-txns.t b/tests/basic/afr/compounded-write-txns.t

deleted file mode 100644

index 7cecd87..0000000

--- a/tests/basic/afr/compounded-write-txns.t

+++ /dev/null

@@ -1,37 +0,0 @@

-#!/bin/bash

-. $(dirname $0)/../../include.rc

-. $(dirname $0)/../../volume.rc

-

-cleanup

-

-TEST glusterd

-TEST pidof glusterd

-TEST $CLI volume create $V0 replica 3 $H0:$B0/${V0}{0,1,2}

-TEST $CLI volume set $V0 write-behind off

-TEST $CLI volume set $V0 client-io-threads off

-TEST $CLI volume start $V0

-TEST $GFS --volfile-id=$V0 --volfile-server=$H0 $M0

-

-# Create and generate data into a src file

-

-TEST `printf %1024s |tr " " "1" > /tmp/source`

-TEST `printf %1024s |tr " " "2" >> /tmp/source`

-

-TEST dd if=/tmp/source of=$M0/file bs=1024 count=2 2>/dev/null

-md5sum_file=$(md5sum $M0/file | awk '{print $1}')

-

-TEST $CLI volume set $V0 cluster.use-compound-fops on

-

-TEST dd if=$M0/file of=$M0/file-copy bs=1024 count=2 2>/dev/null

-

-EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0

-TEST $GFS --volfile-id=$V0 --volfile-server=$H0 $M0

-

-EXPECT "$md5sum_file" echo `md5sum $M0/file-copy | awk '{print $1}'`

-

-EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0

-TEST $CLI volume stop $V0

-TEST $CLI volume delete $V0

-

-TEST rm -f /tmp/source

-cleanup

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

index a790402..04c8613 100644

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

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

@@ -43,7 +43,6 @@

 #include "afr-self-heal.h"

 #include "afr-self-heald.h"

 #include "afr-messages.h"

-#include "compound-fop-utils.h"

 int32_t

 afr_quorum_errno (afr_private_t *priv)

@@ -5246,7 +5245,6 @@ afr_local_init (afr_local_t *local, afr_private_t *priv, int32_t *op_errno)

         local->need_full_crawl = _gf_false;

-        local->compound = _gf_false;

         INIT_LIST_HEAD (&local->healer);

 	return 0;

 out:

@@ -5393,7 +5391,6 @@ afr_transaction_local_init (afr_local_t *local, xlator_t *this)

         if (!local->pending)

                 goto out;

-        local->compound = _gf_false;

 	INIT_LIST_HEAD (&local->transaction.eager_locked);

         ret = 0;

@@ -6142,46 +6139,6 @@ afr_get_msg_id (char *op_type)

         return -1;

 }

-gf_boolean_t

-afr_can_compound_pre_op_and_op (afr_private_t *priv, glusterfs_fop_t fop)

-{

-        if (priv->arbiter_count != 0)

-                return _gf_false;

-

-        if (!priv->use_compound_fops)

-                return _gf_false;

-

-        switch (fop) {

-        case GF_FOP_WRITE:

-                return _gf_true;

-        default:

-                return _gf_false;

-        }

-}

-

-afr_compound_cbk_t

-afr_pack_fop_args (call_frame_t *frame, compound_args_t *args,

-                   glusterfs_fop_t fop, int index)

-{

-        afr_local_t     *local  = frame->local;

-

-        switch (fop) {

-        case GF_FOP_WRITE:

-                COMPOUND_PACK_ARGS (writev, GF_FOP_WRITE,

-                                    args, index,

-                                    local->fd, local->cont.writev.vector,

-                                    local->cont.writev.count,

-                                    local->cont.writev.offset,

-                                    local->cont.writev.flags,

-                                    local->cont.writev.iobref,

-                                    local->xdata_req);

-                return afr_pre_op_writev_cbk;

-        default:

-                break;

-        }

-        return NULL;

-}

-

 int

 afr_fav_child_reset_sink_xattrs_cbk (int ret, call_frame_t *heal_frame,

                                      void *opaque)

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

index 1c80c6b..02fb10a 100644

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

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

@@ -17,7 +17,6 @@

 #include "afr-transaction.h"

 #include "afr-self-heal.h"

 #include "afr-messages.h"

-#include "compound-fop-utils.h"

 #include <signal.h>

@@ -38,10 +37,6 @@ afr_changelog_call_count (afr_transaction_type type,

                           unsigned char *failed_subvols,

                           unsigned int child_count);

 int

-afr_post_op_unlock_do (call_frame_t *frame, xlator_t *this, dict_t *xattr,

-                       afr_changelog_resume_t changelog_resume,

-                       afr_xattrop_type_t op);

-int

 afr_changelog_do (call_frame_t *frame, xlator_t *this, dict_t *xattr,

 		  afr_changelog_resume_t changelog_resume,

                   afr_xattrop_type_t op);

@@ -847,12 +842,10 @@ afr_changelog_post_op_now (call_frame_t *frame, xlator_t *this)

         afr_private_t           *priv           = this->private;

         afr_local_t             *local          = NULL;

         dict_t                  *xattr          = NULL;

-        afr_fd_ctx_t            *fd_ctx         = NULL;

         int                     i               = 0;

         int                     ret             = 0;

         int                     idx             = 0;

         int                     nothing_failed  = 1;

-        gf_boolean_t            compounded_unlock = _gf_true;

         gf_boolean_t            need_undirty    = _gf_false;

         afr_handle_quorum (frame);

@@ -918,36 +911,8 @@ afr_changelog_post_op_now (call_frame_t *frame, xlator_t *this)

 		goto out;

 	}

-        if (local->compound && local->fd) {

-                LOCK (&local->fd->lock);

-                {

-                        fd_ctx = __afr_fd_ctx_get (local->fd, this);

-                        for (i = 0; i < priv->child_count; i++) {

-                                if (local->transaction.pre_op[i] &&

-                                    local->transaction.eager_lock[i]) {

-                                        if (fd_ctx->lock_piggyback[i])

-                                                compounded_unlock = _gf_false;

-                                        else if (fd_ctx->lock_acquired[i])

-                                                compounded_unlock = _gf_false;

-                                }

-                                if (compounded_unlock == _gf_false)

-                                        break;

-                        }

-                }

-                UNLOCK (&local->fd->lock);

-        }

-

-        /* Do not compound if any brick got piggybacked lock as

-         * unlock should not be done for that. */

-        if (local->compound && compounded_unlock) {

-                afr_post_op_unlock_do (frame, this, xattr,

-                                       afr_changelog_post_op_done,

-                                       AFR_TRANSACTION_POST_OP);

-        } else {

-                afr_changelog_do (frame, this, xattr,

-                                  afr_changelog_post_op_done,

-                                  AFR_TRANSACTION_POST_OP);

-        }

+        afr_changelog_do (frame, this, xattr, afr_changelog_post_op_done,

+                          AFR_TRANSACTION_POST_OP);

 out:

 	if (xattr)

                 dict_unref (xattr);

@@ -1277,66 +1242,6 @@ out:

 }

 int

-afr_pre_op_writev_cbk (call_frame_t *frame, void *cookie, xlator_t *this,

-                       int op_ret, int op_errno,

-                       void *data, dict_t *xdata)

-{

-        afr_local_t *local = NULL;

-        call_frame_t    *fop_frame = NULL;

-        default_args_cbk_t *write_args_cbk = NULL;

-        compound_args_cbk_t *args_cbk = data;

-        int call_count = -1;

-        int child_index = -1;

-

-        local = frame->local;

-        child_index = (long) cookie;

-

-	if (local->pre_op_compat)

-		afr_changelog_pre_op_update (frame, this);

-

-        if (op_ret == -1) {

-                local->op_errno = op_errno;

-		afr_transaction_fop_failed (frame, this, child_index);

-        }

-

-        /* If the compound fop failed due to saved_frame_unwind(), then

-         * protocol/client fails it even before args_cbk is allocated.

-         * Handle that case by passing the op_ret, op_errno values explicitly.

-         */

-        if ((op_ret == -1) && (args_cbk == NULL)) {

-                afr_inode_write_fill  (frame, this, child_index, op_ret,

-                                       op_errno, NULL, NULL, NULL);

-        } else {

-                write_args_cbk = &args_cbk->rsp_list[1];

-                afr_inode_write_fill  (frame, this, child_index,

-                                       write_args_cbk->op_ret,

-                                       write_args_cbk->op_errno,

-                                       &write_args_cbk->prestat,

-                                       &write_args_cbk->poststat,

-                                       write_args_cbk->xdata);

-        }

-

-	call_count = afr_frame_return (frame);

-

-        if (call_count == 0) {

-                compound_args_cleanup (local->c_args);

-                local->c_args = NULL;

-                afr_process_post_writev (frame, this);

-                if (!afr_txn_nothing_failed (frame, this)) {

-                        /* Don't unwind until post-op is complete */

-                        local->transaction.resume (frame, this);

-                } else {

-                /* frame change, place frame in post-op delay and unwind */

-                        fop_frame = afr_transaction_detach_fop_frame (frame);

-                        afr_writev_copy_outvars (frame, fop_frame);

-                        local->transaction.resume (frame, this);

-                        afr_writev_unwind (fop_frame, this);

-                }

-        }

-        return 0;

-}

-

-int

 afr_changelog_prepare (xlator_t *this, call_frame_t *frame, int *call_count,

                        afr_changelog_resume_t changelog_resume,

                        afr_xattrop_type_t op, dict_t **xdata,

@@ -1366,223 +1271,6 @@ afr_changelog_prepare (xlator_t *this, call_frame_t *frame, int *call_count,

 }

 int

-afr_pre_op_fop_do (call_frame_t *frame, xlator_t *this, dict_t *xattr,

-                   afr_changelog_resume_t changelog_resume,

-                   afr_xattrop_type_t op)

-{

-        afr_local_t *local = NULL;

-        afr_private_t *priv = NULL;

-        dict_t *xdata = NULL;

-        dict_t *newloc_xdata = NULL;

-        compound_args_t *args = NULL;

-        int i = 0, call_count = 0;

-        afr_compound_cbk_t compound_cbk;

-        int ret = 0;

-        int op_errno = ENOMEM;

-

-        local = frame->local;

-        priv = this->private;

-

-        /* If lock failed on all, just unlock and unwind */

-        ret = afr_changelog_prepare (this, frame, &call_count, changelog_resume,

-                                     op, &xdata, &newloc_xdata);

-

-        if (ret)

-                return 0;

-

-        local->call_count = call_count;

-

-        afr_save_lk_owner (frame);

-        frame->root->lk_owner =

-                local->transaction.main_frame->root->lk_owner;

-

-        args = compound_fop_alloc (2, GF_CFOP_XATTROP_WRITEV, NULL);

-

-        if (!args)

-                goto err;

-

-        /* pack pre-op part */

-        i = 0;

-        COMPOUND_PACK_ARGS (fxattrop, GF_FOP_FXATTROP,

-                            args, i,

-                            local->fd, GF_XATTROP_ADD_ARRAY,

-                            xattr, xdata);

-        i++;

-        /* pack whatever fop needs to be packed

-         * @compound_cbk holds the cbk that would need to be called

-         */

-        compound_cbk = afr_pack_fop_args (frame, args, local->op, i);

-

-        local->c_args = args;

-

-        for (i = 0; i < priv->child_count; i++) {

-                /* Means lock did not succeed on this brick */

-                if (!local->transaction.pre_op[i] ||

-                    local->transaction.failed_subvols[i])

-                        continue;

-

-                STACK_WIND_COOKIE (frame, compound_cbk,

-                                   (void *) (long) i,

-                                   priv->children[i],

-                                   priv->children[i]->fops->compound,

-                                   args,

-                                   NULL);

-                if (!--call_count)

-                        break;

-        }

-

-        if (xdata)

-                dict_unref (xdata);

-        if (newloc_xdata)

-                dict_unref (newloc_xdata);

-        return 0;

-err:

-	local->internal_lock.lock_cbk = local->transaction.done;

-	local->op_ret = -1;

-	local->op_errno = op_errno;

-

-        afr_restore_lk_owner (frame);

-	afr_unlock (frame, this);

-

-        if (xdata)

-                dict_unref (xdata);

-        if (newloc_xdata)

-                dict_unref (newloc_xdata);

-	return 0;

-}

-

-int

-afr_post_op_unlock_cbk (call_frame_t *frame, void *cookie, xlator_t *this,

-                       int op_ret, int op_errno,

-                       void *data, dict_t *xdata)

-{

-        afr_local_t *local = NULL;

-        int call_count = -1;

-        afr_internal_lock_t *int_lock = NULL;

-        int32_t             child_index = (long)cookie;

-

-        local = frame->local;

-        child_index = (long) cookie;

-

-        local = frame->local;

-        int_lock = &local->internal_lock;

-

-        afr_update_uninodelk (local, int_lock, child_index);

-

-        LOCK (&frame->lock);

-        {

-                call_count = --int_lock->lk_call_count;

-        }

-        UNLOCK (&frame->lock);

-

-        if (call_count == 0) {

-                compound_args_cleanup (local->c_args);

-                local->c_args = NULL;

-                if (local->transaction.resume_stub) {

-                        call_resume (local->transaction.resume_stub);

-                        local->transaction.resume_stub = NULL;

-                }

-                gf_msg_trace (this->name, 0,

-                              "All internal locks unlocked");

-                int_lock->lock_cbk (frame, this);

-        }

-

-        return 0;

-}

-

-int

-afr_post_op_unlock_do (call_frame_t *frame, xlator_t *this, dict_t *xattr,

-		       afr_changelog_resume_t changelog_resume,

-                       afr_xattrop_type_t op)

-{

-	afr_local_t             *local          = NULL;

-	afr_private_t           *priv           = NULL;

-        dict_t                  *xdata          = NULL;

-        dict_t                  *newloc_xdata   = NULL;

-        compound_args_t         *args           = NULL;

-        afr_internal_lock_t     *int_lock       = NULL;

-        afr_inodelk_t           *inodelk        = NULL;

-	int                     i               = 0;

-	int                     call_count      = 0;

-        struct gf_flock         flock           = {0,};

-        int                     ret             = 0;

-

-	local = frame->local;

-	priv = this->private;

-        int_lock = &local->internal_lock;

-

-        if (afr_is_inodelk_transaction(local)) {

-                inodelk = afr_get_inodelk (int_lock, int_lock->domain);

-

-                flock.l_start = inodelk->flock.l_start;

-                flock.l_len   = inodelk->flock.l_len;

-                flock.l_type  = F_UNLCK;

-        }

-

-        ret = afr_changelog_prepare (this, frame, &call_count, changelog_resume,

-                                     op, &xdata, &newloc_xdata);

-

-        if (ret)

-                return 0;

-

-        int_lock->lk_call_count = call_count;

-

-        int_lock->lock_cbk = local->transaction.done;

-

-        args = compound_fop_alloc (2, GF_CFOP_XATTROP_UNLOCK, NULL);

-

-        if (!args) {

-		local->op_ret = -1;

-		local->op_errno = ENOMEM;

-		afr_changelog_post_op_done (frame, this);

-		goto out;

-	}

-

-        i = 0;

-        COMPOUND_PACK_ARGS (fxattrop, GF_FOP_FXATTROP,

-                            args, i,

-                            local->fd, GF_XATTROP_ADD_ARRAY,

-                            xattr, xdata);

-        i++;

-

-        if (afr_is_inodelk_transaction(local)) {

-                if (local->fd) {

-                        COMPOUND_PACK_ARGS (finodelk, GF_FOP_FINODELK,

-                                            args, i,

-                                            int_lock->domain, local->fd,

-                                            F_SETLK, &flock, NULL);

-                } else {

-                        COMPOUND_PACK_ARGS (inodelk, GF_FOP_INODELK,

-                                            args, i,

-                                            int_lock->domain, &local->loc,

-                                            F_SETLK, &flock, NULL);

-                }

-        }

-

-        local->c_args = args;

-

-        for (i = 0; i < priv->child_count; i++) {

-                if (!local->transaction.pre_op[i] ||

-                    local->transaction.failed_subvols[i])

-                        continue;

-                STACK_WIND_COOKIE (frame, afr_post_op_unlock_cbk,

-                                   (void *) (long) i,

-                                   priv->children[i],

-                                   priv->children[i]->fops->compound,

-                                   args,

-                                   NULL);

-                if (!--call_count)

-                        break;

-        }

-out:

-        if (xdata)

-                dict_unref (xdata);

-        if (newloc_xdata)

-                dict_unref (newloc_xdata);

-        return 0;

-}

-

-int

 afr_changelog_do (call_frame_t *frame, xlator_t *this, dict_t *xattr,

 		  afr_changelog_resume_t changelog_resume,

                   afr_xattrop_type_t op)

@@ -1793,21 +1481,8 @@ afr_changelog_pre_op (call_frame_t *frame, xlator_t *this)

 		goto next;

 	}

-	/* Till here we have already decided if pre-op needs to be done,

-         * based on various criteria. The only thing that needs to be checked

-         * now on is whether compound-fops is enabled or not.

-         * If it is, then perform pre-op and fop together for writev op.

-         */

-        if (afr_can_compound_pre_op_and_op (priv, local->op)) {

-                local->compound = _gf_true;

-                afr_pre_op_fop_do (frame, this, xdata_req,

-                                   afr_transaction_perform_fop,

-                                   AFR_TRANSACTION_PRE_OP);

-        } else {

-                afr_changelog_do (frame, this, xdata_req,

-                                  afr_transaction_perform_fop,

-                                  AFR_TRANSACTION_PRE_OP);

-        }

+        afr_changelog_do (frame, this, xdata_req, afr_transaction_perform_fop,

+                          AFR_TRANSACTION_PRE_OP);

 	if (xdata_req)

 		dict_unref (xdata_req);

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

index dd19e5b..d01e144 100644

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

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

@@ -58,7 +58,5 @@ afr_pick_error_xdata (afr_local_t *local, afr_private_t *priv,

                       inode_t *inode1, unsigned char *readable1,

                       inode_t *inode2, unsigned char *readable2);

 int

-afr_pre_op_writev_cbk (call_frame_t *frame, void *cookie, xlator_t *this,

-                       int op_ret, int op_errno,

-                       void *data, dict_t *xdata);

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

 #endif /* __TRANSACTION_H__ */

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

index aa48e76..9ed0481 100644

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

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

@@ -245,9 +245,6 @@ reconfigure (xlator_t *this, dict_t *options)

         GF_OPTION_RECONF ("locking-scheme", priv->locking_scheme, options, str,

                           out);

         GF_OPTION_RECONF ("full-lock", priv->full_lock, options, bool, out);

-        GF_OPTION_RECONF ("use-compound-fops", priv->use_compound_fops,

-                          options, bool,

-                          out);

         GF_OPTION_RECONF ("granular-entry-heal", priv->esh_granular, options,

                           bool, out);

@@ -536,8 +533,6 @@ init (xlator_t *this)

         GF_OPTION_INIT ("pre-op-compat", priv->pre_op_compat, bool, out);

         GF_OPTION_INIT ("locking-scheme", priv->locking_scheme, str, out);

         GF_OPTION_INIT ("full-lock", priv->full_lock, bool, out);

-        GF_OPTION_INIT ("use-compound-fops", priv->use_compound_fops,

-                        bool, out);

         GF_OPTION_INIT ("granular-entry-heal", priv->esh_granular, bool, out);

         GF_OPTION_INIT ("eager-lock", priv->eager_lock, bool, out);

@@ -1121,9 +1116,8 @@ struct volume_options options[] = {

         { .key   = {"use-compound-fops"},

           .type  = GF_OPTION_TYPE_BOOL,

           .default_value = "no",

-          .description = "Use compound fops framework to modify afr "

-                         "transaction such that network roundtrips are "

-                         "reduced, thus improving the performance.",

+          .description = "this option exists only for backward compatibility "

+                         "and configuring it doesn't have any effect"

         },

         { .key  = {NULL} },

 };

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

index 11278fb..a5b0d3b 100644

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

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

@@ -44,10 +44,6 @@ typedef int (*afr_inode_refresh_cbk_t) (call_frame_t *frame, xlator_t *this, int

 typedef int (*afr_changelog_resume_t) (call_frame_t *frame, xlator_t *this);

-typedef int (*afr_compound_cbk_t) (call_frame_t *frame, void *cookie,

-                                   xlator_t *this, int op_ret, int op_errno,

-                                   void *data, dict_t *xdata);

-

 #define AFR_COUNT(array,max) ({int __i; int __res = 0; for (__i = 0; __i < max; __i++) if (array[__i]) __res++; __res;})

 #define AFR_INTERSECT(dst,src1,src2,max) ({int __i; for (__i = 0; __i < max; __i++) dst[__i] = src1[__i] && src2[__i];})

 #define AFR_CMP(a1,a2,len) ({int __cmp = 0; int __i; for (__i = 0; __i < len; __i++) if (a1[__i] != a2[__i]) { __cmp = 1; break;} __cmp;})

@@ -181,7 +177,6 @@ typedef struct _afr_private {

         gf_boolean_t           full_lock;

         gf_boolean_t           esh_granular;

         gf_boolean_t           consistent_io;

-        gf_boolean_t           use_compound_fops;

 } afr_private_t;

@@ -843,9 +838,7 @@ typedef struct _afr_local {

         call_frame_t *heal_frame;

         gf_boolean_t need_full_crawl;

-        gf_boolean_t compound;

         afr_fop_lock_state_t fop_lock_state;

-        compound_args_t *c_args;

         gf_boolean_t is_read_txn;

         afr_inode_ctx_t *inode_ctx;

@@ -1252,12 +1245,6 @@ afr_writev_copy_outvars (call_frame_t *src_frame, call_frame_t *dst_frame);

 void

 afr_update_uninodelk (afr_local_t *local, afr_internal_lock_t *int_lock,

                     int32_t child_index);

-gf_boolean_t

-afr_can_compound_pre_op_and_op (afr_private_t *priv, glusterfs_fop_t fop);

-

-afr_compound_cbk_t

-afr_pack_fop_args (call_frame_t *frame, compound_args_t *args,

-                   glusterfs_fop_t fop, int index);

 int

 afr_is_inodelk_transaction(afr_local_t *local);

-- 

1.8.3.1