On Fri, Mar 26, 2021 at 06:48:09AM +0000, Christoph Hellwig wrote:

On Thu, Mar 25, 2021 at 05:21:46PM -0700, Darrick J. Wong wrote:

quoted

From: Darrick J. Wong <djwong@kernel.org>

In preparation for adding another incore inode tree tag, refactor the
code that sets and clears tags from the per-AG inode tree and the tree
of per-AG structures, and remove the open-coded versions used by the
blockgc code.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_icache.c |  127 ++++++++++++++++++++++++---------------------------
 fs/xfs/xfs_icache.h |    2 -
 fs/xfs/xfs_super.c  |    2 -
 fs/xfs/xfs_trace.h  |    6 +-
 4 files changed, 65 insertions(+), 72 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 2b25fe679b0e..4c124bc98f39 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c

@@ -29,6 +29,7 @@
 /* Forward declarations to reduce indirect calls */
 static int xfs_blockgc_scan_inode(struct xfs_inode *ip,
 		struct xfs_eofblocks *eofb);
+static inline void xfs_blockgc_queue(struct xfs_perag *pag);
 static bool xfs_reclaim_inode_grab(struct xfs_inode *ip);
 static void xfs_reclaim_inode(struct xfs_inode *ip, struct xfs_perag *pag);
 

@@ -163,46 +164,78 @@ xfs_reclaim_work_queue(
 	rcu_read_unlock();
 }
 
+/* Set a tag on both the AG incore inode tree and the AG radix tree. */
 static void
+xfs_perag_set_ici_tag(
+	struct xfs_perag	*pag,
+	xfs_agino_t		agino,
+	unsigned int		tag)

Looking at the callers - I think the logic to lookup the pag and set the
inode flag should also go in here.

I deliberately didn't do that here because of what happens in the
deferred inactivation patch.  After calling xfs_inactive, we have to
transition the inode from INACTIVATING to RECLAIMABLE (along with the
radix tree tags) without anybody being able to see intermediate state:

	/*
	 * Move the inode from the inactivation phase to the reclamation phase
	 * by clearing both inactivation inode state flags and marking the
	 * inode reclaimable.  Schedule background reclaim to run later.
	 */
	spin_lock(&pag->pag_ici_lock);
	spin_lock(&ip->i_flags_lock);

	ip->i_flags &= ~(XFS_NEED_INACTIVE | XFS_INACTIVATING);
	ip->i_flags |= XFS_IRECLAIMABLE;

	xfs_perag_clear_ici_tag(pag, agino, XFS_ICI_INODEGC_TAG);
	xfs_perag_set_ici_tag(pag, agino, XFS_ICI_RECLAIM_TAG);

	spin_unlock(&ip->i_flags_lock);
	spin_unlock(&pag->pag_ici_lock);

Which is why the xfs_perag_*_ici_tag callers are left in charge of
looking up the pag and taking locks as needed.

Currently only xfs_inode_destroy
nests i_Flags log inside the pag_ici_lock, but I don't see how that
would harm the xfs_blockgc_set_iflag case.

The other wart is that IEOFBLOCKS and ICOWBLOCKS share the same radix
tree tag, which complicates the clearing logic, and I thought it best
to let the callers deal with that.

I suspect the unlocked
check in xfs_blockgc_set_iflag would harm in the reclaim case either.

"wouldn't"?

quoted

 void
+xfs_inode_destroy(

I find this new name a little confusing.  What about
xfs_inode_mark_reclaimable?

Fixed.

But overall this new scheme looks nice to me.

Thanks!

--D