Re: [PATCH v17 11/11] xfs: Add delay ready attr set routines

[Brian Foster <bfoster@redhat.com>]

On Fri, Apr 16, 2021 at 02:20:45AM -0700, Allison Henderson wrote:
> This patch modifies the attr set 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_set_args has become xfs_attr_set_iter, which uses a
> state machine like switch to keep track of where it was when EAGAIN was
> returned. See xfs_attr.h for a more detailed diagram of the states.
> 
> Two new helper functions have been added: xfs_attr_rmtval_find_space and
> xfs_attr_rmtval_set_blk.  They provide a subset of logic similar to
> xfs_attr_rmtval_set, but they store the current block in the delay attr
> context to allow the caller to roll the transaction between allocations.
> This helps to simplify and consolidate code used by
> xfs_attr_leaf_addname and xfs_attr_node_addname. xfs_attr_set_args has
> now become a simple loop to refresh the transaction until the operation
> is completed.  Lastly, xfs_attr_rmtval_remove is no longer used, and is
> removed.
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> ---

This one looks good to me. My feedback is mostly around some code
formatting and comments, so again I've just appended a diff for your
review. With the various nits addressed:

Reviewed-by: Brian Foster <bfoster@redhat.com>

--- 8< ---

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 302e44efa985..3e242eeac3d7 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -337,15 +337,15 @@ xfs_attr_set_iter(
 	/* State machine switch */
 	switch (dac->dela_state) {
 	case XFS_DAS_UNINIT:
-		if (xfs_attr_is_shortform(dp))
-			return xfs_attr_set_fmt(dac, leaf_bp);
-
 		/*
-		 * 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 to release the hold on the leaf
-		 * buffer.
+		 * If the fork is shortform, attempt to add the attr. If there
+		 * is no space, this converts to leaf format and returns
+		 * -EAGAIN with the leaf buffer held across the roll. The caller
+		 * will deal with a transaction roll error, but otherwise
+		 * release the hold once we return with a clean transaction.
 		 */
+		if (xfs_attr_is_shortform(dp))
+			return xfs_attr_set_fmt(dac, leaf_bp);
 		if (*leaf_bp != NULL) {
 			xfs_trans_bhold_release(args->trans, *leaf_bp);
 			*leaf_bp = NULL;
@@ -354,10 +354,6 @@ xfs_attr_set_iter(
 		if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
 			error = xfs_attr_leaf_try_add(args, *leaf_bp);
 			if (error == -ENOSPC) {
-				/*
-				 * Promote the attribute list to the Btree
-				 * format.
-				 */
 				error = xfs_attr3_leaf_to_node(args);
 				if (error)
 					return error;
@@ -382,8 +378,6 @@ xfs_attr_set_iter(
 			}
 
 			dac->dela_state = XFS_DAS_FOUND_LBLK;
-			return -EAGAIN;
-
 		} else {
 			error = xfs_attr_node_addname_find_attr(dac);
 			if (error)
@@ -394,8 +388,8 @@ xfs_attr_set_iter(
 				return error;
 
 			dac->dela_state = XFS_DAS_FOUND_NBLK;
-			return -EAGAIN;
 		}
+		return -EAGAIN;
 	case XFS_DAS_FOUND_LBLK:
 		/*
 		 * If there was an out-of-line value, allocate the blocks we
@@ -415,14 +409,13 @@ xfs_attr_set_iter(
 		}
 
 		/*
-		 * Roll through the "value", allocating blocks on disk as
-		 * required.
+		 * Repeat allocating remote blocks for the attr value until
+		 * blkcnt drops to zero.
 		 */
 		if (dac->blkcnt > 0) {
 			error = xfs_attr_rmtval_set_blk(dac);
 			if (error)
 				return error;
-
 			return -EAGAIN;
 		}
 
@@ -430,14 +423,13 @@ xfs_attr_set_iter(
 		if (error)
 			return error;
 
+		/*
+		 * If this is not a rename, clear the incomplete flag and we're
+		 * done.
+		 */
 		if (!(args->op_flags & XFS_DA_OP_RENAME)) {
-			/*
-			 * Added a "remote" value, just clear the incomplete
-			 *flag.
-			 */
 			if (args->rmtblkno > 0)
 				error = xfs_attr3_leaf_clearflag(args);
-
 			return error;
 		}
 
@@ -450,7 +442,6 @@ xfs_attr_set_iter(
 		 * In a separate transaction, set the incomplete flag on the
 		 * "old" attr and clear the incomplete flag on the "new" attr.
 		 */
-
 		error = xfs_attr3_leaf_flipflags(args);
 		if (error)
 			return error;
@@ -466,16 +457,14 @@ xfs_attr_set_iter(
 		 * "remote" value (if it exists).
 		 */
 		xfs_attr_restore_rmt_blk(args);
-
 		error = xfs_attr_rmtval_invalidate(args);
 		if (error)
 			return error;
 
-		/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
-		dac->dela_state = XFS_DAS_RM_LBLK;
-
 		/* fallthrough */
 	case XFS_DAS_RM_LBLK:
+		/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
+		dac->dela_state = XFS_DAS_RM_LBLK;
 		if (args->rmtblkno) {
 			error = __xfs_attr_rmtval_remove(dac);
 			if (error)
@@ -488,8 +477,9 @@ xfs_attr_set_iter(
 		/* fallthrough */
 	case XFS_DAS_RD_LEAF:
 		/*
-		 * Read in the block containing the "old" attr, then remove the
-		 * "old" attr from that block (neat, huh!)
+		 * This is the last step for leaf format. Read the block with
+		 * the old attr, remove the old attr, check for shortform
+		 * conversion and return.
 		 */
 		error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno,
 					   &bp);
@@ -498,9 +488,6 @@ xfs_attr_set_iter(
 
 		xfs_attr3_leaf_remove(bp, args);
 
-		/*
-		 * If the result is small enough, shrink it all into the inode.
-		 */
 		forkoff = xfs_attr_shortform_allfit(bp, dp);
 		if (forkoff)
 			error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
@@ -510,36 +497,29 @@ xfs_attr_set_iter(
 
 	case XFS_DAS_FOUND_NBLK:
 		/*
-		 * If there was an out-of-line value, allocate the blocks we
-		 * identified for its storage and copy the value.  This is done
-		 * after we create the attribute so that we don't overflow the
-		 * maximum size of a transaction and/or hit a deadlock.
+		 * Find space for remote blocks and fall into the allocation
+		 * state.
 		 */
 		if (args->rmtblkno > 0) {
-			/*
-			 * Open coded xfs_attr_rmtval_set without trans
-			 * handling
-			 */
 			error = xfs_attr_rmtval_find_space(dac);
 			if (error)
 				return error;
-
-			/*
-			 * Roll through the "value", allocating blocks on disk
-			 * as required.  Set the state in case of -EAGAIN return
-			 * code
-			 */
-			dac->dela_state = XFS_DAS_ALLOC_NODE;
 		}
 
 		/* fallthrough */
 	case XFS_DAS_ALLOC_NODE:
+		/*
+		 * If there was an out-of-line value, allocate the blocks we
+		 * identified for its storage and copy the value.  This is done
+		 * after we create the attribute so that we don't overflow the
+		 * maximum size of a transaction and/or hit a deadlock.
+		 */
+		dac->dela_state = XFS_DAS_ALLOC_NODE;
 		if (args->rmtblkno > 0) {
 			if (dac->blkcnt > 0) {
 				error = xfs_attr_rmtval_set_blk(dac);
 				if (error)
 					return error;
-
 				return -EAGAIN;
 			}
 
@@ -548,11 +528,11 @@ xfs_attr_set_iter(
 				return error;
 		}
 
+		/*
+		 * If this was not a rename, clear the incomplete flag and we're
+		 * done.
+		 */
 		if (!(args->op_flags & XFS_DA_OP_RENAME)) {
-			/*
-			 * Added a "remote" value, just clear the incomplete
-			 * flag.
-			 */
 			if (args->rmtblkno > 0)
 				error = xfs_attr3_leaf_clearflag(args);
 			goto out;
@@ -588,11 +568,10 @@ xfs_attr_set_iter(
 		if (error)
 			return error;
 
-		/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
-		dac->dela_state = XFS_DAS_RM_NBLK;
-
 		/* fallthrough */
 	case XFS_DAS_RM_NBLK:
+		/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
+		dac->dela_state = XFS_DAS_RM_NBLK;
 		if (args->rmtblkno) {
 			error = __xfs_attr_rmtval_remove(dac);
 			if (error)
@@ -604,7 +583,12 @@ xfs_attr_set_iter(
 
 		/* fallthrough */
 	case XFS_DAS_CLR_FLAG:
+		/*
+		 * The last state for node format. Look up the old attr and
+		 * remove it.
+		 */
 		error = xfs_attr_node_addname_clear_incomplete(dac);
+		break;
 	default:
 		ASSERT(dac->dela_state != XFS_DAS_RM_SHRINK);
 		break;