All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, hch@infradead.org
Subject: Re: [PATCH 3/5] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab
Date: Tue, 1 Jun 2021 12:50:51 -0700	[thread overview]
Message-ID: <20210601195051.GB26380@locust> (raw)
In-Reply-To: <20210601002023.GY664593@dread.disaster.area>

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

  reply	other threads:[~2021-06-01 19:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-31 22:40 [PATCHSET v2 0/5] xfs: clean up quotaoff inode walks Darrick J. Wong
2021-05-31 22:40 ` [PATCH 1/5] xfs: move the quotaoff dqrele inode walk into xfs_icache.c Darrick J. Wong
2021-05-31 22:41 ` [PATCH 2/5] xfs: detach inode dquots at the end of inactivation Darrick J. Wong
2021-06-01  0:09   ` Dave Chinner
2021-05-31 22:41 ` [PATCH 3/5] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab Darrick J. Wong
2021-06-01  0:20   ` Dave Chinner
2021-06-01 19:50     ` Darrick J. Wong [this message]
2021-06-01 21:40       ` Dave Chinner
2021-06-01 23:15         ` Darrick J. Wong
2021-05-31 22:41 ` [PATCH 4/5] xfs: drop inactive dquots before inactivating inodes Darrick J. Wong
2021-06-01  0:35   ` Dave Chinner
2021-06-01 19:53     ` Darrick J. Wong
2021-05-31 22:41 ` [PATCH 5/5] xfs: move xfs_inew_wait call into xfs_dqrele_inode Darrick J. Wong
2021-06-01  0:47   ` Dave Chinner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210601195051.GB26380@locust \
    --to=djwong@kernel.org \
    --cc=david@fromorbit.com \
    --cc=hch@infradead.org \
    --cc=linux-xfs@vger.kernel.org \
    --subject='Re: [PATCH 3/5] xfs: separate the dqrele_all inode grab logic from xfs_inode_walk_ag_grab' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.