[PATCH v3 00/11] xfs: refactor incore inode walking

[PATCH v3 00/11] xfs: refactor incore inode walking

[Darrick J. Wong]

Hi all,

This series prepares the incore inode walking code used by the
eofblocks/cowblocks scanner to handle deferred inode inactivation.
First we clean up the eofblocks/cowblocks incore inode walking code to
get rid of some of the warts left by reflink development.  Next, we
rip out the many trivial wrapper functions that don't add much value.
Finally, we refactor the various helpers and predicate functions to
reduce open-coded logic.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=incore-inode-walk

[PATCH 01/11] xfs: move eofblocks conversion function to xfs_ioctl.c

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Move xfs_fs_eofblocks_from_user into the only file that actually uses
it, so that we don't have this function cluttering up the header file.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.h |   35 -----------------------------------
 fs/xfs/xfs_ioctl.c  |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 35 deletions(-)


diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 48f1fd2bb6ad..c13bc8a3e02f 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -81,41 +81,6 @@ 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(
-	struct xfs_fs_eofblocks		*src,
-	struct xfs_eofblocks		*dst)
-{
-	if (src->eof_version != XFS_EOFBLOCKS_VERSION)
-		return -EINVAL;
-
-	if (src->eof_flags & ~XFS_EOF_FLAGS_VALID)
-		return -EINVAL;
-
-	if (memchr_inv(&src->pad32, 0, sizeof(src->pad32)) ||
-	    memchr_inv(src->pad64, 0, sizeof(src->pad64)))
-		return -EINVAL;
-
-	dst->eof_flags = src->eof_flags;
-	dst->eof_prid = src->eof_prid;
-	dst->eof_min_file_size = src->eof_min_file_size;
-
-	dst->eof_uid = INVALID_UID;
-	if (src->eof_flags & XFS_EOF_FLAGS_UID) {
-		dst->eof_uid = make_kuid(current_user_ns(), src->eof_uid);
-		if (!uid_valid(dst->eof_uid))
-			return -EINVAL;
-	}
-
-	dst->eof_gid = INVALID_GID;
-	if (src->eof_flags & XFS_EOF_FLAGS_GID) {
-		dst->eof_gid = make_kgid(current_user_ns(), src->eof_gid);
-		if (!gid_valid(dst->eof_gid))
-			return -EINVAL;
-	}
-	return 0;
-}
-
 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_ioctl.c b/fs/xfs/xfs_ioctl.c
index 309958186d33..6a3c675a8aeb 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -2082,6 +2082,41 @@ xfs_ioc_setlabel(
 	return error;
 }
 
+static inline int
+xfs_fs_eofblocks_from_user(
+	struct xfs_fs_eofblocks		*src,
+	struct xfs_eofblocks		*dst)
+{
+	if (src->eof_version != XFS_EOFBLOCKS_VERSION)
+		return -EINVAL;
+
+	if (src->eof_flags & ~XFS_EOF_FLAGS_VALID)
+		return -EINVAL;
+
+	if (memchr_inv(&src->pad32, 0, sizeof(src->pad32)) ||
+	    memchr_inv(src->pad64, 0, sizeof(src->pad64)))
+		return -EINVAL;
+
+	dst->eof_flags = src->eof_flags;
+	dst->eof_prid = src->eof_prid;
+	dst->eof_min_file_size = src->eof_min_file_size;
+
+	dst->eof_uid = INVALID_UID;
+	if (src->eof_flags & XFS_EOF_FLAGS_UID) {
+		dst->eof_uid = make_kuid(current_user_ns(), src->eof_uid);
+		if (!uid_valid(dst->eof_uid))
+			return -EINVAL;
+	}
+
+	dst->eof_gid = INVALID_GID;
+	if (src->eof_flags & XFS_EOF_FLAGS_GID) {
+		dst->eof_gid = make_kgid(current_user_ns(), src->eof_gid);
+		if (!gid_valid(dst->eof_gid))
+			return -EINVAL;
+	}
+	return 0;
+}
+
 /*
  * Note: some of the ioctl's return positive numbers as a
  * byte count indicating success, such as readlink_by_handle.

Re: [PATCH 01/11] xfs: move eofblocks conversion function to xfs_ioctl.c

[Christoph Hellwig]

On Tue, May 19, 2020 at 06:45:24PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move xfs_fs_eofblocks_from_user into the only file that actually uses
> it, so that we don't have this function cluttering up the header file.
> 

Thanks, the strange inline function really irked me when looking over the
code:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 02/11] xfs: replace open-coded XFS_ICI_NO_TAG

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Use XFS_ICI_NO_TAG instead of -1 when appropriate.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 0ed904c2aa12..b1e2541810be 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -815,7 +815,7 @@ xfs_inode_ag_walk(
 
 		rcu_read_lock();
 
-		if (tag == -1)
+		if (tag == XFS_ICI_NO_TAG)
 			nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
 					(void **)batch, first_index,
 					XFS_LOOKUP_BATCH);
@@ -973,8 +973,8 @@ xfs_inode_ag_iterator_flags(
 	ag = 0;
 	while ((pag = xfs_perag_get(mp, ag))) {
 		ag = pag->pag_agno + 1;
-		error = xfs_inode_ag_walk(mp, pag, execute, flags, args, -1,
-					  iter_flags);
+		error = xfs_inode_ag_walk(mp, pag, execute, flags, args,
+				XFS_ICI_NO_TAG, iter_flags);
 		xfs_perag_put(pag);
 		if (error) {
 			last_error = error;

Re: [PATCH 02/11] xfs: replace open-coded XFS_ICI_NO_TAG

[Christoph Hellwig]

On Tue, May 19, 2020 at 06:45:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use XFS_ICI_NO_TAG instead of -1 when appropriate.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 03/11] xfs: remove unused xfs_inode_ag_iterator function

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Not used by anyone, so get rid of it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c |   11 -----------
 fs/xfs/xfs_icache.h |    3 ---
 2 files changed, 14 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index b1e2541810be..6aafb547f21a 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -985,17 +985,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 c13bc8a3e02f..0556fa32074f 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -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);

Re: [PATCH 03/11] xfs: remove unused xfs_inode_ag_iterator function

[Christoph Hellwig]

On Tue, May 19, 2020 at 06:45:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Not used by anyone, so get rid of it.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 04/11] xfs: remove xfs_inode_ag_iterator_flags

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Combine xfs_inode_ag_iterator_flags and xfs_inode_ag_iterator_tag into a
single wrapper function since there's only one caller of the _flags
variant.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c      |   43 +++++++++++++------------------------------
 fs/xfs/xfs_icache.h      |    5 +----
 fs/xfs/xfs_qm_syscalls.c |    4 ++--
 3 files changed, 16 insertions(+), 36 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 6aafb547f21a..e716b19879c6 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -956,38 +956,22 @@ xfs_cowblocks_worker(
 	xfs_queue_cowblocks(mp);
 }
 
-int
-xfs_inode_ag_iterator_flags(
+/* Fetch the next (possibly tagged) per-AG structure. */
+static inline struct xfs_perag *
+xfs_ici_walk_get_perag(
 	struct xfs_mount	*mp,
-	int			(*execute)(struct xfs_inode *ip, int flags,
-					   void *args),
-	int			flags,
-	void			*args,
-	int			iter_flags)
+	xfs_agnumber_t		agno,
+	int			tag)
 {
-	struct xfs_perag	*pag;
-	int			error = 0;
-	int			last_error = 0;
-	xfs_agnumber_t		ag;
-
-	ag = 0;
-	while ((pag = xfs_perag_get(mp, ag))) {
-		ag = pag->pag_agno + 1;
-		error = xfs_inode_ag_walk(mp, pag, execute, flags, args,
-				XFS_ICI_NO_TAG, iter_flags);
-		xfs_perag_put(pag);
-		if (error) {
-			last_error = error;
-			if (error == -EFSCORRUPTED)
-				break;
-		}
-	}
-	return last_error;
+	if (tag == XFS_ICI_NO_TAG)
+		return xfs_perag_get(mp, agno);
+	return xfs_perag_get_tag(mp, agno, tag);
 }
 
 int
