[PATCHSET 0/3] xfs: reduce indirect function calls in inode walk

[PATCHSET 0/3] xfs: reduce indirect function calls in inode walk

[Darrick J. Wong]

Hi all,

This short series reduces the number of indirect function calls when we
want to iterate the incore inode radix tree, and reduces the number of
arguments that must be passed to the walk function.

I made a few observations about incore inode radix tree walks -- the one
caller (blockgc) that cares about radix tree tags is internal to
xfs_icache.c, and there's a 1:1 mapping between that tag and the
iterator function.  Furthermore, the only other caller (quotaoff) is the
only caller to supply a nonzero flags argument, and it never specifies a
radix tree tag.

On those grounds, we can remove both the flags and tag arguments to
xfs_inode_walk; and for the internal caller, we can replace the indirect
call with direct calls to the blockgc functions.  This makes for less
ugly code and gives us a (negligible) performance bump.

This series is a prerequisite for the next patchset, since deferred
inode inactivation will add another inode radix tree tag and iterator
function to go with it.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=iwalk-remove-indirect-calls-5.13
---
 fs/xfs/xfs_icache.c      |   57 ++++++++++++++++++++++++++++++----------------
 fs/xfs/xfs_icache.h      |    9 ++-----
 fs/xfs/xfs_qm_syscalls.c |    3 +-
 3 files changed, 40 insertions(+), 29 deletions(-)

[PATCH 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}

[Darrick J. Wong]

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

It turns out that there is a 1:1 mapping between the execute and tag
parameters that are passed to xfs_inode_walk_ag:

	xfs_dqrele_inode => XFS_ICI_NO_TAG
	xfs_blockgc_scan_inode => XFS_ICI_BLOCKGC_TAG

The radix tree tags are an implementation detail of the inode cache,
which means that callers outside of xfs_icache.c have no business
passing in radix tree tags.  Since we're about to get rid of the
indirect calls in the BLOCKGC case, eliminate the extra argument in
favor of computing the ICI tag from the execute argument passed into the
function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_icache.c      |   27 ++++++++++++++++++---------
 fs/xfs/xfs_icache.h      |    2 +-
 fs/xfs/xfs_qm_syscalls.c |    3 +--
 3 files changed, 20 insertions(+), 12 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 7353c9fe05db..6924125a3c53 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -26,6 +26,9 @@
 
 #include <linux/iversion.h>
 
+/* Forward declarations to reduce indirect calls in xfs_inode_walk_ag */
+static int xfs_blockgc_scan_inode(struct xfs_inode *ip, void *args);
+
 /*
  * Allocate and initialise an xfs_inode.
  */
@@ -763,6 +766,14 @@ xfs_inode_walk_ag_grab(
 	return false;
 }
 
