From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 04/18] xfs: rework deferred attribute operation setup
Date: Tue, 10 May 2022 16:04:51 -0700 [thread overview]
Message-ID: <20220510230451.GH27195@magnolia> (raw)
In-Reply-To: <20220509004138.762556-5-david@fromorbit.com>
On Mon, May 09, 2022 at 10:41:24AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Logged attribute intents only have set and remove types - there is
> no type of the replace operation. We should ahve a separate type for
"..no separate intent type for a replace operation."?
Also, "ahve" -> "have".
> a replace operation, as it needs to perform operations that neither
> SET or REMOVE can perform.
>
> Add this type to the intent items and rearrange the deferred
> operation setup to reflect the different operations we are
> performing.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Allison Henderson<allison.henderson@oracle.com>
> ---
> fs/xfs/libxfs/xfs_attr.c | 165 +++++++++++++++++++--------------
> fs/xfs/libxfs/xfs_attr.h | 2 -
> fs/xfs/libxfs/xfs_log_format.h | 1 +
> fs/xfs/xfs_attr_item.c | 9 +-
> fs/xfs/xfs_trace.h | 4 +
> 5 files changed, 110 insertions(+), 71 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index a4b0b20a3bab..817e15740f9c 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -671,6 +671,81 @@ xfs_attr_lookup(
> return xfs_attr_node_hasname(args, NULL);
> }
>
> +static int
> +xfs_attr_item_init(
> + struct xfs_da_args *args,
> + unsigned int op_flags, /* op flag (set or remove) */
> + struct xfs_attr_item **attr) /* new xfs_attr_item */
> +{
> +
> + struct xfs_attr_item *new;
> +
> + new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS);
Can this fail?
> + new->xattri_op_flags = op_flags;
> + new->xattri_da_args = args;
> +
> + *attr = new;
> + return 0;
And if it can't, then there's no need for a return value, AFAICT. I
looked at the end of xfs-5.19-compose and there's no other return
statements in this function.
And then you don't need any of the error handling in this patch at all,
right?
*OH*, this is just a hoist of the stuff at the end. Could someone add a
patch on the end of ... whatever patchset there is to clean that up?
In the meantime, with the commit message typos cleaned up,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> +}
> +
> +/* Sets an attribute for an inode as a deferred operation */
> +static int
> +xfs_attr_defer_add(
> + struct xfs_da_args *args)
> +{
> + struct xfs_attr_item *new;
> + int error = 0;
> +
> + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
> + if (error)
> + return error;
> +
> + new->xattri_dela_state = XFS_DAS_UNINIT;
> + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> + trace_xfs_attr_defer_add(new->xattri_dela_state, args->dp);
> +
> + return 0;
> +}
> +
> +/* Sets an attribute for an inode as a deferred operation */
> +static int
> +xfs_attr_defer_replace(
> + struct xfs_da_args *args)
> +{
> + struct xfs_attr_item *new;
> + int error = 0;
> +
> + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REPLACE, &new);
> + if (error)
> + return error;
> +
> + new->xattri_dela_state = XFS_DAS_UNINIT;
> + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> + trace_xfs_attr_defer_replace(new->xattri_dela_state, args->dp);
> +
> + return 0;
> +}
> +
> +/* Removes an attribute for an inode as a deferred operation */
> +static int
> +xfs_attr_defer_remove(
> + struct xfs_da_args *args)
> +{
> +
> + struct xfs_attr_item *new;
> + int error;
> +
> + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new);
> + if (error)
> + return error;
> +
> + new->xattri_dela_state = XFS_DAS_UNINIT;
> + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> + trace_xfs_attr_defer_remove(new->xattri_dela_state, args->dp);
> +
> + return 0;
> +}
> +
> /*
> * Note: If args->value is NULL the attribute will be removed, just like the
> * Linux ->setattr API.
> @@ -759,29 +834,35 @@ xfs_attr_set(
> }
>
> error = xfs_attr_lookup(args);
> - if (args->value) {
> - if (error == -EEXIST && (args->attr_flags & XATTR_CREATE))
> - goto out_trans_cancel;
> - if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
> - goto out_trans_cancel;
> - if (error != -ENOATTR && error != -EEXIST)
> + switch (error) {
> + case -EEXIST:
> + /* if no value, we are performing a remove operation */
> + if (!args->value) {
> + error = xfs_attr_defer_remove(args);
> + break;
> + }
> + /* Pure create fails if the attr already exists */
> + if (args->attr_flags & XATTR_CREATE)
> goto out_trans_cancel;
>
> - error = xfs_attr_set_deferred(args);
> - if (error)
> + error = xfs_attr_defer_replace(args);
> + break;
> + case -ENOATTR:
> + /* Can't remove what isn't there. */
> + if (!args->value)
> goto out_trans_cancel;
>
> - /* shortform attribute has already been committed */
> - if (!args->trans)
> - goto out_unlock;
> - } else {
> - if (error != -EEXIST)
> + /* Pure replace fails if no existing attr to replace. */
> + if (args->attr_flags & XATTR_REPLACE)
> goto out_trans_cancel;
>
> - error = xfs_attr_remove_deferred(args);
> - if (error)
> - goto out_trans_cancel;
> + error = xfs_attr_defer_add(args);
> + break;
> + default:
> + goto out_trans_cancel;
> }
> + if (error)
> + goto out_trans_cancel;
>
> /*
> * If this is a synchronous mount, make sure that the
> @@ -845,58 +926,6 @@ xfs_attrd_destroy_cache(void)
> xfs_attrd_cache = NULL;
> }
>
> -STATIC int
> -xfs_attr_item_init(
> - struct xfs_da_args *args,
> - unsigned int op_flags, /* op flag (set or remove) */
> - struct xfs_attr_item **attr) /* new xfs_attr_item */
> -{
> -
> - struct xfs_attr_item *new;
> -
> - new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS);
> - new->xattri_op_flags = op_flags;
> - new->xattri_da_args = args;
> -
> - *attr = new;
> - return 0;
> -}
> -
> -/* Sets an attribute for an inode as a deferred operation */
> -int
> -xfs_attr_set_deferred(
> - struct xfs_da_args *args)
> -{
> - struct xfs_attr_item *new;
> - int error = 0;
> -
> - error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
> - if (error)
> - return error;
> -
> - xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> -
> - return 0;
> -}
> -
> -/* Removes an attribute for an inode as a deferred operation */
> -int
> -xfs_attr_remove_deferred(
> - struct xfs_da_args *args)
> -{
> -
> - struct xfs_attr_item *new;
> - int error;
> -
> - error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new);
> - if (error)
> - return error;
> -
> - xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> -
> - return 0;
> -}
> -
> /*========================================================================
> * External routines when attribute list is inside the inode
> *========================================================================*/
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index f6c13d2bfbcd..c9c867e3406c 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -521,8 +521,6 @@ bool xfs_attr_namecheck(const void *name, size_t length);
> int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
> void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
> unsigned int *total);
> -int xfs_attr_set_deferred(struct xfs_da_args *args);
> -int xfs_attr_remove_deferred(struct xfs_da_args *args);
>
> extern struct kmem_cache *xfs_attri_cache;
> extern struct kmem_cache *xfs_attrd_cache;
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index a27492e99673..f7edd1ecf6d9 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -908,6 +908,7 @@ struct xfs_icreate_log {
> */
> #define XFS_ATTR_OP_FLAGS_SET 1 /* Set the attribute */
> #define XFS_ATTR_OP_FLAGS_REMOVE 2 /* Remove the attribute */
> +#define XFS_ATTR_OP_FLAGS_REPLACE 3 /* Replace the attribute */
> #define XFS_ATTR_OP_FLAGS_TYPE_MASK 0xFF /* Flags type mask */
>
> /*
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 5f8680b05079..fe1e37696634 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -311,6 +311,7 @@ xfs_xattri_finish_update(
>
> switch (op) {
> case XFS_ATTR_OP_FLAGS_SET:
> + case XFS_ATTR_OP_FLAGS_REPLACE:
> error = xfs_attr_set_iter(attr);
> break;
> case XFS_ATTR_OP_FLAGS_REMOVE:
> @@ -500,8 +501,14 @@ xfs_attri_validate(
> return false;
>
> /* alfi_op_flags should be either a set or remove */
> - if (op != XFS_ATTR_OP_FLAGS_SET && op != XFS_ATTR_OP_FLAGS_REMOVE)
> + switch (op) {
> + case XFS_ATTR_OP_FLAGS_SET:
> + case XFS_ATTR_OP_FLAGS_REPLACE:
> + case XFS_ATTR_OP_FLAGS_REMOVE:
> + break;
> + default:
> return false;
> + }
>
> if (attrp->alfi_value_len > XATTR_SIZE_MAX)
> return false;
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index fec4198b738b..01ce0401aa32 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -4154,6 +4154,10 @@ 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_remove_return);
> +DEFINE_DAS_STATE_EVENT(xfs_attr_defer_add);
> +DEFINE_DAS_STATE_EVENT(xfs_attr_defer_replace);
> +DEFINE_DAS_STATE_EVENT(xfs_attr_defer_remove);
> +
>
> TRACE_EVENT(xfs_force_shutdown,
> TP_PROTO(struct xfs_mount *mp, int ptag, int flags, const char *fname,
> --
> 2.35.1
>
next prev parent reply other threads:[~2022-05-10 23:05 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 [this message]
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
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=20220510230451.GH27195@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox