From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 08/18] xfs: split remote attr setting out from replace path
Date: Tue, 10 May 2022 16:22:58 -0700 [thread overview]
Message-ID: <20220510232258.GL27195@magnolia> (raw)
In-Reply-To: <20220509004138.762556-9-david@fromorbit.com>
On Mon, May 09, 2022 at 10:41:28AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> When we set a new xattr, we have three exit paths:
>
> 1. nothing else to do
> 2. allocate and set the remote xattr value
> 3. perform the rest of a replace operation
>
> Currently we push both 2 and 3 into the same state, regardless of
> whether we just set a remote attribute or not. Once we've set the
> remote xattr, we have two exit states:
>
> 1. nothing else to do
> 2. perform the rest of a replace operation
>
> Hence we can split the remote xattr allocation and setting into
> their own states and factor it out of xfs_attr_set_iter() to further
> clean up the state machine and the implementation of the state
> machine.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Allison Henderson<allison.henderson@oracle.com>
Pretty straightforward,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> ---
> fs/xfs/libxfs/xfs_attr.c | 113 +++++++++++++++++++++------------------
> fs/xfs/libxfs/xfs_attr.h | 14 +++--
> fs/xfs/xfs_trace.h | 9 ++--
> 3 files changed, 77 insertions(+), 59 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index be580da62f08..d2b29f7e103a 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -334,9 +334,11 @@ xfs_attr_leaf_addname(
> * or perform more xattr manipulations. Otherwise there is nothing more
> * to do and we can return success.
> */
> - if (args->rmtblkno ||
> - (args->op_flags & XFS_DA_OP_RENAME)) {
> - attr->xattri_dela_state = XFS_DAS_FOUND_LBLK;
> + if (args->rmtblkno) {
> + attr->xattri_dela_state = XFS_DAS_LEAF_SET_RMT;
> + error = -EAGAIN;
> + } else if (args->op_flags & XFS_DA_OP_RENAME) {
> + attr->xattri_dela_state = XFS_DAS_LEAF_REPLACE;
> error = -EAGAIN;
> } else {
> attr->xattri_dela_state = XFS_DAS_DONE;
> @@ -363,9 +365,11 @@ xfs_attr_node_addname(
> if (error)
> return error;
>
> - if (args->rmtblkno ||
> - (args->op_flags & XFS_DA_OP_RENAME)) {
> - attr->xattri_dela_state = XFS_DAS_FOUND_NBLK;
> + if (args->rmtblkno) {
> + attr->xattri_dela_state = XFS_DAS_NODE_SET_RMT;
> + error = -EAGAIN;
> + } else if (args->op_flags & XFS_DA_OP_RENAME) {
> + attr->xattri_dela_state = XFS_DAS_NODE_REPLACE;
> error = -EAGAIN;
> } else {
> attr->xattri_dela_state = XFS_DAS_DONE;
> @@ -375,6 +379,40 @@ xfs_attr_node_addname(
> return error;
> }
>
> +static int
> +xfs_attr_rmtval_alloc(
> + struct xfs_attr_item *attr)
> +{
> + struct xfs_da_args *args = attr->xattri_da_args;
> + int error = 0;
> +
> + /*
> + * If there was an out-of-line value, allocate the blocks we
> + * identified for its storage and copy the value. This is done
> + * after we create the attribute so that we don't overflow the
> + * maximum size of a transaction and/or hit a deadlock.
> + */
> + if (attr->xattri_blkcnt > 0) {
> + error = xfs_attr_rmtval_set_blk(attr);
> + if (error)
> + return error;
> + error = -EAGAIN;
> + goto out;
> + }
> +
> + error = xfs_attr_rmtval_set_value(args);
> + if (error)
> + return error;
> +
> + /* If this is not a rename, clear the incomplete flag and we're done. */
> + if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> + error = xfs_attr3_leaf_clearflag(args);
> + attr->xattri_dela_state = XFS_DAS_DONE;
> + }
> +out:
> + trace_xfs_attr_rmtval_alloc(attr->xattri_dela_state, args->dp);
> + return error;
> +}
>
> /*
> * Set the attribute specified in @args.
> @@ -406,54 +444,26 @@ xfs_attr_set_iter(
> case XFS_DAS_NODE_ADD:
> return xfs_attr_node_addname(attr);
>
> - case XFS_DAS_FOUND_LBLK:
> - case XFS_DAS_FOUND_NBLK:
> - /*
> - * Find space for remote blocks and fall into the allocation
> - * state.
> - */
> - if (args->rmtblkno > 0) {
> - error = xfs_attr_rmtval_find_space(attr);
> - if (error)
> - return error;
> - }
> + case XFS_DAS_LEAF_SET_RMT:
> + case XFS_DAS_NODE_SET_RMT:
> + error = xfs_attr_rmtval_find_space(attr);
> + if (error)
> + return error;
> attr->xattri_dela_state++;
> fallthrough;
> +
> case XFS_DAS_LEAF_ALLOC_RMT:
> case XFS_DAS_NODE_ALLOC_RMT:
> -
> - /*
> - * If there was an out-of-line value, allocate the blocks we
> - * identified for its storage and copy the value. This is done
> - * after we create the attribute so that we don't overflow the
> - * maximum size of a transaction and/or hit a deadlock.
> - */
> - if (args->rmtblkno > 0) {
> - if (attr->xattri_blkcnt > 0) {
> - error = xfs_attr_rmtval_set_blk(attr);
> - if (error)
> - return error;
> - trace_xfs_attr_set_iter_return(
> - attr->xattri_dela_state,
> - args->dp);
> - return -EAGAIN;
> - }
> -
> - error = xfs_attr_rmtval_set_value(args);
> - if (error)
> - return error;
> - }
> -
> - /*
> - * If this is not a rename, clear the incomplete flag and we're
> - * done.
> - */
> - if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> - if (args->rmtblkno > 0)
> - error = xfs_attr3_leaf_clearflag(args);
> + error = xfs_attr_rmtval_alloc(attr);
> + if (error)
> return error;
> - }
> + if (attr->xattri_dela_state == XFS_DAS_DONE)
> + break;
> + attr->xattri_dela_state++;
> + fallthrough;
>
> + case XFS_DAS_LEAF_REPLACE:
> + case XFS_DAS_NODE_REPLACE:
> /*
> * If this is an atomic rename operation, we must "flip" the
> * incomplete flags on the "new" and "old" attribute/value pairs
> @@ -471,10 +481,9 @@ xfs_attr_set_iter(
> * Commit the flag value change and start the next trans
> * in series at FLIP_FLAG.
> */
> + error = -EAGAIN;
> attr->xattri_dela_state++;
> - trace_xfs_attr_set_iter_return(attr->xattri_dela_state,
> - args->dp);
> - return -EAGAIN;
> + break;
> }
>
> attr->xattri_dela_state++;
> @@ -549,6 +558,8 @@ xfs_attr_set_iter(
> ASSERT(0);
> break;
> }
> +
> + trace_xfs_attr_set_iter_return(attr->xattri_dela_state, args->dp);
> return error;
> }
>
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 37db61649217..1749fd8f7ddd 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -452,15 +452,17 @@ enum xfs_delattr_state {
> XFS_DAS_RM_SHRINK, /* We are shrinking the tree */
>
> /* Leaf state set sequence */
> - XFS_DAS_FOUND_LBLK, /* We found leaf blk for attr */
> + XFS_DAS_LEAF_SET_RMT, /* set a remote xattr from a leaf */
> XFS_DAS_LEAF_ALLOC_RMT, /* We are allocating remote blocks */
> + XFS_DAS_LEAF_REPLACE, /* Perform replace ops on a leaf */
> XFS_DAS_FLIP_LFLAG, /* Flipped leaf INCOMPLETE attr flag */
> XFS_DAS_RM_LBLK, /* A rename is removing leaf blocks */
> XFS_DAS_RD_LEAF, /* Read in the new leaf */
>
> /* Node state set sequence, must match leaf state above */
> - XFS_DAS_FOUND_NBLK, /* We found node blk for attr */
> + XFS_DAS_NODE_SET_RMT, /* set a remote xattr from a node */
> XFS_DAS_NODE_ALLOC_RMT, /* We are allocating remote blocks */
> + XFS_DAS_NODE_REPLACE, /* Perform replace ops on a node */
> XFS_DAS_FLIP_NFLAG, /* Flipped node INCOMPLETE attr flag */
> XFS_DAS_RM_NBLK, /* A rename is removing node blocks */
> XFS_DAS_CLR_FLAG, /* Clear incomplete flag */
> @@ -476,13 +478,15 @@ enum xfs_delattr_state {
> { XFS_DAS_RMTBLK, "XFS_DAS_RMTBLK" }, \
> { XFS_DAS_RM_NAME, "XFS_DAS_RM_NAME" }, \
> { XFS_DAS_RM_SHRINK, "XFS_DAS_RM_SHRINK" }, \
> - { XFS_DAS_FOUND_LBLK, "XFS_DAS_FOUND_LBLK" }, \
> + { XFS_DAS_LEAF_SET_RMT, "XFS_DAS_LEAF_SET_RMT" }, \
> { XFS_DAS_LEAF_ALLOC_RMT, "XFS_DAS_LEAF_ALLOC_RMT" }, \
> - { XFS_DAS_FOUND_NBLK, "XFS_DAS_FOUND_NBLK" }, \
> - { XFS_DAS_NODE_ALLOC_RMT, "XFS_DAS_NODE_ALLOC_RMT" }, \
> + { XFS_DAS_LEAF_REPLACE, "XFS_DAS_LEAF_REPLACE" }, \
> { XFS_DAS_FLIP_LFLAG, "XFS_DAS_FLIP_LFLAG" }, \
> { XFS_DAS_RM_LBLK, "XFS_DAS_RM_LBLK" }, \
> { XFS_DAS_RD_LEAF, "XFS_DAS_RD_LEAF" }, \
> + { XFS_DAS_NODE_SET_RMT, "XFS_DAS_NODE_SET_RMT" }, \
> + { XFS_DAS_NODE_ALLOC_RMT, "XFS_DAS_NODE_ALLOC_RMT" }, \
> + { XFS_DAS_NODE_REPLACE, "XFS_DAS_NODE_REPLACE" }, \
> { XFS_DAS_FLIP_NFLAG, "XFS_DAS_FLIP_NFLAG" }, \
> { XFS_DAS_RM_NBLK, "XFS_DAS_RM_NBLK" }, \
> { XFS_DAS_CLR_FLAG, "XFS_DAS_CLR_FLAG" }, \
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 067ab31d7a20..cb9122327114 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -4136,13 +4136,15 @@ TRACE_DEFINE_ENUM(XFS_DAS_NODE_ADD);
> TRACE_DEFINE_ENUM(XFS_DAS_RMTBLK);
> TRACE_DEFINE_ENUM(XFS_DAS_RM_NAME);
> TRACE_DEFINE_ENUM(XFS_DAS_RM_SHRINK);
> -TRACE_DEFINE_ENUM(XFS_DAS_FOUND_LBLK);
> +TRACE_DEFINE_ENUM(XFS_DAS_LEAF_SET_RMT);
> TRACE_DEFINE_ENUM(XFS_DAS_LEAF_ALLOC_RMT);
> -TRACE_DEFINE_ENUM(XFS_DAS_FOUND_NBLK);
> -TRACE_DEFINE_ENUM(XFS_DAS_NODE_ALLOC_RMT);
> +TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REPLACE);
> TRACE_DEFINE_ENUM(XFS_DAS_FLIP_LFLAG);
> TRACE_DEFINE_ENUM(XFS_DAS_RM_LBLK);
> TRACE_DEFINE_ENUM(XFS_DAS_RD_LEAF);
> +TRACE_DEFINE_ENUM(XFS_DAS_NODE_SET_RMT);
> +TRACE_DEFINE_ENUM(XFS_DAS_NODE_ALLOC_RMT);
> +TRACE_DEFINE_ENUM(XFS_DAS_NODE_REPLACE);
> TRACE_DEFINE_ENUM(XFS_DAS_FLIP_NFLAG);
> TRACE_DEFINE_ENUM(XFS_DAS_RM_NBLK);
> TRACE_DEFINE_ENUM(XFS_DAS_CLR_FLAG);
> @@ -4172,6 +4174,7 @@ DEFINE_DAS_STATE_EVENT(xfs_attr_set_iter_return);
> DEFINE_DAS_STATE_EVENT(xfs_attr_leaf_addname_return);
> DEFINE_DAS_STATE_EVENT(xfs_attr_node_addname_return);
> DEFINE_DAS_STATE_EVENT(xfs_attr_remove_iter_return);
> +DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_alloc);
> DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_remove_return);
> DEFINE_DAS_STATE_EVENT(xfs_attr_defer_add);
> DEFINE_DAS_STATE_EVENT(xfs_attr_defer_replace);
> --
> 2.35.1
>
next prev parent reply other threads:[~2022-05-10 23:23 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-09 0:41 [PATCH 00/18 V4] XFS: LARP state machine and recovery rework Dave Chinner
2022-05-09 0:41 ` [PATCH 01/18] xfs: avoid empty xattr transaction when attrs are inline Dave Chinner
2022-05-09 16:43 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 02/18] xfs: initialise attrd item to zero Dave Chinner
2022-05-09 16:43 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 03/18] xfs: make xattri_leaf_bp more useful Dave Chinner
2022-05-10 22:58 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 04/18] xfs: rework deferred attribute operation setup Dave Chinner
2022-05-10 23:04 ` Darrick J. Wong
2022-05-11 0:57 ` Dave Chinner
2022-05-09 0:41 ` [PATCH 05/18] xfs: separate out initial attr_set states Dave Chinner
2022-05-10 23:12 ` Darrick J. Wong
2022-05-11 1:06 ` Dave Chinner
2022-05-11 1:08 ` Darrick J. Wong
2022-05-11 1:38 ` Dave Chinner
2022-05-11 8:35 ` Dave Chinner
2022-05-11 15:39 ` Darrick J. Wong
2022-05-12 0:57 ` Dave Chinner
2022-05-09 0:41 ` [PATCH 06/18] xfs: kill XFS_DAC_LEAF_ADDNAME_INIT Dave Chinner
2022-05-10 23:15 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 07/18] xfs: consolidate leaf/node states in xfs_attr_set_iter Dave Chinner
2022-05-10 23:20 ` Darrick J. Wong
2022-05-11 1:09 ` Dave Chinner
2022-05-09 0:41 ` [PATCH 08/18] xfs: split remote attr setting out from replace path Dave Chinner
2022-05-10 23:22 ` Darrick J. Wong [this message]
2022-05-09 0:41 ` [PATCH 09/18] xfs: XFS_DAS_LEAF_REPLACE state only needed if !LARP Dave Chinner
2022-05-10 23:24 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 10/18] xfs: remote xattr removal in xfs_attr_set_iter() is conditional Dave Chinner
2022-05-10 23:26 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 11/18] xfs: clean up final attr removal in xfs_attr_set_iter Dave Chinner
2022-05-10 23:29 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 12/18] xfs: xfs_attr_set_iter() does not need to return EAGAIN Dave Chinner
2022-05-10 23:30 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 13/18] xfs: introduce attr remove initial states into xfs_attr_set_iter Dave Chinner
2022-05-10 23:37 ` Darrick J. Wong
2022-05-10 23:40 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 14/18] xfs: switch attr remove to xfs_attri_set_iter Dave Chinner
2022-05-10 23:40 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 15/18] xfs: remove xfs_attri_remove_iter Dave Chinner
2022-05-10 23:42 ` Darrick J. Wong
2022-05-09 0:41 ` [PATCH 16/18] xfs: use XFS_DA_OP flags in deferred attr ops Dave Chinner
2022-05-10 22:20 ` [PATCH 16/18 v2] " Dave Chinner
2022-05-10 23:47 ` Darrick J. Wong
2022-05-10 23:49 ` Alli
2022-05-09 0:41 ` [PATCH 17/18] xfs: ATTR_REPLACE algorithm with LARP enabled needs rework Dave Chinner
2022-05-10 22:31 ` Alli
2022-05-10 23:53 ` Darrick J. Wong
2022-05-11 1:14 ` Dave Chinner
2022-05-09 0:41 ` [PATCH 18/18] xfs: detect empty attr leaf blocks in xfs_attr3_leaf_verify Dave Chinner
2022-05-10 22:31 ` Alli
2022-05-10 23:54 ` Darrick J. Wong
2022-05-10 22:27 ` [PATCH 19/18] xfs: can't use kmem_zalloc() for attribute buffers Dave Chinner
2022-05-10 23:59 ` Darrick J. Wong
2022-05-11 0:54 ` Dave Chinner
2022-05-11 1:10 ` Darrick J. Wong
2022-05-11 0:54 ` Alli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220510232258.GL27195@magnolia \
--to=djwong@kernel.org \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.