Re: [PATCH v11 13/25] xfs: Remove unneeded xfs_trans_roll_inode calls

Allison Collins
2020-07-25T00:08:28Z

On 7/21/20 4:34 PM, Darrick J. Wong wrote:
> On Mon, Jul 20, 2020 at 05:15:54PM -0700, Allison Collins wrote:
>> Some calls to xfs_trans_roll_inode and xfs_defer_finish routines are not
>> needed. If they are the last operations executed in these functions, and
>> no further changes are made, then higher level routines will roll or
>> commit the transactions.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> Looks decent,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Alrighty, thanks!

Allison

>
> --D
>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 61 ++++++------------------------------------------
>> 1 file changed, 7 insertions(+), 54 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 4eff875..1a78023 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -693,34 +693,15 @@ xfs_attr_leaf_addname(
>> /*
>> * If the result is small enough, shrink it all into the inode.
>> */
>> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
>> + 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;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - return error;
>> - }
>> -
>> - /*
>> - * Commit the remove and start the next trans in series.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, dp);
>> -
>> } else if (args->rmtblkno > 0) {
>> /*
>> * Added a "remote" value, just clear the incomplete flag.
>> */
>> error = xfs_attr3_leaf_clearflag(args);
>> - if (error)
>> - return error;
>> -
>> - /*
>> - * Commit the flag value change and start the next trans in
>> - * series.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>> }
>> return error;
>> }
>> @@ -780,15 +761,11 @@ xfs_attr_leaf_removename(
>> /*
>> * If the result is small enough, shrink it all into the inode.
>> */
>> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
>> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> + forkoff = xfs_attr_shortform_allfit(bp, dp);
>> + if (forkoff)
>> + return xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> /* bp is gone due to xfs_da_shrink_inode */
>> - if (error)
>> - return error;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - return error;
>> - }
>> +
>> return 0;
>> }
>>
>> @@ -1070,18 +1047,8 @@ xfs_attr_node_addname(
>> error = xfs_da3_join(state);
>> if (error)
>> goto out;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - goto out;
>> }
>>
>> - /*
>> - * Commit and start the next trans in the chain.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, dp);
>> - if (error)
>> - goto out;
>> -
>> } else if (args->rmtblkno > 0) {
>> /*
>> * Added a "remote" value, just clear the incomplete flag.
>> @@ -1089,14 +1056,6 @@ xfs_attr_node_addname(
>> error = xfs_attr3_leaf_clearflag(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;
>> }
>> retval = error = 0;
>>
>> @@ -1135,16 +1094,10 @@ xfs_attr_node_shrink(
>> if (forkoff) {
>> error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> /* bp is gone due to xfs_da_shrink_inode */
>> - if (error)
>> - return error;
>> -
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - return error;
>> } else
>> xfs_trans_brelse(args->trans, bp);
>>
>> - return 0;
>> + return error;
>> }
>>
>> /*
>> --
>> 2.7.4
>>