Re: [PATCH v17 07/11] xfs: Hoist xfs_attr_node_addname

Allison Henderson
2021-04-19T18:33:00Z

On 4/18/21 10:31 PM, Chandan Babu R wrote:
> 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@gmail.com>
Thank you!!
Allison

>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>> ---
>> 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;
>> }
>
>