Re: [PATCH v7 15/19] xfs: Add helper function xfs_attr_node_shrink

Allison Collins
2020-02-23T18:41:22Z

On 2/23/20 6:22 AM, Amir Goldstein wrote:
> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
> <allison.henderson@oracle.com> wrote:
>>
>> This patch adds a new helper function xfs_attr_node_shrink used to shrink an
>> attr name into an inode if it is small enough. This helps to modularize
>> the greater calling function xfs_attr_node_removename.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 66 +++++++++++++++++++++++++++++-------------------
>> 1 file changed, 40 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 4b788f2..30a16fe 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -1366,6 +1366,43 @@ xfs_attr_node_addname(
>> }
>>
>> /*
>> + * Shrink an attribute from leaf to shortform
>> + */
>> +STATIC int
>> +xfs_attr_node_shrink(
>> + struct xfs_da_args *args,
>> + struct xfs_da_state *state)
>> +{
>> + struct xfs_inode *dp = args->dp;
>> + int error, forkoff;
>> + struct xfs_buf *bp;
>> +
>> + /*
>> + * Have to get rid of the copy of this dabuf in the state.
>> + */
>> + ASSERT(state->path.active == 1);
>> + ASSERT(state->path.blk[0].bp);
>> + state->path.blk[0].bp = NULL;
>> +
>> + error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
>> + if (error)
>> + return error;
>> +
>> + forkoff = xfs_attr_shortform_allfit(bp, dp);
>> + if (forkoff) {
>> + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> + /* bp is gone due to xfs_da_shrink_inode */
>> + if (error)
>> + return error;
>> +
>> + args->dac.flags |= XFS_DAC_FINISH_TRANS;
>
> Why did xfs_defer_finish(&args->trans); turn into the above?
>
> Are you testing reviewers alertness? ;-)
> Please keep logic preserving patches separate from logic change patches.
You're right, I missed this when I was separating it from the previous
patch. Will correct. Thanks for the catch!

Allison

>
> Thanks,
> Amir.
>
>> + } else
>> + xfs_trans_brelse(args->trans, bp);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> * Remove a name from a B-tree attribute list.
>> *
>> * This will involve walking down the Btree, and may involve joining
>> @@ -1383,8 +1420,7 @@ xfs_attr_node_removename(
>> {
>> struct xfs_da_state *state;
>> struct xfs_da_state_blk *blk;
>> - struct xfs_buf *bp;
>> - int retval, error, forkoff;
>> + int retval, error;
>> struct xfs_inode *dp = args->dp;
>>
>> trace_xfs_attr_node_removename(args);
>> @@ -1493,30 +1529,8 @@ xfs_attr_node_removename(
>> /*
>> * If the result is small enough, push it all into the inode.
>> */
>> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>> - /*
>> - * Have to get rid of the copy of this dabuf in the state.
>> - */
>> - ASSERT(state->path.active == 1);
>> - ASSERT(state->path.blk[0].bp);
>> - state->path.blk[0].bp = NULL;
>> -
>> - error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
>> - if (error)
>> - goto out;
>> -
>> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
>> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> - /* bp is gone due to xfs_da_shrink_inode */
>> - if (error)
>> - goto out;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - goto out;
>> - } else
>> - xfs_trans_brelse(args->trans, bp);
>> - }
>> - error = 0;
>> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>> + error = xfs_attr_node_shrink(args, state);
>>
>> out:
>> if (state)
>> --
>> 2.7.4
>>