Re: [PATCH 02/10] xfs: move RT inode locking out of __xfs_bunmapi

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

On Fri, Feb 23, 2024 at 08:14:58AM +0100, Christoph Hellwig wrote:
> __xfs_bunmapi is a bit of an odd place to lock the rtbitmap and rtsummary
> inodes given that it is very high level code.  While this only looks ugly
> right now, it will become a problem when supporting delayed allocations
> for RT inodes as __xfs_bunmapi might end up deleting only delalloc extents
> and thus never unlock the rt inodes.
> 
> Move the locking into xfs_rtfree_blocks instead (where it will also be
> helpful once we support extfree items for RT allocations), and use a new
> flag in the transaction to ensure they aren't locked twice.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c     | 10 ----------
>  fs/xfs/libxfs/xfs_rtbitmap.c | 14 ++++++++++++++
>  fs/xfs/libxfs/xfs_shared.h   |  3 +++
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index b525524a2da4ef..f8cc7c510d7bd5 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5321,16 +5321,6 @@ __xfs_bunmapi(
>  	} else
>  		cur = NULL;
>  
> -	if (isrt) {
> -		/*
> -		 * Synchronize by locking the bitmap inode.
> -		 */
> -		xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
> -		xfs_trans_ijoin(tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> -		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> -		xfs_trans_ijoin(tp, mp->m_rsumip, XFS_ILOCK_EXCL);
> -	}
> -
>  	extno = 0;
>  	while (end != (xfs_fileoff_t)-1 && end >= start &&
>  	       (nexts == 0 || extno < nexts)) {
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index e31663cb7b4349..2759c48390241d 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1001,6 +1001,20 @@ xfs_rtfree_blocks(
>  		return -EIO;
>  	}
>  
> +	/*
> +	 * Ensure the bitmap and summary inodes are locked before modifying
> +	 * them.  We can get called multiples times per transaction, so record
> +	 * the fact that they are locked in the transaction.
> +	 */
> +	if (!(tp->t_flags & XFS_TRANS_RTBITMAP_LOCKED)) {
> +		tp->t_flags |= XFS_TRANS_RTBITMAP_LOCKED;

I don't really like using transaction flags to record lock state.

Would this be cleaner if we tracked this in struct xfs_rtalloc_args, and
had an xfs_rtalloc_acquire(mp, &args, XFS_ILOCK_{SHARED,EXCL}) method
that would set that up for us?

--D

> +
> +		xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
> +		xfs_trans_ijoin(tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> +		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> +		xfs_trans_ijoin(tp, mp->m_rsumip, XFS_ILOCK_EXCL);
> +	}
> +
>  	return xfs_rtfree_extent(tp, start, len);
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 6f1cedb850eb39..1598ff00f6805f 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -83,6 +83,9 @@ void	xfs_log_get_max_trans_res(struct xfs_mount *mp,
>   */
>  #define XFS_TRANS_LOWMODE		(1u << 8)
>  
> +/* Transaction has locked the rtbitmap and rtsum inodes */
> +#define XFS_TRANS_RTBITMAP_LOCKED	(1u << 9)
> +
>  /*
>   * Field values for xfs_trans_mod_sb.
>   */
> -- 
> 2.39.2
> 
>