Re: [PATCH 04/13] xfs: move RT inode locking out of __xfs_bunmapi

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

On Wed, Mar 27, 2024 at 12:03:09PM +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_bmap_del_extent_real just before the call to
> xfs_rtfree_blocks instead and use a new flag in the transaction to ensure
> that the locking happens only once.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c   | 15 ++++++++-------
>  fs/xfs/libxfs/xfs_shared.h |  3 +++
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 282b44deb9f864..e5e199d325982f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5305,6 +5305,14 @@ xfs_bmap_del_extent_real(
>  		if (xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK) {
>  			xfs_refcount_decrease_extent(tp, del);
>  		} else if (xfs_ifork_is_realtime(ip, whichfork)) {
> +			/*
> +			 * Ensure the bitmap and summary inodes are locked
> +			 * and joined to the transaction before modifying them.
> +			 */
> +			if (!(tp->t_flags & XFS_TRANS_RTBITMAP_LOCKED)) {
> +				tp->t_flags |= XFS_TRANS_RTBITMAP_LOCKED;

How does it happen that xfs_rtfree_blocks gets called more than once in
the same transaction?  Is that simply the effect of xfs_bunmapi_range
and xfs_unmap_exten calling __xfs_bunmapi with
nextents == XFS_ITRUNC_MAX_EXTENTS==2?

What if we simply didn't unmap multiple extents per bunmapi call for
realtime files?  Would that eliminate the need for
XFS_TRANS_RTBITMAP_LOCKED?

--D

> +				xfs_rtbitmap_lock(tp, mp);
> +			}
>  			error = xfs_rtfree_blocks(tp, del->br_startblock,
>  					del->br_blockcount);
>  		} else {
> @@ -5406,13 +5414,6 @@ __xfs_bunmapi(
>  	} else
>  		cur = NULL;
>  
> -	if (isrt) {
> -		/*
> -		 * Synchronize by locking the realtime bitmap.
> -		 */
> -		xfs_rtbitmap_lock(tp, mp);
> -	}
> -
>  	extno = 0;
>  	while (end != (xfs_fileoff_t)-1 && end >= start &&
>  	       (nexts == 0 || extno < nexts)) {
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index f35640ad3e7fe4..34f104ed372c09 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -137,6 +137,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
> 
>