fix for selecting a zone with active GC I/O for GC

fix for selecting a zone with active GC I/O for GC

[Christoph Hellwig]

Hi all,

the first patch in the series adds a counter to ensure that we never
select a zone for which there is active GC I/O for GC again, as we'd
try to copy the same blocks again, only to see the mapping had changed
in I/O completion to discard that I/O.

The second one documents an non-obvious version of that GC race detection
that took us way too long to track down.

[PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Christoph Hellwig]

When we are picking a zone for gc it might already be in the pipeline
which can lead to us moving the same data twice resulting in in write
amplification and a very unfortunate case where keep on garbage
collecting the zone we just filled with migrated data stopping all
forward progress.

Fix this by introducing a count of on-going GC operations on a zone, so
and skip any zone with ongoing GC when picking a new victim.

Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
Co-developed-by: Hans Holmberg <hans.holmberg@wdc.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_rtgroup.h |  6 ++++++
 fs/xfs/xfs_zone_gc.c        | 27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_rtgroup.h b/fs/xfs/libxfs/xfs_rtgroup.h
index d36a6ae0abe5..d4fcf591e63d 100644
--- a/fs/xfs/libxfs/xfs_rtgroup.h
+++ b/fs/xfs/libxfs/xfs_rtgroup.h
@@ -50,6 +50,12 @@ struct xfs_rtgroup {
 		uint8_t			*rtg_rsum_cache;
 		struct xfs_open_zone	*rtg_open_zone;
 	};
+
+	/*
+	 * Count of outstanding GC operations for zoned XFS.  Any RTG with a
+	 * non-zero rtg_gccount will not be picked as new GC victim.
+	 */
+	atomic_t		rtg_gccount;
 };
 
 /*
diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 109877d9a6bf..efcb52796d05 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -114,6 +114,8 @@ struct xfs_gc_bio {
 	/* Open Zone being written to */
 	struct xfs_open_zone		*oz;
 
+	struct xfs_rtgroup		*victim_rtg;
+
 	/* Bio used for reads and writes, including the bvec used by it */
 	struct bio_vec			bv;
 	struct bio			bio;	/* must be last */
