From: Brian Foster <bfoster@redhat.com>
To: Allison Collins <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v9 22/24] xfs: Add delay ready attr remove routines
Date: Thu, 7 May 2020 07:54:13 -0400 [thread overview]
Message-ID: <20200507115413.GD9003@bfoster> (raw)
In-Reply-To: <20200430225016.4287-23-allison.henderson@oracle.com>
On Thu, Apr 30, 2020 at 03:50:14PM -0700, Allison Collins wrote:
...
>
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/libxfs/xfs_attr.c | 159 ++++++++++++++++++++++++++++++----------
> fs/xfs/libxfs/xfs_attr.h | 73 ++++++++++++++++++
> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> fs/xfs/libxfs/xfs_attr_remote.c | 31 ++++----
> fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
> fs/xfs/xfs_attr_inactive.c | 2 +-
> 6 files changed, 215 insertions(+), 54 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index c8cae68..7213589 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
...
> @@ -363,23 +384,57 @@ xfs_has_attr(
...
> +/*
> + * Remove the attribute specified in @args.
> + *
> + * This function may return -EAGAIN to signal that the transaction needs to be
> + * rolled. Callers should continue calling this function until they receive a
> + * return value other than -EAGAIN.
> + */
> +int
> +xfs_attr_remove_iter(
> + struct xfs_delattr_context *dac)
> {
> + struct xfs_da_args *args = dac->da_args;
> struct xfs_inode *dp = args->dp;
> - int error;
> +
> + if (dac->dela_state == XFS_DAS_RM_SHRINK)
> + goto node;
If we're in the above state, doesn't that mean we're in node format and
should fall through to the node function based on the logic below
anyways?
>
> if (!xfs_inode_hasattr(dp)) {
> - error = -ENOATTR;
> + return -ENOATTR;
> } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> - error = xfs_attr_shortform_remove(args);
> + return xfs_attr_shortform_remove(args);
> } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> - error = xfs_attr_leaf_removename(args);
> - } else {
> - error = xfs_attr_node_removename(args);
> + return xfs_attr_leaf_removename(args);
> }
> -
> - return error;
> +node:
> + return xfs_attr_node_removename(dac);
> }
>
> /*
...
> @@ -1232,21 +1299,35 @@ xfs_attr_node_removename_rmt (
> * This will involve walking down the Btree, and may involve joining
> * leaf nodes and even joining intermediate nodes up to and including
> * the root node (a special case of an intermediate node).
> + *
> + * This routine is meant to function as either an inline or delayed operation,
> + * and may return -EAGAIN when the transaction needs to be rolled. Calling
> + * functions will need to handle this, and recall the function until a
> + * successful error code is returned.
> */
> STATIC int
> xfs_attr_node_removename(
> - struct xfs_da_args *args)
> + struct xfs_delattr_context *dac)
> {
> - struct xfs_da_state *state;
> - struct xfs_da_state_blk *blk;
> - int retval, error;
> - struct xfs_inode *dp = args->dp;
> + struct xfs_da_args *args = dac->da_args;
> + struct xfs_da_state *state;
> + struct xfs_da_state_blk *blk;
> + int retval, error;
> + struct xfs_inode *dp = args->dp;
>
> trace_xfs_attr_node_removename(args);
> + state = dac->da_state;
> + blk = dac->blk;
>
> - error = xfs_attr_node_removename_setup(args, &state);
> - if (error)
> - goto out;
> + if (dac->dela_state == XFS_DAS_RM_SHRINK)
> + goto das_rm_shrink;
> +
> + if ((dac->flags & XFS_DAC_NODE_RMVNAME_INIT) == 0) {
> + dac->flags |= XFS_DAC_NODE_RMVNAME_INIT;
> + error = xfs_attr_node_removename_setup(dac, &state);
> + if (error)
> + goto out;
> + }
Now that we've factored out this state with the flag, I wonder if we
could factor out this logic and pull it all the way up to
xfs_attr_remove_args(). That would remove the init flag, we'd presumably
just have to duplicate the format checks to make sure the fork is in
node format.
I'd consider that a followup patch, btw. I know it's a bit circuitous to
add the flag only to then remove it, but I think there's value in making
simple, incremental changes. ;)
>
> /*
> * If there is an out-of-line value, de-allocate the blocks.
> @@ -1254,8 +1335,13 @@ xfs_attr_node_removename(
> * overflow the maximum size of a transaction and/or hit a deadlock.
> */
> if (args->rmtblkno > 0) {
> - error = xfs_attr_node_removename_rmt(args, state);
> - if (error)
> + /*
> + * May return -EAGAIN. Remove blocks until args->rmtblkno == 0
> + */
> + error = xfs_attr_node_removename_rmt(dac, state);
> + if (error == -EAGAIN)
> + return error;
> + else if (error)
> goto out;
> }
>
> @@ -1274,17 +1360,14 @@ xfs_attr_node_removename(
> error = xfs_da3_join(state);
> if (error)
> goto out;
> - error = xfs_defer_finish(&args->trans);
> - if (error)
> - goto out;
> - /*
> - * Commit the Btree join operation and start a new trans.
> - */
> - error = xfs_trans_roll_inode(&args->trans, dp);
> - if (error)
> - goto out;
> +
> + dac->flags |= XFS_DAC_DEFER_FINISH;
> + dac->dela_state = XFS_DAS_RM_SHRINK;
> + return -EAGAIN;
> }
>
> +das_rm_shrink:
> +
Alternatively to the state question in xfs_attr_remove_iter(), I wonder
if we could lift the bottom part of this function (that does the tree
collapse and potential node shrink) into the caller. AFAICT that would
isolate all of the xattr remove state tracking code to
xfs_attr_remove_iter(), which would be a pretty nice result IMO.
> /*
> * If the result is small enough, push it all into the inode.
> */
...
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index f770159..f2d46c7 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
...
> @@ -691,10 +693,14 @@ xfs_attr_rmtval_remove(
> lblkno = args->rmtblkno;
> blkcnt = args->rmtblkcnt;
> do {
> - retval = __xfs_attr_rmtval_remove(args);
> + retval = __xfs_attr_rmtval_remove(&dac);
> if (retval && retval != EAGAIN)
> return retval;
>
> + error = xfs_attr_defer_finish(&dac);
> + if (error)
> + break;
> +
This seems to come out of nowhere..?
Brian
> /*
> * Close out trans and start the next one in the chain.
> */
> @@ -713,9 +719,10 @@ xfs_attr_rmtval_remove(
> */
> int
> __xfs_attr_rmtval_remove(
> - struct xfs_da_args *args)
> + struct xfs_delattr_context *dac)
> {
> - int error, done;
> + struct xfs_da_args *args = dac->da_args;
> + int error, done;
>
> /*
> * Unmap value blocks for this attr.
> @@ -725,12 +732,10 @@ __xfs_attr_rmtval_remove(
> if (error)
> return error;
>
> - error = xfs_defer_finish(&args->trans);
> - if (error)
> - return error;
> -
> - if (!done)
> + if (!done) {
> + dac->flags &= ~XFS_DAC_DEFER_FINISH;
> return -EAGAIN;
> + }
>
> return error;
> }
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> index ee3337b..351da00 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.h
> +++ b/fs/xfs/libxfs/xfs_attr_remote.h
> @@ -14,5 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args);
> int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map,
> xfs_buf_flags_t incore_flags);
> int xfs_attr_rmtval_invalidate(struct xfs_da_args *args);
> -int __xfs_attr_rmtval_remove(struct xfs_da_args *args);
> +int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
> #endif /* __XFS_ATTR_REMOTE_H__ */
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index c42f90e..b2150fa 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -15,10 +15,10 @@
> #include "xfs_da_format.h"
> #include "xfs_da_btree.h"
> #include "xfs_inode.h"
> +#include "xfs_attr.h"
> #include "xfs_attr_remote.h"
> #include "xfs_trans.h"
> #include "xfs_bmap.h"
> -#include "xfs_attr.h"
> #include "xfs_attr_leaf.h"
> #include "xfs_quota.h"
> #include "xfs_dir2.h"
> --
> 2.7.4
>
next prev parent reply other threads:[~2020-05-07 11:54 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-30 22:49 [PATCH v9 00/24] xfs: Delay Ready Attributes Allison Collins
2020-04-30 22:49 ` [PATCH v9 01/24] xfs: Add xfs_has_attr and subroutines Allison Collins
2020-04-30 22:49 ` [PATCH v9 02/24] xfs: Check for -ENOATTR or -EEXIST Allison Collins
2020-04-30 22:49 ` [PATCH v9 03/24] xfs: Factor out new helper functions xfs_attr_rmtval_set Allison Collins
2020-04-30 22:49 ` [PATCH v9 04/24] xfs: Pull up trans handling in xfs_attr3_leaf_flipflags Allison Collins
2020-04-30 22:49 ` [PATCH v9 05/24] xfs: Split apart xfs_attr_leaf_addname Allison Collins
2020-05-04 17:33 ` Darrick J. Wong
2020-05-04 22:34 ` Allison Collins
2020-04-30 22:49 ` [PATCH v9 06/24] xfs: Refactor xfs_attr_try_sf_addname Allison Collins
2020-04-30 22:49 ` [PATCH v9 07/24] xfs: Pull up trans roll from xfs_attr3_leaf_setflag Allison Collins
2020-04-30 22:50 ` [PATCH v9 08/24] xfs: Factor out xfs_attr_rmtval_invalidate Allison Collins
2020-04-30 22:50 ` [PATCH v9 09/24] xfs: Pull up trans roll in xfs_attr3_leaf_clearflag Allison Collins
2020-04-30 22:50 ` [PATCH v9 10/24] xfs: Add helper function __xfs_attr_rmtval_remove Allison Collins
2020-05-04 13:27 ` Brian Foster
2020-05-04 21:36 ` Allison Collins
2020-05-05 12:03 ` Brian Foster
2020-05-05 17:35 ` Allison Collins
2020-05-04 17:41 ` Darrick J. Wong
2020-05-04 22:53 ` Allison Collins
2020-05-04 22:57 ` Darrick J. Wong
2020-04-30 22:50 ` [PATCH v9 11/24] xfs: Pull up xfs_attr_rmtval_invalidate Allison Collins
2020-05-04 13:27 ` Brian Foster
2020-05-04 21:37 ` Allison Collins
2020-05-04 17:41 ` Darrick J. Wong
2020-05-04 22:55 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 12/24] xfs: Add helper function xfs_attr_node_shrink Allison Collins
2020-05-04 13:27 ` Brian Foster
2020-05-04 21:37 ` Allison Collins
2020-05-04 17:42 ` Darrick J. Wong
2020-05-04 22:55 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 13/24] xfs: Remove unneeded xfs_trans_roll_inode calls Allison Collins
2020-05-04 13:30 ` Brian Foster
2020-05-04 22:15 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 14/24] xfs: Add helpers xfs_attr_is_shortform and xfs_attr_set_shortform Allison Collins
2020-05-04 18:46 ` Darrick J. Wong
2020-05-05 0:29 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 15/24] xfs: Add helper function xfs_attr_leaf_mark_incomplete Allison Collins
2020-05-04 18:47 ` Darrick J. Wong
2020-04-30 22:50 ` [PATCH v9 16/24] xfs: Add remote block helper functions Allison Collins
2020-05-04 18:55 ` Darrick J. Wong
2020-05-04 23:01 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 17/24] xfs: Add helper function xfs_attr_node_removename_setup Allison Collins
2020-05-04 18:58 ` Darrick J. Wong
2020-05-04 23:02 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 18/24] xfs: Add helper function xfs_attr_node_removename_rmt Allison Collins
2020-05-04 19:00 ` Darrick J. Wong
2020-05-04 23:04 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 19/24] xfs: Simplify xfs_attr_leaf_addname Allison Collins
2020-05-04 19:03 ` Darrick J. Wong
2020-05-04 23:06 ` Allison Collins
2020-05-05 13:11 ` Brian Foster
2020-05-05 17:35 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 20/24] xfs: Simplify xfs_attr_node_addname Allison Collins
2020-05-04 19:06 ` Darrick J. Wong
2020-05-04 23:16 ` Allison Collins
2020-05-05 13:12 ` Brian Foster
2020-05-05 17:35 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 21/24] xfs: Lift -ENOSPC handler from xfs_attr_leaf_addname Allison Collins
2020-05-04 19:10 ` Darrick J. Wong
2020-05-04 23:17 ` Allison Collins
2020-05-05 13:12 ` Brian Foster
2020-05-05 17:37 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 22/24] xfs: Add delay ready attr remove routines Allison Collins
2020-05-04 19:33 ` Darrick J. Wong
2020-05-05 0:26 ` Allison Collins
2020-05-05 1:44 ` Darrick J. Wong
2020-05-05 17:35 ` Allison Collins
2020-05-07 11:54 ` Brian Foster [this message]
2020-04-30 22:50 ` [PATCH v9 23/24] xfs: Add delay ready attr set routines Allison Collins
2020-05-04 19:49 ` Darrick J. Wong
2020-05-05 1:31 ` Allison Collins
2020-05-04 22:36 ` Darrick J. Wong
2020-05-05 1:32 ` Allison Collins
2020-04-30 22:50 ` [PATCH v9 24/24] xfs: Rename __xfs_attr_rmtval_remove Allison Collins
2020-05-04 19:34 ` Darrick J. Wong
2020-05-05 0:27 ` Allison Collins
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=20200507115413.GD9003@bfoster \
--to=bfoster@redhat.com \
--cc=allison.henderson@oracle.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;
as well as URLs for NNTP newsgroup(s).