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

Allison Henderson
2021-04-24T03:27:51Z

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