Re: [PATCH v15 04/22] xfs: Hoist xfs_attr_set_shortform

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 2/24/21 8:04 AM, Brian Foster wrote:
> On Thu, Feb 18, 2021 at 09:53:30AM -0700, Allison Henderson wrote:
> > This patch hoists xfs_attr_set_shortform into the calling function. This
> > will help keep all state management code in the same scope.
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
>
> LGTM:
>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Thanks!
Allison
> >   fs/xfs/libxfs/xfs_attr.c | 81 ++++++++++++++++--------------------------------
> >   1 file changed, 27 insertions(+), 54 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 3cf76e2..a064c5b 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -217,53 +217,6 @@ xfs_attr_is_shortform(
> >   }
> >   
> >   /*
> > - * Attempts to set an attr in shortform, or converts short form 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
> > @@ -272,7 +225,7 @@ xfs_attr_set_args(
> >   {
> >   	struct xfs_inode	*dp = args->dp;
> >   	struct xfs_buf          *leaf_bp = NULL;
> > -	int			error = 0;
> > +	int			error2, error = 0;
> >   
> >   	/*
> >   	 * If the attribute list is already in leaf format, jump straight to
> > @@ -281,16 +234,36 @@ xfs_attr_set_args(
> >   	 * again.
> >   	 */
> >   	if (xfs_attr_is_shortform(dp)) {
> > +		/*
> > +		 * 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 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.
> > +		 * 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.
> >   		 */
> > -		error = xfs_attr_set_shortform(args, &leaf_bp);
> > -		if (error || !args->trans)
> > +		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;
> > +		}
> >   	}
> >   
> >   	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > --
> > 2.7.4