[PATCHSET v2 0/5] xfs: clean up quotaoff inode walks

[PATCHSET v2 0/5] xfs: clean up quotaoff inode walks

[Darrick J. Wong]

Hi all,

This series cleans up the inode walk that quotaoff does to detach dquots
from the current set of incore inodes.  Once we've cleared away all the
baggage that was implemented for "no tag" walks (since this is the only
caller that doesn't use tags), we can speed up quotaoff by making it
detach more aggressively.  This gets us into shape for further inode
walk cleanups in the next series.

Christoph suggested last cycle that we 'simply' change quotaoff not to
allow deactivating quota entirely, but as these cleanups are to enable
one major change in behavior (deferred inode inactivation) I do not want
to add a second behavior change (quotaoff) as a dependency.

To be blunt: Additional cleanups are not in scope for this series.

v2: rebase to 5.13-rc4

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=quotaoff-cleanups-5.14
---
 fs/xfs/xfs_icache.c      |  119 ++++++++++++++++++++++++++++++++++++++++++++--
 fs/xfs/xfs_icache.h      |   17 +++++--
 fs/xfs/xfs_inode.c       |   22 ++++-----
 fs/xfs/xfs_qm.h          |    1 
 fs/xfs/xfs_qm_syscalls.c |   54 +--------------------
 fs/xfs/xfs_super.c       |   32 +++++++++++-
 6 files changed, 167 insertions(+), 78 deletions(-)

[PATCH 1/5] xfs: move the quotaoff dqrele inode walk into xfs_icache.c

[Darrick J. Wong]

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

The only external caller of xfs_inode_walk* happens in quotaoff, when we
want to walk all the incore inodes to detach the dquots.  Move this code
to xfs_icache.c so that we can hide xfs_inode_walk as the starting step
in more cleanups of inode walks.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_icache.c      |   52 +++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_icache.h      |   13 +++++++++--
 fs/xfs/xfs_qm.h          |    1 -
 fs/xfs/xfs_qm_syscalls.c |   54 ++--------------------------------------------
 4 files changed, 63 insertions(+), 57 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 3c81daca0e9a..32c17e84efa0 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -890,7 +890,7 @@ xfs_inode_walk_get_perag(
  * Call the @execute function on all incore inodes matching the radix tree
  * @tag.
  */
-int
+static int
 xfs_inode_walk(
 	struct xfs_mount	*mp,
 	int			iter_flags,
@@ -917,6 +917,56 @@ xfs_inode_walk(
 	return last_error;
 }
 
+/* Drop this inode's dquots. */
+static int
+xfs_dqrele_inode(
+	struct xfs_inode	*ip,
+	void			*priv)
+{
+	struct xfs_eofblocks	*eofb = priv;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	if (eofb->eof_flags & XFS_EOFB_DROP_UDQUOT) {
+		xfs_qm_dqrele(ip->i_udquot);
+		ip->i_udquot = NULL;
+	}
+	if (eofb->eof_flags & XFS_EOFB_DROP_GDQUOT) {
+		xfs_qm_dqrele(ip->i_gdquot);
+		ip->i_gdquot = NULL;
+	}
+	if (eofb->eof_flags & XFS_EOFB_DROP_PDQUOT) {
+		xfs_qm_dqrele(ip->i_pdquot);
+		ip->i_pdquot = NULL;
+	}
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return 0;
+}
+
+/*
+ * Detach all dquots from incore inodes if we can.  The caller must already
+ * have dropped the relevant XFS_[UGP]QUOTA_ACTIVE flags so that dquots will
+ * not get reattached.
+ */
+int
+xfs_dqrele_all_inodes(
+	struct xfs_mount	*mp,
+	unsigned int		qflags)
+{
+	struct xfs_eofblocks	eofb = { .eof_flags = 0 };
+
+	BUILD_BUG_ON(XFS_EOFB_PRIVATE_FLAGS & XFS_EOF_FLAGS_VALID);
+
+	if (qflags & XFS_UQUOTA_ACCT)
+		eofb.eof_flags |= XFS_EOFB_DROP_UDQUOT;
+	if (qflags & XFS_GQUOTA_ACCT)
+		eofb.eof_flags |= XFS_EOFB_DROP_GDQUOT;
+	if (qflags & XFS_PQUOTA_ACCT)
+		eofb.eof_flags |= XFS_EOFB_DROP_PDQUOT;
+
+	return xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
+			&eofb, XFS_ICI_NO_TAG);
+}
+
 /*
  * Grab the inode for reclaim exclusively.
  *
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index d1fddb152420..77029e92ba4c 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -17,6 +17,15 @@ struct xfs_eofblocks {
 	__u64		eof_min_file_size;
 };
 
+/* Special eof_flags for dropping dquots. */
+#define XFS_EOFB_DROP_UDQUOT	(1U << 31)
+#define XFS_EOFB_DROP_GDQUOT	(1U << 30)
+#define XFS_EOFB_DROP_PDQUOT	(1U << 29)
+
+#define XFS_EOFB_PRIVATE_FLAGS	(XFS_EOFB_DROP_UDQUOT | \
+				 XFS_EOFB_DROP_GDQUOT | \
+				 XFS_EOFB_DROP_PDQUOT)
+
 /*
  * tags for inode radix tree
  */
@@ -68,9 +77,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 (*execute)(struct xfs_inode *ip, void *args),
-	void *args, int tag);
+int xfs_dqrele_all_inodes(struct xfs_mount *mp, unsigned int qflags);
 
 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.h b/fs/xfs/xfs_qm.h
