Re: [PATCH v11 00/25] xfs: Delay Ready Attributes

[Dave Chinner <david@fromorbit.com>]

On Mon, Jul 20, 2020 at 05:15:41PM -0700, Allison Collins wrote:
> Hi all,
> 
> This set is a subset of a larger series for delayed attributes. Which is a
> subset of an even larger series, parent pointers. Delayed attributes allow
> attribute operations (set and remove) to be logged and committed in the same
> way that other delayed operations do. This allows more complex operations (like
> parent pointers) to be broken up into multiple smaller transactions. To do
> this, the existing attr operations must be modified to operate as either a
> delayed operation or a inline operation since older filesystems will not be
> able to use the new log entries.  This means that they cannot roll, commit, or
> finish transactions.  Instead, they return -EAGAIN to allow the calling
> function to handle the transaction. In this series, we focus on only the clean
> up and refactoring needed to accomplish this. We will introduce delayed attrs
> and parent pointers in a later set.
> 
> At the moment, I would like people to focus their review efforts on just this
> "delay ready" subseries, as I think that is a more conservative use of peoples
> review time.  I also think the set is a bit much to manage all at once, and we
> need to get the infrastructure ironed out before we focus too much anything
> that depends on it. But I do have the extended series for folks that want to
> see the bigger picture of where this is going.
> 
> To help organize the set, I've arranged the patches to make sort of mini sets.
> I thought it would help reviewers break down the reviewing some. For reviewing
> purposes, the set could be broken up into 4 different phases:
> 
> Error code filtering (patches 1-2):
> These two patches are all about finding and catching error codes that need to
> be sent back up to user space before starting delayed operations.  Errors that
> happen during a delayed operation are treated like internal errors that cause a
> shutdown.  But we wouldnt want that for example: when the user tries to rename
> a non existent attr.  So the idea is that we need to find all such conditions,
> and take care of them before starting a delayed operation.
>    xfs: Add xfs_has_attr and subroutines
>    xfs: Check for -ENOATTR or -EEXIST
> 
> Move transactions upwards (patches 3-12): 
> The goal of this subset is to try and move all the transaction specific code up
> the call stack much as possible.  The idea being that once we get them to the
> top, we can introduce the statemachine to handle the -EAGAIN logic where ever
> the transactions used to be.
>   xfs: Factor out new helper functions xfs_attr_rmtval_set
>   xfs: Pull up trans handling in xfs_attr3_leaf_flipflags
>   xfs: Split apart xfs_attr_leaf_addname
>   xfs: Refactor xfs_attr_try_sf_addname
>   xfs: Pull up trans roll from xfs_attr3_leaf_setflag
>   xfs: Factor out xfs_attr_rmtval_invalidate
>   xfs: Pull up trans roll in xfs_attr3_leaf_clearflag
>   xfs: Refactor xfs_attr_rmtval_remove
>   xfs: Pull up xfs_attr_rmtval_invalidate
>   xfs: Add helper function xfs_attr_node_shrink
> 
> Modularizing and cleanups (patches 13-22):
> Now that we have pulled the transactions up to where we need them, it's time to
> start breaking down the top level functions into new subfunctions. The goal
> being to work towards a top level function that deals mostly with the
> statemachine, and helpers for those states
>   xfs: Remove unneeded xfs_trans_roll_inode calls
>   xfs: Remove xfs_trans_roll in xfs_attr_node_removename
>   xfs: Add helpers xfs_attr_is_shortform and xfs_attr_set_shortform
>   xfs: Add helper function xfs_attr_leaf_mark_incomplete
>   xfs: Add remote block helper functions
>   xfs: Add helper function xfs_attr_node_removename_setup
>   xfs: Add helper function xfs_attr_node_removename_rmt
>   xfs: Simplify xfs_attr_leaf_addname
>   xfs: Simplify xfs_attr_node_addname
>   xfs: Lift -ENOSPC handler from xfs_attr_leaf_addname

I'm happy to see everything up to here merged.

