Re: [PATCH v8 13/20] xfs: Add helpers xfs_attr_is_shortform and xfs_attr_set_shortform

[Allison Collins <allison.henderson@oracle.com>]

On 4/7/20 2:53 PM, Allison Collins wrote:
> 
> 
> On 4/7/20 8:23 AM, Brian Foster wrote:
>> On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote:
>>> In this patch, we hoist code from xfs_attr_set_args into two new helpers
>>> xfs_attr_is_shortform and xfs_attr_set_shortform.  These two will help
>>> to simplify xfs_attr_set_args when we get into delayed attrs later.
>>>
>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>> ---
>>>   fs/xfs/libxfs/xfs_attr.c | 107 
>>> +++++++++++++++++++++++++++++++----------------
>>>   1 file changed, 72 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>> index 4225a94..ba26ffe 100644
>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>> @@ -204,6 +204,66 @@ 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_is_shortform(
>>> +    struct xfs_inode    *ip)
>>> +{
>>> +    return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>>> +          (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>>> +          ip->i_d.di_anextents == 0);
>>
>> Logic should be indented similar to the original:
>>
>>     return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>>            (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>>         ip->i_d.di_anextents == 0);
Did you really mean to have the last line offset like this?  On second 
pass, it doesnt look similar to the original, and looks more like it may 
have been a typo in the review.  Just trying to avoid more cycles on 
spacing goofs.  Thx!

Allison

>>
>> Otherwise looks good:
>>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Alrighty, will fix.  Thanks!
> 
> Allison
> 
>>
>>> +}
>>> +
>>> +/*
>>> + * Attempts to set an attr in shortform, or converts the tree to 
>>> leaf form if
>>> + * there is not enough room.  If the attr is set, the transaction is 
>>> committed
>>> + * and set to NULL.
>>> + */
>>> +STATIC int
>>> +xfs_attr_set_shortform(
>>> +    struct xfs_da_args    *args,
>>> +    struct xfs_buf        **leaf_bp)
>>> +{
>>> +    struct xfs_inode    *dp = args->dp;
>>> +    int            error, error2 = 0;
>>> +
>>> +    /*
>>> +     * 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;
>>> +
>>> +    /*
>>> +     * 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;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/*
>>>    * Set the attribute specified in @args.
>>>    */
>>>   int
>>> @@ -212,48 +272,25 @@ xfs_attr_set_args(
>>>   {
>>>       struct xfs_inode    *dp = args->dp;
>>>       struct xfs_buf          *leaf_bp = NULL;
>>> -    int            error, error2 = 0;
>>> +    int            error = 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 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)) {
>>> -
>>> -        /*
>>> -         * 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;
>>> +    if (xfs_attr_is_shortform(dp)) {
>>>           /*
>>> -         * 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.
>>> +         * If the attr was successfully set in shortform, the
>>> +         * transaction is committed and set to NULL.  Otherwise, is it
>>> +         * converted from shortform to leaf, and the transaction is
>>> +         * retained.
>>>            */
>>> -        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);
>>> +        error = xfs_attr_set_shortform(args, &leaf_bp);
>>> +        if (error || !args->trans)
>>>               return error;
>>> -        }
>>>       }
>>>       if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>>> -- 
>>> 2.7.4
>>>
>>