Re: [PATCH 3/3] xfs: rework zone GC buffer management

[Chris Mason <clm@meta.com>]

On Wed, 14 Jan 2026 14:06:43 +0100 Christoph Hellwig <hch@lst.de> wrote:

> The double buffering where just one scratch area is used at a time does
> not efficiently use the available memory.  It was originally implemented
> when GC I/O could happen out of order, but that was removed before
> upstream submission to avoid fragmentation.  Now that all GC I/Os are
> processed in order, just use a number of buffers as a simple ring buffer.
> 
> For a synthetic benchmark that fills 256MiB HDD zones and punches out
> holes to free half the space this leads to a decrease of GC time by
> a little more than 25%.

Hi everyone,

I'm pretty low confidence on this, but it was flagged when I ran a bunch of
linux-next commits through the AI review prompts.  I'm curious if the AI is
on to something or if I need to adjust things:

> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c

[ ... ]

> @@ -590,7 +578,12 @@ xfs_zone_gc_ensure_target(
>  xfs_zone_gc_scratch_available(
>  	struct xfs_zone_gc_data	*data)
>  {
> -	return XFS_GC_CHUNK_SIZE - data->scratch[data->scratch_idx].offset;
> +	if (!data->scratch_tail)
> +		return data->scratch_size - data->scratch_head;
> +
> +	if (!data->scratch_head)
> +		return data->scratch_tail;
> +	return (data->scratch_size - data->scratch_head) + data->scratch_tail;
>  }

Can this function correctly distinguish between an empty and full ring
buffer?

When scratch_head wraps back to 0 and scratch_tail is also 0 (because no
I/O has completed yet), the first condition returns scratch_size (2MB),
indicating the buffer is empty.  But if the buffer just filled completely
and head wrapped to 0, the buffer is actually full, and the available
space should be 0.

Consider this sequence in xfs_zone_gc_handle_work():
  - Initial: head=0, tail=0, available=2MB (correct, empty)
  - After 1MB chunk: head=1MB, tail=0, available=1MB (correct)
  - After another 1MB chunk: head=0 (wrapped), tail=0, available=2MB (wrong)

The buffer is full but the function reports it as empty.  The while loop
in xfs_zone_gc_handle_work() calling xfs_zone_gc_start_chunk() would then
continue adding more data, overwriting the buffer contents from the first
chunk before its I/O completes.

[ ... ]

> @@ -710,13 +728,9 @@ xfs_zone_gc_start_chunk(
>
>  	bio->bi_iter.bi_sector = xfs_rtb_to_daddr(mp, chunk->old_startblock);
>  	bio->bi_end_io = xfs_zone_gc_end_io;
> -	bio_add_folio_nofail(bio, chunk->scratch->folio, chunk->len,
> -			chunk->scratch->offset);
> -	chunk->scratch->offset += chunk->len;
> -	if (chunk->scratch->offset == XFS_GC_CHUNK_SIZE) {
> -		data->scratch_idx =
> -			(data->scratch_idx + 1) % XFS_ZONE_GC_NR_SCRATCH;
> -	}
> +	xfs_zone_gc_add_data(chunk);
> +	data->scratch_head = (data->scratch_head + len) % data->scratch_size;

This advances scratch_head with modulo arithmetic, which is correct for
wrap-around.  However, if head catches up to tail (buffer full), the code
does not detect this condition.

[ ... ]

> @@ -845,11 +860,8 @@ xfs_zone_gc_finish_chunk(
>  		return;
>  	}
>
> -	chunk->scratch->freed += chunk->len;
> -	if (chunk->scratch->freed == chunk->scratch->offset) {
> -		chunk->scratch->offset = 0;
> -		chunk->scratch->freed = 0;
> -	}
> +	data->scratch_tail =
> +		(data->scratch_tail + chunk->len) % data->scratch_size;

And scratch_tail advances here when I/O completes.  The classic ring
buffer empty/full ambiguity applies: when head == tail, is the buffer
empty or full?  The xfs_zone_gc_scratch_available() function assumes
head=0 and tail=0 always means empty, but after wrapping it could mean
full.

A common solution is to track a separate count, or to sacrifice one slot
so head == tail always means empty, and head == tail-1 means full.