diff mbox series

[11/11] xfs: hide most of the incore inode walk interface

Message ID 158993918698.976105.6231244252663510379.stgit@magnolia (mailing list archive)
State Superseded, archived
Headers show
Series xfs: refactor incore inode walking | expand

Commit Message

Darrick J. Wong May 20, 2020, 1:46 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Hide the incore inode walk interface because callers outside of the
icache code don't need to know about iter_flags and radix tags and other
implementation details of the incore inode cache.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c      |   30 ++++++++++++++++++++++--------
 fs/xfs/xfs_icache.h      |    8 ++++----
 fs/xfs/xfs_qm_syscalls.c |    3 +--
 3 files changed, 27 insertions(+), 14 deletions(-)

Comments

Christoph Hellwig May 20, 2020, 6:46 a.m. UTC | #1 
On Tue, May 19, 2020 at 06:46:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Hide the incore inode walk interface because callers outside of the
> icache code don't need to know about iter_flags and radix tags and other
> implementation details of the incore inode cache.

I don't really see the point here.  It isn't hiding much, and only from
a single caller.  I have to say I also prefer the old naming over _ici_
and find the _all postfix not exactly descriptive.
Darrick J. Wong May 20, 2020, 6:54 p.m. UTC | #2 
On Wed, May 20, 2020 at 08:46:43AM +0200, Christoph Hellwig wrote:
> On Tue, May 19, 2020 at 06:46:27PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Hide the incore inode walk interface because callers outside of the
> > icache code don't need to know about iter_flags and radix tags and other
> > implementation details of the incore inode cache.
> 
> I don't really see the point here.  It isn't hiding much, and only from
> a single caller.  I have to say I also prefer the old naming over _ici_
> and find the _all postfix not exactly descriptive.

This last patch is more of a prep patch for the patchsets that come
after it: cleaning up the block gc stuff and deferred inode
inactivation.  It's getting kinda late so I didn't want to send 11 more
patches, but perhaps that would make it clearer where this is all
heading?

The quota dqrele_all code does not care about inode radix tree tags nor
does it need the ability to grab inodes /while/ they're in INEW state,
so there's no reason to pass those arguments around.

OTOH I guess I could have hid XFS_AGITER_INEW_WAIT in xfs_icache.c and
left the function names unchanged.

--D
Christoph Hellwig May 21, 2020, 9:05 a.m. UTC | #3 
On Wed, May 20, 2020 at 11:54:22AM -0700, Darrick J. Wong wrote:
> On Wed, May 20, 2020 at 08:46:43AM +0200, Christoph Hellwig wrote:
> > On Tue, May 19, 2020 at 06:46:27PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Hide the incore inode walk interface because callers outside of the
> > > icache code don't need to know about iter_flags and radix tags and other
> > > implementation details of the incore inode cache.
> > 
> > I don't really see the point here.  It isn't hiding much, and only from
> > a single caller.  I have to say I also prefer the old naming over _ici_
> > and find the _all postfix not exactly descriptive.
> 
> This last patch is more of a prep patch for the patchsets that come
> after it: cleaning up the block gc stuff and deferred inode
> inactivation.  It's getting kinda late so I didn't want to send 11 more
> patches, but perhaps that would make it clearer where this is all
> heading?

I'd say drop it from this series and resend it with that series if
you are going to send it out.

> The quota dqrele_all code does not care about inode radix tree tags nor

It only started to care about them because you merged the functions
and now exposed them to the dqrele code at the beginning of this series.
But with just a single user not caring and too aring I'm perfectly fine
with exposing the tags in the interface.

> does it need the ability to grab inodes /while/ they're in INEW state,
> so there's no reason to pass those arguments around.
> 
> OTOH I guess I could have hid XFS_AGITER_INEW_WAIT in xfs_icache.c and
> left the function names unchanged.

Maybe just flip the default for the flag if that makes your series
easier?
diff mbox series

