Re: [PATCH v5 07/14] xfs: Factor out xfs_attr_leaf_addname helper

[Allison Collins <allison.henderson@xxxxxxxxxx>]

On 12/13/19 7:15 AM, Brian Foster wrote:
> On Wed, Dec 11, 2019 at 09:15:06PM -0700, Allison Collins wrote:
> > Factor out new helper function xfs_attr_leaf_try_add.
> > Because new delayed attribute routines cannot roll
> > transactions, we carve off the parts of
> > xfs_attr_leaf_addname that we can use.  This will help
> > to reduce repetitive code later when we introduce
> > delayed attributes.
> >
> > Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 83 ++++++++++++++++++++++++++++--------------------
> >   1 file changed, 49 insertions(+), 34 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index ee973d2..36f6a43 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -305,10 +305,30 @@ xfs_attr_set_args(
> >   		}
> >   	}
> >   
> > -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> >   		error = xfs_attr_leaf_addname(args);
> > -	else
> > -		error = xfs_attr_node_addname(args);
> > +		if (error == 0 || error != -ENOSPC)
> > +			return 0;
>
> No need to check for 0 since 0 != -ENOSPC. We also probably want to
> return error instead of zero. With that fixed up:
>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Ok will fix.  Thank you!!

Allison

> > +
> > +		/*
> > +		 * Commit that transaction so that the node_addname()
> > +		 * call can manage its own transactions.
> > +		 */
> > +		error = xfs_defer_finish(&args->trans);
> > +		if (error)
> > +			return error;
> > +
> > +		/*
> > +		 * Commit the current trans (including the inode) and
> > +		 * start a new one.
> > +		 */
> > +		error = xfs_trans_roll_inode(&args->trans, dp);
> > +		if (error)
> > +			return error;
> > +
> > +	}
> > +
> > +	error = xfs_attr_node_addname(args);
> >   	return error;
> >   }
> >   
> > @@ -606,21 +626,12 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
> >    * External routines when attribute list is one block
> >    *========================================================================*/
> >   
> > -/*
> > - * Add a name to the leaf attribute list structure
> > - *
> > - * This leaf block cannot have a "remote" value, we only call this routine
> > - * if bmap_one_block() says there is only one block (ie: no remote blks).
> > - */
> >   STATIC int
> > -xfs_attr_leaf_addname(
> > -	struct xfs_da_args	*args)
> > +xfs_attr_leaf_try_add(
> > +	struct xfs_da_args	*args,
> > +	struct xfs_buf		*bp)
> >   {
> > -	struct xfs_buf		*bp;
> > -	int			retval, error, forkoff;
> > -	struct xfs_inode	*dp = args->dp;
> > -
> > -	trace_xfs_attr_leaf_addname(args);
> > +	int			retval, error;
> >   
> >   	/*
> >   	 * Look up the given attribute in the leaf block.  Figure out if
> > @@ -666,31 +677,35 @@ xfs_attr_leaf_addname(
> >   	retval = xfs_attr3_leaf_add(bp, args);
> >   	if (retval == -ENOSPC) {
> >   		/*
> > -		 * Promote the attribute list to the Btree format, then
> > -		 * Commit that transaction so that the node_addname() call
> > -		 * can manage its own transactions.
> > +		 * Promote the attribute list to the Btree format.
> > +		 * Unless an error occurs, retain the -ENOSPC retval
> >   		 */
> >   		error = xfs_attr3_leaf_to_node(args);
> >   		if (error)
> >   			return error;
> > -		error = xfs_defer_finish(&args->trans);
> > -		if (error)
> > -			return error;
> > +	}
> > +	return retval;
> > +}
> >   
> > -		/*
> > -		 * Commit the current trans (including the inode) and start
> > -		 * a new one.
> > -		 */
> > -		error = xfs_trans_roll_inode(&args->trans, dp);
> > -		if (error)
> > -			return error;
> >   
> > -		/*
> > -		 * Fob the whole rest of the problem off on the Btree code.
> > -		 */
> > -		error = xfs_attr_node_addname(args);
> > +/*
> > + * Add a name to the leaf attribute list structure
> > + *
> > + * This leaf block cannot have a "remote" value, we only call this routine
> > + * if bmap_one_block() says there is only one block (ie: no remote blks).
> > + */
> > +STATIC int
> > +xfs_attr_leaf_addname(struct xfs_da_args	*args)
> > +{
> > +	int			error, forkoff;
> > +	struct xfs_buf		*bp = NULL;
> > +	struct xfs_inode	*dp = args->dp;
> > +
> > +	trace_xfs_attr_leaf_addname(args);
> > +
> > +	error = xfs_attr_leaf_try_add(args, bp);
> > +	if (error)
> >   		return error;
> > -	}
> >   
> >   	/*
> >   	 * Commit the transaction that added the attr name so that
> > --
> > 2.7.4