Re: [PATCH v13 09/10] xfs: Remove unused xfs_attr_*_args

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 11/10/20 1:07 PM, Darrick J. Wong wrote:
> On Thu, Oct 22, 2020 at 11:34:34PM -0700, Allison Henderson wrote:
> > Remove xfs_attr_set_args, xfs_attr_remove_args, and xfs_attr_trans_roll.
> > These high level loops are now driven by the delayed operations code,
> > and can be removed.
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c        | 97 +----------------------------------------
> >   fs/xfs/libxfs/xfs_attr.h        |  9 ++--
> >   fs/xfs/libxfs/xfs_attr_remote.c |  4 +-
> >   3 files changed, 5 insertions(+), 105 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index edd5d10..b5e1e84 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -262,65 +262,6 @@ xfs_attr_set_shortform(
> >   }
> >   
> >   /*
> > - * Checks to see if a delayed attribute transaction should be rolled.  If so,
> > - * also checks for a defer finish.  Transaction is finished and rolled as
> > - * needed, and returns true of false if the delayed operation should continue.
> > - */
> > -STATIC int
> > -xfs_attr_trans_roll(
> > -	struct xfs_delattr_context	*dac)
> > -{
> > -	struct xfs_da_args		*args = dac->da_args;
> > -	int				error = 0;
> > -
> > -	if (dac->flags & XFS_DAC_DEFER_FINISH) {
> > -		/*
> > -		 * The caller wants us to finish all the deferred ops so that we
> > -		 * avoid pinning the log tail with a large number of deferred
> > -		 * ops.
> > -		 */
> > -		dac->flags &= ~XFS_DAC_DEFER_FINISH;
> > -		error = xfs_defer_finish(&args->trans);
> > -		if (error)
> > -			return error;
> > -	}
> > -
> > -	return xfs_trans_roll_inode(&args->trans, args->dp);
> > -}
> > -
> > -/*
> > - * Set the attribute specified in @args.
> > - */
> > -int
> > -xfs_attr_set_args(
> > -	struct xfs_da_args	*args)
> > -{
> > -	struct xfs_buf			*leaf_bp = NULL;
> > -	int				error = 0;
> > -	struct xfs_delattr_context	dac = {
> > -		.da_args	= args,
> > -	};
> > -
> > -	do {
> > -		error = xfs_attr_set_iter(&dac, &leaf_bp);
>
> Now that there's only one caller of xfs_attr_set_iter and it passes
> &dac->leaf_bp, I think you can get rid of this second parameter, right?
>
> It's nice to see so much code disappear now that we track attr
> operations with deferred ops.  Everything else looks ok here. :)
>
> --D

Sure, I will collapse that parameter in then.

Allison

> > -		if (error != -EAGAIN)
> > -			break;
> > -
> > -		error = xfs_attr_trans_roll(&dac);
> > -		if (error)
> > -			return error;
> > -
> > -		if (leaf_bp) {
> > -			xfs_trans_bjoin(args->trans, leaf_bp);
> > -			xfs_trans_bhold(args->trans, leaf_bp);
> > -		}
> > -
> > -	} while (true);
> > -
> > -	return error;
> > -}
> > -
> > -/*
> >    * Set the attribute specified in @args.
> >    * This routine is meant to function as a delayed operation, and may return
> >    * -EAGAIN when the transaction needs to be rolled.  Calling functions will need
> > @@ -363,11 +304,7 @@ xfs_attr_set_iter(
> >   		 * continue.  Otherwise, is it converted from shortform to leaf
> >   		 * and -EAGAIN is returned.
> >   		 */
> > -		error = xfs_attr_set_shortform(args, leaf_bp);
> > -		if (error == -EAGAIN)
> > -			dac->flags |= XFS_DAC_DEFER_FINISH;
> > -
> > -		return error;
> > +		return xfs_attr_set_shortform(args, leaf_bp);
> >   	}
> >   
> >   	/*
> > @@ -398,7 +335,6 @@ xfs_attr_set_iter(
> >   			 * same state (inode locked and joined, transaction
> >   			 * clean) no matter how we got to this step.
> >   			 */
> > -			dac->flags |= XFS_DAC_DEFER_FINISH;
> >   			return -EAGAIN;
> >   		case 0:
> >   			dac->dela_state = XFS_DAS_FOUND_LBLK;
> > @@ -455,32 +391,6 @@ xfs_has_attr(
> >   
> >   /*
> >    * Remove the attribute specified in @args.
> > - */
> > -int
> > -xfs_attr_remove_args(
> > -	struct xfs_da_args	*args)
> > -{
> > -	int				error = 0;
> > -	struct xfs_delattr_context	dac = {
> > -		.da_args	= args,
> > -	};
> > -
> > -	do {
> > -		error = xfs_attr_remove_iter(&dac);
> > -		if (error != -EAGAIN)
> > -			break;
> > -
> > -		error = xfs_attr_trans_roll(&dac);
> > -		if (error)
> > -			return error;
> > -
> > -	} while (true);
> > -
> > -	return error;
> > -}
> > -
> > -/*
> > - * Remove the attribute specified in @args.
> >    *
> >    * This function may return -EAGAIN to signal that the transaction needs to be
> >    * rolled.  Callers should continue calling this function until they receive a
> > @@ -895,7 +805,6 @@ xfs_attr_leaf_addname(
> >   		if (error)
> >   			return error;
> >   
> > -		dac->flags |= XFS_DAC_DEFER_FINISH;
> >   		return -EAGAIN;
> >   	}
> >   
> > @@ -1192,7 +1101,6 @@ xfs_attr_node_addname(
> >   			 * Restart routine from the top.  No need to set  the
> >   			 * state
> >   			 */
> > -			dac->flags |= XFS_DAC_DEFER_FINISH;
> >   			return -EAGAIN;
> >   		}
> >   
> > @@ -1205,7 +1113,6 @@ xfs_attr_node_addname(
> >   		error = xfs_da3_split(state);
> >   		if (error)
> >   			goto out;
> > -		dac->flags |= XFS_DAC_DEFER_FINISH;
> >   	} else {
> >   		/*
> >   		 * Addition succeeded, update Btree hashvals.
> > @@ -1246,7 +1153,6 @@ xfs_attr_node_addname(
> >   			if (error)
> >   				return error;
> >   
> > -			dac->flags |= XFS_DAC_DEFER_FINISH;
> >   			dac->dela_state = XFS_DAS_ALLOC_NODE;
> >   			return -EAGAIN;
> >   		}
> > @@ -1516,7 +1422,6 @@ xfs_attr_node_remove_step(
> >   		if (error)
> >   			return error;
> >   
> > -		dac->flags |= XFS_DAC_DEFER_FINISH;
> >   		dac->dela_state = XFS_DAS_RM_SHRINK;
> >   		return -EAGAIN;
> >   	}
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 8a08411..6d90301 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -244,10 +244,9 @@ enum xfs_delattr_state {
> >   /*
> >    * Defines for xfs_delattr_context.flags
> >    */
> > -#define XFS_DAC_DEFER_FINISH		0x01 /* finish the transaction */
> > -#define XFS_DAC_NODE_RMVNAME_INIT	0x02 /* xfs_attr_node_removename init */
> > -#define XFS_DAC_LEAF_ADDNAME_INIT	0x04 /* xfs_attr_leaf_addname init*/
> > -#define XFS_DAC_DELAYED_OP_INIT		0x08 /* delayed operations init*/
> > +#define XFS_DAC_NODE_RMVNAME_INIT	0x01 /* xfs_attr_node_removename init */
> > +#define XFS_DAC_LEAF_ADDNAME_INIT	0x02 /* xfs_attr_leaf_addname init*/
> > +#define XFS_DAC_DELAYED_OP_INIT		0x04 /* delayed operations init*/
> >   
> >   /*
> >    * Context used for keeping track of delayed attribute operations
> > @@ -297,11 +296,9 @@ int xfs_inode_hasattr(struct xfs_inode *ip);
> >   int xfs_attr_get_ilocked(struct xfs_da_args *args);
> >   int xfs_attr_get(struct xfs_da_args *args);
> >   int xfs_attr_set(struct xfs_da_args *args);
> > -int xfs_attr_set_args(struct xfs_da_args *args);
> >   int xfs_attr_set_iter(struct xfs_delattr_context *dac,
> >   		      struct xfs_buf **leaf_bp);
> >   int xfs_has_attr(struct xfs_da_args *args);
> > -int xfs_attr_remove_args(struct xfs_da_args *args);
> >   int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
> >   bool xfs_attr_namecheck(const void *name, size_t length);
> >   void xfs_delattr_context_init(struct xfs_delattr_context *dac,
> > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > index 45c4bc5..262d1870 100644
> > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > @@ -751,10 +751,8 @@ xfs_attr_rmtval_remove(
> >   	if (error)
> >   		return error;
> >   
> > -	if (!done) {
> > -		dac->flags |= XFS_DAC_DEFER_FINISH;
> > +	if (!done)
> >   		return -EAGAIN;
> > -	}
> >   
> >   	return error;
> >   }
> > --
> > 2.7.4