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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 4/23/21 12:08 PM, Brian Foster wrote:
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@xxxxxxxxxx>
Reviewed-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
---

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@xxxxxxxxxx>
Alrighty, I gave it a look through, and it looks fine to me. I'll test it and fold it into the next version. Thanks again for all the reviews!

Allison


--- 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;




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux