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