diff mbox series

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

Message ID 158993914950.976105.8586367797907212993.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: refactor incore inode walking | expand

Commit Message

Darrick J. Wong May 20, 2020, 1:45 a.m. UTC 
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(-)

Comments

Christoph Hellwig May 20, 2020, 6:38 a.m. UTC | #1 
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.

> @@ -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..

> 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.
Darrick J. Wong May 20, 2020, 6:36 p.m. UTC | #2 
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
Christoph Hellwig May 21, 2020, 9:02 a.m. UTC | #3 
On Wed, May 20, 2020 at 11:36:27AM -0700, Darrick J. Wong wrote:
> > > -		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;' ?

Yikes.  Why does everyone hate the nice, explicit and readable if
statements?
diff mbox series

Patch

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));
 
 	if (!xfs_can_free_eofblocks(ip, false)) {
 		/* inode could be preallocated or append-only */
@@ -1461,8 +1459,7 @@  xfs_inode_free_eofblocks(
 	 * If the mapping is dirty the operation can block and wait for some
 	 * time. Unless we are waiting, skip it.
 	 */
-	if (!(flags & SYNC_WAIT) &&
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))
+	if (!wait && mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))
 		return 0;
 
 	if (eofb) {
@@ -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;
 	}
@@ -1498,16 +1495,10 @@  static int
 __xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
 	struct xfs_eofblocks	*eofb,
-	int			(*execute)(struct xfs_inode *ip, int flags,
-					   void *args),
+	int			(*execute)(struct xfs_inode *ip, void *args),
 	int			tag)
 {
-	int flags = SYNC_TRYLOCK;
-
-	if (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC))
-		flags = SYNC_WAIT;
-
-	return xfs_inode_ag_iterator(mp, 0, execute, flags, eofb, tag);
+	return xfs_inode_ag_iterator(mp, 0, execute, eofb, tag);
 }
 
 int
@@ -1732,7 +1723,6 @@  xfs_prep_free_cowblocks(
 STATIC int
 xfs_inode_free_cowblocks(
 	struct xfs_inode	*ip,
-	int			flags,
 	void			*args)
 {
 	struct xfs_eofblocks	*eofb = args;
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 2d5ab9957d9f..e7f86ebd7b22 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -72,8 +72,8 @@  void xfs_cowblocks_worker(struct work_struct *);
 void xfs_queue_cowblocks(struct xfs_mount *);
 
 int xfs_inode_ag_iterator(struct xfs_mount *mp, int iter_flags,
-	int (*execute)(struct xfs_inode *ip, int flags, void *args),
-	int flags, void *args, int tag);
+	int (*execute)(struct xfs_inode *ip, void *args),
+	void *args, int tag);
 
 int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
 				  xfs_ino_t ino, bool *inuse);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
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;
+};
+
 STATIC int
 xfs_dqrele_inode(
 	struct xfs_inode	*ip,
-	int			flags,
 	void			*args)
 {
+	struct xfs_dqrele	*dqr = args;
+
 	/* skip quota inodes */
 	if (ip == ip->i_mount->m_quotainfo->qi_uquotaip ||
 	    ip == ip->i_mount->m_quotainfo->qi_gquotaip ||
@@ -743,15 +748,15 @@  xfs_dqrele_inode(
 	}
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
+	if ((dqr->flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
 		xfs_qm_dqrele(ip->i_udquot);
 		ip->i_udquot = NULL;
 	}
-	if ((flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) {
+	if ((dqr->flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) {
 		xfs_qm_dqrele(ip->i_gdquot);
 		ip->i_gdquot = NULL;
 	}
-	if ((flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
+	if ((dqr->flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
 		xfs_qm_dqrele(ip->i_pdquot);
 		ip->i_pdquot = NULL;
 	}
@@ -768,10 +773,14 @@  xfs_dqrele_inode(
  */
 void
 xfs_qm_dqrele_all_inodes(
-	struct xfs_mount *mp,
-	uint		 flags)
+	struct xfs_mount	*mp,
+	uint			flags)
 {
+	struct xfs_dqrele	dqr = {
+		.flags		= flags,
+	};
+
 	ASSERT(mp->m_quotainfo);
-	xfs_inode_ag_iterator(mp, XFS_AGITER_INEW_WAIT, xfs_dqrele_inode,
-			flags, NULL, XFS_ICI_NO_TAG);
+	xfs_inode_ag_iterator(mp, XFS_AGITER_INEW_WAIT, xfs_dqrele_inode, &dqr,
+			XFS_ICI_NO_TAG);
 }