Re: [PATCH 1/7] xfs: don't allocate the xfs_extent_busy structure for zoned RTGs

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

On Wed, Jul 16, 2025 at 02:54:01PM +0200, Christoph Hellwig wrote:
> Busy extent tracking is primarily used to ensure that freed blocks are
> not reused for data allocations before the transaction that deleted them
> has been committed to stable storage, and secondarily to drive online
> discard.  None of the use cases applies to zoned RTGs, as the zoned
> allocator can't overwrite blocks before resetting the zone, which already
> flushes out all transactions touching the RTGs.
> 
> So the busy extent tracking is not needed for zoned RTGs, and also not
> called for zoned RTGs.  But somehow the code to skip allocating and
> freeing the structure got lost during the zoned XFS upstreaming process.
> This not only causes these structures to unessecarily allocated, but can

nit: unnecessarily

> also lead to memory leaks as the xg_busy_extents pointer in the
> xfs_group structure is overlayed with the pointer for the linked list
> of to be reset zones.
> 
> Stop allocating and freeing the structure to not pointlessly allocate
> memory which is then leaked when the zone is reset.

Yikes!

Cc: <stable@vger.kernel.org> # v6.15

> Fixes: 080d01c41d44 ("xfs: implement zoned garbage collection")
> Signed-off-by: Christoph Hellwig <hch@lst.de>

With those added,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_group.c | 14 +++++++++-----
>  fs/xfs/xfs_extent_busy.h  |  8 ++++++++
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_group.c b/fs/xfs/libxfs/xfs_group.c
> index e9d76bcdc820..20ad7c309489 100644
> --- a/fs/xfs/libxfs/xfs_group.c
> +++ b/fs/xfs/libxfs/xfs_group.c
> @@ -163,7 +163,8 @@ xfs_group_free(
>  
>  	xfs_defer_drain_free(&xg->xg_intents_drain);
>  #ifdef __KERNEL__
> -	kfree(xg->xg_busy_extents);
> +	if (xfs_group_has_extent_busy(xg->xg_mount, xg->xg_type))
> +		kfree(xg->xg_busy_extents);
>  #endif
>  
>  	if (uninit)
> @@ -189,9 +190,11 @@ xfs_group_insert(
>  	xg->xg_type = type;
>  
>  #ifdef __KERNEL__
> -	xg->xg_busy_extents = xfs_extent_busy_alloc();
> -	if (!xg->xg_busy_extents)
> -		return -ENOMEM;
> +	if (xfs_group_has_extent_busy(mp, type)) {
> +		xg->xg_busy_extents = xfs_extent_busy_alloc();
> +		if (!xg->xg_busy_extents)
> +			return -ENOMEM;
> +	}
>  	spin_lock_init(&xg->xg_state_lock);
>  	xfs_hooks_init(&xg->xg_rmap_update_hooks);
>  #endif
> @@ -210,7 +213,8 @@ xfs_group_insert(
>  out_drain:
>  	xfs_defer_drain_free(&xg->xg_intents_drain);
>  #ifdef __KERNEL__
> -	kfree(xg->xg_busy_extents);
> +	if (xfs_group_has_extent_busy(xg->xg_mount, xg->xg_type))
> +		kfree(xg->xg_busy_extents);
>  #endif
>  	return error;
>  }
> diff --git a/fs/xfs/xfs_extent_busy.h b/fs/xfs/xfs_extent_busy.h
> index f069b04e8ea1..3e6e019b6146 100644
> --- a/fs/xfs/xfs_extent_busy.h
> +++ b/fs/xfs/xfs_extent_busy.h
> @@ -68,4 +68,12 @@ static inline void xfs_extent_busy_sort(struct list_head *list)
>  	list_sort(NULL, list, xfs_extent_busy_ag_cmp);
>  }
>  
> +/*
> + * Zoned RTGs don't need to track busy extents, as the actual block freeing only
> + * happens by a zone reset, which forces out all transactions that touched the
> + * to be reset zone first.
> + */
> +#define xfs_group_has_extent_busy(mp, type) \
> +	((type) == XG_TYPE_AG || !xfs_has_zoned((mp)))
> +
>  #endif /* __XFS_EXTENT_BUSY_H__ */
> -- 
> 2.47.2
> 
>