Re: [PATCH 03/13] xfs: free RT extents after updating the bmap btree

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

On Wed, Mar 27, 2024 at 12:03:08PM +0100, Christoph Hellwig wrote:
> Currently xfs_bmap_del_extent_real frees RT extents before updating
> the bmap btree, while it frees regular blocks after performing the bmap
> btree update.  While this behavior goes back to the original commit,
> I can't find any good reason for handling RT extent vs regular block
> freeing differently.  We use the same transaction, and unless rmaps
> or reflink are enabled (which currently aren't support for RT inodes)
> there are no transactions rolls or deferred ops that can rely on this
> ordering.

...and the realtime rmap/reflink patchsets will want to reuse the data
device's ordering (bmap -> rmap -> refcount -> efi) for the rt volume.

> Signed-off-by: Christoph Hellwig <hch@lst.de>

I'm ok with moving this now since I'm mostly going to pave over it later
anyway :)

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

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 09d4b730ee9709..282b44deb9f864 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5107,8 +5107,7 @@ xfs_bmap_del_extent_real(
>  {
>  	xfs_fsblock_t		del_endblock=0;	/* first block past del */
>  	xfs_fileoff_t		del_endoff;	/* first offset past del */
> -	int			do_fx;	/* free extent at end of routine */
> -	int			error;	/* error return value */
> +	int			error = 0;	/* error return value */
>  	struct xfs_bmbt_irec	got;	/* current extent entry */
>  	xfs_fileoff_t		got_endoff;	/* first offset past got */
>  	int			i;	/* temp state */
> @@ -5151,20 +5150,10 @@ xfs_bmap_del_extent_real(
>  		return -ENOSPC;
>  
>  	*logflagsp = XFS_ILOG_CORE;
> -	if (xfs_ifork_is_realtime(ip, whichfork)) {
> -		if (!(bflags & XFS_BMAPI_REMAP)) {
> -			error = xfs_rtfree_blocks(tp, del->br_startblock,
> -					del->br_blockcount);
> -			if (error)
> -				return error;
> -		}
> -
> -		do_fx = 0;
> +	if (xfs_ifork_is_realtime(ip, whichfork))
>  		qfield = XFS_TRANS_DQ_RTBCOUNT;
> -	} else {
> -		do_fx = 1;
> +	else
>  		qfield = XFS_TRANS_DQ_BCOUNT;
> -	}
>  	nblks = del->br_blockcount;
>  
>  	del_endblock = del->br_startblock + del->br_blockcount;
> @@ -5312,18 +5301,21 @@ xfs_bmap_del_extent_real(
>  	/*
>  	 * If we need to, add to list of extents to delete.
>  	 */
> -	if (do_fx && !(bflags & XFS_BMAPI_REMAP)) {
> +	if (!(bflags & XFS_BMAPI_REMAP)) {
>  		if (xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK) {
>  			xfs_refcount_decrease_extent(tp, del);
> +		} else if (xfs_ifork_is_realtime(ip, whichfork)) {
> +			error = xfs_rtfree_blocks(tp, del->br_startblock,
> +					del->br_blockcount);
>  		} else {
>  			error = xfs_free_extent_later(tp, del->br_startblock,
>  					del->br_blockcount, NULL,
>  					XFS_AG_RESV_NONE,
>  					((bflags & XFS_BMAPI_NODISCARD) ||
>  					del->br_state == XFS_EXT_UNWRITTEN));
> -			if (error)
> -				return error;
>  		}
> +		if (error)
> +			return error;
>  	}
>  
>  	/*
> -- 
> 2.39.2
> 
>