Re: [PATCH 17/24] xfs: remove SYNC_TRYLOCK from inode reclaim

Darrick J. Wong
2020-05-22T23:14:41Z

On Fri, May 22, 2020 at 01:50:22PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> All background reclaim is SYNC_TRYLOCK already, and even blocking
> reclaim (SYNC_WAIT) can use trylock mechanisms as
> xfs_reclaim_inodes_ag() will keep cycling until there are no more
> reclaimable inodes. Hence we can kill SYNC_TRYLOCK from inode
> reclaim and make everything unconditionally non-blocking.

Random question: Does xfs_quiesce_attr need to call xfs_reclaim_inodes
twice now, or does the second SYNC_WAIT call suffice now?

> Signed-off-by: Dave Chinner <dchinner@redhat.com>

This patch itself looks fine though.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> fs/xfs/xfs_icache.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index c020d2379e12e..8b366bc7b53c9 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1049,10 +1049,9 @@ xfs_inode_ag_iterator_tag(
> * Grab the inode for reclaim exclusively.
> * Return 0 if we grabbed it, non-zero otherwise.
> */
> -STATIC int
> +static int
> xfs_reclaim_inode_grab(
> - struct xfs_inode *ip,
> - int flags)
> + struct xfs_inode *ip)
> {
> ASSERT(rcu_read_lock_held());
>
> @@ -1061,12 +1060,10 @@ xfs_reclaim_inode_grab(
> return 1;
>
> /*
> - * If we are asked for non-blocking operation, do unlocked checks to
> - * see if the inode already is being flushed or in reclaim to avoid
> - * lock traffic.
> + * Do unlocked checks to see if the inode already is being flushed or in
> + * reclaim to avoid lock traffic.
> */
> - if ((flags & SYNC_TRYLOCK) &&
> - __xfs_iflags_test(ip, XFS_IFLOCK | XFS_IRECLAIM))
> + if (__xfs_iflags_test(ip, XFS_IFLOCK | XFS_IRECLAIM))
> return 1;
>
> /*
> @@ -1133,8 +1130,7 @@ xfs_reclaim_inode_grab(
> static bool
> xfs_reclaim_inode(
> struct xfs_inode *ip,
> - struct xfs_perag *pag,
> - int sync_mode)
> + struct xfs_perag *pag)
> {
> xfs_ino_t ino = ip->i_ino; /* for radix_tree_delete */
>
> @@ -1224,7 +1220,6 @@ xfs_reclaim_inode(
> static int
> xfs_reclaim_inodes_ag(
> struct xfs_mount *mp,
> - int flags,
> int *nr_to_scan)
> {
> struct xfs_perag *pag;
> @@ -1262,7 +1257,7 @@ xfs_reclaim_inodes_ag(
> for (i = 0; i < nr_found; i++) {
> struct xfs_inode *ip = batch[i];
>
> - if (done || xfs_reclaim_inode_grab(ip, flags))
> + if (done || xfs_reclaim_inode_grab(ip))
> batch[i] = NULL;
>
> /*
> @@ -1293,7 +1288,7 @@ xfs_reclaim_inodes_ag(
> for (i = 0; i < nr_found; i++) {
> if (!batch[i])
> continue;
> - if (!xfs_reclaim_inode(batch[i], pag, flags))
> + if (!xfs_reclaim_inode(batch[i], pag))
> skipped++;
> }
>
> @@ -1319,13 +1314,13 @@ xfs_reclaim_inodes(
> int nr_to_scan = INT_MAX;
> int skipped;
>
> - xfs_reclaim_inodes_ag(mp, mode, &nr_to_scan);
> + xfs_reclaim_inodes_ag(mp, &nr_to_scan);
> if (!(mode & SYNC_WAIT))
> return 0;
>
> do {
> xfs_ail_push_all_sync(mp->m_ail);
> - skipped = xfs_reclaim_inodes_ag(mp, mode, &nr_to_scan);
> + skipped = xfs_reclaim_inodes_ag(mp, &nr_to_scan);
> } while (skipped > 0);
>
> return 0;
> @@ -1349,7 +1344,7 @@ xfs_reclaim_inodes_nr(
> xfs_reclaim_work_queue(mp);
> xfs_ail_push_all(mp->m_ail);
>
> - xfs_reclaim_inodes_ag(mp, SYNC_TRYLOCK, &nr_to_scan);
> + xfs_reclaim_inodes_ag(mp, &nr_to_scan);
> return 0;
> }
>
> --
> 2.26.2.761.g0e0b3e54be
>