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