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:

quoted

On Mon, May 31, 2021 at 03:41:07PM -0700, Darrick J. Wong wrote:

quoted

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...

....

quoted

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