Re: [PATCH 2/3] xfs: move some code out of xfs_iget_recycle

Darrick J. Wong
2025-11-14T17:04:03Z

On Fri, Nov 14, 2025 at 06:52:24AM +0100, Christoph Hellwig wrote:
> Having a function drop locks, reacquire them and release them again
> seems to confuse the clang lock analysis even more than it confuses
> humans. Keep the humans and machines sanity by moving a chunk of
> code into the caller to simplify the lock tracking.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_icache.c | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index e44040206851..546efa6cec72 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -358,7 +358,7 @@ xfs_reinit_inode(
> static int
> xfs_iget_recycle(
> struct xfs_perag *pag,
> - struct xfs_inode *ip) __releases(&ip->i_flags_lock)
> + struct xfs_inode *ip)
> {
> struct xfs_mount *mp = ip->i_mount;
> struct inode *inode = VFS_I(ip);
> @@ -366,20 +366,6 @@ xfs_iget_recycle(
>
> trace_xfs_iget_recycle(ip);
>
> - if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL))
> - return -EAGAIN;
> -
> - /*
> - * We need to make it look like the inode is being reclaimed to prevent
> - * the actual reclaim workers from stomping over us while we recycle
> - * the inode. We can't clear the radix tree tag yet as it requires
> - * pag_ici_lock to be held exclusive.
> - */
> - ip->i_flags |= XFS_IRECLAIM;
> -
> - spin_unlock(&ip->i_flags_lock);
> - rcu_read_unlock();
> -
> ASSERT(!rwsem_is_locked(&inode->i_rwsem));
> error = xfs_reinit_inode(mp, inode);
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> @@ -576,10 +562,19 @@ xfs_iget_cache_hit(
>
> /* The inode fits the selection criteria; process it. */
> if (ip->i_flags & XFS_IRECLAIMABLE) {
> - /* Drops i_flags_lock and RCU read lock. */
> - error = xfs_iget_recycle(pag, ip);
> - if (error == -EAGAIN)
> + /*
> + * We need to make it look like the inode is being reclaimed to
> + * prevent the actual reclaim workers from stomping over us
> + * while we recycle the inode. We can't clear the radix tree
> + * tag yet as it requires pag_ici_lock to be held exclusive.
> + */
> + if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL))
> goto out_skip;
> + ip->i_flags |= XFS_IRECLAIM;
> + spin_unlock(&ip->i_flags_lock);
> + rcu_read_unlock();

I wonder, does sparse get confused by rcu_read_lock having been taken by
the caller but unlocked here?

The code move looks correct though.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +
> + error = xfs_iget_recycle(pag, ip);
> if (error)
> return error;
> } else {
> --
> 2.47.3
>
>