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

[Brian Foster <bfoster@redhat.com>]

On Fri, Apr 10, 2020 at 09:55:55AM -0700, Allison Collins wrote:
> 
> 
> 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!
> 

Nope. What is quoted here is not how my reply appears in my mailer. :/
Anyways, the last line is supposed to be aligned to the first character
inside the opening brace of the previous line.

Brian

> 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
> > > > 
> > > 
>