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

[Keith Busch <kbusch@kernel.org>]

On Sat, Jan 24, 2026 at 04:03:04PM -0800, Chris Mason wrote:
> On Wed, 14 Jan 2026 14:06:43 +0100 Christoph Hellwig <hch@lst.de> wrote:
> > @@ -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.

I think you're right that ring wrap can't distinguish full vs. empty here.
 
> 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.

The buffer size is a power of two, so I suggest just let scratch_head
and scratch_tail only increment without modulo, and rely on the unsigned
int wrapping. We can get the actual offset by masking the head with the
scratch size.

---
diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index ba4f8e011e36c..7d2bd0dc8b322 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -578,12 +578,7 @@ static unsigned int
 xfs_zone_gc_scratch_available(
 	struct xfs_zone_gc_data	*data)
 {
-	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;
+	return data->scratch_size - (data->scratch_head - data->scratch_tail);
 }
 
 static bool
@@ -663,7 +658,7 @@ xfs_zone_gc_add_data(
 {
 	struct xfs_zone_gc_data	*data = chunk->data;
 	unsigned int		len = chunk->len;
-	unsigned int		off = data->scratch_head;
+	unsigned int		off = data->scratch_head & (data->scratch_size - 1);
 
 	do {
 		unsigned int	this_off = off % XFS_GC_BUF_SIZE;
@@ -729,7 +724,7 @@ 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;
 	xfs_zone_gc_add_data(chunk);
-	data->scratch_head = (data->scratch_head + len) % data->scratch_size;
+	data->scratch_head = data->scratch_head + len;
 
 	WRITE_ONCE(chunk->state, XFS_GC_BIO_NEW);
 	list_add_tail(&chunk->entry, &data->reading);
@@ -860,8 +855,7 @@ xfs_zone_gc_finish_chunk(
 		return;
 	}
 
-	data->scratch_tail =
-		(data->scratch_tail + chunk->len) % data->scratch_size;
+	data->scratch_tail = data->scratch_tail + chunk->len;
 
 	/*
 	 * Cycle through the iolock and wait for direct I/O and layouts to

--