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

Allison Henderson
2021-04-19T18:32:49Z

On 4/18/21 11:53 PM, Chandan Babu R wrote:
> On 16 Apr 2021 at 14:50, Allison Henderson wrote:
>> This patch modifies the attr remove 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_remove_args is merged with
>> xfs_attr_node_removename become a new function, xfs_attr_remove_iter.
>> This new version uses a sort of state machine like switch to keep track
>> of where it was when EAGAIN was returned. A new version of
>> xfs_attr_remove_args consists of a simple loop to refresh the
>> transaction until the operation is completed. A new XFS_DAC_DEFER_FINISH
>> flag is used to finish the transaction where ever the existing code used
>> to.
>>
>> Calls to xfs_attr_rmtval_remove are replaced with the delay ready
>> version __xfs_attr_rmtval_remove. We will rename
>> __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
>> done.
>>
>> xfs_attr_rmtval_remove itself is still in use by the set routines (used
>> during a rename). For reasons of preserving existing function, we
>> modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
>> set. Similar to how xfs_attr_remove_args does here. Once we transition
>> the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
>> used and will be removed.
>>
>> This patch also adds a new struct xfs_delattr_context, which we will use
>> to keep track of the current state of an attribute operation. The new
>> xfs_delattr_state enum is used to track various operations that are in
>> progress so that we know not to repeat them, and resume where we left
>> off before EAGAIN was returned to cycle out the transaction. Other
>> members take the place of local variables that need to retain their
>> values across multiple function recalls. See xfs_attr.h for a more
>> detailed diagram of the states.
>>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 208 +++++++++++++++++++++++++++-------------
>> fs/xfs/libxfs/xfs_attr.h | 131 +++++++++++++++++++++++++
>> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
>> fs/xfs/libxfs/xfs_attr_remote.c | 48 ++++++----
>> fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
>> fs/xfs/xfs_attr_inactive.c | 2 +-
>> 6 files changed, 305 insertions(+), 88 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index ed06b60..0bea8dd 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -57,7 +57,6 @@ STATIC int xfs_attr_node_addname(struct xfs_da_args *args,
>> struct xfs_da_state *state);
>> STATIC int xfs_attr_node_addname_find_attr(struct xfs_da_args *args,
>> struct xfs_da_state **state);
>> -STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
>> STATIC int xfs_attr_node_addname_clear_incomplete(struct xfs_da_args *args);
>> STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>> struct xfs_da_state **state);
>> @@ -221,6 +220,31 @@ xfs_attr_is_shortform(
>> ip->i_afp->if_nextents == 0);
>> }
>>
>> +/*
>> + * Checks to see if a delayed attribute transaction should be rolled. If so,
>> + * transaction is finished or rolled as needed.
>> + */
>> +int
>> +xfs_attr_trans_roll(
>> + struct xfs_delattr_context *dac)
>> +{
>> + struct xfs_da_args *args = dac->da_args;
>> + int error;
>> +
>> + if (dac->flags & XFS_DAC_DEFER_FINISH) {
>> + /*
>> + * The caller wants us to finish all the deferred ops so that we
>> + * avoid pinning the log tail with a large number of deferred
>> + * ops.
>> + */
>> + dac->flags &= ~XFS_DAC_DEFER_FINISH;
>> + error = xfs_defer_finish(&args->trans);
>> + } else
>> + error = xfs_trans_roll_inode(&args->trans, args->dp);
>> +
>> + return error;
>> +}
>> +
>> STATIC int
>> xfs_attr_set_fmt(
>> struct xfs_da_args *args)
>> @@ -527,21 +551,23 @@ xfs_has_attr(
>> */
>> int
>> xfs_attr_remove_args(
>> - struct xfs_da_args *args)
>> + struct xfs_da_args *args)
>> {
>> - struct xfs_inode *dp = args->dp;
>> - int error;
>> + int error;
>> + struct xfs_delattr_context dac = {
>> + .da_args = args,
>> + };
>>
>> - if (!xfs_inode_hasattr(dp)) {
>> - error = -ENOATTR;
>> - } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
>> - ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
>> - error = xfs_attr_shortform_remove(args);
>> - } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>> - error = xfs_attr_leaf_removename(args);
>> - } else {
>> - error = xfs_attr_node_removename(args);
>> - }
>> + do {
>> + error = xfs_attr_remove_iter(&dac);
>> + if (error != -EAGAIN)
>> + break;
>> +
>> + error = xfs_attr_trans_roll(&dac);
>> + if (error)
>> + return error;
>> +
>> + } while (true);
>>
>> return error;
>> }
>> @@ -1187,14 +1213,16 @@ xfs_attr_leaf_mark_incomplete(
>> */
>> STATIC
>> int xfs_attr_node_removename_setup(
>> - struct xfs_da_args *args,
>> - struct xfs_da_state **state)
>> + struct xfs_delattr_context *dac)
>> {
>> - int error;
>> + struct xfs_da_args *args = dac->da_args;
>> + struct xfs_da_state **state = &dac->da_state;
>> + int error;
>>
>> error = xfs_attr_node_hasname(args, state);
>> if (error != -EEXIST)
>> return error;
>> + error = 0;
>>
>> ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL);
>> ASSERT((*state)->path.blk[(*state)->path.active - 1].magic ==
>> @@ -1203,10 +1231,13 @@ int xfs_attr_node_removename_setup(
>> if (args->rmtblkno > 0) {
>> error = xfs_attr_leaf_mark_incomplete(args, *state);
>> if (error)
>> - return error;
>> + goto out;
>>
>> - return xfs_attr_rmtval_invalidate(args);
>> + error = xfs_attr_rmtval_invalidate(args);
>> }
>> +out:
>> + if (error)
>> + xfs_da_state_free(*state);
>>
>> return 0;
>
> The above should be "return error". Otherwise, we might be missing out on
> reporting failures from xfs_attr_leaf_mark_incomplete() or
> xfs_attr_rmtval_invalidate().
Ok, will update the return

>
>> }
>> @@ -1231,70 +1262,117 @@ xfs_attr_node_remove_cleanup(
>> }
>>
>> /*
>> - * Remove a name from a B-tree attribute list.
>> + * Remove the attribute specified in @args.
>> *
>> * This will involve walking down the Btree, and may involve joining
>> * leaf nodes and even joining intermediate nodes up to and including
>> * the root node (a special case of an intermediate node).
>> + *
>> + * This routine is meant to function as either an in-line or 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_node_removename(
>> - struct xfs_da_args *args)
>> +int
>> +xfs_attr_remove_iter(
>> + struct xfs_delattr_context *dac)
>> {
>> - struct xfs_da_state *state;
>> - int retval, error;
>> - struct xfs_inode *dp = args->dp;
>> + struct xfs_da_args *args = dac->da_args;
>> + struct xfs_da_state *state = dac->da_state;
>> + int retval, error;
>> + struct xfs_inode *dp = args->dp;
>>
>> trace_xfs_attr_node_removename(args);
>>
>> - error = xfs_attr_node_removename_setup(args, &state);
>> - if (error)
>> - goto out;
>> + switch (dac->dela_state) {
>> + case XFS_DAS_UNINIT:
>> + if (!xfs_inode_hasattr(dp))
>> + return -ENOATTR;
>>
>> - /*
>> - * If there is an out-of-line value, de-allocate the blocks.
>> - * This is done before we remove the attribute so that we don't
>> - * overflow the maximum size of a transaction and/or hit a deadlock.
>> - */
>> - if (args->rmtblkno > 0) {
>> - error = xfs_attr_rmtval_remove(args);
>> - if (error)
>> - goto out;
>> + if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
>> + ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
>> + return xfs_attr_shortform_remove(args);
>> + }
>> +
>> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>> + return xfs_attr_leaf_removename(args);
>> +
>> + if (!dac->da_state) {
>> + error = xfs_attr_node_removename_setup(dac);
>> + if (error)
>> + return error;
>> + state = dac->da_state;
>> + }
>> +
>> + /* fallthrough */
>> + case XFS_DAS_RMTBLK:
>> + dac->dela_state = XFS_DAS_RMTBLK;
>>
>> /*
>> - * Refill the state structure with buffers, the prior calls
>> - * released our buffers.
>> + * If there is an out-of-line value, de-allocate the blocks.
>> + * This is done before we remove the attribute so that we don't
>> + * overflow the maximum size of a transaction and/or hit a
>> + * deadlock.
>> */
>> - error = xfs_attr_refillstate(state);
>> - if (error)
>> - goto out;
>> - }
>> - retval = xfs_attr_node_remove_cleanup(args, state);
>> + if (args->rmtblkno > 0) {
>> + /*
>> + * May return -EAGAIN. Remove blocks until 0 is returned
>> + */
>> + error = __xfs_attr_rmtval_remove(dac);
>> + if (error == -EAGAIN)
>> + return error;
>> + else if (error)
>> + goto out;
>> +
>> + /*
>> + * Refill the state structure with buffers, the prior
>> + * calls released our buffers.
>> + */
>> + ASSERT(args->rmtblkno == 0);
>> + error = xfs_attr_refillstate(state);
>> + if (error)
>> + goto out;
>> +
>> + dac->dela_state = XFS_DAS_CLNUP;
>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>> + return -EAGAIN;
>> + }
>> +
>> + case XFS_DAS_CLNUP:
>> + retval = xfs_attr_node_remove_cleanup(args, state);
>>
>> - /*
>> - * Check to see if the tree needs to be collapsed.
>> - */
>> - if (retval && (state->path.active > 1)) {
>> - error = xfs_da3_join(state);
>> - if (error)
>> - goto out;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - goto out;
>> /*
>> - * Commit the Btree join operation and start a new trans.
>> + * Check to see if the tree needs to be collapsed. Set the flag
>> + * to indicate that the calling function needs to move the
>> + * shrink operation
>> */
>> - error = xfs_trans_roll_inode(&args->trans, dp);
>> - if (error)
>> - goto out;
>> - }
>> + if (retval && (state->path.active > 1)) {
>> + error = xfs_da3_join(state);
>> + if (error)
>> + goto out;
>>
>> - /*
>> - * If the result is small enough, push it all into the inode.
>> - */
>> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>> - error = xfs_attr_node_shrink(args, state);
>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>> + dac->dela_state = XFS_DAS_RM_SHRINK;
>> + return -EAGAIN;
>> + }
>> +
>> + /* fallthrough */
>> + case XFS_DAS_RM_SHRINK:
>> + /*
>> + * If the result is small enough, push it all into the inode.
>> + */
>> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>> + error = xfs_attr_node_shrink(args, state);
>> +
>> + break;
>> + default:
>> + ASSERT(0);
>> + error = -EINVAL;
>> + goto out;
>> + }
>>
>> + if (error == -EAGAIN)
>> + return error;
>
> The above two statements probably not required. AFAICT, the call to
> xfs_attr_node_shrink() is the only instance which can cause "error" to have a
> non-zero return value if control reaches this point in the function. All other
> locations in the function seem to either return from the function or jump to
> "out" label on detecting an error.
Ok, i suppose it doesnt hurt anything, but i'll go ahead and bump it
since I need to pick up the change above.

>
>> out:
>> if (state)
>> xfs_da_state_free(state);
>
> --
> chandan
Thanks for the reviews!
Allison

>