On 16 Apr 2021 at 14:50, 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
>
The changes look good to me.
Reviewed-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
> Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 157 +++++++++++++++++++++++------------------------
> 1 file changed, 76 insertions(+), 81 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 16159f6..80212d2 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -52,6 +52,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> * Internal routines when attribute list is more than one block.
> */
> STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> +STATIC void xfs_attr_restore_rmt_blk(struct xfs_da_args *args);
> 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,
> @@ -270,8 +271,8 @@ xfs_attr_set_args(
> struct xfs_da_args *args)
> {
> struct xfs_inode *dp = args->dp;
> - struct xfs_da_state *state;
> - int error;
> + struct xfs_da_state *state = NULL;
> + int error = 0;
>
> /*
> * If the attribute list is already in leaf format, jump straight to
> @@ -322,8 +323,77 @@ 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);
> + 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_clear_incomplete(args);
> +out:
> return error;
> +
> }
>
> /*
> @@ -957,7 +1027,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);
>
> @@ -965,8 +1035,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
> @@ -1012,85 +1082,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_clear_incomplete(args);
> out:
> if (state)
> xfs_da_state_free(state);
> - if (error)
> - return error;
> - return retval;
> + return error;
> }
--
chandan