Patch

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 05614758fb1e..73ac0e18498e 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -752,7 +752,7 @@  xfs_inode_ag_walk_grab(
 	int			flags)
 {
 	struct inode		*inode = VFS_I(ip);
-	bool			newinos = !!(flags & XFS_AGITER_INEW_WAIT);
+	bool			newinos = !!(flags & XFS_ICI_WALK_INEW_WAIT);
 
 	ASSERT(rcu_read_lock_held());
 
@@ -796,7 +796,7 @@  xfs_inode_ag_walk_grab(
  * inodes with the given radix tree @tag.
  */
 STATIC int
-xfs_inode_ag_walk(
+xfs_ici_walk_ag(
 	struct xfs_mount	*mp,
 	struct xfs_perag	*pag,
 	int			(*execute)(struct xfs_inode *ip, void *args),
@@ -872,7 +872,7 @@  xfs_inode_ag_walk(
 		for (i = 0; i < nr_found; i++) {
 			if (!batch[i])
 				continue;
-			if ((iter_flags & XFS_AGITER_INEW_WAIT) &&
+			if ((iter_flags & XFS_ICI_WALK_INEW_WAIT) &&
 			    xfs_iflags_test(batch[i], XFS_INEW))
 				xfs_inew_wait(batch[i]);
 			error = execute(batch[i], args);
@@ -916,8 +916,8 @@  xfs_ici_walk_get_perag(
  * Call the @execute function on all incore inodes matching the radix tree
  * @tag.
  */
-int
-xfs_inode_ag_iterator(
+STATIC int
+xfs_ici_walk(
 	struct xfs_mount	*mp,
 	int			iter_flags,
 	int			(*execute)(struct xfs_inode *ip, void *args),
@@ -932,7 +932,7 @@  xfs_inode_ag_iterator(
 	ag = 0;
 	while ((pag = xfs_ici_walk_get_perag(mp, ag, tag))) {
 		ag = pag->pag_agno + 1;
-		error = xfs_inode_ag_walk(mp, pag, execute, args, tag,
+		error = xfs_ici_walk_ag(mp, pag, execute, args, tag,
 				iter_flags);
 		xfs_perag_put(pag);
 		if (error) {
@@ -944,6 +944,20 @@  xfs_inode_ag_iterator(
 	return last_error;
 }
 
+/*
+ * Walk all incore inodes in the filesystem.  Knowledge of radix tree tags
+ * is hidden and we always wait for INEW inodes.
+ */
+int
+xfs_ici_walk_all(
+	struct xfs_mount	*mp,
+	int			(*execute)(struct xfs_inode *ip, void *args),
+	void			*args)
+{
+	return xfs_ici_walk(mp, XFS_ICI_WALK_INEW_WAIT, execute, args,
+			XFS_ICI_NO_TAG);
+}
+
 /*
  * Background scanning to trim post-EOF preallocated space. This is queued
  * based on the 'speculative_prealloc_lifetime' tunable (5m by default).
@@ -1524,7 +1538,7 @@  xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
 	struct xfs_eofblocks	*eofb)
 {
-	return xfs_inode_ag_iterator(mp, 0, xfs_inode_free_eofblocks, eofb,
+	return xfs_ici_walk(mp, 0, xfs_inode_free_eofblocks, eofb,
 			XFS_ICI_EOFBLOCKS_TAG);
 }
 
@@ -1774,7 +1788,7 @@  xfs_icache_free_cowblocks(
 	struct xfs_mount	*mp,
 	struct xfs_eofblocks	*eofb)
 {
-	return xfs_inode_ag_iterator(mp, 0, xfs_inode_free_cowblocks, eofb,
+	return xfs_ici_walk(mp, 0, xfs_inode_free_cowblocks, eofb,
 			XFS_ICI_COWBLOCKS_TAG);
 }
 
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index e7f86ebd7b22..0dc85a03dc6c 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -38,9 +38,9 @@  struct xfs_eofblocks {
 #define XFS_IGET_INCORE		0x8	/* don't read from disk or reinit */
 
 /*
- * flags for AG inode iterator
+ * flags for incore inode iterator
  */
-#define XFS_AGITER_INEW_WAIT	0x1	/* wait on new inodes */
+#define XFS_ICI_WALK_INEW_WAIT	0x1	/* wait on new inodes */
 
 int xfs_iget(struct xfs_mount *mp, struct xfs_trans *tp, xfs_ino_t ino,
 	     uint flags, uint lock_flags, xfs_inode_t **ipp);
@@ -71,9 +71,9 @@  int xfs_inode_free_quota_cowblocks(struct xfs_inode *ip);
 void xfs_cowblocks_worker(struct work_struct *);
 void xfs_queue_cowblocks(struct xfs_mount *);
 
-int xfs_inode_ag_iterator(struct xfs_mount *mp, int iter_flags,
+int xfs_ici_walk_all(struct xfs_mount *mp,
 	int (*execute)(struct xfs_inode *ip, void *args),
-	void *args, int tag);
+	void *args);
 
 int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
 				  xfs_ino_t ino, bool *inuse);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 571ecb17b3bf..95c44d5c57a8 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -781,6 +781,5 @@  xfs_qm_dqrele_all_inodes(
 	};
 
 	ASSERT(mp->m_quotainfo);
-	xfs_inode_ag_iterator(mp, XFS_AGITER_INEW_WAIT, xfs_dqrele_inode, &dqr,
-			XFS_ICI_NO_TAG);
+	xfs_ici_walk_all(mp, xfs_dqrele_inode, &dqr);
 }