index e3dabab44097..ebbb484c49dc 100644
--- a/fs/xfs/xfs_qm.h
+++ b/fs/xfs/xfs_qm.h
@@ -142,7 +142,6 @@ extern void		xfs_qm_destroy_quotainfo(struct xfs_mount *);
 
 /* dquot stuff */
 extern void		xfs_qm_dqpurge_all(struct xfs_mount *, uint);
-extern void		xfs_qm_dqrele_all_inodes(struct xfs_mount *, uint);
 
 /* quota ops */
 extern int		xfs_qm_scall_trunc_qfiles(struct xfs_mount *, uint);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 11f1e2fbf22f..13a56e1ea15c 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -201,7 +201,8 @@ xfs_qm_scall_quotaoff(
 	 * depend on the quota inodes (and other things) being valid as long as
 	 * we keep the lock(s).
 	 */
-	xfs_qm_dqrele_all_inodes(mp, flags);
+	error = xfs_dqrele_all_inodes(mp, flags);
+	ASSERT(!error);
 
 	/*
 	 * Next we make the changes in the quota flag in the mount struct.
@@ -747,54 +748,3 @@ xfs_qm_scall_getquota_next(
 	xfs_qm_dqput(dqp);
 	return error;
 }
-
-STATIC int
-xfs_dqrele_inode(
-	struct xfs_inode	*ip,
-	void			*args)
-{
-	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) {
-		xfs_qm_dqrele(ip->i_udquot);
-		ip->i_udquot = NULL;
-	}
-	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) {
-		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.
- *
- * Note that the mount structure gets modified to indicate that quotas are off
- * AFTER this, in the case of quotaoff.
- */
-void
-xfs_qm_dqrele_all_inodes(
-	struct xfs_mount	*mp,
-	uint			flags)
-{
-	ASSERT(mp->m_quotainfo);
-	xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
-			&flags, XFS_ICI_NO_TAG);
-}

[PATCH 2/5] xfs: detach inode dquots at the end of inactivation

[Darrick J. Wong]

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

