inode iterator cleanups

inode iterator cleanups

[Christoph Hellwig]

Hi all,

this series cleans up a bunch of lose ends in the inode iterator
functions.

[PATCH 1/6] xfs: remove xfs_inode_ag_iterator

[Christoph Hellwig]

This function is entirely unused.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_icache.c | 11 -----------
 fs/xfs/xfs_icache.h |  5 +----
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 922a29032e374..1f9b86768f054 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -979,17 +979,6 @@ xfs_inode_ag_iterator_flags(
 	return last_error;
 }
 
-int
-xfs_inode_ag_iterator(
-	struct xfs_mount	*mp,
-	int			(*execute)(struct xfs_inode *ip, int flags,
-					   void *args),
-	int			flags,
-	void			*args)
-{
-	return xfs_inode_ag_iterator_flags(mp, execute, flags, args, 0);
-}
-
 int
 xfs_inode_ag_iterator_tag(
 	struct xfs_mount	*mp,
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 48f1fd2bb6ad8..39712737c0d5c 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -24,7 +24,7 @@ struct xfs_eofblocks {
  * tags for inode radix tree
  */
 #define XFS_ICI_NO_TAG		(-1)	/* special flag for an untagged lookup
-					   in xfs_inode_ag_iterator */
+					   in xfs_inode_ag_iterator* */
 #define XFS_ICI_RECLAIM_TAG	0	/* inode is to be reclaimed */
 #define XFS_ICI_EOFBLOCKS_TAG	1	/* inode has blocks beyond EOF */
 #define XFS_ICI_COWBLOCKS_TAG	2	/* inode can have cow blocks to gc */
@@ -71,9 +71,6 @@ 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 (*execute)(struct xfs_inode *ip, int flags, void *args),
-	int flags, void *args);
 int xfs_inode_ag_iterator_flags(struct xfs_mount *mp,
 	int (*execute)(struct xfs_inode *ip, int flags, void *args),
 	int flags, void *args, int iter_flags);
-- 
2.26.2

[PATCH 2/6] xfs: mark xfs_inode_ag_iterator_tag static

[Christoph Hellwig]

No users outside of xfs_icache.c.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_icache.c | 2 +-
 fs/xfs/xfs_icache.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 1f9b86768f054..6c6f8015ab6ef 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -979,7 +979,7 @@ xfs_inode_ag_iterator_flags(
 	return last_error;
 }
 
-int
+static int
 xfs_inode_ag_iterator_tag(
 	struct xfs_mount	*mp,
 	int			(*execute)(struct xfs_inode *ip, int flags,
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 39712737c0d5c..11fd6d877e112 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -74,9 +74,6 @@ void xfs_queue_cowblocks(struct xfs_mount *);
 int xfs_inode_ag_iterator_flags(struct xfs_mount *mp,
 	int (*execute)(struct xfs_inode *ip, int flags, void *args),
 	int flags, void *args, int iter_flags);
-int xfs_inode_ag_iterator_tag(struct xfs_mount *mp,
-	int (*execute)(struct xfs_inode *ip, int flags, void *args),
-	int flags, void *args, int tag);
 
 static inline int
 xfs_fs_eofblocks_from_user(
-- 
2.26.2

[PATCH 3/6] xfs: remove __xfs_icache_free_eofblocks

[Christoph Hellwig]

Just open code the trivial logic in the two callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_icache.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 6c6f8015ab6ef..b638d12fb56a2 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1504,30 +1504,17 @@ xfs_inode_free_eofblocks(
 	return ret;
 }
 
-static int
-__xfs_icache_free_eofblocks(
+int
+xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
-	struct xfs_eofblocks	*eofb,
-	int			(*execute)(struct xfs_inode *ip, int flags,
-					   void *args),
-	int			tag)
+	struct xfs_eofblocks	*eofb)
 {
 	int flags = SYNC_TRYLOCK;
 
 	if (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC))
 		flags = SYNC_WAIT;
-
-	return xfs_inode_ag_iterator_tag(mp, execute, flags,
-					 eofb, tag);
-}
-
-int
-xfs_icache_free_eofblocks(
-	struct xfs_mount	*mp,
-	struct xfs_eofblocks	*eofb)
-{
-	return __xfs_icache_free_eofblocks(mp, eofb, xfs_inode_free_eofblocks,
-			XFS_ICI_EOFBLOCKS_TAG);
+	return xfs_inode_ag_iterator_tag(mp, xfs_inode_free_eofblocks, flags,
+					 eofb, XFS_ICI_EOFBLOCKS_TAG);
 }
 
 /*
@@ -1789,8 +1776,12 @@ xfs_icache_free_cowblocks(
 	struct xfs_mount	*mp,
 	struct xfs_eofblocks	*eofb)
 {
-	return __xfs_icache_free_eofblocks(mp, eofb, xfs_inode_free_cowblocks,
-			XFS_ICI_COWBLOCKS_TAG);
+	int flags = SYNC_TRYLOCK;
+
+	if (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC))
+		flags = SYNC_WAIT;
+	return xfs_inode_ag_iterator_tag(mp, xfs_inode_free_cowblocks, flags,
+					 eofb, XFS_ICI_COWBLOCKS_TAG);
 }
 
 int
-- 
2.26.2

[PATCH 4/6] xfs: merge xfs_inode_free_quota_{eof,cow}blocks

[Christoph Hellwig]

These two are always called together.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c   |  5 +----
 fs/xfs/xfs_icache.c | 28 +++++++---------------------
 fs/xfs/xfs_icache.h |  3 +--
 3 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 4b8bdecc38635..a1a62647f3935 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -650,10 +650,7 @@ xfs_file_buffered_aio_write(
 	 */
 	if (ret == -EDQUOT && !enospc) {
 		xfs_iunlock(ip, iolock);
-		enospc = xfs_inode_free_quota_eofblocks(ip);
-		if (enospc)
-			goto write_retry;
-		enospc = xfs_inode_free_quota_cowblocks(ip);
+		enospc = xfs_inode_free_quota_blocks(ip);
 		if (enospc)
 			goto write_retry;
 		iolock = 0;
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index b638d12fb56a2..aa664be49fd50 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1523,11 +1523,9 @@ xfs_icache_free_eofblocks(
  * failure. We make a best effort by including each quota under low free space
  * conditions (less than 1% free space) in the scan.
  */
-static int
-__xfs_inode_free_quota_eofblocks(
-	struct xfs_inode	*ip,
-	int			(*execute)(struct xfs_mount *mp,
-					   struct xfs_eofblocks	*eofb))
+int
+xfs_inode_free_quota_blocks(
+	struct xfs_inode	*ip)
 {
 	int scan = 0;
 	struct xfs_eofblocks eofb = {0};
@@ -1557,19 +1555,14 @@ __xfs_inode_free_quota_eofblocks(
 		}
 	}
 
-	if (scan)
-		execute(ip->i_mount, &eofb);
+	if (scan) {
+		xfs_icache_free_eofblocks(ip->i_mount, &eofb);
+		xfs_icache_free_cowblocks(ip->i_mount, &eofb);
+	}
 
 	return scan;
 }
 
-int
-xfs_inode_free_quota_eofblocks(
-	struct xfs_inode *ip)
-{
-	return __xfs_inode_free_quota_eofblocks(ip, xfs_icache_free_eofblocks);
-}
-
 static inline unsigned long
 xfs_iflag_for_tag(
 	int		tag)
@@ -1784,13 +1777,6 @@ xfs_icache_free_cowblocks(
 					 eofb, XFS_ICI_COWBLOCKS_TAG);
 }
 
-int
-xfs_inode_free_quota_cowblocks(
-	struct xfs_inode *ip)
-{
-	return __xfs_inode_free_quota_eofblocks(ip, xfs_icache_free_cowblocks);
-}
-
 void
 xfs_inode_set_cowblocks_tag(
 	xfs_inode_t	*ip)
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 11fd6d877e112..3e4a8b3913f51 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -60,14 +60,13 @@ void xfs_inode_set_reclaim_tag(struct xfs_inode *ip);
 void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
 void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
 int xfs_icache_free_eofblocks(struct xfs_mount *, struct xfs_eofblocks *);
-int xfs_inode_free_quota_eofblocks(struct xfs_inode *ip);
+int xfs_inode_free_quota_blocks(struct xfs_inode *ip);
 void xfs_eofblocks_worker(struct work_struct *);
 void xfs_queue_eofblocks(struct xfs_mount *);
 
 void xfs_inode_set_cowblocks_tag(struct xfs_inode *ip);
 void xfs_inode_clear_cowblocks_tag(struct xfs_inode *ip);
 int xfs_icache_free_cowblocks(struct xfs_mount *, struct xfs_eofblocks *);
-int xfs_inode_free_quota_cowblocks(struct xfs_inode *ip);
 void xfs_cowblocks_worker(struct work_struct *);
 void xfs_queue_cowblocks(struct xfs_mount *);
 
-- 
2.26.2

[PATCH 5/6] xfs: add a flags argument to xfs_icache_free_cowblocks

[Christoph Hellwig]

Pass the SYNC_* flags directly instead of hiding the information in the
eofblocks structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c   |  2 +-
 fs/xfs/xfs_icache.c | 11 ++++-------
 fs/xfs/xfs_icache.h |  3 ++-
 fs/xfs/xfs_super.c  |  2 +-
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index a1a62647f3935..6fc1a4a2f1966 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -663,7 +663,7 @@ xfs_file_buffered_aio_write(
 		xfs_iunlock(ip, iolock);
 		eofb.eof_flags = XFS_EOF_FLAGS_SYNC;
 		xfs_icache_free_eofblocks(ip->i_mount, &eofb);
-		xfs_icache_free_cowblocks(ip->i_mount, &eofb);
+		xfs_icache_free_cowblocks(ip->i_mount, &eofb, SYNC_WAIT);
 		goto write_retry;
 	}
 
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index aa664be49fd50..43273b809fa34 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -944,7 +944,7 @@ xfs_cowblocks_worker(
 
 	if (!sb_start_write_trylock(mp->m_super))
 		return;
-	xfs_icache_free_cowblocks(mp, NULL);
+	xfs_icache_free_cowblocks(mp, NULL, SYNC_TRYLOCK);
 	sb_end_write(mp->m_super);
 
 	xfs_queue_cowblocks(mp);
@@ -1557,7 +1557,7 @@ xfs_inode_free_quota_blocks(
 
 	if (scan) {
 		xfs_icache_free_eofblocks(ip->i_mount, &eofb);
-		xfs_icache_free_cowblocks(ip->i_mount, &eofb);
+		xfs_icache_free_cowblocks(ip->i_mount, &eofb, SYNC_WAIT);
 	}
 
 	return scan;
@@ -1767,12 +1767,9 @@ xfs_inode_free_cowblocks(
 int
 xfs_icache_free_cowblocks(
 	struct xfs_mount	*mp,
-	struct xfs_eofblocks	*eofb)
+	struct xfs_eofblocks	*eofb,
+	int			flags)
 {
-	int flags = SYNC_TRYLOCK;
-
-	if (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC))
-		flags = SYNC_WAIT;
 	return xfs_inode_ag_iterator_tag(mp, xfs_inode_free_cowblocks, flags,
 					 eofb, XFS_ICI_COWBLOCKS_TAG);
 }
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 3e4a8b3913f51..a850142769226 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -66,7 +66,8 @@ void xfs_queue_eofblocks(struct xfs_mount *);
 
 void xfs_inode_set_cowblocks_tag(struct xfs_inode *ip);
 void xfs_inode_clear_cowblocks_tag(struct xfs_inode *ip);
-int xfs_icache_free_cowblocks(struct xfs_mount *, struct xfs_eofblocks *);
+int xfs_icache_free_cowblocks(struct xfs_mount *mp, struct xfs_eofblocks *eofb,
+		int flags);
 void xfs_cowblocks_worker(struct work_struct *);
 void xfs_queue_cowblocks(struct xfs_mount *);
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index aae469f73efeb..67efcc5c87929 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1670,7 +1670,7 @@ xfs_remount_ro(
 	xfs_stop_block_reaping(mp);
 
 	/* Get rid of any leftover CoW reservations... */
-	error = xfs_icache_free_cowblocks(mp, NULL);
+	error = xfs_icache_free_cowblocks(mp, NULL, SYNC_TRYLOCK);
 	if (error) {
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
 		return error;
-- 
2.26.2

[PATCH 6/6] xfs: add a flags argument to xfs_icache_free_eofblocks

[Christoph Hellwig]

Pass the SYNC_* flags directly instead of hiding the information in the
eofblocks structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c   |  3 +--
 fs/xfs/xfs_icache.c | 11 ++++-------
 fs/xfs/xfs_icache.h |  3 ++-
 fs/xfs/xfs_ioctl.c  |  4 +++-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 6fc1a4a2f1966..62fbc0105e45d 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -661,8 +661,7 @@ xfs_file_buffered_aio_write(
 		xfs_flush_inodes(ip->i_mount);
 
 		xfs_iunlock(ip, iolock);
-		eofb.eof_flags = XFS_EOF_FLAGS_SYNC;
-		xfs_icache_free_eofblocks(ip->i_mount, &eofb);
+		xfs_icache_free_eofblocks(ip->i_mount, &eofb, SYNC_WAIT);
 		xfs_icache_free_cowblocks(ip->i_mount, &eofb, SYNC_WAIT);
 		goto write_retry;
 	}
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 43273b809fa34..11bef3e349a68 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -912,7 +912,7 @@ xfs_eofblocks_worker(
 
 	if (!sb_start_write_trylock(mp->m_super))
 		return;
-	xfs_icache_free_eofblocks(mp, NULL);
+	xfs_icache_free_eofblocks(mp, NULL, SYNC_TRYLOCK);
 	sb_end_write(mp->m_super);
 
 	xfs_queue_eofblocks(mp);
@@ -1507,12 +1507,9 @@ xfs_inode_free_eofblocks(
 int
 xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
-	struct xfs_eofblocks	*eofb)
+	struct xfs_eofblocks	*eofb,
+	int			flags)
 {
-	int flags = SYNC_TRYLOCK;
-
-	if (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC))
-		flags = SYNC_WAIT;
 	return xfs_inode_ag_iterator_tag(mp, xfs_inode_free_eofblocks, flags,
 					 eofb, XFS_ICI_EOFBLOCKS_TAG);
 }
@@ -1556,7 +1553,7 @@ xfs_inode_free_quota_blocks(
 	}
 
 	if (scan) {
-		xfs_icache_free_eofblocks(ip->i_mount, &eofb);
+		xfs_icache_free_eofblocks(ip->i_mount, &eofb, SYNC_WAIT);
 		xfs_icache_free_cowblocks(ip->i_mount, &eofb, SYNC_WAIT);
 	}
 
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index a850142769226..a82ff6457993b 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -59,7 +59,8 @@ void xfs_inode_set_reclaim_tag(struct xfs_inode *ip);
 
 void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
 void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
-int xfs_icache_free_eofblocks(struct xfs_mount *, struct xfs_eofblocks *);
+int xfs_icache_free_eofblocks(struct xfs_mount *mp, struct xfs_eofblocks *eofb,
+		int flags);
 int xfs_inode_free_quota_blocks(struct xfs_inode *ip);
 void xfs_eofblocks_worker(struct work_struct *);
 void xfs_queue_eofblocks(struct xfs_mount *);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 4ee0d13232f3f..4aed4df98722f 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -2330,7 +2330,9 @@ xfs_file_ioctl(
 			return error;
 
 		sb_start_write(mp->m_super);
-		error = xfs_icache_free_eofblocks(mp, &keofb);
+		error = xfs_icache_free_eofblocks(mp, &keofb,
+			(keofb.eof_flags & XFS_EOF_FLAGS_SYNC) ?
+			 SYNC_WAIT : SYNC_TRYLOCK);
 		sb_end_write(mp->m_super);
 		return error;
 	}
-- 
2.26.2

Re: inode iterator cleanups

[Darrick J. Wong]

On Mon, May 18, 2020 at 07:04:31PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up a bunch of lose ends in the inode iterator
> functions.

Funny, I was going to send a series next cycle that refactors a fair
amount of the incore inode iterators and then collapses the
free_cowblocks/free_eofblocks code into a single worker and radix tree
tag that takes care of all the speculative preallocation extent gc
stuff...

incore walks:
https://lore.kernel.org/linux-xfs/157784075463.1360343.1278255546758019580.stgit@magnolia/

reworking of the quota flushing:
https://lore.kernel.org/linux-xfs/157784083298.1361522.7064886067520069080.stgit@magnolia/

posteof/cowblocks consolidation:
https://lore.kernel.org/linux-xfs/157784087594.1361683.5987233633798863051.stgit@magnolia/

--D

Re: inode iterator cleanups

[Christoph Hellwig]

On Mon, May 18, 2020 at 06:23:37PM -0700, Darrick J. Wong wrote:
> On Mon, May 18, 2020 at 07:04:31PM +0200, Christoph Hellwig wrote:
> > Hi all,
> > 
> > this series cleans up a bunch of lose ends in the inode iterator
> > functions.
> 
> Funny, I was going to send a series next cycle that refactors a fair
> amount of the incore inode iterators and then collapses the
> free_cowblocks/free_eofblocks code into a single worker and radix tree
> tag that takes care of all the speculative preallocation extent gc
> stuff...
> 
> incore walks:
> https://lore.kernel.org/linux-xfs/157784075463.1360343.1278255546758019580.stgit@magnolia/

Except for the last few those seem non-brainers tat would could merge
ASAP..

The rest also doesn't look bad, so I'll happily defer this series.
OTOH the first on really should go in, either your or my version :)

Re: inode iterator cleanups

[Darrick J. Wong]

On Tue, May 19, 2020 at 08:40:17AM +0200, Christoph Hellwig wrote:
> On Mon, May 18, 2020 at 06:23:37PM -0700, Darrick J. Wong wrote:
> > On Mon, May 18, 2020 at 07:04:31PM +0200, Christoph Hellwig wrote:
> > > Hi all,
> > > 
> > > this series cleans up a bunch of lose ends in the inode iterator
> > > functions.
> > 
> > Funny, I was going to send a series next cycle that refactors a fair
> > amount of the incore inode iterators and then collapses the
> > free_cowblocks/free_eofblocks code into a single worker and radix tree
> > tag that takes care of all the speculative preallocation extent gc
> > stuff...
> > 
> > incore walks:
> > https://lore.kernel.org/linux-xfs/157784075463.1360343.1278255546758019580.stgit@magnolia/
> 
> Except for the last few those seem non-brainers tat would could merge
> ASAP..
> 
> The rest also doesn't look bad, so I'll happily defer this series.
> OTOH the first on really should go in, either your or my version :)

Well, I'll send the set for a formal review... :)

--D