Re: [PATCH v15 03/22] xfs: Hoist transaction handling in xfs_attr_node_remove_step

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 2/24/21 8:04 AM, Brian Foster wrote:
> On Thu, Feb 18, 2021 at 09:53:29AM -0700, Allison Henderson wrote:
> > This patch hoists transaction handling in xfs_attr_node_removename to
> > xfs_attr_node_remove_step.  This will help keep transaction handling in
> > higher level functions instead of buried in subfunctions when we
> > introduce delay attributes
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Great, thank you!
Allison
> >   fs/xfs/libxfs/xfs_attr.c | 45 ++++++++++++++++++++++-----------------------
> >   1 file changed, 22 insertions(+), 23 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 4e6c89d..3cf76e2 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -1251,9 +1251,7 @@ xfs_attr_node_remove_step(
> >   	struct xfs_da_args	*args,
> >   	struct xfs_da_state	*state)
> >   {
> > -	int			retval, error;
> > -	struct xfs_inode	*dp = args->dp;
> > -
> > +	int			error = 0;
> >   
> >   	/*
> >   	 * If there is an out-of-line value, de-allocate the blocks.
> > @@ -1265,25 +1263,6 @@ xfs_attr_node_remove_step(
> >   		if (error)
> >   			return error;
> >   	}
> > -	retval = xfs_attr_node_remove_cleanup(args, state);
> > -
> > -	/*
> > -	 * Check to see if the tree needs to be collapsed.
> > -	 */
> > -	if (retval && (state->path.active > 1)) {
> > -		error = xfs_da3_join(state);
> > -		if (error)
> > -			return error;
> > -		error = xfs_defer_finish(&args->trans);
> > -		if (error)
> > -			return error;
> > -		/*
> > -		 * Commit the Btree join operation and start a new trans.
> > -		 */
> > -		error = xfs_trans_roll_inode(&args->trans, dp);
> > -		if (error)
> > -			return error;
> > -	}
> >   
> >   	return error;
> >   }
> > @@ -1299,7 +1278,7 @@ xfs_attr_node_removename(
> >   	struct xfs_da_args	*args)
> >   {
> >   	struct xfs_da_state	*state = NULL;
> > -	int			error;
> > +	int			retval, error;
> >   	struct xfs_inode	*dp = args->dp;
> >   
> >   	trace_xfs_attr_node_removename(args);
> > @@ -1312,6 +1291,26 @@ xfs_attr_node_removename(
> >   	if (error)
> >   		goto out;
> >   
> > +	retval = xfs_attr_node_remove_cleanup(args, state);
> > +
> > +	/*
> > +	 * Check to see if the tree needs to be collapsed.
> > +	 */
> > +	if (retval && (state->path.active > 1)) {
> > +		error = xfs_da3_join(state);
> > +		if (error)
> > +			goto out;
> > +		error = xfs_defer_finish(&args->trans);
> > +		if (error)
> > +			goto out;
> > +		/*
> > +		 * Commit the Btree join operation and start a new trans.
> > +		 */
> > +		error = xfs_trans_roll_inode(&args->trans, dp);
> > +		if (error)
> > +			goto out;
> > +	}
> > +
> >   	/*
> >   	 * If the result is small enough, push it all into the inode.
> >   	 */
> > --
> > 2.7.4