> Introduce statemachine (patches 23-25):
> Now that we have re-arranged the code such that we can remove the transaction
> handling, we proceed to do so.  The behavior of the attr set/remove routines
> are now also compatible as a .finish_item callback
>   xfs: Add delay ready attr remove routines
>   xfs: Add delay ready attr set routines
>   xfs: Rename __xfs_attr_rmtval_remove

However, I think these still need work. The state machine mechanism
needs more factoring so that there is a function per state/action
that moves the state forwards one step, rather than the current
setup where a single function might handle 3-5 different states by
jumping to different parts of the code.

I started thinking that this was largely just code re-arrangement,
but as I started to to do a bit of cleanup onit as an example, I
think there's some bugs in the code that might be leading to leaking
buffers and other such stuff. So I think this code really needs a
good cleanup and going over before it will be ready to merge.

I've attached an untested, uncompiled patch below to demonstrate
how I think we should start flattening this out. It takes
xfs_attr_set_iter() and flattens it into a switch statement which
calls fine grained functions. There are a couple of new states to
efficiently jump into the state machine based on initial attr fork
format, and xfs_attr_set_iter() goes away entirely.

Note that I haven't flattened the second level of function calls out
into a function per state, which is where it's like to see this end
up. i.e. we don't need to call through xfs_attr_leaf_addname() to
just get to another switch statement to jump to the code that flips
the flags.

The factoring and flattening process is the same, though: isolate
the code that needs to run to a single function, call it from the
state machine switch. That function then selects the next state to
run, whether a transaction roll is necessary, etc, and the main loop
then takes care of it from there.

Allison, if you want I can put together another couple of patches
like the one below that flatten it right out into fine grained
states and functions. I think having all the state machien
transitions in one spot makes the code much easier to understand
and, later, to simplify and optimise.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

---
 fs/xfs/libxfs/xfs_attr.c | 246 ++++++++++++++++++++++++++---------------------
 fs/xfs/libxfs/xfs_attr.h |   2 +
 2 files changed, 139 insertions(+), 109 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 88172459c8f4..8ebe09b7c26a 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -59,8 +59,6 @@ STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
 STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
 STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
 STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp);
