On Mon, Feb 24, 2020 at 10:25:55AM -0500, Brian Foster wrote:
> On Sat, Feb 22, 2020 at 07:06:05PM -0700, Allison Collins wrote:
> > This patch modifies the attr remove routines to be delay ready. This means they no
> > longer roll or commit transactions, but instead return -EAGAIN to have the calling
> > routine roll and refresh the transaction. In this series, xfs_attr_remove_args has
> > become xfs_attr_remove_iter, which uses a sort of state machine like switch to keep
> > track of where it was when EAGAIN was returned. xfs_attr_node_removename has also
> > been modified to use the switch, and a new version of xfs_attr_remove_args
> > consists of a simple loop to refresh the transaction until the operation is
> > completed.
> >
> > This patch also adds a new struct xfs_delattr_context, which we will use to keep
> > track of the current state of an attribute operation. The new xfs_delattr_state
> > enum is used to track various operations that are in progress so that we know not
> > to repeat them, and resume where we left off before EAGAIN was returned to cycle
> > out the transaction. Other members take the place of local variables that need
> > to retain their values across multiple function recalls.
> >
> > Below is a state machine diagram for attr remove operations. The XFS_DAS_* states
> > indicate places where the function would return -EAGAIN, and then immediately
> > resume from after being recalled by the calling function. States marked as a
> > "subroutine state" indicate that they belong to a subroutine, and so the calling
> > function needs to pass them back to that subroutine to allow it to finish where
> > it left off. But they otherwise do not have a role in the calling function other
> > than just passing through.
> >
> > xfs_attr_remove_iter()
> > XFS_DAS_RM_SHRINK ─┐
> > (subroutine state) │
> > │
> > XFS_DAS_RMTVAL_REMOVE ─┤
> > (subroutine state) │
> > └─>xfs_attr_node_removename()
> > │
> > v
> > need to remove
> > ┌─n── rmt blocks?
> > │ │
> > │ y
> > │ │
> > │ v
> > │ ┌─>XFS_DAS_RMTVAL_REMOVE
> > │ │ │
> > │ │ v
> > │ └──y── more blks
> > │ to remove?
> > │ │
> > │ n
> > │ │
> > │ v
> > │ need to
> > └─────> shrink tree? ─n─┐
> > │ │
> > y │
> > │ │
> > v │
> > XFS_DAS_RM_SHRINK │
> > │ │
> > v │
> > done <─────┘
> >
>
> Wow. :P I guess I have nothing against verbose commit logs, but I wonder
> how useful this level of documentation is for a patch that shouldn't
> really change the existing flow of the operation.
>
> > Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_attr.c | 114 +++++++++++++++++++++++++++++++++++++------
> > fs/xfs/libxfs/xfs_attr.h | 1 +
> > fs/xfs/libxfs/xfs_da_btree.h | 30 ++++++++++++
> > fs/xfs/scrub/common.c | 2 +
> > fs/xfs/xfs_acl.c | 2 +
> > fs/xfs/xfs_attr_list.c | 1 +
> > fs/xfs/xfs_ioctl.c | 2 +
> > fs/xfs/xfs_ioctl32.c | 2 +
> > fs/xfs/xfs_iops.c | 2 +
> > fs/xfs/xfs_xattr.c | 1 +
> > 10 files changed, 141 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 5d73bdf..cd3a3f7 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -368,11 +368,60 @@ xfs_has_attr(
> > */
> > int
> > xfs_attr_remove_args(
> > + struct xfs_da_args *args)
> > +{
> > + int error = 0;
> > + int err2 = 0;
> > +
> > + do {
> > + error = xfs_attr_remove_iter(args);
> > + if (error && error != -EAGAIN)
> > + goto out;
> > +
>
> I'm a little confused on the logic of this loop given that the only
> caller commits the transaction (which also finishes dfops). IOW, it
> seems we shouldn't ever need to finish/roll when error != -EAGAIN. If
> that is the case, this can be simplified to something like:
>
> int
> xfs_attr_remove_args(
> struct xfs_da_args *args)
> {
> int error;
>
> do {
> error = xfs_attr_remove_iter(args);
> if (error != -EAGAIN)
> break;
>
> if (args->dac.flags & XFS_DAC_FINISH_TRANS) {
> args->dac.flags &= ~XFS_DAC_FINISH_TRANS;
> error = xfs_defer_finish(&args->trans);
> if (error)
> break;
> }
>
> error = xfs_trans_roll_inode(&args->trans, args->dp);
> if (error)
> break;
> } while (true);
>
> return error;
> }
>
> That has the added benefit of eliminating the whole err2 pattern, which
> always strikes me as a landmine.
>
> > + if (args->dac.flags & XFS_DAC_FINISH_TRANS) {
>
> BTW, _FINISH_TRANS also seems misnamed given that we finish deferred
> operations, not necessarily the transaction. XFS_DAC_DEFER_FINISH?
>
> > + args->dac.flags &= ~XFS_DAC_FINISH_TRANS;
> > +
> > + err2 = xfs_defer_finish(&args->trans);
> > + if (err2) {
> > + error = err2;
> > + goto out;
> > + }
> > + }
> > +
> > + err2 = xfs_trans_roll_inode(&args->trans, args->dp);
> > + if (err2) {
> > + error = err2;
> > + goto out;
> > + }
> > +
> > + } while (error == -EAGAIN);
> > +out:
> > + return error;
> > +}
> > +
> > +/*
> > + * 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_da_args *args)
> > {
> > struct xfs_inode *dp = args->dp;
> > int error;
> >
> > + /* State machine switch */
> > + switch (args->dac.dela_state) {
> > + case XFS_DAS_RM_SHRINK:
> > + case XFS_DAS_RMTVAL_REMOVE:
> > + goto node;
> > + default:
> > + break;
> > + }
> > +
> > if (!xfs_inode_hasattr(dp)) {
> > error = -ENOATTR;
> > } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> > @@ -381,6 +430,7 @@ xfs_attr_remove_args(
> > } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > error = xfs_attr_leaf_removename(args);
> > } else {
> > +node:
> > error = xfs_attr_node_removename(args);
> > }
> >
> > @@ -895,9 +945,8 @@ xfs_attr_leaf_removename(
> > /* bp is gone due to xfs_da_shrink_inode */
> > if (error)
> > return error;
> > - error = xfs_defer_finish(&args->trans);
> > - if (error)
> > - return error;
> > +
> > + args->dac.flags |= XFS_DAC_FINISH_TRANS;
> > }
> > return 0;
> > }
> > @@ -1218,6 +1267,11 @@ xfs_attr_node_addname(
> > * 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(
> > @@ -1230,10 +1284,24 @@ xfs_attr_node_removename(
> > struct xfs_inode *dp = args->dp;
> >
> > trace_xfs_attr_node_removename(args);
> > + state = args->dac.da_state;
> > + blk = args->dac.blk;
> > +
> > + /* State machine switch */
> > + switch (args->dac.dela_state) {
> > + case XFS_DAS_RMTVAL_REMOVE:
> > + goto rm_node_blks;
> > + case XFS_DAS_RM_SHRINK:
> > + goto rm_shrink;
> > + default:
> > + break;
> > + }
> >
> > error = xfs_attr_node_hasname(args, &state);
> > if (error != -EEXIST)
> > goto out;
> > + else
> > + error = 0;
>
> This doesn't look necessary.
>
> >
> > /*
> > * If there is an out-of-line value, de-allocate the blocks.
> > @@ -1243,6 +1311,14 @@ xfs_attr_node_removename(
> > blk = &state->path.blk[ state->path.active-1 ];
> > ASSERT(blk->bp != NULL);
> > ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
> > +
> > + /*
> > + * Store blk and state in the context incase we need to cycle out the
> > + * transaction
> > + */
> > + args->dac.blk = blk;
> > + args->dac.da_state = state;
> > +
> > if (args->rmtblkno > 0) {
> > /*
> > * Fill in disk block numbers in the state structure
> > @@ -1261,13 +1337,21 @@ xfs_attr_node_removename(
> > if (error)
> > goto out;
> >
> > - error = xfs_trans_roll_inode(&args->trans, args->dp);
> > + error = xfs_attr_rmtval_invalidate(args);
>
> Remind me why we lose the above trans roll? I vaguely recall that this
> was intentional, but I could be mistaken...
>
> > if (error)
> > goto out;
> > + }
> >
> > - error = xfs_attr_rmtval_remove(args);
> > - if (error)
> > - goto out;
> > +rm_node_blks:
> > +
> > + if (args->rmtblkno > 0) {
> > + error = xfs_attr_rmtval_unmap(args);
> > +
> > + if (error) {
> > + if (error == -EAGAIN)
> > + args->dac.dela_state = XFS_DAS_RMTVAL_REMOVE;
>
> Might be helpful for the code labels to match the state names. I.e., use
> das_rmtval_remove: for the label above.
>
> > + return error;
> > + }
> >
> > /*
> > * Refill the state structure with buffers, the prior calls
> > @@ -1293,17 +1377,15 @@ 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;
> > +
> > + args->dac.flags |= XFS_DAC_FINISH_TRANS;
> > + args->dac.dela_state = XFS_DAS_RM_SHRINK;
> > + return -EAGAIN;
> > }
> >
> > +rm_shrink:
> > + args->dac.dela_state = XFS_DAS_RM_SHRINK;
> > +
>
> There's an xfs_defer_finish() call further down this function. Should
> that be replaced with the flag?
>
> Finally, I mentioned in a previous review that this function should
> probably be further broken down before fitting in the state management
> stuff. It doesn't look like that happened so I've attached a diff that
> is just intended to give an idea of what I mean by sectioning off the
> hunks that might be able to break down into helpers. The helpers
> wouldn't contain any state management, so we create a clear separation
> between the state code and functional components. I think this initial
> refactoring would make the introduction of state much more simple (and
> perhaps alleviate the need for the huge diagram). It might also be
> interesting to see how much of the result could be folded up further
> into _removename_iter()...
>
Gah.. attached for real this time.
Brian
> Brian
>
> > /*
> > * If the result is small enough, push it all into the inode.
> > */
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index ce7b039..ea873a5 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -155,6 +155,7 @@ int xfs_attr_set_args(struct xfs_da_args *args);
> > int xfs_attr_remove(struct xfs_inode *dp, struct xfs_name *name, int flags);
> > int xfs_has_attr(struct xfs_da_args *args);
> > int xfs_attr_remove_args(struct xfs_da_args *args);
> > +int xfs_attr_remove_iter(struct xfs_da_args *args);
> > int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> > int flags, struct attrlist_cursor_kern *cursor);
> > bool xfs_attr_namecheck(const void *name, size_t length);
> > diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
> > index 14f1be3..3c78498 100644
> > --- a/fs/xfs/libxfs/xfs_da_btree.h
> > +++ b/fs/xfs/libxfs/xfs_da_btree.h
> > @@ -50,9 +50,39 @@ enum xfs_dacmp {
> > };
> >
> > /*
> > + * Enum values for xfs_delattr_context.da_state
> > + *
> > + * These values are used by delayed attribute operations to keep track of where
> > + * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
> > + * calling function to roll the transaction, and then recall the subroutine to
> > + * finish the operation. The enum is then used by the subroutine to jump back
> > + * to where it was and resume executing where it left off.
> > + */
> > +enum xfs_delattr_state {
> > + XFS_DAS_RM_SHRINK, /* We are shrinking the tree */
> > + XFS_DAS_RMTVAL_REMOVE, /* We are removing remote value blocks */
> > +};
> > +
> > +/*
> > + * Defines for xfs_delattr_context.flags
> > + */
> > +#define XFS_DAC_FINISH_TRANS 0x1 /* indicates to finish the transaction */
> > +
> > +/*
> > + * Context used for keeping track of delayed attribute operations
> > + */
> > +struct xfs_delattr_context {
> > + struct xfs_da_state *da_state;
> > + struct xfs_da_state_blk *blk;
> > + unsigned int flags;
> > + enum xfs_delattr_state dela_state;
> > +};
> > +
> > +/*
> > * Structure to ease passing around component names.
> > */
> > typedef struct xfs_da_args {
> > + struct xfs_delattr_context dac; /* context used for delay attr ops */
> > struct xfs_da_geometry *geo; /* da block geometry */
> > struct xfs_name name; /* name, length and argument flags*/
> > uint8_t filetype; /* filetype of inode for directories */
> > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> > index 1887605..9a649d1 100644
> > --- a/fs/xfs/scrub/common.c
> > +++ b/fs/xfs/scrub/common.c
> > @@ -24,6 +24,8 @@
> > #include "xfs_rmap_btree.h"
> > #include "xfs_log.h"
> > #include "xfs_trans_priv.h"
> > +#include "xfs_da_format.h"
> > +#include "xfs_da_btree.h"
> > #include "xfs_attr.h"
> > #include "xfs_reflink.h"
> > #include "scrub/scrub.h"
> > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> > index 42ac847..d65e6d8 100644
> > --- a/fs/xfs/xfs_acl.c
> > +++ b/fs/xfs/xfs_acl.c
> > @@ -10,6 +10,8 @@
> > #include "xfs_trans_resv.h"
> > #include "xfs_mount.h"
> > #include "xfs_inode.h"
> > +#include "xfs_da_format.h"
> > +#include "xfs_da_btree.h"
> > #include "xfs_attr.h"
> > #include "xfs_trace.h"
> > #include "xfs_error.h"
> > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> > index d37743b..881b9a4 100644
> > --- a/fs/xfs/xfs_attr_list.c
> > +++ b/fs/xfs/xfs_attr_list.c
> > @@ -12,6 +12,7 @@
> > #include "xfs_trans_resv.h"
> > #include "xfs_mount.h"
> > #include "xfs_da_format.h"
> > +#include "xfs_da_btree.h"
> > #include "xfs_inode.h"
> > #include "xfs_trans.h"
> > #include "xfs_bmap.h"
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 28c07c9..7c1d9da 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -15,6 +15,8 @@
> > #include "xfs_iwalk.h"
> > #include "xfs_itable.h"
> > #include "xfs_error.h"
> > +#include "xfs_da_format.h"
> > +#include "xfs_da_btree.h"
> > #include "xfs_attr.h"
> > #include "xfs_bmap.h"
> > #include "xfs_bmap_util.h"
> > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> > index 769581a..d504f8f 100644
> > --- a/fs/xfs/xfs_ioctl32.c
> > +++ b/fs/xfs/xfs_ioctl32.c
> > @@ -17,6 +17,8 @@
> > #include "xfs_itable.h"
> > #include "xfs_fsops.h"
> > #include "xfs_rtalloc.h"
> > +#include "xfs_da_format.h"
> > +#include "xfs_da_btree.h"
> > #include "xfs_attr.h"
> > #include "xfs_ioctl.h"
> > #include "xfs_ioctl32.h"
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index e85bbf5..a2d299f 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -13,6 +13,8 @@
> > #include "xfs_inode.h"
> > #include "xfs_acl.h"
> > #include "xfs_quota.h"
> > +#include "xfs_da_format.h"
> > +#include "xfs_da_btree.h"
> > #include "xfs_attr.h"
> > #include "xfs_trans.h"
> > #include "xfs_trace.h"
> > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> > index 74133a5..d8dc72d 100644
> > --- a/fs/xfs/xfs_xattr.c
> > +++ b/fs/xfs/xfs_xattr.c
> > @@ -10,6 +10,7 @@
> > #include "xfs_log_format.h"
> > #include "xfs_da_format.h"
> > #include "xfs_inode.h"
> > +#include "xfs_da_btree.h"
> > #include "xfs_attr.h"
> > #include "xfs_acl.h"
> >
> > --
> > 2.7.4
> >
>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index cd3a3f75c429..e0eaa274b70b 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1297,6 +1297,7 @@ xfs_attr_node_removename(
break;
}
+#if 0
error = xfs_attr_node_hasname(args, &state);
if (error != -EEXIST)
goto out;
@@ -1341,9 +1342,13 @@ xfs_attr_node_removename(
if (error)
goto out;
}
+#else
+ error = xfs_attr_node_removename_setup();
+#endif
rm_node_blks:
+#if 0
if (args->rmtblkno > 0) {
error = xfs_attr_rmtval_unmap(args);
@@ -1361,6 +1366,11 @@ xfs_attr_node_removename(
if (error)
goto out;
}
+#else
+ args->dac.dela_state = XFS_DAS_RMTVAL_REMOVE;
+ error = xfs_attr_node_removename_rmt();
+ /* -EAGAIN */
+#endif
/*
* Remove the name and update the hashvals in the tree.
@@ -1370,6 +1380,7 @@ xfs_attr_node_removename(
retval = xfs_attr3_leaf_remove(blk->bp, args);
xfs_da3_fixhashpath(state, &state->path);
+#if 0
/*
* Check to see if the tree needs to be collapsed.
*/
@@ -1413,6 +1424,12 @@ xfs_attr_node_removename(
xfs_trans_brelse(args->trans, bp);
}
error = 0;
+#else
+rm_shrink:
+ args->dac.dela_state = XFS_DAS_RM_SHRINK;
+ error = xfs_attr_node_removename_shrink();
+ /* -EAGAIN */
+#endif
out:
if (state)