@@ -264,6 +266,7 @@ xfs_zone_gc_iter_init(
 	iter->rec_count = 0;
 	iter->rec_idx = 0;
 	iter->victim_rtg = victim_rtg;
+	atomic_inc(&victim_rtg->rtg_gccount);
 }
 
 /*
@@ -362,6 +365,7 @@ xfs_zone_gc_query(
 
 	return 0;
 done:
+	atomic_dec(&iter->victim_rtg->rtg_gccount);
 	xfs_rtgroup_rele(iter->victim_rtg);
 	iter->victim_rtg = NULL;
 	return 0;
@@ -451,6 +455,20 @@ xfs_zone_gc_pick_victim_from(
 		if (!rtg)
 			continue;
 
+		/*
+		 * If the zone is already undergoing GC, don't pick it again.
+		 *
+		 * This prevents us from picking one of the zones for which we
+		 * already submitted GC I/O, but for which the remapping hasn't
+		 * concluded again.  This won't cause data corruption, but
+		 * increases write amplification and slows down GC, so this is
+		 * a bad thing.
+		 */
+		if (atomic_read(&rtg->rtg_gccount)) {
+			xfs_rtgroup_rele(rtg);
+			continue;
+		}
+
 		/* skip zones that are just waiting for a reset */
 		if (rtg_rmap(rtg)->i_used_blocks == 0 ||
 		    rtg_rmap(rtg)->i_used_blocks >= victim_used) {
@@ -688,6 +706,9 @@ xfs_zone_gc_start_chunk(
 	chunk->scratch = &data->scratch[data->scratch_idx];
 	chunk->data = data;
 	chunk->oz = oz;
+	chunk->victim_rtg = iter->victim_rtg;
+	atomic_inc(&chunk->victim_rtg->rtg_group.xg_active_ref);
+	atomic_inc(&chunk->victim_rtg->rtg_gccount);
 
 	bio->bi_iter.bi_sector = xfs_rtb_to_daddr(mp, chunk->old_startblock);
 	bio->bi_end_io = xfs_zone_gc_end_io;
@@ -710,6 +731,8 @@ static void
 xfs_zone_gc_free_chunk(
 	struct xfs_gc_bio	*chunk)
 {
+	atomic_dec(&chunk->victim_rtg->rtg_gccount);
+	xfs_rtgroup_rele(chunk->victim_rtg);
 	list_del(&chunk->entry);
 	xfs_open_zone_put(chunk->oz);
 	xfs_irele(chunk->ip);
@@ -770,6 +793,10 @@ xfs_zone_gc_split_write(
 	split_chunk->oz = chunk->oz;
 	atomic_inc(&chunk->oz->oz_ref);
 
+	split_chunk->victim_rtg = chunk->victim_rtg;
+	atomic_inc(&chunk->victim_rtg->rtg_group.xg_active_ref);
+	atomic_inc(&chunk->victim_rtg->rtg_gccount);
+
 	chunk->offset += split_len;
 	chunk->len -= split_len;
 	chunk->old_startblock += XFS_B_TO_FSB(data->mp, split_len);
-- 
2.47.3

Re: [PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Carlos Maiolino]

On Fri, Oct 17, 2025 at 08:07:02AM +0200, Christoph Hellwig wrote:
> When we are picking a zone for gc it might already be in the pipeline
> which can lead to us moving the same data twice resulting in in write
> amplification and a very unfortunate case where keep on garbage
> collecting the zone we just filled with migrated data stopping all
> forward progress.
> 
> Fix this by introducing a count of on-going GC operations on a zone, so

					  This 'so' shouldn't be here? ^

> and skip any zone with ongoing GC when picking a new victim.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> Co-developed-by: Hans Holmberg <hans.holmberg@wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This looks good, but I think it's worth to add a:

Fixes: 080d01c41 ("xfs: implement zoned garbage collection")


If you agree I can update both nitpicks above when I pull the series,
otherwise, feel free to add to the next version:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  fs/xfs/libxfs/xfs_rtgroup.h |  6 ++++++
>  fs/xfs/xfs_zone_gc.c        | 27 +++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtgroup.h b/fs/xfs/libxfs/xfs_rtgroup.h
> index d36a6ae0abe5..d4fcf591e63d 100644
> --- a/fs/xfs/libxfs/xfs_rtgroup.h
> +++ b/fs/xfs/libxfs/xfs_rtgroup.h
> @@ -50,6 +50,12 @@ struct xfs_rtgroup {
>  		uint8_t			*rtg_rsum_cache;
>  		struct xfs_open_zone	*rtg_open_zone;
>  	};
> +
> +	/*
> +	 * Count of outstanding GC operations for zoned XFS.  Any RTG with a
> +	 * non-zero rtg_gccount will not be picked as new GC victim.
> +	 */
> +	atomic_t		rtg_gccount;
>  };
> 
>  /*
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index 109877d9a6bf..efcb52796d05 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -114,6 +114,8 @@ struct xfs_gc_bio {
>  	/* Open Zone being written to */
>  	struct xfs_open_zone		*oz;
> 
> +	struct xfs_rtgroup		*victim_rtg;
> +
>  	/* Bio used for reads and writes, including the bvec used by it */
>  	struct bio_vec			bv;
>  	struct bio			bio;	/* must be last */
> @@ -264,6 +266,7 @@ xfs_zone_gc_iter_init(
>  	iter->rec_count = 0;
>  	iter->rec_idx = 0;
>  	iter->victim_rtg = victim_rtg;
> +	atomic_inc(&victim_rtg->rtg_gccount);
>  }
> 
>  /*
> @@ -362,6 +365,7 @@ xfs_zone_gc_query(
> 
>  	return 0;
>  done:
> +	atomic_dec(&iter->victim_rtg->rtg_gccount);
>  	xfs_rtgroup_rele(iter->victim_rtg);
>  	iter->victim_rtg = NULL;
>  	return 0;
> @@ -451,6 +455,20 @@ xfs_zone_gc_pick_victim_from(
>  		if (!rtg)
>  			continue;
> 
> +		/*
> +		 * If the zone is already undergoing GC, don't pick it again.
> +		 *
> +		 * This prevents us from picking one of the zones for which we
> +		 * already submitted GC I/O, but for which the remapping hasn't
> +		 * concluded again.  This won't cause data corruption, but
> +		 * increases write amplification and slows down GC, so this is
> +		 * a bad thing.
> +		 */
> +		if (atomic_read(&rtg->rtg_gccount)) {
> +			xfs_rtgroup_rele(rtg);
> +			continue;
> +		}
> +
>  		/* skip zones that are just waiting for a reset */
>  		if (rtg_rmap(rtg)->i_used_blocks == 0 ||
>  		    rtg_rmap(rtg)->i_used_blocks >= victim_used) {
> @@ -688,6 +706,9 @@ xfs_zone_gc_start_chunk(
>  	chunk->scratch = &data->scratch[data->scratch_idx];
>  	chunk->data = data;
>  	chunk->oz = oz;
> +	chunk->victim_rtg = iter->victim_rtg;
> +	atomic_inc(&chunk->victim_rtg->rtg_group.xg_active_ref);
> +	atomic_inc(&chunk->victim_rtg->rtg_gccount);
> 
>  	bio->bi_iter.bi_sector = xfs_rtb_to_daddr(mp, chunk->old_startblock);
>  	bio->bi_end_io = xfs_zone_gc_end_io;
> @@ -710,6 +731,8 @@ static void
>  xfs_zone_gc_free_chunk(
>  	struct xfs_gc_bio	*chunk)
>  {
> +	atomic_dec(&chunk->victim_rtg->rtg_gccount);
> +	xfs_rtgroup_rele(chunk->victim_rtg);
>  	list_del(&chunk->entry);
>  	xfs_open_zone_put(chunk->oz);
>  	xfs_irele(chunk->ip);
> @@ -770,6 +793,10 @@ xfs_zone_gc_split_write(
>  	split_chunk->oz = chunk->oz;
>  	atomic_inc(&chunk->oz->oz_ref);
> 
> +	split_chunk->victim_rtg = chunk->victim_rtg;
> +	atomic_inc(&chunk->victim_rtg->rtg_group.xg_active_ref);
> +	atomic_inc(&chunk->victim_rtg->rtg_gccount);
> +
>  	chunk->offset += split_len;
>  	chunk->len -= split_len;
>  	chunk->old_startblock += XFS_B_TO_FSB(data->mp, split_len);
> --
> 2.47.3
> 

Re: [PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Christoph Hellwig]

On Fri, Oct 17, 2025 at 02:37:02PM +0200, Carlos Maiolino wrote:
> This looks good, but I think it's worth to add a:
> 
> Fixes: 080d01c41 ("xfs: implement zoned garbage collection")
> 
> 
> If you agree I can update both nitpicks above when I pull the series,
> otherwise, feel free to add to the next version:

Sure.  Let's see if I need a resend for another reason, but otherwise
your tweaks look good.

Re: [PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Damien Le Moal]

On 10/17/25 15:07, Christoph Hellwig wrote:
> When we are picking a zone for gc it might already be in the pipeline
> which can lead to us moving the same data twice resulting in in write
> amplification and a very unfortunate case where keep on garbage

s/keep/we keep

> collecting the zone we just filled with migrated data stopping all
> forward progress.
> 
> Fix this by introducing a count of on-going GC operations on a zone, so
> and skip any zone with ongoing GC when picking a new victim.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> Co-developed-by: Hans Holmberg <hans.holmberg@wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Tested-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Darrick J. Wong]

On Fri, Oct 17, 2025 at 08:07:02AM +0200, Christoph Hellwig wrote:
> When we are picking a zone for gc it might already be in the pipeline
> which can lead to us moving the same data twice resulting in in write
> amplification and a very unfortunate case where keep on garbage
> collecting the zone we just filled with migrated data stopping all
> forward progress.
> 
> Fix this by introducing a count of on-going GC operations on a zone, so
> and skip any zone with ongoing GC when picking a new victim.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> Co-developed-by: Hans Holmberg <hans.holmberg@wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_rtgroup.h |  6 ++++++
>  fs/xfs/xfs_zone_gc.c        | 27 +++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtgroup.h b/fs/xfs/libxfs/xfs_rtgroup.h
> index d36a6ae0abe5..d4fcf591e63d 100644
> --- a/fs/xfs/libxfs/xfs_rtgroup.h
> +++ b/fs/xfs/libxfs/xfs_rtgroup.h
> @@ -50,6 +50,12 @@ struct xfs_rtgroup {
>  		uint8_t			*rtg_rsum_cache;
>  		struct xfs_open_zone	*rtg_open_zone;
>  	};
> +
> +	/*
> +	 * Count of outstanding GC operations for zoned XFS.  Any RTG with a
> +	 * non-zero rtg_gccount will not be picked as new GC victim.
> +	 */
> +	atomic_t		rtg_gccount;
>  };
>  
>  /*
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index 109877d9a6bf..efcb52796d05 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -114,6 +114,8 @@ struct xfs_gc_bio {
>  	/* Open Zone being written to */
>  	struct xfs_open_zone		*oz;
>  
> +	struct xfs_rtgroup		*victim_rtg;
> +
>  	/* Bio used for reads and writes, including the bvec used by it */
>  	struct bio_vec			bv;
>  	struct bio			bio;	/* must be last */
> @@ -264,6 +266,7 @@ xfs_zone_gc_iter_init(
>  	iter->rec_count = 0;
>  	iter->rec_idx = 0;
>  	iter->victim_rtg = victim_rtg;
> +	atomic_inc(&victim_rtg->rtg_gccount);
>  }
>  
>  /*
> @@ -362,6 +365,7 @@ xfs_zone_gc_query(
>  
>  	return 0;
>  done:
> +	atomic_dec(&iter->victim_rtg->rtg_gccount);
>  	xfs_rtgroup_rele(iter->victim_rtg);
>  	iter->victim_rtg = NULL;
>  	return 0;
> @@ -451,6 +455,20 @@ xfs_zone_gc_pick_victim_from(
>  		if (!rtg)
>  			continue;
>  
> +		/*
> +		 * If the zone is already undergoing GC, don't pick it again.
> +		 *
> +		 * This prevents us from picking one of the zones for which we
> +		 * already submitted GC I/O, but for which the remapping hasn't
> +		 * concluded again.  This won't cause data corruption, but

"...but that I/O hasn't yet finished."

With that and the other comments corrected and a Fixes tag applied,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +		 * increases write amplification and slows down GC, so this is
> +		 * a bad thing.
> +		 */
> +		if (atomic_read(&rtg->rtg_gccount)) {
> +			xfs_rtgroup_rele(rtg);
> +			continue;
> +		}
> +
>  		/* skip zones that are just waiting for a reset */
>  		if (rtg_rmap(rtg)->i_used_blocks == 0 ||
>  		    rtg_rmap(rtg)->i_used_blocks >= victim_used) {
> @@ -688,6 +706,9 @@ xfs_zone_gc_start_chunk(
>  	chunk->scratch = &data->scratch[data->scratch_idx];
>  	chunk->data = data;
>  	chunk->oz = oz;
> +	chunk->victim_rtg = iter->victim_rtg;
> +	atomic_inc(&chunk->victim_rtg->rtg_group.xg_active_ref);
> +	atomic_inc(&chunk->victim_rtg->rtg_gccount);
>  
>  	bio->bi_iter.bi_sector = xfs_rtb_to_daddr(mp, chunk->old_startblock);
>  	bio->bi_end_io = xfs_zone_gc_end_io;
> @@ -710,6 +731,8 @@ static void
>  xfs_zone_gc_free_chunk(
>  	struct xfs_gc_bio	*chunk)
>  {
> +	atomic_dec(&chunk->victim_rtg->rtg_gccount);
> +	xfs_rtgroup_rele(chunk->victim_rtg);
>  	list_del(&chunk->entry);
>  	xfs_open_zone_put(chunk->oz);
>  	xfs_irele(chunk->ip);
> @@ -770,6 +793,10 @@ xfs_zone_gc_split_write(
>  	split_chunk->oz = chunk->oz;
>  	atomic_inc(&chunk->oz->oz_ref);
>  
> +	split_chunk->victim_rtg = chunk->victim_rtg;
> +	atomic_inc(&chunk->victim_rtg->rtg_group.xg_active_ref);
> +	atomic_inc(&chunk->victim_rtg->rtg_gccount);
> +
>  	chunk->offset += split_len;
>  	chunk->len -= split_len;
>  	chunk->old_startblock += XFS_B_TO_FSB(data->mp, split_len);
> -- 
> 2.47.3
> 
> 

Re: [PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Christoph Hellwig]

On Wed, Oct 22, 2025 at 11:16:22PM -0700, Darrick J. Wong wrote:
> > +		/*
> > +		 * If the zone is already undergoing GC, don't pick it again.
> > +		 *
> > +		 * This prevents us from picking one of the zones for which we
> > +		 * already submitted GC I/O, but for which the remapping hasn't
> > +		 * concluded again.  This won't cause data corruption, but
> 
> "...but that I/O hasn't yet finished."

It's really the remapping after the I/O that has to finish, but given
that we talk about I/O earlier, maybe your wording is less confusing.

Re: [PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Darrick J. Wong]

On Thu, Oct 23, 2025 at 08:28:29AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 22, 2025 at 11:16:22PM -0700, Darrick J. Wong wrote:
> > > +		/*
> > > +		 * If the zone is already undergoing GC, don't pick it again.
> > > +		 *
> > > +		 * This prevents us from picking one of the zones for which we
> > > +		 * already submitted GC I/O, but for which the remapping hasn't
> > > +		 * concluded again.  This won't cause data corruption, but
> > 
> > "...but that I/O hasn't yet finished."
> 
> It's really the remapping after the I/O that has to finish, but given
> that we talk about I/O earlier, maybe your wording is less confusing.

The word 'again' at the end of the sentence tripped me up.  I think the
word you want there is "yet" (because "again" implies that this isn't
the first time we've remapped the space, which isn't true) but you could
also end the sentence wit` "concluded".

--D

Re: [PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Christoph Hellwig]

On Thu, Oct 23, 2025 at 08:02:32AM -0700, Darrick J. Wong wrote:
> > > > +		 *
> > > > +		 * This prevents us from picking one of the zones for which we
> > > > +		 * already submitted GC I/O, but for which the remapping hasn't
> > > > +		 * concluded again.  This won't cause data corruption, but
> > > 
> > > "...but that I/O hasn't yet finished."
> > 
> > It's really the remapping after the I/O that has to finish, but given
> > that we talk about I/O earlier, maybe your wording is less confusing.
> 
> The word 'again' at the end of the sentence tripped me up.  I think the
> word you want there is "yet" (because "again" implies that this isn't
> the first time we've remapped the space, which isn't true) but you could
> also end the sentence wit` "concluded".

Both sounds fine.

[PATCH 2/2] xfs: document another racy GC case in xfs_zoned_map_extent

[Christoph Hellwig]

Besides blocks being invalidated, there is another case when the original
mapping could have changed between querying the rmap for GC and calling
xfs_zoned_map_extent.  Document it there as it took us quite some time
to figure out what is going on while developing the multiple-GC
protection fix.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_zone_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
index e7e439918f6d..2790001ee0f1 100644
--- a/fs/xfs/xfs_zone_alloc.c
+++ b/fs/xfs/xfs_zone_alloc.c
@@ -246,6 +246,14 @@ xfs_zoned_map_extent(
 	 * If a data write raced with this GC write, keep the existing data in
 	 * the data fork, mark our newly written GC extent as reclaimable, then
 	 * move on to the next extent.
+	 *
+	 * Note that this can also happen when racing with operations that do
+	 * not actually invalidate the data, but just move it to a different
+	 * inode (XFS_IOC_EXCHANGE_RANGE), or to a different offset inside the
+	 * inode (FALLOC_FL_COLLAPSE_RANGE / FALLOC_FL_INSERT_RANGE).  If the
+	 * data was just moved around, GC fails to free the zone, but the zone
+	 * becomes a GC candidate again as soon as all previous GC I/O has
+	 * finished and these blocks will be moved out eventually.
 	 */
 	if (old_startblock != NULLFSBLOCK &&
 	    old_startblock != data.br_startblock)
-- 
2.47.3

Re: [PATCH 2/2] xfs: document another racy GC case in xfs_zoned_map_extent

[Carlos Maiolino]

On Fri, Oct 17, 2025 at 08:07:03AM +0200, Christoph Hellwig wrote:
> Besides blocks being invalidated, there is another case when the original
> mapping could have changed between querying the rmap for GC and calling
> xfs_zoned_map_extent.  Document it there as it took us quite some time
> to figure out what is going on while developing the multiple-GC
> protection fix.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_alloc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index e7e439918f6d..2790001ee0f1 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -246,6 +246,14 @@ xfs_zoned_map_extent(
>  	 * If a data write raced with this GC write, keep the existing data in
>  	 * the data fork, mark our newly written GC extent as reclaimable, then
>  	 * move on to the next extent.
> +	 *
> +	 * Note that this can also happen when racing with operations that do
> +	 * not actually invalidate the data, but just move it to a different
> +	 * inode (XFS_IOC_EXCHANGE_RANGE), or to a different offset inside the
> +	 * inode (FALLOC_FL_COLLAPSE_RANGE / FALLOC_FL_INSERT_RANGE).  If the
> +	 * data was just moved around, GC fails to free the zone, but the zone
> +	 * becomes a GC candidate again as soon as all previous GC I/O has
> +	 * finished and these blocks will be moved out eventually.
>  	 */
>  	if (old_startblock != NULLFSBLOCK &&
>  	    old_startblock != data.br_startblock)
> --
> 2.47.3

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 

Re: [PATCH 2/2] xfs: document another racy GC case in xfs_zoned_map_extent

[Damien Le Moal]

On 10/17/25 15:07, Christoph Hellwig wrote:
> Besides blocks being invalidated, there is another case when the original
> mapping could have changed between querying the rmap for GC and calling
> xfs_zoned_map_extent.  Document it there as it took us quite some time
> to figure out what is going on while developing the multiple-GC
> protection fix.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] xfs: document another racy GC case in xfs_zoned_map_extent

[Hans Holmberg]

On 17/10/2025 08:07, Christoph Hellwig wrote:
> Besides blocks being invalidated, there is another case when the original
> mapping could have changed between querying the rmap for GC and calling
> xfs_zoned_map_extent.  Document it there as it took us quite some time
> to figure out what is going on while developing the multiple-GC
> protection fix.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_alloc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index e7e439918f6d..2790001ee0f1 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -246,6 +246,14 @@ xfs_zoned_map_extent(
>  	 * If a data write raced with this GC write, keep the existing data in
>  	 * the data fork, mark our newly written GC extent as reclaimable, then
>  	 * move on to the next extent.
> +	 *
> +	 * Note that this can also happen when racing with operations that do
> +	 * not actually invalidate the data, but just move it to a different
> +	 * inode (XFS_IOC_EXCHANGE_RANGE), or to a different offset inside the
> +	 * inode (FALLOC_FL_COLLAPSE_RANGE / FALLOC_FL_INSERT_RANGE).  If the
> +	 * data was just moved around, GC fails to free the zone, but the zone
> +	 * becomes a GC candidate again as soon as all previous GC I/O has
> +	 * finished and these blocks will be moved out eventually.
>  	 */
>  	if (old_startblock != NULLFSBLOCK &&
>  	    old_startblock != data.br_startblock)

Reviewed-by: Hans Holmberg <hans.holmberg@wdc.com>

Re: [PATCH 2/2] xfs: document another racy GC case in xfs_zoned_map_extent

[Darrick J. Wong]

On Fri, Oct 17, 2025 at 08:07:03AM +0200, Christoph Hellwig wrote:
> Besides blocks being invalidated, there is another case when the original
> mapping could have changed between querying the rmap for GC and calling
> xfs_zoned_map_extent.  Document it there as it took us quite some time
> to figure out what is going on while developing the multiple-GC
> protection fix.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_alloc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index e7e439918f6d..2790001ee0f1 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -246,6 +246,14 @@ xfs_zoned_map_extent(
>  	 * If a data write raced with this GC write, keep the existing data in
>  	 * the data fork, mark our newly written GC extent as reclaimable, then
>  	 * move on to the next extent.
> +	 *
> +	 * Note that this can also happen when racing with operations that do
> +	 * not actually invalidate the data, but just move it to a different
> +	 * inode (XFS_IOC_EXCHANGE_RANGE), or to a different offset inside the
> +	 * inode (FALLOC_FL_COLLAPSE_RANGE / FALLOC_FL_INSERT_RANGE).  If the

Or (eventually) this will also be possible if zonegc races with a plain
remapping via FICLONERANGE, right?

(Like, whenever reflink gets implemented)

I guess the zonegc write completion could just iterate the rtrmapbt for
records that overlap the old extent and remap them until there are no
rtrmapbt entries returned.  Then you'd be able to catch the
exchange-range and reflink cases, right?

(Not asking for a reflink-aware zonegc right this second, just checking
my understanding.)

If the answers to both questions are yes then I've grokked this well
enough to say
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +	 * data was just moved around, GC fails to free the zone, but the zone
> +	 * becomes a GC candidate again as soon as all previous GC I/O has
> +	 * finished and these blocks will be moved out eventually.
>  	 */
>  	if (old_startblock != NULLFSBLOCK &&
>  	    old_startblock != data.br_startblock)
> -- 
> 2.47.3
> 
> 

Re: [PATCH 2/2] xfs: document another racy GC case in xfs_zoned_map_extent

[Christoph Hellwig]

On Wed, Oct 22, 2025 at 11:21:25PM -0700, Darrick J. Wong wrote:
> > +	 * Note that this can also happen when racing with operations that do
> > +	 * not actually invalidate the data, but just move it to a different
> > +	 * inode (XFS_IOC_EXCHANGE_RANGE), or to a different offset inside the
> > +	 * inode (FALLOC_FL_COLLAPSE_RANGE / FALLOC_FL_INSERT_RANGE).  If the
> 
> Or (eventually) this will also be possible if zonegc races with a plain
> remapping via FICLONERANGE, right?
> 
> (Like, whenever reflink gets implemented)

Yes.

> I guess the zonegc write completion could just iterate the rtrmapbt for
> records that overlap the old extent and remap them until there are no
> rtrmapbt entries returned.  Then you'd be able to catch the
> exchange-range and reflink cases, right?

I don't think so.  All these remap operation would at this point would
have remove the rmapbt record for the old record because they changed
the logical mapping.  So I don't think we'd find anything useful there.

fix for selecting a zone with active GC I/O for GC v2

[Christoph Hellwig]

Hi all,

the first patch in the series adds a counter to ensure that we never
select a zone for which there is active GC I/O for GC again, as we'd
try to copy the same blocks again, only to see the mapping had changed
in I/O completion to discard that I/O.

The second one documents an non-obvious version of that GC race detection
that took us way too long to track down.

Changes since v1:
 - fix text in a comment and a commit message

[PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Christoph Hellwig]

When we are picking a zone for gc it might already be in the pipeline
which can lead to us moving the same data twice resulting in in write
amplification and a very unfortunate case where we keep on garbage
collecting the zone we just filled with migrated data stopping all
forward progress.

Fix this by introducing a count of on-going GC operations on a zone, and
skip any zone with ongoing GC when picking a new victim.

Fixes: 080d01c41 ("xfs: implement zoned garbage collection")
Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
Co-developed-by: Hans Holmberg <hans.holmberg@wdc.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Tested-by: Damien Le Moal <dlemoal@kernel.org>
---
 fs/xfs/libxfs/xfs_rtgroup.h |  6 ++++++
 fs/xfs/xfs_zone_gc.c        | 27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_rtgroup.h b/fs/xfs/libxfs/xfs_rtgroup.h
index d36a6ae0abe5..d4fcf591e63d 100644
--- a/fs/xfs/libxfs/xfs_rtgroup.h
+++ b/fs/xfs/libxfs/xfs_rtgroup.h
@@ -50,6 +50,12 @@ struct xfs_rtgroup {
 		uint8_t			*rtg_rsum_cache;
 		struct xfs_open_zone	*rtg_open_zone;
 	};
+
+	/*
+	 * Count of outstanding GC operations for zoned XFS.  Any RTG with a
+	 * non-zero rtg_gccount will not be picked as new GC victim.
+	 */
+	atomic_t		rtg_gccount;
 };
 
 /*
diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 109877d9a6bf..4ade54445532 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -114,6 +114,8 @@ struct xfs_gc_bio {
 	/* Open Zone being written to */
 	struct xfs_open_zone		*oz;
 
+	struct xfs_rtgroup		*victim_rtg;
+
 	/* Bio used for reads and writes, including the bvec used by it */
 	struct bio_vec			bv;
 	struct bio			bio;	/* must be last */
@@ -264,6 +266,7 @@ xfs_zone_gc_iter_init(
 	iter->rec_count = 0;
 	iter->rec_idx = 0;
 	iter->victim_rtg = victim_rtg;
+	atomic_inc(&victim_rtg->rtg_gccount);
 }
 
 /*
@@ -362,6 +365,7 @@ xfs_zone_gc_query(
 
 	return 0;
 done:
+	atomic_dec(&iter->victim_rtg->rtg_gccount);
 	xfs_rtgroup_rele(iter->victim_rtg);
 	iter->victim_rtg = NULL;
 	return 0;
@@ -451,6 +455,20 @@ xfs_zone_gc_pick_victim_from(
 		if (!rtg)
 			continue;
 
+		/*
+		 * If the zone is already undergoing GC, don't pick it again.
+		 *
+		 * This prevents us from picking one of the zones for which we
+		 * already submitted GC I/O, but for which the remapping hasn't
+		 * concluded yet.  This won't cause data corruption, but
+		 * increases write amplification and slows down GC, so this is
+		 * a bad thing.
+		 */
+		if (atomic_read(&rtg->rtg_gccount)) {
+			xfs_rtgroup_rele(rtg);
+			continue;
+		}
+
 		/* skip zones that are just waiting for a reset */
 		if (rtg_rmap(rtg)->i_used_blocks == 0 ||
 		    rtg_rmap(rtg)->i_used_blocks >= victim_used) {
@@ -688,6 +706,9 @@ xfs_zone_gc_start_chunk(
 	chunk->scratch = &data->scratch[data->scratch_idx];
 	chunk->data = data;
 	chunk->oz = oz;
+	chunk->victim_rtg = iter->victim_rtg;
+	atomic_inc(&chunk->victim_rtg->rtg_group.xg_active_ref);
+	atomic_inc(&chunk->victim_rtg->rtg_gccount);
 
 	bio->bi_iter.bi_sector = xfs_rtb_to_daddr(mp, chunk->old_startblock);
 	bio->bi_end_io = xfs_zone_gc_end_io;
@@ -710,6 +731,8 @@ static void
 xfs_zone_gc_free_chunk(
 	struct xfs_gc_bio	*chunk)
 {
+	atomic_dec(&chunk->victim_rtg->rtg_gccount);
+	xfs_rtgroup_rele(chunk->victim_rtg);
 	list_del(&chunk->entry);
 	xfs_open_zone_put(chunk->oz);
 	xfs_irele(chunk->ip);
@@ -770,6 +793,10 @@ xfs_zone_gc_split_write(
 	split_chunk->oz = chunk->oz;
 	atomic_inc(&chunk->oz->oz_ref);
 
+	split_chunk->victim_rtg = chunk->victim_rtg;
+	atomic_inc(&chunk->victim_rtg->rtg_group.xg_active_ref);
+	atomic_inc(&chunk->victim_rtg->rtg_gccount);
+
 	chunk->offset += split_len;
 	chunk->len -= split_len;
 	chunk->old_startblock += XFS_B_TO_FSB(data->mp, split_len);
-- 
2.47.3

Re: [PATCH 1/2] xfs: prevent gc from picking the same zone twice

[Carlos Maiolino]

On Thu, 23 Oct 2025 17:17:02 +0200, Christoph Hellwig wrote:
> When we are picking a zone for gc it might already be in the pipeline
> which can lead to us moving the same data twice resulting in in write
> amplification and a very unfortunate case where we keep on garbage
> collecting the zone we just filled with migrated data stopping all
> forward progress.
> 
> Fix this by introducing a count of on-going GC operations on a zone, and
> skip any zone with ongoing GC when picking a new victim.
> 
> [...]

Applied to for-next, thanks!

[1/2] xfs: prevent gc from picking the same zone twice
      commit: 83bac569c762651ac6dff9a86f54ecc13d911f7d
[2/2] xfs: document another racy GC case in xfs_zoned_map_extent
      commit: 0db22d7ee462c42c1284e98d47840932792c1adb

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>