Re: [PATCH 05/11] xfs: remove flags argument from xfs_inode_ag_walk

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, May 20, 2020 at 08:38:25AM +0200, Christoph Hellwig wrote:
> On Tue, May 19, 2020 at 06:45:49PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The incore inode walk code passes a flags argument and a pointer from
> > the xfs_inode_ag_iterator caller all the way to the iteration function.
> > We can reduce the function complexity by passing flags through the
> > private pointer.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_icache.c      |   38 ++++++++++++++------------------------
> >  fs/xfs/xfs_icache.h      |    4 ++--
> >  fs/xfs/xfs_qm_syscalls.c |   25 +++++++++++++++++--------
> >  3 files changed, 33 insertions(+), 34 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index e716b19879c6..87b98bfdf27d 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -790,9 +790,7 @@ STATIC int
> >  xfs_inode_ag_walk(
> >  	struct xfs_mount	*mp,
> >  	struct xfs_perag	*pag,
> > -	int			(*execute)(struct xfs_inode *ip, int flags,
> > -					   void *args),
> > -	int			flags,
> > +	int			(*execute)(struct xfs_inode *ip, void *args),
> >  	void			*args,
> >  	int			tag,
> >  	int			iter_flags)
> > @@ -868,7 +866,7 @@ xfs_inode_ag_walk(
> >  			if ((iter_flags & XFS_AGITER_INEW_WAIT) &&
> >  			    xfs_iflags_test(batch[i], XFS_INEW))
> >  				xfs_inew_wait(batch[i]);
> > -			error = execute(batch[i], flags, args);
> > +			error = execute(batch[i], args);
> >  			xfs_irele(batch[i]);
> >  			if (error == -EAGAIN) {
> >  				skipped++;
> > @@ -972,9 +970,7 @@ int
> >  xfs_inode_ag_iterator(
> >  	struct xfs_mount	*mp,
> >  	int			iter_flags,
> > -	int			(*execute)(struct xfs_inode *ip, int flags,
> > -					   void *args),
> > -	int			flags,
> > +	int			(*execute)(struct xfs_inode *ip, void *args),
> >  	void			*args,
> >  	int			tag)
> >  {
> > @@ -986,7 +982,7 @@ xfs_inode_ag_iterator(
> >  	ag = 0;
> >  	while ((pag = xfs_ici_walk_get_perag(mp, ag, tag))) {
> >  		ag = pag->pag_agno + 1;
> > -		error = xfs_inode_ag_walk(mp, pag, execute, flags, args, tag,
> > +		error = xfs_inode_ag_walk(mp, pag, execute, args, tag,
> >  				iter_flags);
> >  		xfs_perag_put(pag);
> >  		if (error) {
> > @@ -1443,12 +1439,14 @@ xfs_inode_match_id_union(
> >  STATIC int
> >  xfs_inode_free_eofblocks(
> >  	struct xfs_inode	*ip,
> > -	int			flags,
> >  	void			*args)
> >  {
> > -	int ret = 0;
> > -	struct xfs_eofblocks *eofb = args;
> > -	int match;
> > +	struct xfs_eofblocks	*eofb = args;
> > +	bool			wait;
> > +	int			match;
> > +	int			ret = 0;
> > +
> > +	wait = (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC));
> 
> No need for the outer braces.

Fixed.

> > @@ -1484,7 +1481,7 @@ xfs_inode_free_eofblocks(
> >  	 * scanner moving and revisit the inode in a subsequent pass.
> >  	 */
> >  	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
> > -		if (flags & SYNC_WAIT)
> > +		if (wait)
> >  			ret = -EAGAIN;
> >  		return ret;
> 
> Just me, but I'd prefer an explicit:
> 
> 		if (wait)
> 			return -EAGAIN;
> 		return 0;
> 
> here.  Not really new in this patch, but if you touch this area anyway..

How about 'return wait ? -EAGAIN : 0;' ?

> > index a9460bdcca87..571ecb17b3bf 100644
> > --- a/fs/xfs/xfs_qm_syscalls.c
> > +++ b/fs/xfs/xfs_qm_syscalls.c
> > @@ -726,12 +726,17 @@ xfs_qm_scall_getquota_next(
> >  	return error;
> >  }
> >  
> > +struct xfs_dqrele {
> > +	uint		flags;
> > +};
> 
> > +	struct xfs_dqrele	dqr = {
> > +		.flags		= flags,
> > +	};
> 
> Instead of a new structure we could just take the address of flags and
> pass that to simplify the code a bit.

Fixed.

--D