From 18093998f1e5178e707055904b32149c52acc774 Mon Sep 17 00:00:00 2001 From: Pranith Kumar K 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 Reviewed-on: https://code.engineering.redhat.com/gerrit/131943 Tested-by: RHGS Build Bot --- 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 @@ -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