On Thu, Feb 18, 2021 at 09:53:34AM -0700, Allison Henderson wrote:
> This patch hoists the later half of xfs_attr_node_addname into
> the calling function. We do this because it is this area that
> will need the most state management, and we want to keep such
> code in the same scope as much as possible
>
> Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 165 ++++++++++++++++++++++++-----------------------
> 1 file changed, 83 insertions(+), 82 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 4333b61..19a532a 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
...
> @@ -320,8 +322,82 @@ xfs_attr_set_args(
> return error;
> error = xfs_attr_node_addname(args, state);
> } while (error == -EAGAIN);
> + if (error)
> + return error;
> +
> + /*
> + * Commit the leaf addition or btree split and start the next
> + * trans in the chain.
> + */
> + error = xfs_trans_roll_inode(&args->trans, dp);
> + if (error)
> + goto out;
> +
> + /*
> + * 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.
> + */
> + if (args->rmtblkno > 0) {
> + error = xfs_attr_rmtval_set(args);
> + if (error)
> + return error;
> + }
> +
> + 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);
> + retval = error;
It looks like this is the only use of retval. Otherwise this function is
getting a bit big, but the factoring LGTM:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> + goto out;
> + }
> +
> + /*
> + * If this is an atomic rename operation, we must "flip" the incomplete
> + * flags on the "new" and "old" attribute/value pairs so that one
> + * disappears and one appears atomically. Then we must remove the "old"
> + * attribute/value pair.
> + *
> + * 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)
> + goto out;
> + /*
> + * Commit the flag value change and start the next trans in series
> + */
> + error = xfs_trans_roll_inode(&args->trans, args->dp);
> + if (error)
> + goto out;
> +
> + /*
> + * Dismantle the "old" attribute/value pair by removing a "remote" value
> + * (if it exists).
> + */
> + xfs_attr_restore_rmt_blk(args);
> +
> + if (args->rmtblkno) {
> + error = xfs_attr_rmtval_invalidate(args);
> + if (error)
> + return error;
> +
> + error = xfs_attr_rmtval_remove(args);
> + if (error)
> + return error;
> + }
> +
> + error = xfs_attr_node_addname_work(args);
> +out:
> + if (state)
> + xfs_da_state_free(state);
> + if (error)
> + return error;
> + return retval;
>
> - return error;
> }
>
> /*
> @@ -955,7 +1031,7 @@ xfs_attr_node_addname(
> {
> struct xfs_da_state_blk *blk;
> struct xfs_inode *dp;
> - int retval, error;
> + int error;
>
> trace_xfs_attr_node_addname(args);
>
> @@ -963,8 +1039,8 @@ xfs_attr_node_addname(
> blk = &state->path.blk[state->path.active-1];
> ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
>
> - retval = xfs_attr3_leaf_add(blk->bp, state->args);
> - if (retval == -ENOSPC) {
> + error = xfs_attr3_leaf_add(blk->bp, state->args);
> + if (error == -ENOSPC) {
> if (state->path.active == 1) {
> /*
> * Its really a single leaf node, but it had
> @@ -1010,85 +1086,10 @@ xfs_attr_node_addname(
> xfs_da3_fixhashpath(state, &state->path);
> }
>
> - /*
> - * Kill the state structure, we're done with it and need to
> - * allow the buffers to come back later.
> - */
> - xfs_da_state_free(state);
> - state = NULL;
> -
> - /*
> - * Commit the leaf addition or btree split and start the next
> - * trans in the chain.
> - */
> - error = xfs_trans_roll_inode(&args->trans, dp);
> - if (error)
> - goto out;
> -
> - /*
> - * 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.
> - */
> - if (args->rmtblkno > 0) {
> - error = xfs_attr_rmtval_set(args);
> - if (error)
> - return error;
> - }
> -
> - 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);
> - retval = error;
> - goto out;
> - }
> -
> - /*
> - * If this is an atomic rename operation, we must "flip" the incomplete
> - * flags on the "new" and "old" attribute/value pairs so that one
> - * disappears and one appears atomically. Then we must remove the "old"
> - * attribute/value pair.
> - *
> - * 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)
> - goto out;
> - /*
> - * Commit the flag value change and start the next trans in series
> - */
> - error = xfs_trans_roll_inode(&args->trans, args->dp);
> - if (error)
> - goto out;
> -
> - /*
> - * Dismantle the "old" attribute/value pair by removing a "remote" value
> - * (if it exists).
> - */
> - xfs_attr_restore_rmt_blk(args);
> -
> - if (args->rmtblkno) {
> - error = xfs_attr_rmtval_invalidate(args);
> - if (error)
> - return error;
> -
> - error = xfs_attr_rmtval_remove(args);
> - if (error)
> - return error;
> - }
> -
> - error = xfs_attr_node_addname_work(args);
> out:
> if (state)
> xfs_da_state_free(state);
> - if (error)
> - return error;
> - return retval;
> + return error;
> }
>
>
> --
> 2.7.4
>
