Re: [PATCH v7 16/19] xfs: Simplify xfs_attr_set_iter

Allison Collins
2020-02-23T18:42:11Z

On 2/23/20 6:26 AM, Amir Goldstein wrote:
> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
> <allison.henderson@oracle.com> wrote:
>>
>> Delayed attribute mechanics make frequent use of goto statements. We can use this
>> to further simplify xfs_attr_set_iter. Because states tend to fall between if
>> conditions, we can invert the if logic and jump to the goto. This helps to reduce
>> indentation and simplify things.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>
> Looks better IMO and doesn't change logic.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Alrighty, thanks!

Allison

>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 71 ++++++++++++++++++++++++++++--------------------
>> 1 file changed, 42 insertions(+), 29 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 30a16fe..dd935ff 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -254,6 +254,19 @@ xfs_attr_try_sf_addname(
>> }
>>
>> /*
>> + * Check to see if the attr should be upgraded from non-existent or shortform to
>> + * single-leaf-block attribute list.
>> + */
>> +static inline bool
>> +xfs_attr_fmt_needs_update(
>> + struct xfs_inode *dp)
>> +{
>> + return dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>> + (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>> + dp->i_d.di_anextents == 0);
>> +}
>> +
>> +/*
>> * Set the attribute specified in @args.
>> */
>> int
>> @@ -342,40 +355,40 @@ xfs_attr_set_iter(
>> }
>>
>> /*
>> - * If the attribute list is non-existent or a shortform list,
>> - * upgrade it to a single-leaf-block attribute list.
>> + * If the attribute list is already in leaf format, jump straight to
>> + * leaf handling. Otherwise, try to add the attribute to the shortform
>> + * list; if there's no room then convert the list to leaf format and try
>> + * again.
>> */
>> - if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>> - (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>> - dp->i_d.di_anextents == 0)) {
>> + if (!xfs_attr_fmt_needs_update(dp))
>> + goto add_leaf;
>>
>> - /*
>> - * Try to add the attr to the attribute list in the inode.
>> - */
>> - error = xfs_attr_try_sf_addname(dp, args);
>> + /*
>> + * Try to add the attr to the attribute list in the inode.
>> + */
>> + error = xfs_attr_try_sf_addname(dp, args);
>>
>> - /* Should only be 0, -EEXIST or ENOSPC */
>> - if (error != -ENOSPC)
>> - return error;
>> + /* Should only be 0, -EEXIST or ENOSPC */
>> + if (error != -ENOSPC)
>> + return error;
>>
>> - /*
>> - * It won't fit in the shortform, transform to a leaf block.
>> - * GROT: another possible req'mt for a double-split btree op.
>> - */
>> - error = xfs_attr_shortform_to_leaf(args, leaf_bp);
>> - if (error)
>> - return error;
>> + /*
>> + * It won't fit in the shortform, transform to a leaf block.
>> + * GROT: another possible req'mt for a double-split btree op.
>> + */
>> + error = xfs_attr_shortform_to_leaf(args, leaf_bp);
>> + if (error)
>> + return error;
>>
>> - /*
>> - * Prevent the leaf buffer from being unlocked so that a
>> - * concurrent AIL push cannot grab the half-baked leaf
>> - * buffer and run into problems with the write verifier.
>> - */
>> - xfs_trans_bhold(args->trans, *leaf_bp);
>> - args->dac.flags |= XFS_DAC_FINISH_TRANS;
>> - args->dac.dela_state = XFS_DAS_ADD_LEAF;
>> - return -EAGAIN;
>> - }
>> + /*
>> + * Prevent the leaf buffer from being unlocked so that a
>> + * concurrent AIL push cannot grab the half-baked leaf
>> + * buffer and run into problems with the write verifier.
>> + */
>> + xfs_trans_bhold(args->trans, *leaf_bp);
>> + args->dac.flags |= XFS_DAC_FINISH_TRANS;
>> + args->dac.dela_state = XFS_DAS_ADD_LEAF;
>> + return -EAGAIN;
>>
>> add_leaf:
>>
>> --
>> 2.7.4
>>