-STATIC int xfs_attr_set_iter(struct xfs_delattr_context *dac,
-			     struct xfs_buf **leaf_bp);
 
 int
 xfs_inode_hasattr(
@@ -252,13 +250,7 @@ xfs_attr_set_shortform(
 	if (error)
 		return error;
 
-	/*
-	 * Prevent the leaf buffer from being unlocked so that a concurrent AIL
-	 * push cannot grab the half-baked leaf buffer and run into problems
-	 * with the write verifier.
-	 */
-	xfs_trans_bhold(args->trans, *leaf_bp);
-	return -EAGAIN;
+	return -ENOSPC;
 }
 
 /*
@@ -288,47 +280,35 @@ xfs_attr_trans_roll(
 	return xfs_trans_roll_inode(&args->trans, args->dp);
 }
 
-/*
- * Set the attribute specified in @args.
- */
-int
-xfs_attr_set_args(
-	struct xfs_da_args	*args)
+static int
+xfs_attr_das_set_sf(
+	struct xfs_delattr_context	*dac,
+	struct xfs_buf			**leaf_bp)
 {
-	struct xfs_buf			*leaf_bp = NULL;
-	int				error = 0;
-	struct xfs_delattr_context	dac = {
-		.da_args	= args,
-	};
-
-	do {
-		error = xfs_attr_set_iter(&dac, &leaf_bp);
-		if (error != -EAGAIN)
-			break;
 
-		error = xfs_attr_trans_roll(&dac);
-		if (error)
-			return error;
-
-		if (leaf_bp) {
-			xfs_trans_bjoin(args->trans, leaf_bp);
-			xfs_trans_bhold(args->trans, leaf_bp);
-		}
+	/*
+	 * If the attr was successfully set in shortform, no need to
+	 * continue.  Otherwise, is it converted from shortform to leaf
+	 * and -EAGAIN is returned.
+	 */
+	error = xfs_attr_set_shortform(args, leaf_bp);
+	if (error != ENOSPC)
+		return error;
 
-	} while (true);
+	dac->flags |= XFS_DAC_DEFER_FINISH;
+	dac->state = XFS_DAS_FIND_LBLK;
 
-	return error;
+	/*
+	 * Prevent the leaf buffer from being unlocked while we roll the
+	 * transaction so that a concurrent AIL push cannot grab the half-baked
+	 * leaf buffer and run into problems with the write verifier.
+	 */
+	xfs_trans_bhold(args->trans, *leaf_bp);
+	return -EAGAIN;
 }
 
-/*
- * Set the attribute specified in @args.
- * This routine is meant to function as a 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_set_iter(
+static int
+xfs_attr_das_find_leaf(
 	struct xfs_delattr_context	*dac,
 	struct xfs_buf			**leaf_bp)
 {
@@ -336,89 +316,137 @@ xfs_attr_set_iter(
 	struct xfs_inode		*dp = args->dp;
 	int				error = 0;
 
-	/* State machine switch */
-	switch (dac->dela_state) {
-	case XFS_DAS_FLIP_LFLAG:
-	case XFS_DAS_FOUND_LBLK:
-		goto das_leaf;
-	case XFS_DAS_FOUND_NBLK:
-	case XFS_DAS_FLIP_NFLAG:
-	case XFS_DAS_ALLOC_NODE:
-		goto das_node;
-	default:
-		break;
-	}
-
 	/*
-	 * If the attribute list is already in leaf format, jump straight to
-	 * leaf handling.  Otherwise, try to add the attribute to the shortform
-	 * list; if there's no room then convert the list to leaf format and try
-	 * again. No need to set state as we will be in leaf form when we come
-	 * back
+	 * After a shortform to leaf conversion, we don't need to hold the leaf
+	 * block we allocated anymore. Release the hold if we are passed a leaf
+	 * block and clear the pointer.
 	 */
-	if (xfs_attr_is_shortform(dp)) {
+	if (*leaf_bp) {
+		xfs_trans_bhold_release(args->trans, *leaf_bp);
+		*leaf_bp = NULL;
+	}
 
+	error = xfs_attr_leaf_try_add(args, leaf_bp);
+	switch (error) {
+	case -ENOSPC:
 		/*
-		 * If the attr was successfully set in shortform, no need to
-		 * continue.  Otherwise, is it converted from shortform to leaf
-		 * and -EAGAIN is returned.
+		 * Promote the attribute list to the Btree format.
 		 */
-		error = xfs_attr_set_shortform(args, leaf_bp);
-		if (error == -EAGAIN)
-			dac->flags |= XFS_DAC_DEFER_FINISH;
+		error = xfs_attr3_leaf_to_node(args);
+		if (error)
+			return error;
 
+		/*
+		 * Finish any deferred work items and roll the
+		 * transaction once more.  The goal here is to call
+		 * node_addname with the inode  and transaction in the
+		 * same state (inode locked and joined, transaction
+		 * clean) no matter how we got to this step.
+		 */
+		dac->flags |= XFS_DAC_DEFER_FINISH;
+		return -EAGAIN;
+	case 0:
+		dac->dela_state = XFS_DAS_FOUND_LBLK;
+		return -EAGAIN;
+	default:
 		return error;
 	}
+}
+
+static int
+xfs_attr_das_set_leaf(
+	struct xfs_delattr_context	*dac)
+{
+	int				error = 0;
+
+	error = xfs_attr_leaf_addname(dac);
+	if (error == -ENOSPC) {
+		/*
+		 * No need to set state.  We will be in node form when
+		 * we are recalled
+		 */
+		return -EAGAIN;
+	}
+	return error;
+}
+
+static int
+xfs_attr_das_set_node(
+	struct xfs_delattr_context	*dac)
+{
+	return  xfs_attr_node_addname(dac);
+}
+
+/*
+ * Set the attribute specified in @args.
+ */
+int
+xfs_attr_set_args(
+	struct xfs_da_args	*args)
+{
+	struct xfs_buf			*leaf_bp = NULL;
+	int				error = 0;
+	struct xfs_delattr_context	dac = {
+		.da_args	= args,
+	};
 
 	/*
-	 * After a shortform to leaf conversion, we need to hold the leaf and
-	 * cycle out the transaction.  When we get back, we need to release
-	 * the leaf.
+	 * Initial condition setup is based on detected form of
+	 * the attribute tree.
 	 */
-	if (*leaf_bp != NULL) {
-		xfs_trans_bhold_release(args->trans, *leaf_bp);
-		*leaf_bp = NULL;
+	if (xfs_attr_is_shortform(dp)) {
+		dac->dela_state = XFS_DAS_FIND_SF;
+	} else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
+		dac->dela_state = XFS_DAS_FIND_LBLK;
+	} else {
+		dac->dela_state = XFS_DAS_FIND_NBLK;
 	}
 
-	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
-		error = xfs_attr_leaf_try_add(args, *leaf_bp);
-		switch (error) {
-		case -ENOSPC:
-			/*
-			 * Promote the attribute list to the Btree format.
-			 */
-			error = xfs_attr3_leaf_to_node(args);
-			if (error)
-				return error;
+	do {
+		/*
+		 * XXX: Need two returns here:
+		 *	- state change occurred, no trans roll required
+		 *	- state change occurred, trans roll required.
+		 */
+		switch (dac->dela_state) {
+		case XFS_DAS_FIND_SF:
+			error = xfs_attr_das_set_sf(dac, &leaf_bp);
+			break;
+		case XFS_DAS_FIND_LBLK:
+			/* XXX: should we really be getting leaf_bp back here? */
+			error = xfs_attr_das_find_leaf(dac, &leaf_bp);
+			break;
 
-			/*
-			 * Finish any deferred work items and roll the
-			 * transaction once more.  The goal here is to call
-			 * node_addname with the inode  and transaction in the
-			 * same state (inode locked and joined, transaction
-			 * clean) no matter how we got to this step.
-			 */
-			dac->flags |= XFS_DAC_DEFER_FINISH;
-			return -EAGAIN;
-		case 0:
-			dac->dela_state = XFS_DAS_FOUND_LBLK;
-			return -EAGAIN;
+		case XFS_DAS_FLIP_LFLAG:
+		case XFS_DAS_FOUND_LBLK:
+			error = xfs_attr_das_set_leaf(dac);
+			break;
+
+		case XFS_DAS_FOUND_NBLK:
+		case XFS_DAS_FLIP_NFLAG:
+		case XFS_DAS_ALLOC_NODE:
+			error = xfs_attr_das_set_node(dac);
+			break;
 		default:
+			error = xfs_attr_das_set_sf(dac, &leaf_bp);
+			break;
+		}
+		if (error != -EAGAIN)
+			break;
+
+		error = xfs_attr_trans_roll(&dac);
+		if (error)
 			return error;
+
+		if (leaf_bp) {
+			xfs_trans_bjoin(args->trans, leaf_bp);
+			xfs_trans_bhold(args->trans, leaf_bp);
 		}
-das_leaf:
-		error = xfs_attr_leaf_addname(dac);
-		if (error == -ENOSPC)
-			/*
-			 * No need to set state.  We will be in node form when
-			 * we are recalled
-			 */
-			return -EAGAIN;
 
-		return error;
-	}
-das_node:
-	error = xfs_attr_node_addname(dac);
+	} while (true);
+
+	/* XXX: shouldn't this drop the hold on leaf_bp here? */
+
 	return error;
 }
 
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 4f6bba87fd42..9f80b937c22a 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -232,6 +232,8 @@ struct xfs_attr_list_context {
 enum xfs_delattr_state {
 				      /* Zero is uninitalized */
 	XFS_DAS_RM_SHRINK	= 1,  /* We are shrinking the tree */
+	XFS_DAS_FIND_SF,
+	XFS_DAS_FIND_LBLK,
 	XFS_DAS_FOUND_LBLK,	      /* We found leaf blk for attr */
 	XFS_DAS_FOUND_NBLK,	      /* We found node blk for attr */
 	XFS_DAS_FLIP_LFLAG,	      /* Flipped leaf INCOMPLETE attr flag */