Re: [PATCH v6 14/16] xfs: Simplify xfs_attr_set_args

Allison Collins
2020-01-22T07:39:49Z

On 1/21/20 4:31 PM, Darrick J. Wong wrote:
> On Sat, Jan 18, 2020 at 03:50:33PM -0700, Allison Collins wrote:
>> Delayed attribute mechanics make frequent use of goto statements. We
>> can use this to further simplify xfs_attr_set_args. In this patch we
>> introduce one of the gotos now to help pre-simplify the routine. This
>> will help make proceeding patches in this area easier to read.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 71 +++++++++++++++++++++++++-----------------------
>> 1 file changed, 37 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 453ea59..90e0b2d 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -264,47 +264,50 @@ xfs_attr_set_args(
>> int error, error2 = 0;
>>
>> /*
>> - * 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 non-existent or a shortform list, proceed to
>> + * upgrade it to a single-leaf-block attribute list. Otherwise jump
>> + * straight to leaf handling
>
> We don't jump to "upgrade it to a single leaf block attr list" quite
> that soon.
>
> "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." ?
Sure, I think that sounds good.

>
>> */
>> - if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>> + 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)) {
>> + dp->i_d.di_anextents == 0)))
>
> Also... maybe this should just be a separate predicate?
>
> static inline bool
> xfs_attr_is_leaf_format(
> struct xfs_inode *dp)
> {
> return dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> dp->i_d.di_anextents > 0;
> }
>
>
> if (xfs_attr_is_leaf_format(dp))
> goto add_leaf;

Ok, I think that looks better too. Will add.

>
>> + goto sf_to_leaf;
>>
>> - /*
>> - * Try to add the attr to the attribute list in the inode.
>> - */
>> - error = xfs_attr_try_sf_addname(dp, args);
>> - if (error != -ENOSPC) {
>> - error2 = xfs_trans_commit(args->trans);
>> - args->trans = NULL;
>> - return error ? error : error2;
>> - }
>> + /*
>> + * Try to add the attr to the attribute list in the inode.
>> + */
>> + error = xfs_attr_try_sf_addname(dp, args);
>> + if (error != -ENOSPC) {
>> + error2 = xfs_trans_commit(args->trans);
>> + args->trans = NULL;
>> + return error ? error : error2;
>> + }
>>
>> - /*
>> - * 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.
>> - * Once we're done rolling the transaction we can release
>> - * the hold and add the attr to the leaf.
>> - */
>> - xfs_trans_bhold(args->trans, leaf_bp);
>> - error = xfs_defer_finish(&args->trans);
>> - xfs_trans_bhold_release(args->trans, leaf_bp);
>> - if (error) {
>> - xfs_trans_brelse(args->trans, leaf_bp);
>> - 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.
>> + * Once we're done rolling the transaction we can release
>> + * the hold and add the attr to the leaf.
>> + */
>> + xfs_trans_bhold(args->trans, leaf_bp);
>> + error = xfs_defer_finish(&args->trans);
>> + xfs_trans_bhold_release(args->trans, leaf_bp);
>> + if (error) {
>> + xfs_trans_brelse(args->trans, leaf_bp);
>> + return error;
>> }
>>
>> +sf_to_leaf:
>
> The attr list already has to be in list format by the time it gets here,
> which means that the label name is misleading. How about "add_leaf" ?
Sure, I'll update the state machine names to match too then.

Thanks for the review!
Allison

>
> --D
>
>> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>> error = xfs_attr_leaf_addname(args);
>> if (error != -ENOSPC)
>> --
>> 2.7.4
>>