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

["Darrick J. Wong" <djwong@kernel.org>]

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