diff mbox series

[05/14] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab

Message ID 162259518016.662681.13322964506776234493.stgit@locust (mailing list archive)
State Superseded
Headers show
Series xfs: clean up incore inode walk functions | expand

Commit Message

Darrick J. Wong June 2, 2021, 12:53 a.m. UTC 
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 xfs_inode_walk_ag_grab is now
only used for blockgc, rename it to reflect that.

Ultimately, there will be four reasons to perform a walk of incore
inodes: quotaoff dquote releasing (dqrele), garbage collection of
speculative preallocations (blockgc), reclamation of incore inodes
(reclaim), and deferred inactivation (inodegc).  Each of these four have
their own slightly different criteria for deciding if they want to
handle an inode, so it makes more sense to have four cohesive igrab
functions than one confusing parameteric grab function like we do now.

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

Comments

Dave Chinner June 2, 2021, 1:51 a.m. UTC | #1 
On Tue, Jun 01, 2021 at 05:53:00PM -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 xfs_inode_walk_ag_grab is now
> only used for blockgc, rename it to reflect that.
> 
> Ultimately, there will be four reasons to perform a walk of incore
> inodes: quotaoff dquote releasing (dqrele), garbage collection of
> speculative preallocations (blockgc), reclamation of incore inodes
> (reclaim), and deferred inactivation (inodegc).  Each of these four have
> their own slightly different criteria for deciding if they want to
> handle an inode, so it makes more sense to have four cohesive igrab
> functions than one confusing parameteric grab function like we do now.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_icache.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 61 insertions(+), 5 deletions(-)

Looks ok - one minor nit:

> @@ -1642,6 +1682,22 @@ xfs_blockgc_free_quota(
>  
>  /* XFS Incore Inode Walking Code */
>  
> +static inline bool
> +xfs_grabbed_for_walk(
> +	enum xfs_icwalk_goal	goal,
> +	struct xfs_inode	*ip,
> +	int			iter_flags)
> +{
> +	switch (goal) {
> +	case XFS_ICWALK_BLOCKGC:
> +		return xfs_blockgc_igrab(ip, iter_flags);
> +	case XFS_ICWALK_DQRELE:
> +		return xfs_dqrele_igrab(ip);
> +	default:
> +		return false;
> +	}
> +}

xfs_icwalk_grab() seems to make more sense here.

/me is wondering if all this should eventually end up under a
xfs_icwalk namespace?

Cheers,

Dave.
Darrick J. Wong June 2, 2021, 3:28 a.m. UTC | #2 
On Wed, Jun 02, 2021 at 11:51:47AM +1000, Dave Chinner wrote:
> On Tue, Jun 01, 2021 at 05:53:00PM -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 xfs_inode_walk_ag_grab is now
> > only used for blockgc, rename it to reflect that.
> > 
> > Ultimately, there will be four reasons to perform a walk of incore
> > inodes: quotaoff dquote releasing (dqrele), garbage collection of
> > speculative preallocations (blockgc), reclamation of incore inodes
> > (reclaim), and deferred inactivation (inodegc).  Each of these four have
> > their own slightly different criteria for deciding if they want to
> > handle an inode, so it makes more sense to have four cohesive igrab
> > functions than one confusing parameteric grab function like we do now.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_icache.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 61 insertions(+), 5 deletions(-)
> 
> Looks ok - one minor nit:
> 
> > @@ -1642,6 +1682,22 @@ xfs_blockgc_free_quota(
> >  
> >  /* XFS Incore Inode Walking Code */
> >  
> > +static inline bool
> > +xfs_grabbed_for_walk(
> > +	enum xfs_icwalk_goal	goal,
> > +	struct xfs_inode	*ip,
> > +	int			iter_flags)
> > +{
> > +	switch (goal) {
> > +	case XFS_ICWALK_BLOCKGC:
> > +		return xfs_blockgc_igrab(ip, iter_flags);
> > +	case XFS_ICWALK_DQRELE:
> > +		return xfs_dqrele_igrab(ip);
> > +	default:
> > +		return false;
> > +	}
> > +}
> 
> xfs_icwalk_grab() seems to make more sense here.

Ok, changed.

> /me is wondering if all this should eventually end up under a
> xfs_icwalk namespace?

Yeah, I'll throw a renamer patch on the end of this series.  Or possibly
just do it after I move the xfs_inode_walk functions to the bottom.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 018b3f8bdd21..bc88d33f7f24 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -757,6 +757,44 @@  xfs_icache_inode_is_allocated(
 #define XFS_LOOKUP_BATCH	32
 
 #ifdef CONFIG_XFS_QUOTA
+/* Decide if we want to grab this inode to drop its dquots. */
+static bool
+xfs_dqrele_igrab(
+	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(
@@ -806,6 +844,8 @@  xfs_dqrele_all_inodes(
 	return xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode,
 			&eofb, XFS_ICWALK_DQRELE);
 }
+#else
+# define xfs_dqrele_igrab(ip)		(false)
 #endif /* CONFIG_XFS_QUOTA */
 
 /*
@@ -1478,12 +1518,12 @@  xfs_blockgc_start(
 }
 
 /*
- * Decide if the given @ip is eligible to be a part of the inode walk, and
- * grab it if so.  Returns true if it's ready to go or false if we should just
- * ignore it.
+ * Decide if the given @ip is eligible for garbage collection of speculative
+ * preallocations, and grab it if so.  Returns true if it's ready to go or
+ * false if we should just ignore it.
  */
 static bool
-xfs_inode_walk_ag_grab(
+xfs_blockgc_igrab(
 	struct xfs_inode	*ip,
 	int			flags)
 {
@@ -1642,6 +1682,22 @@  xfs_blockgc_free_quota(
 
 /* XFS Incore Inode Walking Code */
 
+static inline bool
+xfs_grabbed_for_walk(
+	enum xfs_icwalk_goal	goal,
+	struct xfs_inode	*ip,
+	int			iter_flags)
+{
+	switch (goal) {
+	case XFS_ICWALK_BLOCKGC:
+		return xfs_blockgc_igrab(ip, iter_flags);
+	case XFS_ICWALK_DQRELE:
+		return xfs_dqrele_igrab(ip);
+	default:
+		return false;
+	}
+}
+
 /*
  * For a given per-AG structure @pag, grab, @execute, and rele all incore
  * inodes with the given radix tree @tag.
@@ -1695,7 +1751,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(goal, ip, iter_flags))
 				batch[i] = NULL;
 
 			/*