diff mbox series

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

Message ID 162250086766.490412.9229536536315438431.stgit@locust (mailing list archive)
State Superseded
Headers show
Series xfs: clean up quotaoff inode walks | expand

Commit Message

Darrick J. Wong May 31, 2021, 10:41 p.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 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(-)

Comments

Dave Chinner June 1, 2021, 12:20 a.m. UTC | #1 
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.
Darrick J. Wong June 1, 2021, 7:50 p.m. UTC | #2 
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
Dave Chinner June 1, 2021, 9:40 p.m. UTC | #3 
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.
Darrick J. Wong June 1, 2021, 11:15 p.m. UTC | #4 
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
diff mbox series

Patch

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