Once we're done with inactivating an inode, we're finished updating
metadata for that inode.  This means that we can detach the dquots at
the end and not have to wait for reclaim to do it for us.

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


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 32c17e84efa0..34b8b5fbd60d 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1080,7 +1080,7 @@ xfs_reclaim_inode(
 	 * unlocked after the lookup before we go ahead and free it.
 	 */
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_qm_dqdetach(ip);
+	ASSERT(!ip->i_udquot && !ip->i_gdquot && !ip->i_pdquot);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	ASSERT(xfs_inode_clean(ip));
 
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 1e28997c6f78..718ab0c17b9d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1713,7 +1713,7 @@ xfs_inactive(
 	 */
 	if (VFS_I(ip)->i_mode == 0) {
 		ASSERT(ip->i_df.if_broot_bytes == 0);
-		return;
+		goto out;
 	}
 
 	mp = ip->i_mount;
@@ -1721,11 +1721,11 @@ xfs_inactive(
 
 	/* If this is a read-only mount, don't do this (would generate I/O) */
 	if (mp->m_flags & XFS_MOUNT_RDONLY)
-		return;
+		goto out;
 
 	/* Metadata inodes require explicit resource cleanup. */
 	if (xfs_is_metadata_inode(ip))
-		return;
+		goto out;
 
 	/* Try to clean out the cow blocks if there are any. */
 	if (xfs_inode_has_cow_data(ip))
@@ -1744,7 +1744,7 @@ xfs_inactive(
 		if (xfs_can_free_eofblocks(ip, true))
 			xfs_free_eofblocks(ip);
 
-		return;
+		goto out;
 	}
 
 	if (S_ISREG(VFS_I(ip)->i_mode) &&
@@ -1754,14 +1754,14 @@ xfs_inactive(
 
 	error = xfs_qm_dqattach(ip);
 	if (error)
-		return;
+		goto out;
 
 	if (S_ISLNK(VFS_I(ip)->i_mode))
 		error = xfs_inactive_symlink(ip);
 	else if (truncate)
 		error = xfs_inactive_truncate(ip);
 	if (error)
-		return;
+		goto out;
 
 	/*
 	 * If there are attributes associated with the file then blow them away
@@ -1771,7 +1771,7 @@ xfs_inactive(
 	if (XFS_IFORK_Q(ip)) {
 		error = xfs_attr_inactive(ip);
 		if (error)
-			return;
+			goto out;
 	}
 
 	ASSERT(!ip->i_afp);
@@ -1780,12 +1780,12 @@ xfs_inactive(
 	/*
 	 * Free the inode.
 	 */
-	error = xfs_inactive_ifree(ip);
-	if (error)
-		return;
+	xfs_inactive_ifree(ip);
 
+out:
 	/*
-	 * Release the dquots held by inode, if any.
+	 * We're done making metadata updates for this inode, so we can release
+	 * the attached dquots.
 	 */
 	xfs_qm_dqdetach(ip);
 }

[PATCH 3/5] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab

[Darrick J. Wong]

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

Disentangle the dqrele_all inode grab code from the "generic" inode walk
grabbing code, and and use the opportunity to document why the dqrele
grab function does what it does.

Since dqrele_all is the only user of XFS_ICI_NO_TAG, rename it to
something more specific for what we're doing.

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


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 34b8b5fbd60d..5501318b5db0 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -26,6 +26,8 @@
 
 #include <linux/iversion.h>
 
+static bool xfs_dqrele_inode_grab(struct xfs_inode *ip);
+
 /*
  * Allocate and initialise an xfs_inode.
  */
@@ -765,6 +767,22 @@ xfs_inode_walk_ag_grab(
 	return false;
 }
 
+static inline bool
+xfs_grabbed_for_walk(
+	int			tag,
+	struct xfs_inode	*ip,
+	int			iter_flags)
+{
+	switch (tag) {
+	case XFS_ICI_BLOCKGC_TAG:
+		return xfs_inode_walk_ag_grab(ip, iter_flags);
+	case XFS_ICI_DQRELE_NONTAG:
+		return xfs_dqrele_inode_grab(ip);
+	default:
+		return false;
+	}
+}
+
 /*
  * For a given per-AG structure @pag, grab, @execute, and rele all incore
  * inodes with the given radix tree @tag.
@@ -796,7 +814,7 @@ xfs_inode_walk_ag(
 
 		rcu_read_lock();
 
-		if (tag == XFS_ICI_NO_TAG)
+		if (tag == XFS_ICI_DQRELE_NONTAG)
 			nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
 					(void **)batch, first_index,
 					XFS_LOOKUP_BATCH);
@@ -818,7 +836,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_grabbed_for_walk(tag, ip, iter_flags))
 				batch[i] = NULL;
 
 			/*
@@ -881,7 +899,7 @@ xfs_inode_walk_get_perag(
 	xfs_agnumber_t		agno,
 	int			tag)
 {
-	if (tag == XFS_ICI_NO_TAG)
+	if (tag == XFS_ICI_DQRELE_NONTAG)
 		return xfs_perag_get(mp, agno);
 	return xfs_perag_get_tag(mp, agno, tag);
 }
@@ -917,6 +935,44 @@ xfs_inode_walk(
 	return last_error;
 }
 
+/* Decide if we want to grab this inode to drop its dquots. */
+static bool
+xfs_dqrele_inode_grab(
+	struct xfs_inode	*ip)
+{
+	bool			ret = false;
+
+	ASSERT(rcu_read_lock_held());
+
+	/* Check for stale RCU freed inode */
+	spin_lock(&ip->i_flags_lock);
+	if (!ip->i_ino)
+		goto out_unlock;
+
+	/*
+	 * Skip inodes that are anywhere in the reclaim machinery because we
+	 * drop dquots before tagging an inode for reclamation.
+	 */
+	if (ip->i_flags & (XFS_IRECLAIM | XFS_IRECLAIMABLE))
+		goto out_unlock;
+
+	/*
+	 * The inode looks alive; try to grab a VFS reference so that it won't
+	 * get destroyed.  If we got the reference, return true to say that
+	 * we grabbed the inode.
+	 *
+	 * If we can't get the reference, then we know the inode had its VFS
+	 * state torn down and hasn't yet entered the reclaim machinery.  Since
+	 * we also know that dquots are detached from an inode before it enters
+	 * reclaim, we can skip the inode.
+	 */
+	ret = igrab(VFS_I(ip)) != NULL;
+
+out_unlock:
+	spin_unlock(&ip->i_flags_lock);
+	return ret;
+}
+
 /* Drop this inode's dquots. */
 static int
 xfs_dqrele_inode(
@@ -964,7 +1020,7 @@ xfs_dqrele_all_inodes(
 		eofb.eof_flags |= XFS_EOFB_DROP_PDQUOT;
 
 	return xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
-			&eofb, XFS_ICI_NO_TAG);
+			&eofb, XFS_ICI_DQRELE_NONTAG);
 }
 
 /*
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 77029e92ba4c..fcfcdad7f977 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -29,8 +29,8 @@ 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_DQRELE_NONTAG	(-1)	/* quotaoff dqdetach inode walk uses
+					   untagged lookups */
 #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

[PATCH 4/5] xfs: drop inactive dquots before inactivating inodes

[Darrick J. Wong]

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

During quotaoff, the incore inode scan to detach dquots from inodes
won't touch inodes that have lost their VFS state but haven't yet been
queued for reclaim.  This isn't strictly a problem because we drop the
dquots at the end of inactivation, but if we detect this situation
before starting inactivation, we can drop the inactive dquots early to
avoid delaying quotaoff further.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_super.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index a2dab05332ac..79f1cd1a0221 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -637,22 +637,46 @@ xfs_fs_destroy_inode(
 	struct inode		*inode)
 {
 	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
 
 	trace_xfs_destroy_inode(ip);
 
 	ASSERT(!rwsem_is_locked(&inode->i_rwsem));
-	XFS_STATS_INC(ip->i_mount, vn_rele);
-	XFS_STATS_INC(ip->i_mount, vn_remove);
+	XFS_STATS_INC(mp, vn_rele);
+	XFS_STATS_INC(mp, vn_remove);
+
+	/*
+	 * If a quota type is turned off but we still have a dquot attached to
+	 * the inode, detach it before processing this inode to avoid delaying
+	 * quotaoff for longer than is necessary.
+	 *
+	 * The inode has no VFS state and hasn't been tagged for any kind of
+	 * reclamation, which means that iget, quotaoff, blockgc, and reclaim
+	 * will not touch it.  It is therefore safe to do this locklessly
+	 * because we have the only reference here.
+	 */
+	if (!XFS_IS_UQUOTA_ON(mp)) {
+		xfs_qm_dqrele(ip->i_udquot);
+		ip->i_udquot = NULL;
+	}
+	if (!XFS_IS_GQUOTA_ON(mp)) {
+		xfs_qm_dqrele(ip->i_gdquot);
+		ip->i_gdquot = NULL;
+	}
+	if (!XFS_IS_PQUOTA_ON(mp)) {
+		xfs_qm_dqrele(ip->i_pdquot);
+		ip->i_pdquot = NULL;
+	}
 
 	xfs_inactive(ip);
 
-	if (!XFS_FORCED_SHUTDOWN(ip->i_mount) && ip->i_delayed_blks) {
+	if (!XFS_FORCED_SHUTDOWN(mp) && ip->i_delayed_blks) {
 		xfs_check_delalloc(ip, XFS_DATA_FORK);
 		xfs_check_delalloc(ip, XFS_COW_FORK);
 		ASSERT(0);
 	}
 
-	XFS_STATS_INC(ip->i_mount, vn_reclaim);
+	XFS_STATS_INC(mp, vn_reclaim);
 
 	/*
 	 * We should never get here with one of the reclaim flags already set.

[PATCH 5/5] xfs: move xfs_inew_wait call into xfs_dqrele_inode

[Darrick J. Wong]

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

Move the INEW wait into xfs_dqrele_inode so that we can drop the
iter_flags parameter in the next patch.

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


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 5501318b5db0..859ab1279d8d 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -981,6 +981,9 @@ xfs_dqrele_inode(
 {
 	struct xfs_eofblocks	*eofb = priv;
 
+	if (xfs_iflags_test(ip, XFS_INEW))
+		xfs_inew_wait(ip);
+
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	if (eofb->eof_flags & XFS_EOFB_DROP_UDQUOT) {
 		xfs_qm_dqrele(ip->i_udquot);
@@ -1019,8 +1022,8 @@ xfs_dqrele_all_inodes(
 	if (qflags & XFS_PQUOTA_ACCT)
 		eofb.eof_flags |= XFS_EOFB_DROP_PDQUOT;
 
-	return xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
-			&eofb, XFS_ICI_DQRELE_NONTAG);
+	return xfs_inode_walk(mp, 0, xfs_dqrele_inode, &eofb,
+			XFS_ICI_DQRELE_NONTAG);
 }
 
 /*

Re: [PATCH 2/5] xfs: detach inode dquots at the end of inactivation

[Dave Chinner]

On Mon, May 31, 2021 at 03:41:02PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Once we're done with inactivating an inode, we're finished updating
> metadata for that inode.  This means that we can detach the dquots at
> the end and not have to wait for reclaim to do it for us.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/5] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab

[Dave Chinner]

On Mon, May 31, 2021 at 03:41:07PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Disentangle the dqrele_all inode grab code from the "generic" inode walk
> grabbing code, and and use the opportunity to document why the dqrele
> grab function does what it does.
> 
> Since dqrele_all is the only user of XFS_ICI_NO_TAG, rename it to
> something more specific for what we're doing.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_icache.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  fs/xfs/xfs_icache.h |    4 ++-
>  2 files changed, 62 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 34b8b5fbd60d..5501318b5db0 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -26,6 +26,8 @@
>  
>  #include <linux/iversion.h>
>  
> +static bool xfs_dqrele_inode_grab(struct xfs_inode *ip);
> +

Just mov the function higher up in the file rather than add forward
declarations....

>  /*
>   * Allocate and initialise an xfs_inode.
>   */
> @@ -765,6 +767,22 @@ xfs_inode_walk_ag_grab(
>  	return false;
>  }
>  
> +static inline bool
> +xfs_grabbed_for_walk(
> +	int			tag,
> +	struct xfs_inode	*ip,
> +	int			iter_flags)
> +{
> +	switch (tag) {
> +	case XFS_ICI_BLOCKGC_TAG:
> +		return xfs_inode_walk_ag_grab(ip, iter_flags);
> +	case XFS_ICI_DQRELE_NONTAG:
> +		return xfs_dqrele_inode_grab(ip);
> +	default:
> +		return false;
> +	}
> +}

Not really a fan of this XFS_ICI_DQRELE_NONTAG rename. It kinda
smears caller context across the walk API. We really have two
different things here - we want a tagless lookup, and we want a
dquot specific grab function.

This API change just means we're going to have to rename the "no
tag" lookup yet again when we need some other non tag-based lookup.

And I think this is redundant, because....

> +/* Decide if we want to grab this inode to drop its dquots. */
> +static bool
> +xfs_dqrele_inode_grab(
> +	struct xfs_inode	*ip)
> +{
> +	bool			ret = false;
> +
> +	ASSERT(rcu_read_lock_held());
> +
> +	/* Check for stale RCU freed inode */
> +	spin_lock(&ip->i_flags_lock);
> +	if (!ip->i_ino)
> +		goto out_unlock;
> +
> +	/*
> +	 * Skip inodes that are anywhere in the reclaim machinery because we
> +	 * drop dquots before tagging an inode for reclamation.
> +	 */
> +	if (ip->i_flags & (XFS_IRECLAIM | XFS_IRECLAIMABLE))
> +		goto out_unlock;
> +
> +	/*
> +	 * The inode looks alive; try to grab a VFS reference so that it won't
> +	 * get destroyed.  If we got the reference, return true to say that
> +	 * we grabbed the inode.
> +	 *
> +	 * If we can't get the reference, then we know the inode had its VFS
> +	 * state torn down and hasn't yet entered the reclaim machinery.  Since
> +	 * we also know that dquots are detached from an inode before it enters
> +	 * reclaim, we can skip the inode.
> +	 */
> +	ret = igrab(VFS_I(ip)) != NULL;
> +
> +out_unlock:
> +	spin_unlock(&ip->i_flags_lock);
> +	return ret;
> +}

This is basically just duplication of xfs_inode_walk_ag_grab()
without the XFS_INODE_WALK_INEW_WAIT check in it. At this point I
just don't see a reason for this function or the
XFS_ICI_DQRELE_NONTAG rename just to use this grab function...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/5] xfs: drop inactive dquots before inactivating inodes

[Dave Chinner]

On Mon, May 31, 2021 at 03:41:13PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> During quotaoff, the incore inode scan to detach dquots from inodes
> won't touch inodes that have lost their VFS state but haven't yet been
> queued for reclaim.  This isn't strictly a problem because we drop the
> dquots at the end of inactivation, but if we detect this situation
> before starting inactivation, we can drop the inactive dquots early to
> avoid delaying quotaoff further.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_super.c |   32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index a2dab05332ac..79f1cd1a0221 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -637,22 +637,46 @@ xfs_fs_destroy_inode(
>  	struct inode		*inode)
>  {
>  	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_mount	*mp = ip->i_mount;
>  
>  	trace_xfs_destroy_inode(ip);
>  
>  	ASSERT(!rwsem_is_locked(&inode->i_rwsem));
> -	XFS_STATS_INC(ip->i_mount, vn_rele);
> -	XFS_STATS_INC(ip->i_mount, vn_remove);
> +	XFS_STATS_INC(mp, vn_rele);
> +	XFS_STATS_INC(mp, vn_remove);
> +
> +	/*
> +	 * If a quota type is turned off but we still have a dquot attached to
> +	 * the inode, detach it before processing this inode to avoid delaying
> +	 * quotaoff for longer than is necessary.
> +	 *
> +	 * The inode has no VFS state and hasn't been tagged for any kind of
> +	 * reclamation, which means that iget, quotaoff, blockgc, and reclaim
> +	 * will not touch it.  It is therefore safe to do this locklessly
> +	 * because we have the only reference here.
> +	 */
> +	if (!XFS_IS_UQUOTA_ON(mp)) {
> +		xfs_qm_dqrele(ip->i_udquot);
> +		ip->i_udquot = NULL;
> +	}
> +	if (!XFS_IS_GQUOTA_ON(mp)) {
> +		xfs_qm_dqrele(ip->i_gdquot);
> +		ip->i_gdquot = NULL;
> +	}
> +	if (!XFS_IS_PQUOTA_ON(mp)) {
> +		xfs_qm_dqrele(ip->i_pdquot);
> +		ip->i_pdquot = NULL;
> +	}
>  
>  	xfs_inactive(ip);

Shouldn't we just make xfs_inactive() unconditionally detatch dquots
rather than just in the conditional case it does now after attaching
dquots because it has to make modifications? For inodes that don't
require any inactivation work, we get the same thing, and for those
that do running a few extra transactions before dropping the dquots
isn't going to make a huge difference to the quotaoff latency....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/5] xfs: move xfs_inew_wait call into xfs_dqrele_inode

[Dave Chinner]

On Mon, May 31, 2021 at 03:41:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Move the INEW wait into xfs_dqrele_inode so that we can drop the
> iter_flags parameter in the next patch.

What next patch? :/

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_icache.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 5501318b5db0..859ab1279d8d 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -981,6 +981,9 @@ xfs_dqrele_inode(
>  {
>  	struct xfs_eofblocks	*eofb = priv;
>  
> +	if (xfs_iflags_test(ip, XFS_INEW))
> +		xfs_inew_wait(ip);
> +
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	if (eofb->eof_flags & XFS_EOFB_DROP_UDQUOT) {
>  		xfs_qm_dqrele(ip->i_udquot);
> @@ -1019,8 +1022,8 @@ xfs_dqrele_all_inodes(
>  	if (qflags & XFS_PQUOTA_ACCT)
>  		eofb.eof_flags |= XFS_EOFB_DROP_PDQUOT;
>  
> -	return xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
> -			&eofb, XFS_ICI_DQRELE_NONTAG);
> +	return xfs_inode_walk(mp, 0, xfs_dqrele_inode, &eofb,
> +			XFS_ICI_DQRELE_NONTAG);
>  }

In isolation, this doesn't mean a whole lot. It seems somewhat
related to the earlier patch that kinda duplicated
xfs_inode_walk_ag_grab(), and it looks like this removes the only
user of XFS_INODE_WALK_INEW_WAIT, so does the missing next patch
remove XFS_INODE_WALK_INEW_WAIT and the rest of the that machinery?

If so, this seems like it should follow up the earlier patch or even
precede it, because getting rid of XFS_INODE_WALK_INEW_WAIT first
means that xfs_inode_walk_ag_grab() and xfs_inode_walk_dquot_grab()
are then identical....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/5] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab

[Darrick J. Wong]

On Tue, Jun 01, 2021 at 10:20:23AM +1000, Dave Chinner wrote:
> On Mon, May 31, 2021 at 03:41:07PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Disentangle the dqrele_all inode grab code from the "generic" inode walk
> > grabbing code, and and use the opportunity to document why the dqrele
> > grab function does what it does.
> > 
> > Since dqrele_all is the only user of XFS_ICI_NO_TAG, rename it to
> > something more specific for what we're doing.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_icache.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++---
> >  fs/xfs/xfs_icache.h |    4 ++-
> >  2 files changed, 62 insertions(+), 6 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index 34b8b5fbd60d..5501318b5db0 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -26,6 +26,8 @@
> >  
> >  #include <linux/iversion.h>
> >  
> > +static bool xfs_dqrele_inode_grab(struct xfs_inode *ip);
> > +
> 
> Just mov the function higher up in the file rather than add forward
> declarations....

Ugh, this will cause churn that will ripple through this and the next
iwalk refactoring patchsets and deferred inactivation.  Can I please
please please defer the churn cleanup until the end of all that?

> 
> >  /*
> >   * Allocate and initialise an xfs_inode.
> >   */
> > @@ -765,6 +767,22 @@ xfs_inode_walk_ag_grab(
> >  	return false;
> >  }
> >  
> > +static inline bool
> > +xfs_grabbed_for_walk(
> > +	int			tag,
> > +	struct xfs_inode	*ip,
> > +	int			iter_flags)
> > +{
> > +	switch (tag) {
> > +	case XFS_ICI_BLOCKGC_TAG:
> > +		return xfs_inode_walk_ag_grab(ip, iter_flags);
> > +	case XFS_ICI_DQRELE_NONTAG:
> > +		return xfs_dqrele_inode_grab(ip);
> > +	default:
> > +		return false;
> > +	}
> > +}
> 
> Not really a fan of this XFS_ICI_DQRELE_NONTAG rename. It kinda
> smears caller context across the walk API. We really have two
> different things here - we want a tagless lookup, and we want a
> dquot specific grab function.
> 
> This API change just means we're going to have to rename the "no
> tag" lookup yet again when we need some other non tag-based lookup.
> 
> And I think this is redundant, because....
> 
> > +/* Decide if we want to grab this inode to drop its dquots. */
> > +static bool
> > +xfs_dqrele_inode_grab(
> > +	struct xfs_inode	*ip)
> > +{
> > +	bool			ret = false;
> > +
> > +	ASSERT(rcu_read_lock_held());
> > +
> > +	/* Check for stale RCU freed inode */
> > +	spin_lock(&ip->i_flags_lock);
> > +	if (!ip->i_ino)
> > +		goto out_unlock;
> > +
> > +	/*
> > +	 * Skip inodes that are anywhere in the reclaim machinery because we
> > +	 * drop dquots before tagging an inode for reclamation.
> > +	 */
> > +	if (ip->i_flags & (XFS_IRECLAIM | XFS_IRECLAIMABLE))
> > +		goto out_unlock;
> > +
> > +	/*
> > +	 * The inode looks alive; try to grab a VFS reference so that it won't
> > +	 * get destroyed.  If we got the reference, return true to say that
> > +	 * we grabbed the inode.
> > +	 *
> > +	 * If we can't get the reference, then we know the inode had its VFS
> > +	 * state torn down and hasn't yet entered the reclaim machinery.  Since
> > +	 * we also know that dquots are detached from an inode before it enters
> > +	 * reclaim, we can skip the inode.
> > +	 */
> > +	ret = igrab(VFS_I(ip)) != NULL;
> > +
> > +out_unlock:
> > +	spin_unlock(&ip->i_flags_lock);
> > +	return ret;
> > +}
> 
> This is basically just duplication of xfs_inode_walk_ag_grab()
> without the XFS_INODE_WALK_INEW_WAIT check in it. At this point I
> just don't see a reason for this function or the
> XFS_ICI_DQRELE_NONTAG rename just to use this grab function...

Ugh.  I should have sent the /next/ iwalk refactoring series along with
this one so that it would become more obvious that the end goal is to
seal all the incore inode walk code in xfs_icache.c, since there are
only four of them (reclaim, inodegc, blockgc, quotaoff) and the grab
functions for all four are just different enough that it's not really
worth it to keep them combined in one function full of conditionals.

Once that's done, the only user of xfs_inode_walk_ag_grab is the blockgc
code and I can rename it.

Ofc the reason I held back is that the next series adds 8 more iwalk
cleanup patches, and the more patches I send all at once the longer it
takes for anyone to start looking at it.  I /still/ can't figure out the
balance between risking overwhelming everyone with too many patches vs.
sending insufficient patches to convey where I'm really going with
something.

<shrug> I might just ping you on irc so that we can have a conversation
about this and summarize whatever we come up with for the list.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 4/5] xfs: drop inactive dquots before inactivating inodes

[Darrick J. Wong]

On Tue, Jun 01, 2021 at 10:35:06AM +1000, Dave Chinner wrote:
> On Mon, May 31, 2021 at 03:41:13PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > During quotaoff, the incore inode scan to detach dquots from inodes
> > won't touch inodes that have lost their VFS state but haven't yet been
> > queued for reclaim.  This isn't strictly a problem because we drop the
> > dquots at the end of inactivation, but if we detect this situation
> > before starting inactivation, we can drop the inactive dquots early to
> > avoid delaying quotaoff further.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_super.c |   32 ++++++++++++++++++++++++++++----
> >  1 file changed, 28 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index a2dab05332ac..79f1cd1a0221 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -637,22 +637,46 @@ xfs_fs_destroy_inode(
> >  	struct inode		*inode)
> >  {
> >  	struct xfs_inode	*ip = XFS_I(inode);
> > +	struct xfs_mount	*mp = ip->i_mount;
> >  
> >  	trace_xfs_destroy_inode(ip);
> >  
> >  	ASSERT(!rwsem_is_locked(&inode->i_rwsem));
> > -	XFS_STATS_INC(ip->i_mount, vn_rele);
> > -	XFS_STATS_INC(ip->i_mount, vn_remove);
> > +	XFS_STATS_INC(mp, vn_rele);
> > +	XFS_STATS_INC(mp, vn_remove);
> > +
> > +	/*
> > +	 * If a quota type is turned off but we still have a dquot attached to
> > +	 * the inode, detach it before processing this inode to avoid delaying
> > +	 * quotaoff for longer than is necessary.
> > +	 *
> > +	 * The inode has no VFS state and hasn't been tagged for any kind of
> > +	 * reclamation, which means that iget, quotaoff, blockgc, and reclaim
> > +	 * will not touch it.  It is therefore safe to do this locklessly
> > +	 * because we have the only reference here.
> > +	 */
> > +	if (!XFS_IS_UQUOTA_ON(mp)) {
> > +		xfs_qm_dqrele(ip->i_udquot);
> > +		ip->i_udquot = NULL;
> > +	}
> > +	if (!XFS_IS_GQUOTA_ON(mp)) {
> > +		xfs_qm_dqrele(ip->i_gdquot);
> > +		ip->i_gdquot = NULL;
> > +	}
> > +	if (!XFS_IS_PQUOTA_ON(mp)) {
> > +		xfs_qm_dqrele(ip->i_pdquot);
> > +		ip->i_pdquot = NULL;
> > +	}
> >  
> >  	xfs_inactive(ip);
> 
> Shouldn't we just make xfs_inactive() unconditionally detatch dquots
> rather than just in the conditional case it does now after attaching
> dquots because it has to make modifications? For inodes that don't
> require any inactivation work, we get the same thing, and for those
> that do running a few extra transactions before dropping the dquots
> isn't going to make a huge difference to the quotaoff latency....

Actually... the previous patch does exactly that.  I'll drop this patch.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 3/5] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab

[Dave Chinner]

On Tue, Jun 01, 2021 at 12:50:51PM -0700, Darrick J. Wong wrote:
> On Tue, Jun 01, 2021 at 10:20:23AM +1000, Dave Chinner wrote:
> > On Mon, May 31, 2021 at 03:41:07PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Disentangle the dqrele_all inode grab code from the "generic" inode walk
> > > grabbing code, and and use the opportunity to document why the dqrele
> > > grab function does what it does.
> > > 
> > > Since dqrele_all is the only user of XFS_ICI_NO_TAG, rename it to
> > > something more specific for what we're doing.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/xfs_icache.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  fs/xfs/xfs_icache.h |    4 ++-
> > >  2 files changed, 62 insertions(+), 6 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > > index 34b8b5fbd60d..5501318b5db0 100644
> > > --- a/fs/xfs/xfs_icache.c
> > > +++ b/fs/xfs/xfs_icache.c
> > > @@ -26,6 +26,8 @@
> > >  
> > >  #include <linux/iversion.h>
> > >  
> > > +static bool xfs_dqrele_inode_grab(struct xfs_inode *ip);
> > > +
> > 
> > Just mov the function higher up in the file rather than add forward
> > declarations....
> 
> Ugh, this will cause churn that will ripple through this and the next
> iwalk refactoring patchsets and deferred inactivation.  Can I please
> please please defer the churn cleanup until the end of all that?

Yes, by all means. I don't want to make it harder to get stuff done,
so moving stuff around at the end of the series is fine...

....

> > This is basically just duplication of xfs_inode_walk_ag_grab()
> > without the XFS_INODE_WALK_INEW_WAIT check in it. At this point I
> > just don't see a reason for this function or the
> > XFS_ICI_DQRELE_NONTAG rename just to use this grab function...
> 
> Ugh.  I should have sent the /next/ iwalk refactoring series along with
> this one so that it would become more obvious that the end goal is to
> seal all the incore inode walk code in xfs_icache.c, since there are
> only four of them (reclaim, inodegc, blockgc, quotaoff) and the grab
> functions for all four are just different enough that it's not really
> worth it to keep them combined in one function full of conditionals.
> 
> Once that's done, the only user of xfs_inode_walk_ag_grab is the blockgc
> code and I can rename it.

Ok, that context is missing from the patch series. :/

> Ofc the reason I held back is that the next series adds 8 more iwalk
> cleanup patches, and the more patches I send all at once the longer it
> takes for anyone to start looking at it.  I /still/ can't figure out the
> balance between risking overwhelming everyone with too many patches vs.
> sending insufficient patches to convey where I'm really going with
> something.

Yeah, can be difficult. I prefer to err on the side of "complete
change" rather than splitting two parts of a larger work
arbitrarily...

> <shrug> I might just ping you on irc so that we can have a conversation
> about this and summarize whatever we come up with for the list.

You've got a branch with the full series in it somewhere, I'm
guessing? point me at it so I can see where this ends up....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/5] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab

[Darrick J. Wong]

On Wed, Jun 02, 2021 at 07:40:27AM +1000, Dave Chinner wrote:
> On Tue, Jun 01, 2021 at 12:50:51PM -0700, Darrick J. Wong wrote:
> > On Tue, Jun 01, 2021 at 10:20:23AM +1000, Dave Chinner wrote:
> > > On Mon, May 31, 2021 at 03:41:07PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Disentangle the dqrele_all inode grab code from the "generic" inode walk
> > > > grabbing code, and and use the opportunity to document why the dqrele
> > > > grab function does what it does.
> > > > 
> > > > Since dqrele_all is the only user of XFS_ICI_NO_TAG, rename it to
> > > > something more specific for what we're doing.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  fs/xfs/xfs_icache.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++---
> > > >  fs/xfs/xfs_icache.h |    4 ++-
> > > >  2 files changed, 62 insertions(+), 6 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > > > index 34b8b5fbd60d..5501318b5db0 100644
> > > > --- a/fs/xfs/xfs_icache.c
> > > > +++ b/fs/xfs/xfs_icache.c
> > > > @@ -26,6 +26,8 @@
> > > >  
> > > >  #include <linux/iversion.h>
> > > >  
> > > > +static bool xfs_dqrele_inode_grab(struct xfs_inode *ip);
> > > > +
> > > 
> > > Just mov the function higher up in the file rather than add forward
> > > declarations....
> > 
> > Ugh, this will cause churn that will ripple through this and the next
> > iwalk refactoring patchsets and deferred inactivation.  Can I please
> > please please defer the churn cleanup until the end of all that?
> 
> Yes, by all means. I don't want to make it harder to get stuff done,
> so moving stuff around at the end of the series is fine...
> 
> ....

In the end it was easy enough to do it (as a separate prep patch) once I
concluded that separate the goal of the inode_walk from the radix tree
tags to eliminate the confusing XFS_ICI_NONTAG cases (i.e. quotaoff).

> > > This is basically just duplication of xfs_inode_walk_ag_grab()
> > > without the XFS_INODE_WALK_INEW_WAIT check in it. At this point I
> > > just don't see a reason for this function or the
> > > XFS_ICI_DQRELE_NONTAG rename just to use this grab function...
> > 
> > Ugh.  I should have sent the /next/ iwalk refactoring series along with
> > this one so that it would become more obvious that the end goal is to
> > seal all the incore inode walk code in xfs_icache.c, since there are
> > only four of them (reclaim, inodegc, blockgc, quotaoff) and the grab
> > functions for all four are just different enough that it's not really
> > worth it to keep them combined in one function full of conditionals.
> > 
> > Once that's done, the only user of xfs_inode_walk_ag_grab is the blockgc
> > code and I can rename it.
> 
> Ok, that context is missing from the patch series. :/

Sorry.

> > Ofc the reason I held back is that the next series adds 8 more iwalk
> > cleanup patches, and the more patches I send all at once the longer it
> > takes for anyone to start looking at it.  I /still/ can't figure out the
> > balance between risking overwhelming everyone with too many patches vs.
> > sending insufficient patches to convey where I'm really going with
> > something.
> 
> Yeah, can be difficult. I prefer to err on the side of "complete
> change" rather than splitting two parts of a larger work
> arbitrarily...

<nod> I'll combine this set and the next one when I resend this patch
pile.

> > <shrug> I might just ping you on irc so that we can have a conversation
> > about this and summarize whatever we come up with for the list.
> 
> You've got a branch with the full series in it somewhere, I'm
> guessing? point me at it so I can see where this ends up....

Yup.

https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=inode-walk-cleanups-5.14

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com