simplify the blockgc iwalk infrastructure

simplify the blockgc iwalk infrastructure

[Christoph Hellwig]

Hi all,

this series switches quotaoff to use the VFS s_inodes list and then
simplifies the iwalk infrastructure for its only remaining user.

[PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes

[Christoph Hellwig]

Using xfs_inode_walk in xfs_qm_dqrele_all_inodes is complete overkill,
given that function simplify wants to iterate all live inodes known
to the VFS.  Just iterate over the s_inodes list.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_icache.c      |  2 +-
 fs/xfs/xfs_icache.h      |  2 ++
 fs/xfs/xfs_qm_syscalls.c | 56 +++++++++++++++++++++++++---------------
 3 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 1d7720a0c068b7..c0d084e3526a9c 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -233,7 +233,7 @@ xfs_inode_clear_reclaim_tag(
 	xfs_perag_clear_reclaim_tag(pag);
 }
 
-static void
+void
 xfs_inew_wait(
 	struct xfs_inode	*ip)
 {
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index d1fddb1524200b..d70d93c2bb1084 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -78,4 +78,6 @@ int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
 void xfs_blockgc_stop(struct xfs_mount *mp);
 void xfs_blockgc_start(struct xfs_mount *mp);
 
+void xfs_inew_wait(struct xfs_inode *ip);
+
 #endif
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index ca1b57d291dc90..88dfd520ae91de 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -748,41 +748,27 @@ xfs_qm_scall_getquota_next(
 	return error;
 }
 
-STATIC int
+static void
 xfs_dqrele_inode(
 	struct xfs_inode	*ip,
-	void			*args)
+	uint			flags)
 {
-	uint			*flags = args;
-
-	/* skip quota inodes */
-	if (ip == ip->i_mount->m_quotainfo->qi_uquotaip ||
-	    ip == ip->i_mount->m_quotainfo->qi_gquotaip ||
-	    ip == ip->i_mount->m_quotainfo->qi_pquotaip) {
-		ASSERT(ip->i_udquot == NULL);
-		ASSERT(ip->i_gdquot == NULL);
-		ASSERT(ip->i_pdquot == NULL);
-		return 0;
-	}
-
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	if ((*flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
+	if ((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 ((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 ((flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
 		xfs_qm_dqrele(ip->i_pdquot);
 		ip->i_pdquot = NULL;
 	}
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	return 0;
 }
 
-
 /*
  * Go thru all the inodes in the file system, releasing their dquots.
  *
@@ -794,7 +780,35 @@ xfs_qm_dqrele_all_inodes(
 	struct xfs_mount	*mp,
 	uint			flags)
 {
+	struct super_block	*sb = mp->m_super;
+	struct inode		*inode, *old_inode = NULL;
+
 	ASSERT(mp->m_quotainfo);
-	xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
-			&flags, XFS_ICI_NO_TAG);
+
+	spin_lock(&sb->s_inode_list_lock);
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+		struct xfs_inode	*ip = XFS_I(inode);
+
+		if (XFS_FORCED_SHUTDOWN(mp))
+			break;
+		if (xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
+			continue;
+		if (xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
+			continue;
+		if (!igrab(inode))
+			continue;
+		spin_unlock(&sb->s_inode_list_lock);
+
+		iput(old_inode);
+		old_inode = inode;
+
+		if (xfs_iflags_test(ip, XFS_INEW))
+			xfs_inew_wait(ip);
+		xfs_dqrele_inode(ip, flags);
+
+		spin_lock(&sb->s_inode_list_lock);
+	}
+	spin_unlock(&sb->s_inode_list_lock);
+
+	iput(old_inode);
 }
-- 
2.30.1

[PATCH 2/3] xfs: simplify the perage inode walk infrastructure

[Christoph Hellwig]

Remove the generic xfs_inode_walk and just open code the only caller.
Note that this leads to a somewhat ugly forward delcaration for
xfs_blockgc_scan_inode, but with other changes in that area pending
it seems like the lesser evil.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_icache.c | 103 +++++++++++++-------------------------------
 fs/xfs/xfs_icache.h |  11 -----
 2 files changed, 30 insertions(+), 84 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index c0d084e3526a9c..7fdf77df66269c 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -728,11 +728,9 @@ xfs_icache_inode_is_allocated(
  */
 STATIC bool
 xfs_inode_walk_ag_grab(
-	struct xfs_inode	*ip,
-	int			flags)
+	struct xfs_inode	*ip)
 {
 	struct inode		*inode = VFS_I(ip);
-	bool			newinos = !!(flags & XFS_INODE_WALK_INEW_WAIT);
 
 	ASSERT(rcu_read_lock_held());
 
@@ -742,8 +740,7 @@ xfs_inode_walk_ag_grab(
 		goto out_unlock_noent;
 
 	/* avoid new or reclaimable inodes. Leave for reclaim code to flush */
-	if ((!newinos && __xfs_iflags_test(ip, XFS_INEW)) ||
-	    __xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
+	if (__xfs_iflags_test(ip, XFS_INEW | XFS_IRECLAIMABLE | XFS_IRECLAIM))
 		goto out_unlock_noent;
 	spin_unlock(&ip->i_flags_lock);
 
@@ -763,17 +760,19 @@ xfs_inode_walk_ag_grab(
 	return false;
 }
 
+static int
+xfs_blockgc_scan_inode(
+	struct xfs_inode	*ip,
+	void			*args);
+
 /*
  * For a given per-AG structure @pag, grab, @execute, and rele all incore
  * inodes with the given radix tree @tag.
  */
 STATIC int
-xfs_inode_walk_ag(
+xfs_blockgc_free_space_ag(
 	struct xfs_perag	*pag,
-	int			iter_flags,
-	int			(*execute)(struct xfs_inode *ip, void *args),
-	void			*args,
-	int			tag)
+	struct xfs_eofblocks	*eofb)
 {
 	struct xfs_mount	*mp = pag->pag_mount;
 	uint32_t		first_index;
@@ -793,17 +792,9 @@ xfs_inode_walk_ag(
 		int		i;
 
 		rcu_read_lock();
-
-		if (tag == XFS_ICI_NO_TAG)
-			nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
-					(void **)batch, first_index,
-					XFS_LOOKUP_BATCH);
-		else
-			nr_found = radix_tree_gang_lookup_tag(
-					&pag->pag_ici_root,
-					(void **) batch, first_index,
-					XFS_LOOKUP_BATCH, tag);
-
+		nr_found = radix_tree_gang_lookup_tag(&pag->pag_ici_root,
+				(void **) batch, first_index,
+				XFS_LOOKUP_BATCH, XFS_ICI_BLOCKGC_TAG);
 		if (!nr_found) {
 			rcu_read_unlock();
 			break;
@@ -816,7 +807,7 @@ xfs_inode_walk_ag(
 		for (i = 0; i < nr_found; i++) {
 			struct xfs_inode *ip = batch[i];
 
-			if (done || !xfs_inode_walk_ag_grab(ip, iter_flags))
+			if (done || !xfs_inode_walk_ag_grab(ip))
 				batch[i] = NULL;
 
 			/*
@@ -844,10 +835,7 @@ xfs_inode_walk_ag(
 		for (i = 0; i < nr_found; i++) {
 			if (!batch[i])
 				continue;
-			if ((iter_flags & XFS_INODE_WALK_INEW_WAIT) &&
-			    xfs_iflags_test(batch[i], XFS_INEW))
-				xfs_inew_wait(batch[i]);
-			error = execute(batch[i], args);
+			error = xfs_blockgc_scan_inode(batch[i], eofb);
 			xfs_irele(batch[i]);
 			if (error == -EAGAIN) {
 				skipped++;
@@ -872,49 +860,6 @@ xfs_inode_walk_ag(
 	return last_error;
 }
 
-/* Fetch the next (possibly tagged) per-AG structure. */
-static inline struct xfs_perag *
-xfs_inode_walk_get_perag(
-	struct xfs_mount	*mp,
-	xfs_agnumber_t		agno,
-	int			tag)
-{
-	if (tag == XFS_ICI_NO_TAG)
-		return xfs_perag_get(mp, agno);
-	return xfs_perag_get_tag(mp, agno, tag);
-}
-
-/*
- * Call the @execute function on all incore inodes matching the radix tree
- * @tag.
- */
-int
-xfs_inode_walk(
-	struct xfs_mount	*mp,
-	int			iter_flags,
-	int			(*execute)(struct xfs_inode *ip, void *args),
-	void			*args,
-	int			tag)
-{
-	struct xfs_perag	*pag;
-	int			error = 0;
-	int			last_error = 0;
-	xfs_agnumber_t		ag;
-
-	ag = 0;
-	while ((pag = xfs_inode_walk_get_perag(mp, ag, tag))) {
-		ag = pag->pag_agno + 1;
-		error = xfs_inode_walk_ag(pag, iter_flags, execute, args, tag);
-		xfs_perag_put(pag);
-		if (error) {
-			last_error = error;
-			if (error == -EFSCORRUPTED)
-				break;
-		}
-	}
-	return last_error;
-}
-
 /*
  * Grab the inode for reclaim exclusively.
  *
@@ -1617,8 +1562,7 @@ xfs_blockgc_worker(
 
 	if (!sb_start_write_trylock(mp->m_super))
 		return;
-	error = xfs_inode_walk_ag(pag, 0, xfs_blockgc_scan_inode, NULL,
-			XFS_ICI_BLOCKGC_TAG);
+	error = xfs_blockgc_free_space_ag(pag, NULL);
 	if (error)
 		xfs_info(mp, "AG %u preallocation gc worker failed, err=%d",
 				pag->pag_agno, error);
@@ -1634,10 +1578,23 @@ xfs_blockgc_free_space(
 	struct xfs_mount	*mp,
 	struct xfs_eofblocks	*eofb)
 {
+	struct xfs_perag	*pag;
+	xfs_agnumber_t		ag = 0;
+	int			error = 0, last_error = 0;
+
 	trace_xfs_blockgc_free_space(mp, eofb, _RET_IP_);
 
-	return xfs_inode_walk(mp, 0, xfs_blockgc_scan_inode, eofb,
-			XFS_ICI_BLOCKGC_TAG);
+	while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_BLOCKGC_TAG))) {
+		ag = pag->pag_agno + 1;
+		error = xfs_blockgc_free_space_ag(pag, eofb);
+		xfs_perag_put(pag);
+		if (error) {
+			if (error == -EFSCORRUPTED)
+				return error;
+			last_error = error;
+		}
+	}
+	return last_error;
 }
 
 /*
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index d70d93c2bb1084..da390097546c67 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -20,8 +20,6 @@ struct xfs_eofblocks {
 /*
  * tags for inode radix tree
  */
-#define XFS_ICI_NO_TAG		(-1)	/* special flag for an untagged lookup
-					   in xfs_inode_walk */
 #define XFS_ICI_RECLAIM_TAG	0	/* inode is to be reclaimed */
 /* Inode has speculative preallocations (posteof or cow) to clean. */
 #define XFS_ICI_BLOCKGC_TAG	1
@@ -34,11 +32,6 @@ struct xfs_eofblocks {
 #define XFS_IGET_DONTCACHE	0x4
 #define XFS_IGET_INCORE		0x8	/* don't read from disk or reinit */
 
-/*
- * flags for AG inode iterator
- */
-#define XFS_INODE_WALK_INEW_WAIT	0x1	/* wait on new inodes */
-
 int xfs_iget(struct xfs_mount *mp, struct xfs_trans *tp, xfs_ino_t ino,
 	     uint flags, uint lock_flags, xfs_inode_t **ipp);
 
@@ -68,10 +61,6 @@ void xfs_inode_clear_cowblocks_tag(struct xfs_inode *ip);
 
 void xfs_blockgc_worker(struct work_struct *work);
 
-int xfs_inode_walk(struct xfs_mount *mp, int iter_flags,
-	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);
 
-- 
2.30.1

[PATCH 3/3] xfs: pass struct xfs_eofblocks to the inode scan callback

[Christoph Hellwig]

Pass the actual structure instead of a void pointer here now
that none of the functions is used as a callback.

Signed-off-by: Christoph Hellwig <hch@lst.de>
 xfs_inode_free_cowblocks(
---
 fs/xfs/xfs_icache.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 7fdf77df66269c..06286b5b613252 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -763,7 +763,7 @@ xfs_inode_walk_ag_grab(
 static int
 xfs_blockgc_scan_inode(
 	struct xfs_inode	*ip,
-	void			*args);
+	struct xfs_eofblocks	*eofb);
 
 /*
  * For a given per-AG structure @pag, grab, @execute, and rele all incore
@@ -1228,10 +1228,9 @@ xfs_reclaim_worker(
 STATIC int
 xfs_inode_free_eofblocks(
 	struct xfs_inode	*ip,
-	void			*args,
+	struct xfs_eofblocks	*eofb,
 	unsigned int		*lockflags)
 {
-	struct xfs_eofblocks	*eofb = args;
 	bool			wait;
 
 	wait = eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC);
@@ -1436,10 +1435,9 @@ xfs_prep_free_cowblocks(
 STATIC int
 xfs_inode_free_cowblocks(
 	struct xfs_inode	*ip,
-	void			*args,
+	struct xfs_eofblocks	*eofb,
 	unsigned int		*lockflags)
 {
-	struct xfs_eofblocks	*eofb = args;
 	bool			wait;
 	int			ret = 0;
 
@@ -1534,16 +1532,16 @@ xfs_blockgc_start(
 static int
 xfs_blockgc_scan_inode(
 	struct xfs_inode	*ip,
-	void			*args)
+	struct xfs_eofblocks	*eofb)
 {
 	unsigned int		lockflags = 0;
 	int			error;
 
-	error = xfs_inode_free_eofblocks(ip, args, &lockflags);
+	error = xfs_inode_free_eofblocks(ip, eofb, &lockflags);
 	if (error)
 		goto unlock;
 
-	error = xfs_inode_free_cowblocks(ip, args, &lockflags);
+	error = xfs_inode_free_cowblocks(ip, eofb, &lockflags);
 unlock:
 	if (lockflags)
 		xfs_iunlock(ip, lockflags);
-- 
2.30.1

Re: [PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes

[Darrick J. Wong]

On Wed, Mar 24, 2021 at 08:03:05AM +0100, Christoph Hellwig wrote:
> Using xfs_inode_walk in xfs_qm_dqrele_all_inodes is complete overkill,
> given that function simplify wants to iterate all live inodes known
> to the VFS.  Just iterate over the s_inodes list.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems fine to me...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_icache.c      |  2 +-
>  fs/xfs/xfs_icache.h      |  2 ++
>  fs/xfs/xfs_qm_syscalls.c | 56 +++++++++++++++++++++++++---------------
>  3 files changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 1d7720a0c068b7..c0d084e3526a9c 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -233,7 +233,7 @@ xfs_inode_clear_reclaim_tag(
>  	xfs_perag_clear_reclaim_tag(pag);
>  }
>  
> -static void
> +void
>  xfs_inew_wait(
>  	struct xfs_inode	*ip)
>  {
> diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> index d1fddb1524200b..d70d93c2bb1084 100644
> --- a/fs/xfs/xfs_icache.h
> +++ b/fs/xfs/xfs_icache.h
> @@ -78,4 +78,6 @@ int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
>  void xfs_blockgc_stop(struct xfs_mount *mp);
>  void xfs_blockgc_start(struct xfs_mount *mp);
>  
> +void xfs_inew_wait(struct xfs_inode *ip);
> +
>  #endif
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index ca1b57d291dc90..88dfd520ae91de 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -748,41 +748,27 @@ xfs_qm_scall_getquota_next(
>  	return error;
>  }
>  
> -STATIC int
> +static void
>  xfs_dqrele_inode(
>  	struct xfs_inode	*ip,
> -	void			*args)
> +	uint			flags)
>  {
> -	uint			*flags = args;
> -
> -	/* skip quota inodes */
> -	if (ip == ip->i_mount->m_quotainfo->qi_uquotaip ||
> -	    ip == ip->i_mount->m_quotainfo->qi_gquotaip ||
> -	    ip == ip->i_mount->m_quotainfo->qi_pquotaip) {
> -		ASSERT(ip->i_udquot == NULL);
> -		ASSERT(ip->i_gdquot == NULL);
> -		ASSERT(ip->i_pdquot == NULL);
> -		return 0;
> -	}
> -
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	if ((*flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
> +	if ((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 ((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 ((flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
>  		xfs_qm_dqrele(ip->i_pdquot);
>  		ip->i_pdquot = NULL;
>  	}
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -	return 0;
>  }
>  
> -
>  /*
>   * Go thru all the inodes in the file system, releasing their dquots.
>   *
> @@ -794,7 +780,35 @@ xfs_qm_dqrele_all_inodes(
>  	struct xfs_mount	*mp,
>  	uint			flags)
>  {
> +	struct super_block	*sb = mp->m_super;
> +	struct inode		*inode, *old_inode = NULL;
> +
>  	ASSERT(mp->m_quotainfo);
> -	xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
> -			&flags, XFS_ICI_NO_TAG);
> +
> +	spin_lock(&sb->s_inode_list_lock);
> +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> +		struct xfs_inode	*ip = XFS_I(inode);
> +
> +		if (XFS_FORCED_SHUTDOWN(mp))
> +			break;
> +		if (xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
> +			continue;
> +		if (xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
> +			continue;
> +		if (!igrab(inode))
> +			continue;
> +		spin_unlock(&sb->s_inode_list_lock);
> +
> +		iput(old_inode);
> +		old_inode = inode;
> +
> +		if (xfs_iflags_test(ip, XFS_INEW))
> +			xfs_inew_wait(ip);
> +		xfs_dqrele_inode(ip, flags);
> +
> +		spin_lock(&sb->s_inode_list_lock);
> +	}
> +	spin_unlock(&sb->s_inode_list_lock);
> +
> +	iput(old_inode);
>  }
> -- 
> 2.30.1
> 

Re: [PATCH 2/3] xfs: simplify the perage inode walk infrastructure

[Darrick J. Wong]

On Wed, Mar 24, 2021 at 08:03:06AM +0100, Christoph Hellwig wrote:
> Remove the generic xfs_inode_walk and just open code the only caller.

This is going in the wrong direction for me.  Maybe.

I was planning to combine the reclaim inode walk into this function, and
later on share it with inactivation.  This made for one switch-happy
iteration function, but it meant there was only one loop.

OFC maybe the point that you and/or Dave were trying to make is that I
should be doing the opposite, and combining the inactivation loop into
what is now the (badly misnamed) xfs_reclaim_inodes_ag?  And leave this
blockgc loop alone?

<shrug>

--D

> Note that this leads to a somewhat ugly forward delcaration for
> xfs_blockgc_scan_inode, but with other changes in that area pending
> it seems like the lesser evil.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_icache.c | 103 +++++++++++++-------------------------------
>  fs/xfs/xfs_icache.h |  11 -----
>  2 files changed, 30 insertions(+), 84 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index c0d084e3526a9c..7fdf77df66269c 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -728,11 +728,9 @@ xfs_icache_inode_is_allocated(
>   */
>  STATIC bool
>  xfs_inode_walk_ag_grab(
> -	struct xfs_inode	*ip,
> -	int			flags)
> +	struct xfs_inode	*ip)
>  {
>  	struct inode		*inode = VFS_I(ip);
> -	bool			newinos = !!(flags & XFS_INODE_WALK_INEW_WAIT);
>  
>  	ASSERT(rcu_read_lock_held());
>  
> @@ -742,8 +740,7 @@ xfs_inode_walk_ag_grab(
>  		goto out_unlock_noent;
>  
>  	/* avoid new or reclaimable inodes. Leave for reclaim code to flush */
> -	if ((!newinos && __xfs_iflags_test(ip, XFS_INEW)) ||
> -	    __xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
> +	if (__xfs_iflags_test(ip, XFS_INEW | XFS_IRECLAIMABLE | XFS_IRECLAIM))
>  		goto out_unlock_noent;
>  	spin_unlock(&ip->i_flags_lock);
>  
> @@ -763,17 +760,19 @@ xfs_inode_walk_ag_grab(
>  	return false;
>  }
>  
> +static int
> +xfs_blockgc_scan_inode(
> +	struct xfs_inode	*ip,
> +	void			*args);
> +
>  /*
>   * For a given per-AG structure @pag, grab, @execute, and rele all incore
>   * inodes with the given radix tree @tag.
>   */
>  STATIC int
> -xfs_inode_walk_ag(
> +xfs_blockgc_free_space_ag(
>  	struct xfs_perag	*pag,
> -	int			iter_flags,
> -	int			(*execute)(struct xfs_inode *ip, void *args),
> -	void			*args,
> -	int			tag)
> +	struct xfs_eofblocks	*eofb)
>  {
>  	struct xfs_mount	*mp = pag->pag_mount;
>  	uint32_t		first_index;
> @@ -793,17 +792,9 @@ xfs_inode_walk_ag(
>  		int		i;
>  
>  		rcu_read_lock();
> -
> -		if (tag == XFS_ICI_NO_TAG)
> -			nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
> -					(void **)batch, first_index,
> -					XFS_LOOKUP_BATCH);
> -		else
> -			nr_found = radix_tree_gang_lookup_tag(
> -					&pag->pag_ici_root,
> -					(void **) batch, first_index,
> -					XFS_LOOKUP_BATCH, tag);
> -
> +		nr_found = radix_tree_gang_lookup_tag(&pag->pag_ici_root,
> +				(void **) batch, first_index,
> +				XFS_LOOKUP_BATCH, XFS_ICI_BLOCKGC_TAG);
>  		if (!nr_found) {
>  			rcu_read_unlock();
>  			break;
> @@ -816,7 +807,7 @@ xfs_inode_walk_ag(
>  		for (i = 0; i < nr_found; i++) {
>  			struct xfs_inode *ip = batch[i];
>  
> -			if (done || !xfs_inode_walk_ag_grab(ip, iter_flags))
> +			if (done || !xfs_inode_walk_ag_grab(ip))
>  				batch[i] = NULL;
>  
>  			/*
> @@ -844,10 +835,7 @@ xfs_inode_walk_ag(
>  		for (i = 0; i < nr_found; i++) {
>  			if (!batch[i])
>  				continue;
> -			if ((iter_flags & XFS_INODE_WALK_INEW_WAIT) &&
> -			    xfs_iflags_test(batch[i], XFS_INEW))
> -				xfs_inew_wait(batch[i]);
> -			error = execute(batch[i], args);
> +			error = xfs_blockgc_scan_inode(batch[i], eofb);
>  			xfs_irele(batch[i]);
>  			if (error == -EAGAIN) {
>  				skipped++;
> @@ -872,49 +860,6 @@ xfs_inode_walk_ag(
>  	return last_error;
>  }
>  
> -/* Fetch the next (possibly tagged) per-AG structure. */
> -static inline struct xfs_perag *
> -xfs_inode_walk_get_perag(
> -	struct xfs_mount	*mp,
> -	xfs_agnumber_t		agno,
> -	int			tag)
> -{
> -	if (tag == XFS_ICI_NO_TAG)
> -		return xfs_perag_get(mp, agno);
> -	return xfs_perag_get_tag(mp, agno, tag);
> -}
> -
> -/*
> - * Call the @execute function on all incore inodes matching the radix tree
> - * @tag.
> - */
> -int
> -xfs_inode_walk(
> -	struct xfs_mount	*mp,
> -	int			iter_flags,
> -	int			(*execute)(struct xfs_inode *ip, void *args),
> -	void			*args,
> -	int			tag)
> -{
> -	struct xfs_perag	*pag;
> -	int			error = 0;
> -	int			last_error = 0;
> -	xfs_agnumber_t		ag;
> -
> -	ag = 0;
> -	while ((pag = xfs_inode_walk_get_perag(mp, ag, tag))) {
> -		ag = pag->pag_agno + 1;
> -		error = xfs_inode_walk_ag(pag, iter_flags, execute, args, tag);
> -		xfs_perag_put(pag);
> -		if (error) {
> -			last_error = error;
> -			if (error == -EFSCORRUPTED)
> -				break;
> -		}
> -	}
> -	return last_error;
> -}
> -
>  /*
>   * Grab the inode for reclaim exclusively.
>   *
> @@ -1617,8 +1562,7 @@ xfs_blockgc_worker(
>  
>  	if (!sb_start_write_trylock(mp->m_super))
>  		return;
> -	error = xfs_inode_walk_ag(pag, 0, xfs_blockgc_scan_inode, NULL,
> -			XFS_ICI_BLOCKGC_TAG);
> +	error = xfs_blockgc_free_space_ag(pag, NULL);
>  	if (error)
>  		xfs_info(mp, "AG %u preallocation gc worker failed, err=%d",
>  				pag->pag_agno, error);
> @@ -1634,10 +1578,23 @@ xfs_blockgc_free_space(
>  	struct xfs_mount	*mp,
>  	struct xfs_eofblocks	*eofb)
>  {
> +	struct xfs_perag	*pag;
> +	xfs_agnumber_t		ag = 0;
> +	int			error = 0, last_error = 0;
> +
>  	trace_xfs_blockgc_free_space(mp, eofb, _RET_IP_);
>  
> -	return xfs_inode_walk(mp, 0, xfs_blockgc_scan_inode, eofb,
> -			XFS_ICI_BLOCKGC_TAG);
> +	while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_BLOCKGC_TAG))) {
> +		ag = pag->pag_agno + 1;
> +		error = xfs_blockgc_free_space_ag(pag, eofb);
> +		xfs_perag_put(pag);
> +		if (error) {
> +			if (error == -EFSCORRUPTED)
> +				return error;
> +			last_error = error;
> +		}
> +	}
> +	return last_error;
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> index d70d93c2bb1084..da390097546c67 100644
> --- a/fs/xfs/xfs_icache.h
> +++ b/fs/xfs/xfs_icache.h
> @@ -20,8 +20,6 @@ struct xfs_eofblocks {
>  /*
>   * tags for inode radix tree
>   */
> -#define XFS_ICI_NO_TAG		(-1)	/* special flag for an untagged lookup
> -					   in xfs_inode_walk */
>  #define XFS_ICI_RECLAIM_TAG	0	/* inode is to be reclaimed */
>  /* Inode has speculative preallocations (posteof or cow) to clean. */
>  #define XFS_ICI_BLOCKGC_TAG	1
> @@ -34,11 +32,6 @@ struct xfs_eofblocks {
>  #define XFS_IGET_DONTCACHE	0x4
>  #define XFS_IGET_INCORE		0x8	/* don't read from disk or reinit */
>  
> -/*
> - * flags for AG inode iterator
> - */
> -#define XFS_INODE_WALK_INEW_WAIT	0x1	/* wait on new inodes */
> -
>  int xfs_iget(struct xfs_mount *mp, struct xfs_trans *tp, xfs_ino_t ino,
>  	     uint flags, uint lock_flags, xfs_inode_t **ipp);
>  
> @@ -68,10 +61,6 @@ void xfs_inode_clear_cowblocks_tag(struct xfs_inode *ip);
>  
>  void xfs_blockgc_worker(struct work_struct *work);
>  
> -int xfs_inode_walk(struct xfs_mount *mp, int iter_flags,
> -	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);
>  
> -- 
> 2.30.1
> 

Re: [PATCH 2/3] xfs: simplify the perage inode walk infrastructure

[Christoph Hellwig]

On Wed, Mar 24, 2021 at 10:57:35AM -0700, Darrick J. Wong wrote:
> On Wed, Mar 24, 2021 at 08:03:06AM +0100, Christoph Hellwig wrote:
> > Remove the generic xfs_inode_walk and just open code the only caller.
> 
> This is going in the wrong direction for me.  Maybe.
> 
> I was planning to combine the reclaim inode walk into this function, and
> later on share it with inactivation.  This made for one switch-happy
> iteration function, but it meant there was only one loop.

Ok, we can skip this for now if this gets in your way.  Or I can resend
a different patch 2 that just removes the no tag case for now.

> OFC maybe the point that you and/or Dave were trying to make is that I
> should be doing the opposite, and combining the inactivation loop into
> what is now the (badly misnamed) xfs_reclaim_inodes_ag?  And leave this
> blockgc loop alone?

That is my gut feeling.  No guarantee it actually works out, and given
that I've lead you down the wrong road a few times I already feel guily
ahead of time..

Re: [PATCH 3/3] xfs: pass struct xfs_eofblocks to the inode scan callback

[Darrick J. Wong]

On Wed, Mar 24, 2021 at 08:03:07AM +0100, Christoph Hellwig wrote:
> Pass the actual structure instead of a void pointer here now
> that none of the functions is used as a callback.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>  xfs_inode_free_cowblocks(

Assuming this ends up fitting in somewhere,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_icache.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 7fdf77df66269c..06286b5b613252 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -763,7 +763,7 @@ xfs_inode_walk_ag_grab(
>  static int
>  xfs_blockgc_scan_inode(
>  	struct xfs_inode	*ip,
> -	void			*args);
> +	struct xfs_eofblocks	*eofb);
>  
>  /*
>   * For a given per-AG structure @pag, grab, @execute, and rele all incore
> @@ -1228,10 +1228,9 @@ xfs_reclaim_worker(
>  STATIC int
>  xfs_inode_free_eofblocks(
>  	struct xfs_inode	*ip,
> -	void			*args,
> +	struct xfs_eofblocks	*eofb,
>  	unsigned int		*lockflags)
>  {
> -	struct xfs_eofblocks	*eofb = args;
>  	bool			wait;
>  
>  	wait = eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC);
> @@ -1436,10 +1435,9 @@ xfs_prep_free_cowblocks(
>  STATIC int
>  xfs_inode_free_cowblocks(
>  	struct xfs_inode	*ip,
> -	void			*args,
> +	struct xfs_eofblocks	*eofb,
>  	unsigned int		*lockflags)
>  {
> -	struct xfs_eofblocks	*eofb = args;
>  	bool			wait;
>  	int			ret = 0;
>  
> @@ -1534,16 +1532,16 @@ xfs_blockgc_start(
>  static int
>  xfs_blockgc_scan_inode(
>  	struct xfs_inode	*ip,
> -	void			*args)
> +	struct xfs_eofblocks	*eofb)
>  {
>  	unsigned int		lockflags = 0;
>  	int			error;
>  
> -	error = xfs_inode_free_eofblocks(ip, args, &lockflags);
> +	error = xfs_inode_free_eofblocks(ip, eofb, &lockflags);
>  	if (error)
>  		goto unlock;
>  
> -	error = xfs_inode_free_cowblocks(ip, args, &lockflags);
> +	error = xfs_inode_free_cowblocks(ip, eofb, &lockflags);
>  unlock:
>  	if (lockflags)
>  		xfs_iunlock(ip, lockflags);
> -- 
> 2.30.1
> 

Re: [PATCH 2/3] xfs: simplify the perage inode walk infrastructure

[Darrick J. Wong]

On Wed, Mar 24, 2021 at 06:59:37PM +0100, Christoph Hellwig wrote:
> On Wed, Mar 24, 2021 at 10:57:35AM -0700, Darrick J. Wong wrote:
> > On Wed, Mar 24, 2021 at 08:03:06AM +0100, Christoph Hellwig wrote:
> > > Remove the generic xfs_inode_walk and just open code the only caller.
> > 
> > This is going in the wrong direction for me.  Maybe.
> > 
> > I was planning to combine the reclaim inode walk into this function, and
> > later on share it with inactivation.  This made for one switch-happy
> > iteration function, but it meant there was only one loop.
> 
> Ok, we can skip this for now if this gets in your way.  Or I can resend
> a different patch 2 that just removes the no tag case for now.
> 
> > OFC maybe the point that you and/or Dave were trying to make is that I
> > should be doing the opposite, and combining the inactivation loop into
> > what is now the (badly misnamed) xfs_reclaim_inodes_ag?  And leave this
> > blockgc loop alone?
> 
> That is my gut feeling.  No guarantee it actually works out, and given
> that I've lead you down the wrong road a few times I already feel guily
> ahead of time..

Actually, collapsing all of the tag walkers into xfs_inode_walk was
pretty straightforward, and in the end I just borrowed bits and pieces
from patches 2 and 3 to make it happen and clean up the arguments.  The
net change is 55 lines deleted and ~1k less code (granted with all the
debugging and ubsan crud turned on).

--D