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@xxxxxxxxxxxxxxx> # v6.15
> Fixes: 080d01c41d44 ("xfs: implement zoned garbage collection")
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
With those added,
Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
--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
>
>
