Re: [PATCH] xfs: fix double xfs_perag_rele() in xfs_filestream_pick_ag()

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, May 29, 2023 at 12:59:50PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_bmap_longest_free_extent() can return an error when accessing
> the AGF fails. In this case, the behaviour of
> xfs_filestream_pick_ag() is conditional on the error. We may
> continue the loop, or break out of it. The error handling after the
> loop cleans up the perag reference held when the break occurs. If we
> continue, the next loop iteration handles cleaning up the perag
> reference.
> 
> EIther way, we don't need to release the active perag reference when
> xfs_bmap_longest_free_extent() fails. Doing so means we do a double
> decrement on the active reference count, and this causes tha active
> reference count to fall to zero. At this point, new active
> references will fail.
> 
> This leads to unmount hanging because it tries to grab active
> references to that perag, only for it to fail. This happens inside a
> loop that retries until a inode tree radix tree tag is cleared,
> which cannot happen because we can't get an active reference to the
> perag.

Wouldn't it be nice if C had^W^Wthe kernel coding rules allowed
iterators that managed these active refs for us...

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> The unmount livelocks in this path:
> 
>   xfs_reclaim_inodes+0x80/0xc0
>   xfs_unmount_flush_inodes+0x5b/0x70
>   xfs_unmountfs+0x5b/0x1a0
>   xfs_fs_put_super+0x49/0x110
>   generic_shutdown_super+0x7c/0x1a0
>   kill_block_super+0x27/0x50
>   deactivate_locked_super+0x30/0x90
>   deactivate_super+0x3c/0x50
>   cleanup_mnt+0xc2/0x160
>   __cleanup_mnt+0x12/0x20
>   task_work_run+0x5e/0xa0
>   exit_to_user_mode_prepare+0x1bc/0x1c0
>   syscall_exit_to_user_mode+0x16/0x40
>   do_syscall_64+0x40/0x80
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Fixes: eb70aa2d8ed9 ("xfs: use for_each_perag_wrap in xfs_filestream_pick_ag")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_filestream.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index 22c13933c8f8..2fc98d313708 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -78,7 +78,6 @@ xfs_filestream_pick_ag(
>  		*longest = 0;
>  		err = xfs_bmap_longest_free_extent(pag, NULL, longest);
>  		if (err) {
> -			xfs_perag_rele(pag);
>  			if (err != -EAGAIN)
>  				break;
>  			/* Couldn't lock the AGF, skip this AG. */
> -- 
> 2.40.1
>