Re: [PATCH v7 16/19] xfs: Simplify xfs_attr_set_iter

[Allison Collins <allison.henderson@xxxxxxxxxx>]

On 3/3/20 9:30 PM, Chandan Rajendra wrote:
> On Sunday, February 23, 2020 7:36 AM Allison Collins wrote:
> > Delayed attribute mechanics make frequent use of goto statements.  We can use this
> > to further simplify xfs_attr_set_iter.  Because states tend to fall between if
> > conditions, we can invert the if logic and jump to the goto. This helps to reduce
> > indentation and simplify things.
>
> I don't see any logical errors.
>
> Reviewed-by: Chandan Rajendra <chandanrlinux@xxxxxxxxx>
Alrighty, thanks for the reviews!  I got some feed back in other reviews 
to move the patches 13 and 14 to the end of the set.  Which means the 
patches ahead of them may change a bit in order to seat correctly.  For 
example, this patch will likely go back to being more like it's v6 version:

https://www.spinics.net/lists/linux-xfs/msg36072.html

Would you prefer I keep or drop your RVB's in this case?  Functionally 
they wont change much, but I understand that function is a lot of what 
your are analyzing too.  Let me know what you are comfortable with.  Thanks!

Allison

> > Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 71 ++++++++++++++++++++++++++++--------------------
> >   1 file changed, 42 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 30a16fe..dd935ff 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -254,6 +254,19 @@ 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_fmt_needs_update(
> > +	struct xfs_inode    *dp)
> > +{
> > +	return dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> > +	      (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> > +	      dp->i_d.di_anextents == 0);
> > +}
> > +
> > +/*
> >    * Set the attribute specified in @args.
> >    */
> >   int
> > @@ -342,40 +355,40 @@ xfs_attr_set_iter(
> >   	}
> >   
> >   	/*
> > -	 * 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)) {
> > +	if (!xfs_attr_fmt_needs_update(dp))
> > +		goto add_leaf;
> >   
> > -		/*
> > -		 * Try to add the attr to the attribute list in the inode.
> > -		 */
> > -		error = xfs_attr_try_sf_addname(dp, args);
> > +	/*
> > +	 * Try to add the attr to the attribute list in the inode.
> > +	 */
> > +	error = xfs_attr_try_sf_addname(dp, args);
> >   
> > -		/* Should only be 0, -EEXIST or ENOSPC */
> > -		if (error != -ENOSPC)
> > -			return error;
> > +	/* Should only be 0, -EEXIST or ENOSPC */
> > +	if (error != -ENOSPC)
> > +		return error;
> >   
> > -		/*
> > -		 * 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;
> > +	/*
> > +	 * 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);
> > -		args->dac.flags |= XFS_DAC_FINISH_TRANS;
> > -		args->dac.dela_state = XFS_DAS_ADD_LEAF;
> > -		return -EAGAIN;
> > -	}
> > +	/*
> > +	 * 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);
> > +	args->dac.flags |= XFS_DAC_FINISH_TRANS;
> > +	args->dac.dela_state = XFS_DAS_ADD_LEAF;
> > +	return -EAGAIN;
> >   
> >   add_leaf:
> >