Re: [PATCH RESEND v18 04/11] xfs: Add helper xfs_attr_set_fmt

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 5/13/21 4:46 PM, Darrick J. Wong wrote:
> On Wed, May 12, 2021 at 09:14:01AM -0700, Allison Henderson wrote:
> > This patch adds a helper function xfs_attr_set_fmt.  This will help
> > isolate the code that will require state management from the portions
> > that do not.  xfs_attr_set_fmt returns 0 when the attr has been set and
> > no further action is needed.  It returns -EAGAIN when shortform has been
> > transformed to leaf, and the calling function should proceed the set the
> > attr in leaf form.
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > Reviewed-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
>
> Er... can't you combine patches 3 and 4 into a single patch that
> renames xfs_attr_set_shortform -> xfs_attr_set_fmt and drops the
> **leafbp parameter?  Smushing the two together it's a bit more obvious
> what's really changing here (which really isn't that much!) so:
>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> (Though I think I would like the two combined for v19.  But let's see
> what I think of the whole series by the time I reach the end, eh? :) )
So... did your feelings change much by the end of the set?  I have to 
admit, looking at the combination of these two patches, the diff does 
not look particularly attractive.  During all our refactoring efforts, I 
think we did a little bit of a circle between these two.

Rather than sending out a v19 with a poor patch that will most certainly 
result in a v20... how about I slap them together, and send them out in 
an RFC explaining what it is?  That way people can look at it and we can 
discuss what we really want to keep.  Because from looking at the diff, 
there's really only a few bits of functional changes, that would 
probably be appropriate to lump in with patch 11 if everyone is in 
agreement.  Then possibly just drop 3 and 4?

Allison


> --D
>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 79 ++++++++++++++++++++++++++++--------------------
> >   1 file changed, 46 insertions(+), 33 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 32133a0..1a618a2 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -236,6 +236,48 @@ xfs_attr_is_shortform(
> >   		ip->i_afp->if_nextents == 0);
> >   }
> >   
> > +STATIC int
> > +xfs_attr_set_fmt(
> > +	struct xfs_da_args	*args)
> > +{
> > +	struct xfs_buf          *leaf_bp = NULL;
> > +	struct xfs_inode	*dp = args->dp;
> > +	int			error2, error = 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.
> > +	 */
> > +	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 -EAGAIN;
> > +}
> > +
> >   /*
> >    * Set the attribute specified in @args.
> >    */
> > @@ -244,8 +286,7 @@ xfs_attr_set_args(
> >   	struct xfs_da_args	*args)
> >   {
> >   	struct xfs_inode	*dp = args->dp;
> > -	struct xfs_buf          *leaf_bp = NULL;
> > -	int			error2, error = 0;
> > +	int			error;
> >   
> >   	/*
> >   	 * If the attribute list is already in leaf format, jump straight to
> > @@ -254,36 +295,9 @@ 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;
> > -
> > -		/*
> > -		 * 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.
> > -		 */
> > -		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_fmt(args);
> > +		if (error != -EAGAIN)
> >   			return error;
> > -		}
> >   	}
> >   
> >   	if (xfs_attr_is_leaf(dp)) {
> > @@ -317,8 +331,7 @@ xfs_attr_set_args(
> >   			return error;
> >   	}
> >   
> > -	error = xfs_attr_node_addname(args);
> > -	return error;
> > +	return xfs_attr_node_addname(args);
> >   }
> >   
> >   /*
> > --
> > 2.7.4