-xfs_inode_ag_iterator_tag(
+xfs_inode_ag_iterator(
 	struct xfs_mount	*mp,
+	int			iter_flags,
 	int			(*execute)(struct xfs_inode *ip, int flags,
 					   void *args),
 	int			flags,
@@ -1000,10 +984,10 @@ xfs_inode_ag_iterator_tag(
 	xfs_agnumber_t		ag;
 
 	ag = 0;
-	while ((pag = xfs_perag_get_tag(mp, ag, tag))) {
+	while ((pag = xfs_ici_walk_get_perag(mp, ag, tag))) {
 		ag = pag->pag_agno + 1;
 		error = xfs_inode_ag_walk(mp, pag, execute, flags, args, tag,
-					  0);
+				iter_flags);
 		xfs_perag_put(pag);
 		if (error) {
 			last_error = error;
@@ -1523,8 +1507,7 @@ __xfs_icache_free_eofblocks(
 	if (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC))
 		flags = SYNC_WAIT;
 
-	return xfs_inode_ag_iterator_tag(mp, execute, flags,
-					 eofb, tag);
+	return xfs_inode_ag_iterator(mp, 0, execute, flags, eofb, tag);
 }
 
 int
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 0556fa32074f..2d5ab9957d9f 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -71,10 +71,7 @@ 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_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 xfs_inode_ag_iterator(struct xfs_mount *mp, int iter_flags,
 	int (*execute)(struct xfs_inode *ip, int flags, void *args),
 	int flags, void *args, int tag);
 
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 5d5ac65aa1cc..a9460bdcca87 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -772,6 +772,6 @@ xfs_qm_dqrele_all_inodes(
 	uint		 flags)
 {
 	ASSERT(mp->m_quotainfo);
-	xfs_inode_ag_iterator_flags(mp, xfs_dqrele_inode, flags, NULL,
-				    XFS_AGITER_INEW_WAIT);
+	xfs_inode_ag_iterator(mp, XFS_AGITER_INEW_WAIT, xfs_dqrele_inode,
+			flags, NULL, XFS_ICI_NO_TAG);
 }

Re: [PATCH 04/11] xfs: remove xfs_inode_ag_iterator_flags

[Christoph Hellwig]

On Tue, May 19, 2020 at 06:45:43PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Combine xfs_inode_ag_iterator_flags and xfs_inode_ag_iterator_tag into a
> single wrapper function since there's only one caller of the _flags
> variant.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 05/11] xfs: remove flags argument from xfs_inode_ag_walk

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The incore inode walk code passes a flags argument and a pointer from
the xfs_inode_ag_iterator caller all the way to the iteration function.
We can reduce the function complexity by passing flags through the
private pointer.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c      |   38 ++++++++++++++------------------------
 fs/xfs/xfs_icache.h      |    4 ++--
 fs/xfs/xfs_qm_syscalls.c |   25 +++++++++++++++++--------
 3 files changed, 33 insertions(+), 34 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index e716b19879c6..87b98bfdf27d 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -790,9 +790,7 @@ STATIC int
 xfs_inode_ag_walk(
 	struct xfs_mount	*mp,
 	struct xfs_perag	*pag,
-	int			(*execute)(struct xfs_inode *ip, int flags,
-					   void *args),
-	int			flags,
+	int			(*execute)(struct xfs_inode *ip, void *args),
 	void			*args,
 	int			tag,
 	int			iter_flags)
@@ -868,7 +866,7 @@ xfs_inode_ag_walk(
 			if ((iter_flags & XFS_AGITER_INEW_WAIT) &&
 			    xfs_iflags_test(batch[i], XFS_INEW))
 				xfs_inew_wait(batch[i]);
-			error = execute(batch[i], flags, args);
+			error = execute(batch[i], args);
 			xfs_irele(batch[i]);
 			if (error == -EAGAIN) {
 				skipped++;
@@ -972,9 +970,7 @@ int
 xfs_inode_ag_iterator(
 	struct xfs_mount	*mp,
 	int			iter_flags,
-	int			(*execute)(struct xfs_inode *ip, int flags,
-					   void *args),
-	int			flags,
+	int			(*execute)(struct xfs_inode *ip, void *args),
 	void			*args,
 	int			tag)
 {
@@ -986,7 +982,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, flags, args, tag,
+		error = xfs_inode_ag_walk(mp, pag, execute, args, tag,
 				iter_flags);
 		xfs_perag_put(pag);
 		if (error) {
@@ -1443,12 +1439,14 @@ xfs_inode_match_id_union(
 STATIC int
 xfs_inode_free_eofblocks(
 	struct xfs_inode	*ip,
-	int			flags,
 	void			*args)
 {
-	int ret = 0;
-	struct xfs_eofblocks *eofb = args;
-	int match;
+	struct xfs_eofblocks	*eofb = args;
+	bool			wait;
+	int			match;
+	int			ret = 0;
+
+	wait = (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC));
 
 	if (!xfs_can_free_eofblocks(ip, false)) {
 		/* inode could be preallocated or append-only */
@@ -1461,8 +1459,7 @@ xfs_inode_free_eofblocks(
 	 * If the mapping is dirty the operation can block and wait for some
 	 * time. Unless we are waiting, skip it.
 	 */
-	if (!(flags & SYNC_WAIT) &&
-	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))
+	if (!wait && mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))
 		return 0;
 
 	if (eofb) {
@@ -1484,7 +1481,7 @@ xfs_inode_free_eofblocks(
 	 * scanner moving and revisit the inode in a subsequent pass.
 	 */
 	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
-		if (flags & SYNC_WAIT)
+		if (wait)
 			ret = -EAGAIN;
 		return ret;
 	}
@@ -1498,16 +1495,10 @@ static int
 __xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
 	struct xfs_eofblocks	*eofb,
-	int			(*execute)(struct xfs_inode *ip, int flags,
-					   void *args),
+	int			(*execute)(struct xfs_inode *ip, void *args),
 	int			tag)
 {
-	int flags = SYNC_TRYLOCK;
-
-	if (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC))
-		flags = SYNC_WAIT;
-
-	return xfs_inode_ag_iterator(mp, 0, execute, flags, eofb, tag);
+	return xfs_inode_ag_iterator(mp, 0, execute, eofb, tag);
 }
 
 int
@@ -1732,7 +1723,6 @@ xfs_prep_free_cowblocks(
 STATIC int
 xfs_inode_free_cowblocks(
 	struct xfs_inode	*ip,
-	int			flags,
 	void			*args)
 {
 	struct xfs_eofblocks	*eofb = args;
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 2d5ab9957d9f..e7f86ebd7b22 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -72,8 +72,8 @@ 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 (*execute)(struct xfs_inode *ip, int flags, void *args),
-	int flags, void *args, int tag);
+	int (*execute)(struct xfs_inode *ip, void *args),
+	void *args, int tag);
 
 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 a9460bdcca87..571ecb17b3bf 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -726,12 +726,17 @@ xfs_qm_scall_getquota_next(
 	return error;
 }
 
+struct xfs_dqrele {
+	uint		flags;
+};
+
 STATIC int
 xfs_dqrele_inode(
 	struct xfs_inode	*ip,
-	int			flags,
 	void			*args)
 {
+	struct xfs_dqrele	*dqr = args;
+
 	/* skip quota inodes */
 	if (ip == ip->i_mount->m_quotainfo->qi_uquotaip ||
 	    ip == ip->i_mount->m_quotainfo->qi_gquotaip ||
@@ -743,15 +748,15 @@ xfs_dqrele_inode(
 	}
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
+	if ((dqr->flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
 		xfs_qm_dqrele(ip->i_udquot);
 		ip->i_udquot = NULL;
 	}
-	if ((flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) {
+	if ((dqr->flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) {
 		xfs_qm_dqrele(ip->i_gdquot);
 		ip->i_gdquot = NULL;
 	}
-	if ((flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
+	if ((dqr->flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) {
 		xfs_qm_dqrele(ip->i_pdquot);
 		ip->i_pdquot = NULL;
 	}
@@ -768,10 +773,14 @@ xfs_dqrele_inode(
  */
 void
 xfs_qm_dqrele_all_inodes(
-	struct xfs_mount *mp,
-	uint		 flags)
+	struct xfs_mount	*mp,
+	uint			flags)
 {
+	struct xfs_dqrele	dqr = {
+		.flags		= flags,
+	};
+
 	ASSERT(mp->m_quotainfo);
-	xfs_inode_ag_iterator(mp, XFS_AGITER_INEW_WAIT, xfs_dqrele_inode,
-			flags, NULL, XFS_ICI_NO_TAG);
+	xfs_inode_ag_iterator(mp, XFS_AGITER_INEW_WAIT, xfs_dqrele_inode, &dqr,
+			XFS_ICI_NO_TAG);
 }

Re: [PATCH 05/11] xfs: remove flags argument from xfs_inode_ag_walk

[Christoph Hellwig]

On Tue, May 19, 2020 at 06:45:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The incore inode walk code passes a flags argument and a pointer from
> the xfs_inode_ag_iterator caller all the way to the iteration function.
> We can reduce the function complexity by passing flags through the
> private pointer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_icache.c      |   38 ++++++++++++++------------------------
>  fs/xfs/xfs_icache.h      |    4 ++--
>  fs/xfs/xfs_qm_syscalls.c |   25 +++++++++++++++++--------
>  3 files changed, 33 insertions(+), 34 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index e716b19879c6..87b98bfdf27d 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -790,9 +790,7 @@ STATIC int
>  xfs_inode_ag_walk(
>  	struct xfs_mount	*mp,
>  	struct xfs_perag	*pag,
> -	int			(*execute)(struct xfs_inode *ip, int flags,
> -					   void *args),
> -	int			flags,
> +	int			(*execute)(struct xfs_inode *ip, void *args),
>  	void			*args,
>  	int			tag,
>  	int			iter_flags)
> @@ -868,7 +866,7 @@ xfs_inode_ag_walk(
>  			if ((iter_flags & XFS_AGITER_INEW_WAIT) &&
>  			    xfs_iflags_test(batch[i], XFS_INEW))
>  				xfs_inew_wait(batch[i]);
> -			error = execute(batch[i], flags, args);
> +			error = execute(batch[i], args);
>  			xfs_irele(batch[i]);
>  			if (error == -EAGAIN) {
>  				skipped++;
> @@ -972,9 +970,7 @@ int
>  xfs_inode_ag_iterator(
>  	struct xfs_mount	*mp,
>  	int			iter_flags,
> -	int			(*execute)(struct xfs_inode *ip, int flags,
> -					   void *args),
> -	int			flags,
> +	int			(*execute)(struct xfs_inode *ip, void *args),
>  	void			*args,
>  	int			tag)
>  {
> @@ -986,7 +982,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, flags, args, tag,
> +		error = xfs_inode_ag_walk(mp, pag, execute, args, tag,
>  				iter_flags);
>  		xfs_perag_put(pag);
>  		if (error) {
> @@ -1443,12 +1439,14 @@ xfs_inode_match_id_union(
>  STATIC int
>  xfs_inode_free_eofblocks(
>  	struct xfs_inode	*ip,
> -	int			flags,
>  	void			*args)
>  {
> -	int ret = 0;
> -	struct xfs_eofblocks *eofb = args;
> -	int match;
> +	struct xfs_eofblocks	*eofb = args;
> +	bool			wait;
> +	int			match;
> +	int			ret = 0;
> +
> +	wait = (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC));

No need for the outer braces.

> @@ -1484,7 +1481,7 @@ xfs_inode_free_eofblocks(
>  	 * scanner moving and revisit the inode in a subsequent pass.
>  	 */
>  	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
> -		if (flags & SYNC_WAIT)
> +		if (wait)
>  			ret = -EAGAIN;
>  		return ret;

Just me, but I'd prefer an explicit:

		if (wait)
			return -EAGAIN;
		return 0;

here.  Not really new in this patch, but if you touch this area anyway..

> index a9460bdcca87..571ecb17b3bf 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -726,12 +726,17 @@ xfs_qm_scall_getquota_next(
>  	return error;
>  }
>  
> +struct xfs_dqrele {
> +	uint		flags;
> +};

> +	struct xfs_dqrele	dqr = {
> +		.flags		= flags,
> +	};

Instead of a new structure we could just take the address of flags and
pass that to simplify the code a bit.

Re: [PATCH 05/11] xfs: remove flags argument from xfs_inode_ag_walk

[Darrick J. Wong]

On Wed, May 20, 2020 at 08:38:25AM +0200, Christoph Hellwig wrote:
> On Tue, May 19, 2020 at 06:45:49PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The incore inode walk code passes a flags argument and a pointer from
> > the xfs_inode_ag_iterator caller all the way to the iteration function.
> > We can reduce the function complexity by passing flags through the
> > private pointer.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_icache.c      |   38 ++++++++++++++------------------------
> >  fs/xfs/xfs_icache.h      |    4 ++--
> >  fs/xfs/xfs_qm_syscalls.c |   25 +++++++++++++++++--------
> >  3 files changed, 33 insertions(+), 34 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index e716b19879c6..87b98bfdf27d 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -790,9 +790,7 @@ STATIC int
> >  xfs_inode_ag_walk(
> >  	struct xfs_mount	*mp,
> >  	struct xfs_perag	*pag,
> > -	int			(*execute)(struct xfs_inode *ip, int flags,
> > -					   void *args),
> > -	int			flags,
> > +	int			(*execute)(struct xfs_inode *ip, void *args),
> >  	void			*args,
> >  	int			tag,
> >  	int			iter_flags)
> > @@ -868,7 +866,7 @@ xfs_inode_ag_walk(
> >  			if ((iter_flags & XFS_AGITER_INEW_WAIT) &&
> >  			    xfs_iflags_test(batch[i], XFS_INEW))
> >  				xfs_inew_wait(batch[i]);
> > -			error = execute(batch[i], flags, args);
> > +			error = execute(batch[i], args);
> >  			xfs_irele(batch[i]);
> >  			if (error == -EAGAIN) {
> >  				skipped++;
> > @@ -972,9 +970,7 @@ int
> >  xfs_inode_ag_iterator(
> >  	struct xfs_mount	*mp,
> >  	int			iter_flags,
> > -	int			(*execute)(struct xfs_inode *ip, int flags,
> > -					   void *args),
> > -	int			flags,
> > +	int			(*execute)(struct xfs_inode *ip, void *args),
> >  	void			*args,
> >  	int			tag)
> >  {
> > @@ -986,7 +982,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, flags, args, tag,
> > +		error = xfs_inode_ag_walk(mp, pag, execute, args, tag,
> >  				iter_flags);
> >  		xfs_perag_put(pag);
> >  		if (error) {
> > @@ -1443,12 +1439,14 @@ xfs_inode_match_id_union(
> >  STATIC int
> >  xfs_inode_free_eofblocks(
> >  	struct xfs_inode	*ip,
> > -	int			flags,
> >  	void			*args)
> >  {
> > -	int ret = 0;
> > -	struct xfs_eofblocks *eofb = args;
> > -	int match;
> > +	struct xfs_eofblocks	*eofb = args;
> > +	bool			wait;
> > +	int			match;
> > +	int			ret = 0;
> > +
> > +	wait = (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC));
> 
> No need for the outer braces.

Fixed.

> > @@ -1484,7 +1481,7 @@ xfs_inode_free_eofblocks(
> >  	 * scanner moving and revisit the inode in a subsequent pass.
> >  	 */
> >  	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
> > -		if (flags & SYNC_WAIT)
> > +		if (wait)
> >  			ret = -EAGAIN;
> >  		return ret;
> 
> Just me, but I'd prefer an explicit:
> 
> 		if (wait)
> 			return -EAGAIN;
> 		return 0;
> 
> here.  Not really new in this patch, but if you touch this area anyway..

How about 'return wait ? -EAGAIN : 0;' ?

> > index a9460bdcca87..571ecb17b3bf 100644
> > --- a/fs/xfs/xfs_qm_syscalls.c
> > +++ b/fs/xfs/xfs_qm_syscalls.c
> > @@ -726,12 +726,17 @@ xfs_qm_scall_getquota_next(
> >  	return error;
> >  }
> >  
> > +struct xfs_dqrele {
> > +	uint		flags;
> > +};
> 
> > +	struct xfs_dqrele	dqr = {
> > +		.flags		= flags,
> > +	};
> 
> Instead of a new structure we could just take the address of flags and
> pass that to simplify the code a bit.

Fixed.

--D

Re: [PATCH 05/11] xfs: remove flags argument from xfs_inode_ag_walk

[Christoph Hellwig]

On Wed, May 20, 2020 at 11:36:27AM -0700, Darrick J. Wong wrote:
> > > -		if (flags & SYNC_WAIT)
> > > +		if (wait)
> > >  			ret = -EAGAIN;
> > >  		return ret;
> > 
> > Just me, but I'd prefer an explicit:
> > 
> > 		if (wait)
> > 			return -EAGAIN;
> > 		return 0;
> > 
> > here.  Not really new in this patch, but if you touch this area anyway..
> 
> How about 'return wait ? -EAGAIN : 0;' ?

Yikes.  Why does everyone hate the nice, explicit and readable if
statements?

[PATCH 06/11] xfs: remove __xfs_icache_free_eofblocks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

This is now a pointless wrapper, so kill it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c |   14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 87b98bfdf27d..ac66e7d8698d 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1491,22 +1491,12 @@ xfs_inode_free_eofblocks(
 	return ret;
 }
 
-static int
-__xfs_icache_free_eofblocks(
-	struct xfs_mount	*mp,
-	struct xfs_eofblocks	*eofb,
-	int			(*execute)(struct xfs_inode *ip, void *args),
-	int			tag)
-{
-	return xfs_inode_ag_iterator(mp, 0, execute, 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,
+	return xfs_inode_ag_iterator(mp, 0, xfs_inode_free_eofblocks, eofb,
 			XFS_ICI_EOFBLOCKS_TAG);
 }
 
@@ -1768,7 +1758,7 @@ xfs_icache_free_cowblocks(
 	struct xfs_mount	*mp,
 	struct xfs_eofblocks	*eofb)
 {
-	return __xfs_icache_free_eofblocks(mp, eofb, xfs_inode_free_cowblocks,
+	return xfs_inode_ag_iterator(mp, 0, xfs_inode_free_cowblocks, eofb,
 			XFS_ICI_COWBLOCKS_TAG);
 }
 

Re: [PATCH 06/11] xfs: remove __xfs_icache_free_eofblocks

[Christoph Hellwig]

On Tue, May 19, 2020 at 06:45:55PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> This is now a pointless wrapper, so kill it.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 07/11] xfs: refactor eofb matching into a single helper

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the two eofb-matching logics into a single helper so that we
don't repeat ourselves.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c |   59 +++++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index ac66e7d8698d..1f12c6a0c48e 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1436,6 +1436,33 @@ xfs_inode_match_id_union(
 	return 0;
 }
 
+/*
+ * Is this inode @ip eligible for eof/cow block reclamation, given some
+ * filtering parameters @eofb?  The inode is eligible if @eofb is null or
+ * if the predicate functions match.
+ */
+static bool
+xfs_inode_matches_eofb(
+	struct xfs_inode	*ip,
+	struct xfs_eofblocks	*eofb)
+{
+	int			match;
+
+	if (!eofb)
+		return true;
+
+	if (eofb->eof_flags & XFS_EOF_FLAGS_UNION)
+		match = xfs_inode_match_id_union(ip, eofb);
+	else
+		match = xfs_inode_match_id(ip, eofb);
+	if (match)
+		return true;
+
+	/* skip the inode if the file size is too small */
+	return !(eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE &&
+		 XFS_ISIZE(ip) < eofb->eof_min_file_size);
+}
+
 STATIC int
 xfs_inode_free_eofblocks(
 	struct xfs_inode	*ip,
@@ -1443,7 +1470,6 @@ xfs_inode_free_eofblocks(
 {
 	struct xfs_eofblocks	*eofb = args;
 	bool			wait;
-	int			match;
 	int			ret = 0;
 
 	wait = (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC));
@@ -1462,19 +1488,8 @@ xfs_inode_free_eofblocks(
 	if (!wait && mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))
 		return 0;
 
-	if (eofb) {
-		if (eofb->eof_flags & XFS_EOF_FLAGS_UNION)
-			match = xfs_inode_match_id_union(ip, eofb);
-		else
-			match = xfs_inode_match_id(ip, eofb);
-		if (!match)
-			return 0;
-
-		/* skip the inode if the file size is too small */
-		if (eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE &&
-		    XFS_ISIZE(ip) < eofb->eof_min_file_size)
-			return 0;
-	}
+	if (!xfs_inode_matches_eofb(ip, eofb))
+		return 0;
 
 	/*
 	 * If the caller is waiting, return -EAGAIN to keep the background
@@ -1716,25 +1731,13 @@ xfs_inode_free_cowblocks(
 	void			*args)
 {
 	struct xfs_eofblocks	*eofb = args;
-	int			match;
 	int			ret = 0;
 
 	if (!xfs_prep_free_cowblocks(ip))
 		return 0;
 
-	if (eofb) {
-		if (eofb->eof_flags & XFS_EOF_FLAGS_UNION)
-			match = xfs_inode_match_id_union(ip, eofb);
-		else
-			match = xfs_inode_match_id(ip, eofb);
-		if (!match)
-			return 0;
-
-		/* skip the inode if the file size is too small */
-		if (eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE &&
-		    XFS_ISIZE(ip) < eofb->eof_min_file_size)
-			return 0;
-	}
+	if (!xfs_inode_matches_eofb(ip, eofb))
+		return 0;
 
 	/* Free the CoW blocks */
 	xfs_ilock(ip, XFS_IOLOCK_EXCL);

Re: [PATCH 07/11] xfs: refactor eofb matching into a single helper

[Christoph Hellwig]

On Tue, May 19, 2020 at 06:46:02PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor the two eofb-matching logics into a single helper so that we
> don't repeat ourselves.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_icache.c |   59 +++++++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index ac66e7d8698d..1f12c6a0c48e 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1436,6 +1436,33 @@ xfs_inode_match_id_union(
>  	return 0;
>  }
>  
> +/*
> + * Is this inode @ip eligible for eof/cow block reclamation, given some
> + * filtering parameters @eofb?  The inode is eligible if @eofb is null or
> + * if the predicate functions match.
> + */
> +static bool
> +xfs_inode_matches_eofb(
> +	struct xfs_inode	*ip,
> +	struct xfs_eofblocks	*eofb)
> +{
> +	int			match;
> +
> +	if (!eofb)
> +		return true;
> +
> +	if (eofb->eof_flags & XFS_EOF_FLAGS_UNION)
> +		match = xfs_inode_match_id_union(ip, eofb);
> +	else
> +		match = xfs_inode_match_id(ip, eofb);
> +	if (match)
> +		return true;
> +
> +	/* skip the inode if the file size is too small */
> +	return !(eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE &&
> +		 XFS_ISIZE(ip) < eofb->eof_min_file_size);

This looks wrong - the size check should be applied if we did already
find a match and not override it based on the current code, e.g.:

	if (eofb->eof_flags & XFS_EOF_FLAGS_UNION)
		match = xfs_inode_match_id_union(ip, eofb);
	else
		match = xfs_inode_match_id(ip, eofb);

	if (match) {
		/* skip the inode if the file size is too small */
		if ((eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE) &&
		    XFS_ISIZE(ip) < eofb->eof_min_file_size)
			return false;
	}

	return match;

Re: [PATCH 07/11] xfs: refactor eofb matching into a single helper

[Darrick J. Wong]

On Wed, May 20, 2020 at 08:42:11AM +0200, Christoph Hellwig wrote:
> On Tue, May 19, 2020 at 06:46:02PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Refactor the two eofb-matching logics into a single helper so that we
> > don't repeat ourselves.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_icache.c |   59 +++++++++++++++++++++++++++------------------------
> >  1 file changed, 31 insertions(+), 28 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index ac66e7d8698d..1f12c6a0c48e 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -1436,6 +1436,33 @@ xfs_inode_match_id_union(
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Is this inode @ip eligible for eof/cow block reclamation, given some
> > + * filtering parameters @eofb?  The inode is eligible if @eofb is null or
> > + * if the predicate functions match.
> > + */
> > +static bool
> > +xfs_inode_matches_eofb(
> > +	struct xfs_inode	*ip,
> > +	struct xfs_eofblocks	*eofb)
> > +{
> > +	int			match;
> > +
> > +	if (!eofb)
> > +		return true;
> > +
> > +	if (eofb->eof_flags & XFS_EOF_FLAGS_UNION)
> > +		match = xfs_inode_match_id_union(ip, eofb);
> > +	else
> > +		match = xfs_inode_match_id(ip, eofb);
> > +	if (match)
> > +		return true;
> > +
> > +	/* skip the inode if the file size is too small */
> > +	return !(eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE &&
> > +		 XFS_ISIZE(ip) < eofb->eof_min_file_size);
> 
> This looks wrong - the size check should be applied if we did already
> find a match and not override it based on the current code, e.g.:
> 
> 	if (eofb->eof_flags & XFS_EOF_FLAGS_UNION)
> 		match = xfs_inode_match_id_union(ip, eofb);
> 	else
> 		match = xfs_inode_match_id(ip, eofb);
> 
> 	if (match) {
> 		/* skip the inode if the file size is too small */
> 		if ((eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE) &&
> 		    XFS_ISIZE(ip) < eofb->eof_min_file_size)
> 			return false;
> 	}
> 
> 	return match;

Ah, I see what I did wrong here; the size check was another opportunity
for us to say no even if the inode matched.  Ok, I'll go fix it.

--D

[PATCH 08/11] xfs: fix inode ag walk predicate function return values

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

There are a number of predicate functions that help the incore inode
walking code decide if we really want to apply the iteration function to
the inode.  These are boolean decisions, so change the return types to
boolean to match.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c |   39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 1f12c6a0c48e..926927a5f42e 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -741,7 +741,12 @@ xfs_icache_inode_is_allocated(
  */
 #define XFS_LOOKUP_BATCH	32
 
-STATIC int
+/*
+ * Decide if the given @ip is eligible to be a part of the inode walk, and
+ * grab it if so.  Returns true if it's ready to go or false if we should just
+ * ignore it.
+ */
+STATIC bool
 xfs_inode_ag_walk_grab(
 	struct xfs_inode	*ip,
 	int			flags)
@@ -772,18 +777,18 @@ xfs_inode_ag_walk_grab(
 
 	/* nothing to sync during shutdown */
 	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
-		return -EFSCORRUPTED;
+		return false;
 
 	/* If we can't grab the inode, it must on it's way to reclaim. */
 	if (!igrab(inode))
-		return -ENOENT;
+		return false;
 
 	/* inode is valid */
-	return 0;
+	return true;
 
 out_unlock_noent:
 	spin_unlock(&ip->i_flags_lock);
-	return -ENOENT;
+	return false;
 }
 
 STATIC int
@@ -835,7 +840,7 @@ xfs_inode_ag_walk(
 		for (i = 0; i < nr_found; i++) {
 			struct xfs_inode *ip = batch[i];
 
-			if (done || xfs_inode_ag_walk_grab(ip, iter_flags))
+			if (done || !xfs_inode_ag_walk_grab(ip, iter_flags))
 				batch[i] = NULL;
 
 			/*
@@ -1392,48 +1397,48 @@ xfs_reclaim_inodes_count(
 	return reclaimable;
 }
 
-STATIC int
+STATIC bool
 xfs_inode_match_id(
 	struct xfs_inode	*ip,
 	struct xfs_eofblocks	*eofb)
 {
 	if ((eofb->eof_flags & XFS_EOF_FLAGS_UID) &&
 	    !uid_eq(VFS_I(ip)->i_uid, eofb->eof_uid))
-		return 0;
+		return false;
 
 	if ((eofb->eof_flags & XFS_EOF_FLAGS_GID) &&
 	    !gid_eq(VFS_I(ip)->i_gid, eofb->eof_gid))
-		return 0;
+		return false;
 
 	if ((eofb->eof_flags & XFS_EOF_FLAGS_PRID) &&
 	    ip->i_d.di_projid != eofb->eof_prid)
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 /*
  * A union-based inode filtering algorithm. Process the inode if any of the
  * criteria match. This is for global/internal scans only.
  */
-STATIC int
+STATIC bool
 xfs_inode_match_id_union(
 	struct xfs_inode	*ip,
 	struct xfs_eofblocks	*eofb)
 {
 	if ((eofb->eof_flags & XFS_EOF_FLAGS_UID) &&
 	    uid_eq(VFS_I(ip)->i_uid, eofb->eof_uid))
-		return 1;
+		return true;
 
 	if ((eofb->eof_flags & XFS_EOF_FLAGS_GID) &&
 	    gid_eq(VFS_I(ip)->i_gid, eofb->eof_gid))
-		return 1;
+		return true;
 
 	if ((eofb->eof_flags & XFS_EOF_FLAGS_PRID) &&
 	    ip->i_d.di_projid == eofb->eof_prid)
-		return 1;
+		return true;
 
-	return 0;
+	return false;
 }
 
 /*
@@ -1446,7 +1451,7 @@ xfs_inode_matches_eofb(
 	struct xfs_inode	*ip,
 	struct xfs_eofblocks	*eofb)
 {
-	int			match;
+	bool			match;
 
 	if (!eofb)
 		return true;

Re: [PATCH 08/11] xfs: fix inode ag walk predicate function return values

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 09/11] xfs: use bool for done in xfs_inode_ag_walk

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

This is a boolean variable, so use the bool type.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 926927a5f42e..6c8c626e7ef1 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -803,11 +803,11 @@ xfs_inode_ag_walk(
 	uint32_t		first_index;
 	int			last_error = 0;
 	int			skipped;
-	int			done;
+	bool			done;
 	int			nr_found;
 
 restart:
-	done = 0;
+	done = false;
 	skipped = 0;
 	first_index = 0;
 	nr_found = 0;
@@ -859,7 +859,7 @@ xfs_inode_ag_walk(
 				continue;
 			first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
 			if (first_index < XFS_INO_TO_AGINO(mp, ip->i_ino))
-				done = 1;
+				done = true;
 		}
 
 		/* unlock now we've grabbed the inodes. */

Re: [PATCH 09/11] xfs: use bool for done in xfs_inode_ag_walk

[Christoph Hellwig]

On Tue, May 19, 2020 at 06:46:14PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> This is a boolean variable, so use the bool type.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 10/11] xfs: move xfs_inode_ag_iterator to be closer to the perag walking code

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Move the xfs_inode_ag_iterator function to be nearer xfs_inode_ag_walk
so that we don't have to scroll back and forth to figure out how the
incore inode walking function works.  No functional changes.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.c |   88 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 40 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 6c8c626e7ef1..05614758fb1e 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -791,6 +791,10 @@ xfs_inode_ag_walk_grab(
 	return false;
 }
 
+/*
+ * For a given per-AG structure @pag, @grab, @execute, and @rele all incore
+ * inodes with the given radix tree @tag.
+ */
 STATIC int
 xfs_inode_ag_walk(
 	struct xfs_mount	*mp,
@@ -896,6 +900,50 @@ xfs_inode_ag_walk(
 	return last_error;
 }
 
+/* Fetch the next (possibly tagged) per-AG structure. */
+static inline struct xfs_perag *
+xfs_ici_walk_get_perag(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	int			tag)
+{
+	if (tag == XFS_ICI_NO_TAG)
+		return xfs_perag_get(mp, agno);
+	return xfs_perag_get_tag(mp, agno, tag);
+}
+
+/*
+ * Call the @execute function on all incore inodes matching the radix tree
+ * @tag.
+ */
+int
+xfs_inode_ag_iterator(
+	struct xfs_mount	*mp,
+	int			iter_flags,
+	int			(*execute)(struct xfs_inode *ip, void *args),
+	void			*args,
+	int			tag)
+{
+	struct xfs_perag	*pag;
+	int			error = 0;
+	int			last_error = 0;
+	xfs_agnumber_t		ag;
+
+	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,
+				iter_flags);
+		xfs_perag_put(pag);
+		if (error) {
+			last_error = error;
+			if (error == -EFSCORRUPTED)
+				break;
+		}
+	}
+	return last_error;
+}
+
 /*
  * Background scanning to trim post-EOF preallocated space. This is queued
  * based on the 'speculative_prealloc_lifetime' tunable (5m by default).
@@ -959,46 +1007,6 @@ xfs_cowblocks_worker(
 	xfs_queue_cowblocks(mp);
 }
 
-/* Fetch the next (possibly tagged) per-AG structure. */
-static inline struct xfs_perag *
-xfs_ici_walk_get_perag(
-	struct xfs_mount	*mp,
-	xfs_agnumber_t		agno,
-	int			tag)
-{
-	if (tag == XFS_ICI_NO_TAG)
-		return xfs_perag_get(mp, agno);
-	return xfs_perag_get_tag(mp, agno, tag);
-}
-
-int
-xfs_inode_ag_iterator(
-	struct xfs_mount	*mp,
-	int			iter_flags,
-	int			(*execute)(struct xfs_inode *ip, void *args),
-	void			*args,
-	int			tag)
-{
-	struct xfs_perag	*pag;
-	int			error = 0;
-	int			last_error = 0;
-	xfs_agnumber_t		ag;
-
-	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,
-				iter_flags);
-		xfs_perag_put(pag);
-		if (error) {
-			last_error = error;
-			if (error == -EFSCORRUPTED)
-				break;
-		}
-	}
-	return last_error;
-}
-
 /*
  * Grab the inode for reclaim exclusively.
  * Return 0 if we grabbed it, non-zero otherwise.

Re: [PATCH 10/11] xfs: move xfs_inode_ag_iterator to be closer to the perag walking code

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Darrick J. Wong]

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(-)


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);
 }

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

[Christoph Hellwig]

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.

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

[Darrick J. Wong]

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

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

[Christoph Hellwig]

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?

[PATCH v2 00/11] xfs: refactor incore inode walking

[Darrick J. Wong]

Hi all,

This series prepares the incore inode walking code used by the
eofblocks/cowblocks scanner to handle deferred inode inactivation.
First we clean up the eofblocks/cowblocks incore inode walking code to
get rid of some of the warts left by reflink development.  Next, we
rip out the many trivial wrapper functions that don't add much value.
Finally, we refactor the various helpers and predicate functions to
reduce open-coded logic.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=incore-inode-walk

[PATCH 01/11] xfs: move eofblocks conversion function to xfs_ioctl.c

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Move xfs_fs_eofblocks_from_user into the only file that actually uses
it, so that we don't have this function cluttering up the header file.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_icache.h |   35 -----------------------------------
 fs/xfs/xfs_ioctl.c  |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 35 deletions(-)


diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index 48f1fd2bb6ad..c13bc8a3e02f 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -81,41 +81,6 @@ 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(
-	struct xfs_fs_eofblocks		*src,
-	struct xfs_eofblocks		*dst)
-{
-	if (src->eof_version != XFS_EOFBLOCKS_VERSION)
-		return -EINVAL;
-
-	if (src->eof_flags & ~XFS_EOF_FLAGS_VALID)
-		return -EINVAL;
-
-	if (memchr_inv(&src->pad32, 0, sizeof(src->pad32)) ||
-	    memchr_inv(src->pad64, 0, sizeof(src->pad64)))
-		return -EINVAL;
-
-	dst->eof_flags = src->eof_flags;
-	dst->eof_prid = src->eof_prid;
-	dst->eof_min_file_size = src->eof_min_file_size;
-
-	dst->eof_uid = INVALID_UID;
-	if (src->eof_flags & XFS_EOF_FLAGS_UID) {
-		dst->eof_uid = make_kuid(current_user_ns(), src->eof_uid);
-		if (!uid_valid(dst->eof_uid))
-			return -EINVAL;
-	}
-
-	dst->eof_gid = INVALID_GID;
-	if (src->eof_flags & XFS_EOF_FLAGS_GID) {
-		dst->eof_gid = make_kgid(current_user_ns(), src->eof_gid);
-		if (!gid_valid(dst->eof_gid))
-			return -EINVAL;
-	}
-	return 0;
-}
-
 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_ioctl.c b/fs/xfs/xfs_ioctl.c
index 7b35d62ede9f..1f1c40b20fa6 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1969,6 +1969,41 @@ xfs_ioc_setlabel(
 	return error;
 }
 
+static inline int
+xfs_fs_eofblocks_from_user(
+	struct xfs_fs_eofblocks		*src,
+	struct xfs_eofblocks		*dst)
+{
+	if (src->eof_version != XFS_EOFBLOCKS_VERSION)
+		return -EINVAL;
+
+	if (src->eof_flags & ~XFS_EOF_FLAGS_VALID)
+		return -EINVAL;
+
+	if (memchr_inv(&src->pad32, 0, sizeof(src->pad32)) ||
+	    memchr_inv(src->pad64, 0, sizeof(src->pad64)))
+		return -EINVAL;
+
+	dst->eof_flags = src->eof_flags;
+	dst->eof_prid = src->eof_prid;
+	dst->eof_min_file_size = src->eof_min_file_size;
+
+	dst->eof_uid = INVALID_UID;
+	if (src->eof_flags & XFS_EOF_FLAGS_UID) {
+		dst->eof_uid = make_kuid(current_user_ns(), src->eof_uid);
+		if (!uid_valid(dst->eof_uid))
+			return -EINVAL;
+	}
+
+	dst->eof_gid = INVALID_GID;
+	if (src->eof_flags & XFS_EOF_FLAGS_GID) {
+		dst->eof_gid = make_kgid(current_user_ns(), src->eof_gid);
+		if (!gid_valid(dst->eof_gid))
+			return -EINVAL;
+	}
+	return 0;
+}
+
 /*
  * Note: some of the ioctl's return positive numbers as a
  * byte count indicating success, such as readlink_by_handle.