+static inline int
+inode_walk_fn_to_tag(int (*execute)(struct xfs_inode *ip, void *args))
+{
+	if (execute == xfs_blockgc_scan_inode)
+		return XFS_ICI_BLOCKGC_TAG;
+	return XFS_ICI_NO_TAG;
+}
+
 /*
  * For a given per-AG structure @pag, grab, @execute, and rele all incore
  * inodes with the given radix tree @tag.
@@ -772,14 +783,14 @@ xfs_inode_walk_ag(
 	struct xfs_perag	*pag,
 	int			iter_flags,
 	int			(*execute)(struct xfs_inode *ip, void *args),
-	void			*args,
-	int			tag)
+	void			*args)
 {
 	struct xfs_mount	*mp = pag->pag_mount;
 	uint32_t		first_index;
 	int			last_error = 0;
 	int			skipped;
 	bool			done;
+	int			tag = inode_walk_fn_to_tag(execute);
 	int			nr_found;
 
 restart:
@@ -893,18 +904,18 @@ xfs_inode_walk(
 	struct xfs_mount	*mp,
 	int			iter_flags,
 	int			(*execute)(struct xfs_inode *ip, void *args),
-	void			*args,
-	int			tag)
+	void			*args)
 {
 	struct xfs_perag	*pag;
 	int			error = 0;
 	int			last_error = 0;
+	int			tag = inode_walk_fn_to_tag(execute);
 	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);
+		error = xfs_inode_walk_ag(pag, iter_flags, execute, args);
 		xfs_perag_put(pag);
 		if (error) {
 			last_error = error;
@@ -1613,8 +1624,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_inode_walk_ag(pag, 0, xfs_blockgc_scan_inode, NULL);
 	if (error)
 		xfs_info(mp, "AG %u preallocation gc worker failed, err=%d",
 				pag->pag_agno, error);
@@ -1632,8 +1642,7 @@ xfs_blockgc_free_space(
 {
 	trace_xfs_blockgc_free_space(mp, eofb, _RET_IP_);
 
-	return xfs_inode_walk(mp, 0, xfs_blockgc_scan_inode, eofb,
-			XFS_ICI_BLOCKGC_TAG);
+	return xfs_inode_walk(mp, 0, xfs_blockgc_scan_inode, eofb);
 }
 
 /*
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index d1fddb152420..a20bb89e3a38 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -70,7 +70,7 @@ 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);
+	void *args);
 
 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 ca1b57d291dc..2f42ea8a09ab 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -795,6 +795,5 @@ xfs_qm_dqrele_all_inodes(
 	uint			flags)
 {
 	ASSERT(mp->m_quotainfo);
-	xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
-			&flags, XFS_ICI_NO_TAG);
+	xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode, &flags);
 }

[PATCH 2/3] xfs: reduce indirect calls in xfs_inode_walk{,_ag}

[Darrick J. Wong]

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

Since the previous patch requires the forward static declaration of
xfs_blockgc_scan_inode, we can eliminate an indirect call from the body
of xfs_inode_walk_ag for a (probably minor) decrease in overhead.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_icache.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 6924125a3c53..9198c7a7c3ca 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -858,8 +858,20 @@ xfs_inode_walk_ag(
 			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);
-			xfs_irele(batch[i]);
+			switch (tag) {
+			case XFS_ICI_BLOCKGC_TAG:
+				error = xfs_blockgc_scan_inode(batch[i], args);
+				xfs_irele(batch[i]);
+				break;
+			case XFS_ICI_NO_TAG:
+				error = execute(batch[i], args);
+				xfs_irele(batch[i]);
+				break;
+			default:
+				ASSERT(0);
+				error = -EFSCORRUPTED;
+				break;
+			}
 			if (error == -EAGAIN) {
 				skipped++;
 				continue;

[PATCH 3/3] xfs: remove iter_flags parameter from xfs_inode_walk_*

[Darrick J. Wong]

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

The sole user of the INEW_WAIT flag to xfs_inode_walk is a caller that
is external to the inode cache.  Since external callers have no business
messing with INEW inodes or inode radix tree tags, we can get rid of the
iter_flag entirely.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_icache.c      |   20 ++++++++------------
 fs/xfs/xfs_icache.h      |    7 +------
 fs/xfs/xfs_qm_syscalls.c |    2 +-
 3 files changed, 10 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 9198c7a7c3ca..563865140a99 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -732,10 +732,9 @@ xfs_icache_inode_is_allocated(
 STATIC bool
 xfs_inode_walk_ag_grab(
 	struct xfs_inode	*ip,
-	int			flags)
+	int			tag)
 {
 	struct inode		*inode = VFS_I(ip);
-	bool			newinos = !!(flags & XFS_INODE_WALK_INEW_WAIT);
 
 	ASSERT(rcu_read_lock_held());
 
@@ -745,7 +744,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)) ||
+	if ((tag != XFS_ICI_NO_TAG && __xfs_iflags_test(ip, XFS_INEW)) ||
 	    __xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
 		goto out_unlock_noent;
 	spin_unlock(&ip->i_flags_lock);
@@ -781,7 +780,6 @@ inode_walk_fn_to_tag(int (*execute)(struct xfs_inode *ip, void *args))
 STATIC int
 xfs_inode_walk_ag(
 	struct xfs_perag	*pag,
-	int			iter_flags,
 	int			(*execute)(struct xfs_inode *ip, void *args),
 	void			*args)
 {
@@ -827,7 +825,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, tag))
 				batch[i] = NULL;
 
 			/*
@@ -855,15 +853,14 @@ 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]);
 			switch (tag) {
 			case XFS_ICI_BLOCKGC_TAG:
 				error = xfs_blockgc_scan_inode(batch[i], args);
 				xfs_irele(batch[i]);
 				break;
 			case XFS_ICI_NO_TAG:
+				if (xfs_iflags_test(batch[i], XFS_INEW))
+					xfs_inew_wait(batch[i]);
 				error = execute(batch[i], args);
 				xfs_irele(batch[i]);
 				break;
@@ -914,7 +911,6 @@ xfs_inode_walk_get_perag(
 int
 xfs_inode_walk(
 	struct xfs_mount	*mp,
-	int			iter_flags,
 	int			(*execute)(struct xfs_inode *ip, void *args),
 	void			*args)
 {
@@ -927,7 +923,7 @@ xfs_inode_walk(
 	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);
+		error = xfs_inode_walk_ag(pag, execute, args);
 		xfs_perag_put(pag);
 		if (error) {
 			last_error = error;
@@ -1636,7 +1632,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);
+	error = xfs_inode_walk_ag(pag, xfs_blockgc_scan_inode, NULL);
 	if (error)
 		xfs_info(mp, "AG %u preallocation gc worker failed, err=%d",
 				pag->pag_agno, error);
@@ -1654,7 +1650,7 @@ xfs_blockgc_free_space(
 {
 	trace_xfs_blockgc_free_space(mp, eofb, _RET_IP_);
 
-	return xfs_inode_walk(mp, 0, xfs_blockgc_scan_inode, eofb);
+	return xfs_inode_walk(mp, xfs_blockgc_scan_inode, eofb);
 }
 
 /*
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index a20bb89e3a38..04e59b775432 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -34,11 +34,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,7 +63,7 @@ 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 xfs_inode_walk(struct xfs_mount *mp,
 	int (*execute)(struct xfs_inode *ip, void *args),
 	void *args);
 
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 2f42ea8a09ab..dad4d3fc3df3 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -795,5 +795,5 @@ xfs_qm_dqrele_all_inodes(
 	uint			flags)
 {
 	ASSERT(mp->m_quotainfo);
-	xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode, &flags);
+	xfs_inode_walk(mp, xfs_dqrele_inode, &flags);
 }

Re: [PATCH 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}

[Christoph Hellwig]

On Thu, Mar 18, 2021 at 03:33:45PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> It turns out that there is a 1:1 mapping between the execute and tag
> parameters that are passed to xfs_inode_walk_ag:
> 
> 	xfs_dqrele_inode => XFS_ICI_NO_TAG
> 	xfs_blockgc_scan_inode => XFS_ICI_BLOCKGC_TAG
> 
> The radix tree tags are an implementation detail of the inode cache,
> which means that callers outside of xfs_icache.c have no business
> passing in radix tree tags.  Since we're about to get rid of the
> indirect calls in the BLOCKGC case, eliminate the extra argument in
> favor of computing the ICI tag from the execute argument passed into the
> function.

This seems backwards to me.  I'd rather deduce the function from the
talk, which seems like a more sensible pattern.

That being said, the quota inode walk is a little different in that it
doesn't use any tags, so switching it to a plain list_for_each_entry_safe
on sb->s_inodes would seems more sensible, something like this untested
patch:

---
From 9ae07b6bf8c6b1337a627c8f0ad619c56511b343 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 19 Mar 2021 07:16:31 +0100
Subject: xfs: use s_inodes in xfs_qm_dqrele_all_inodes

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_qm_syscalls.c | 50 +++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 11f1e2fbf22f44..4e33919ed04b56 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,29 @@ 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_is_quota_inode(&mp->m_sb, ip->i_ino))
+			continue;
+		if (!igrab(inode))
+			continue;
+		spin_unlock(&sb->s_inode_list_lock);
+
+		iput(old_inode);
+		old_inode = inode;
+
+		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 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}

[Christoph Hellwig]

On Fri, Mar 19, 2021 at 06:25:01AM +0000, Christoph Hellwig wrote:
> On Thu, Mar 18, 2021 at 03:33:45PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > It turns out that there is a 1:1 mapping between the execute and tag
> > parameters that are passed to xfs_inode_walk_ag:
> > 
> > 	xfs_dqrele_inode => XFS_ICI_NO_TAG
> > 	xfs_blockgc_scan_inode => XFS_ICI_BLOCKGC_TAG
> > 
> > The radix tree tags are an implementation detail of the inode cache,
> > which means that callers outside of xfs_icache.c have no business
> > passing in radix tree tags.  Since we're about to get rid of the
> > indirect calls in the BLOCKGC case, eliminate the extra argument in
> > favor of computing the ICI tag from the execute argument passed into the
> > function.
> 
> This seems backwards to me.  I'd rather deduce the function from the
> talk, which seems like a more sensible pattern.
> 
> That being said, the quota inode walk is a little different in that it
> doesn't use any tags, so switching it to a plain list_for_each_entry_safe
> on sb->s_inodes would seems more sensible, something like this untested
> patch:

Something like this is probably going to work better (maybe split into
two patches).  I'm going to kick off some testing on it:

---
From 9ae07b6bf8c6b1337a627c8f0ad619c56511b343 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 19 Mar 2021 07:16:31 +0100
Subject: xfs: use s_inodes in xfs_qm_dqrele_all_inodes

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_qm_syscalls.c | 50 +++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 11f1e2fbf22f44..4e33919ed04b56 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,29 @@ 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_is_quota_inode(&mp->m_sb, ip->i_ino))
+			continue;
+		if (!igrab(inode))
+			continue;
+		spin_unlock(&sb->s_inode_list_lock);
+
+		iput(old_inode);
+		old_inode = inode;
+
+		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 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}

[Darrick J. Wong]

On Fri, Mar 19, 2021 at 06:25:01AM +0000, Christoph Hellwig wrote:
> On Thu, Mar 18, 2021 at 03:33:45PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > It turns out that there is a 1:1 mapping between the execute and tag
> > parameters that are passed to xfs_inode_walk_ag:
> > 
> > 	xfs_dqrele_inode => XFS_ICI_NO_TAG
> > 	xfs_blockgc_scan_inode => XFS_ICI_BLOCKGC_TAG
> > 
> > The radix tree tags are an implementation detail of the inode cache,
> > which means that callers outside of xfs_icache.c have no business
> > passing in radix tree tags.  Since we're about to get rid of the
> > indirect calls in the BLOCKGC case, eliminate the extra argument in
> > favor of computing the ICI tag from the execute argument passed into the
> > function.
> 
> This seems backwards to me.  I'd rather deduce the function from the
> talk, which seems like a more sensible pattern.

Fair enough.

> That being said, the quota inode walk is a little different in that it
> doesn't use any tags, so switching it to a plain list_for_each_entry_safe
> on sb->s_inodes would seems more sensible, something like this untested
> patch:

Hmm, well, I look forward to hearing the results of your testing. :)

I /think/ this will work, since quotaoff doesn't touch inodes that can't
be igrabbed (i.e. their VFS state is gone), so walking sb->s_inodes
/should/ be the same.  The only thing I'm not sure about is that the vfs
removes the inode from the sb list before clear_inode sets I_FREEING
(to prevent further igrab), which /could/ introduce a behavioral change?
Though I think even if quotaoff ends up racing with evict_inodes, the
xfs_fs_destroy_inode call will inactivate the inode and drop the dquots
(before the next patchset) or queue the inode for inactivation and
detach the dquots.

One thing that occurs to me -- do the quota and rt metadata inodes end
up on the sb inode list?  The rt metadata inodes definitely contribute
to the root dquot's inode counts.

--D

> ---
> From 9ae07b6bf8c6b1337a627c8f0ad619c56511b343 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 19 Mar 2021 07:16:31 +0100
> Subject: xfs: use s_inodes in xfs_qm_dqrele_all_inodes
> 
> 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_qm_syscalls.c | 50 +++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 11f1e2fbf22f44..4e33919ed04b56 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,29 @@ 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_is_quota_inode(&mp->m_sb, ip->i_ino))
> +			continue;
> +		if (!igrab(inode))
> +			continue;
> +		spin_unlock(&sb->s_inode_list_lock);
> +
> +		iput(old_inode);
> +		old_inode = inode;
> +
> +		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 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}

[Christoph Hellwig]

On Fri, Mar 19, 2021 at 09:43:54AM -0700, Darrick J. Wong wrote:
> One thing that occurs to me -- do the quota and rt metadata inodes end
> up on the sb inode list?  The rt metadata inodes definitely contribute
> to the root dquot's inode counts.

Yes, everything going through xfs_iget ends up on ->s_inodes.  But they
a) don't have dquots and b) we specifically skip the quota inodes (but
not the RT ones) in the existing code already.

Re: [PATCH 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}

[Christoph Hellwig]

On Fri, Mar 19, 2021 at 09:43:54AM -0700, Darrick J. Wong wrote:
> > That being said, the quota inode walk is a little different in that it
> > doesn't use any tags, so switching it to a plain list_for_each_entry_safe
> > on sb->s_inodes would seems more sensible, something like this untested
> > patch:
> 
> Hmm, well, I look forward to hearing the results of your testing. :)

I've thrown a whole lot ot of load onto it and it seems to survive just
fine.

> I /think/ this will work, since quotaoff doesn't touch inodes that can't
> be igrabbed (i.e. their VFS state is gone), so walking sb->s_inodes
> /should/ be the same.  The only thing I'm not sure about is that the vfs
> removes the inode from the sb list before clear_inode sets I_FREEING
> (to prevent further igrab), which /could/ introduce a behavioral change?

inode_sb_list_delinode_sb_list_del( is called from evict(), which
has a BUG_ON to assert I_FREEING is already set.