On Tue, Dec 22, 2020 at 12:11:48PM -0500, Brian Foster wrote:
> On Fri, Dec 18, 2020 at 12:29:06AM -0700, Allison Henderson 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. A new XFS_DAC_DEFER_FINISH flag is used to finish the
> > transaction where ever the existing code used to.
> >
> > Calls to xfs_attr_rmtval_remove are replaced with the delay ready
> > version __xfs_attr_rmtval_remove. We will rename
> > __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
> > done.
> >
> > xfs_attr_rmtval_remove itself is still in use by the set routines (used
> > during a rename). For reasons of preserving existing function, we
> > modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
> > set. Similar to how xfs_attr_remove_args does here. Once we transition
> > the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
> > used and will be removed.
> >
> > 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. See xfs_attr.h for a more
> > detailed diagram of the states.
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
>
> I started with a couple small comments on this patch but inevitably
> started thinking more about the factoring again and ended up with a
> couple patches on top. The first is more of some small tweaks and
> open-coding that IMO makes this patch a bit easier to follow. The
> second is more of an RFC so I'll follow up with that in a second email.
> I'm curious what folks' thoughts might be on either. Also note that I'm
> primarily focusing on code structure and whatnot here, so these are fast
> and loose, compile tested only and likely to be broken.
>
... and here's the second diff (applies on top of the first).
This one popped up after staring at the previous changes for a bit and
wondering whether using "done flags" might make the whole thing easier
to follow than incremental state transitions. I think the attr remove
path is easy enough to follow with either method, but the attr set path
is a beast and so this is more with that in mind. Initial thoughts?
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 2e466c4ac283..106e3c070131 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1271,14 +1271,12 @@ int xfs_attr_node_removename_setup(
* successful error code is returned.
*/
STATIC int
-xfs_attr_node_remove_step(
- struct xfs_delattr_context *dac,
- bool *joined)
+xfs_attr_node_remove_rmt_step(
+ struct xfs_delattr_context *dac)
{
struct xfs_da_args *args = dac->da_args;
struct xfs_da_state *state = dac->da_state;
- struct xfs_da_state_blk *blk;
- int error = 0, retval, done;
+ int error, done;
/*
* If there is an out-of-line value, de-allocate the blocks. This is
@@ -1300,6 +1298,19 @@ xfs_attr_node_remove_step(
return error;
}
+ dac->dela_state |= XFS_DAS_RMT_DONE;
+ return error;
+}
+
+STATIC int
+xfs_attr_node_remove_join_step(
+ struct xfs_delattr_context *dac)
+{
+ struct xfs_da_args *args = dac->da_args;
+ struct xfs_da_state *state = dac->da_state;
+ struct xfs_da_state_blk *blk;
+ int error, retval;
+
/*
* Remove the name and update the hashvals in the tree.
*/
@@ -1317,9 +1328,12 @@ xfs_attr_node_remove_step(
error = xfs_da3_join(state);
if (error)
return error;
- *joined = true;
+
+ error = -EAGAIN;
+ dac->flags |= XFS_DAC_DEFER_FINISH;
}
+ dac->dela_state |= XFS_DAS_JOIN_DONE;
return error;
}
@@ -1342,36 +1356,23 @@ xfs_attr_node_removename_iter(
struct xfs_da_state *state = dac->da_state;
int error;
struct xfs_inode *dp = args->dp;
- bool joined = false;
- switch (dac->dela_state) {
- case XFS_DAS_UNINIT:
- /*
- * repeatedly remove remote blocks, remove the entry and join.
- * returns -EAGAIN or 0 for completion of the step.
- */
- error = xfs_attr_node_remove_step(dac, &joined);
+ if (!(dac->dela_state & XFS_DAS_RMT_DONE)) {
+ error = xfs_attr_node_remove_rmt_step(dac);
if (error)
goto out;
- if (joined) {
- dac->flags |= XFS_DAC_DEFER_FINISH;
- dac->dela_state = XFS_DAS_RM_SHRINK;
- return -EAGAIN;
- }
- /* fallthrough */
- case XFS_DAS_RM_SHRINK:
- /*
- * If the result is small enough, push it all into the inode.
- */
- if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
- error = xfs_attr_node_shrink(args, state);
- break;
- default:
- ASSERT(0);
- error = -EINVAL;
- goto out;
}
+ if (!(dac->dela_state & XFS_DAS_JOIN_DONE)) {
+ error = xfs_attr_node_remove_join_step(dac);
+ if (error)
+ goto out;
+ }
+
+ if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
+ error = xfs_attr_node_shrink(args, state);
+ ASSERT(error != -EAGAIN);
+
out:
if (state && error != -EAGAIN)
xfs_da_state_free(state);
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 3154ef4b7833..67e730cd3267 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -151,6 +151,9 @@ enum xfs_delattr_state {
XFS_DAS_RM_SHRINK, /* We are shrinking the tree */
};
+#define XFS_DAS_RMT_DONE 0x1
+#define XFS_DAS_JOIN_DONE 0x2
+
/*
* Defines for xfs_delattr_context.flags
*/
