Re: [PATCH] xfs: split xfs_zone_record_blocks

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

On Wed, Jul 23, 2025 at 07:35:44AM +0200, Christoph Hellwig wrote:
> xfs_zone_record_blocks not only records successfully written blocks that
> now back file data, but is also used for blocks speculatively written by
> garbage collection that were never linked to an inode and instantly
> become invalid.
> 
> Split the latter functionality out to be easier to understand.  This also
> make it clear that we don't need to attach the rmap inode to a
> transaction for the skipped blocks case as we never dirty any peristent
> data structure.
> 
> Also make the argument order to xfs_zone_record_blocks a bit more
> natural.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_trace.h      |  1 +
>  fs/xfs/xfs_zone_alloc.c | 42 ++++++++++++++++++++++++++++-------------
>  2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 10d4fd671dcf..178b204dee3b 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -455,6 +455,7 @@ DEFINE_EVENT(xfs_zone_alloc_class, name,			\
>  		 xfs_extlen_t len),				\
>  	TP_ARGS(oz, rgbno, len))
>  DEFINE_ZONE_ALLOC_EVENT(xfs_zone_record_blocks);
> +DEFINE_ZONE_ALLOC_EVENT(xfs_zone_skip_blocks);
>  DEFINE_ZONE_ALLOC_EVENT(xfs_zone_alloc_blocks);
>  
>  TRACE_EVENT(xfs_zone_gc_select_victim,
> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index 33f7eee521a8..f8bd6d741755 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -166,10 +166,9 @@ xfs_open_zone_mark_full(
>  static void
>  xfs_zone_record_blocks(
>  	struct xfs_trans	*tp,
> -	xfs_fsblock_t		fsbno,
> -	xfs_filblks_t		len,
>  	struct xfs_open_zone	*oz,
> -	bool			used)
> +	xfs_fsblock_t		fsbno,
> +	xfs_filblks_t		len)
>  {
>  	struct xfs_mount	*mp = tp->t_mountp;
>  	struct xfs_rtgroup	*rtg = oz->oz_rtg;
> @@ -179,18 +178,37 @@ xfs_zone_record_blocks(
>  
>  	xfs_rtgroup_lock(rtg, XFS_RTGLOCK_RMAP);
>  	xfs_rtgroup_trans_join(tp, rtg, XFS_RTGLOCK_RMAP);
> -	if (used) {
> -		rmapip->i_used_blocks += len;
> -		ASSERT(rmapip->i_used_blocks <= rtg_blocks(rtg));
> -	} else {
> -		xfs_add_frextents(mp, len);
> -	}
> +	rmapip->i_used_blocks += len;
> +	ASSERT(rmapip->i_used_blocks <= rtg_blocks(rtg));
>  	oz->oz_written += len;
>  	if (oz->oz_written == rtg_blocks(rtg))
>  		xfs_open_zone_mark_full(oz);
>  	xfs_trans_log_inode(tp, rmapip, XFS_ILOG_CORE);
>  }
>  
> +/*
> + * Called for blocks that have been written to disk, but not actually linked to
> + * an inode, which can happen when garbage collection races with user data
> + * writes to a file.
> + */
> +static void
> +xfs_zone_skip_blocks(
> +	struct xfs_open_zone	*oz,
> +	xfs_filblks_t		len)
> +{
> +	struct xfs_rtgroup	*rtg = oz->oz_rtg;
> +
> +	trace_xfs_zone_skip_blocks(oz, 0, len);
> +
> +	xfs_rtgroup_lock(rtg, XFS_RTGLOCK_RMAP);
> +	oz->oz_written += len;
> +	if (oz->oz_written == rtg_blocks(rtg))
> +		xfs_open_zone_mark_full(oz);
> +	xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_RMAP);
> +
> +	xfs_add_frextents(rtg_mount(rtg), len);
> +}
> +
>  static int
>  xfs_zoned_map_extent(
>  	struct xfs_trans	*tp,
> @@ -250,8 +268,7 @@ xfs_zoned_map_extent(
>  		}
>  	}
>  
> -	xfs_zone_record_blocks(tp, new->br_startblock, new->br_blockcount, oz,
> -			true);
> +	xfs_zone_record_blocks(tp, oz, new->br_startblock, new->br_blockcount);
>  
>  	/* Map the new blocks into the data fork. */
>  	xfs_bmap_map_extent(tp, ip, XFS_DATA_FORK, new);
> @@ -259,8 +276,7 @@ xfs_zoned_map_extent(
>  
>  skip:
>  	trace_xfs_reflink_cow_remap_skip(ip, new);
> -	xfs_zone_record_blocks(tp, new->br_startblock, new->br_blockcount, oz,
> -			false);
> +	xfs_zone_skip_blocks(oz, new->br_blockcount);
>  	return 0;
>  }
>  
> -- 
> 2.47.2
> 
>