cleanup open GC zone handling

cleanup open GC zone handling

[Christoph Hellwig]

Hi all,

this series cleans up the handling of open GC zones so that the
reference counting works more similar to normal open GC zones.
This cleans up the code in general, and also helps with experiments
to support multiple open zones for GC I've started.

Diffstat
 xfs_trace.h      |    1 
 xfs_zone_alloc.c |   21 +++--
 xfs_zone_gc.c    |  230 +++++++++++++++++++++++++------------------------------
 xfs_zone_info.c  |   12 --
 xfs_zone_priv.h  |   15 ---
 5 files changed, 129 insertions(+), 150 deletions(-)

[PATCH 1/7] xfs: delay opening the GC zone

[Christoph Hellwig]

The code that selects a new GC zone when the previous one is full also
handles a zone not being set at all.  Make use of that to simplify the
logic in xfs_zone_gc_mount.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_zone_gc.c | 45 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 0ff710fa0ee7..f076f66c5d57 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -527,7 +527,7 @@ xfs_zone_gc_select_victim(
 	return true;
 }
 
-static struct xfs_open_zone *
+static int
 xfs_zone_gc_steal_open(
 	struct xfs_zone_info	*zi)
 {
@@ -538,15 +538,18 @@ xfs_zone_gc_steal_open(
 		if (!found || oz->oz_allocated < found->oz_allocated)
 			found = oz;
 	}
-
-	if (found) {
-		found->oz_is_gc = true;
-		list_del_init(&found->oz_entry);
-		zi->zi_nr_open_zones--;
+	if (!found) {
+		spin_unlock(&zi->zi_open_zones_lock);
+		return -EIO;
 	}
 
+	trace_xfs_zone_gc_target_opened(found->oz_rtg);
+	found->oz_is_gc = true;
+	list_del_init(&found->oz_entry);
+	zi->zi_nr_open_zones--;
+	zi->zi_open_gc_zone = found;
 	spin_unlock(&zi->zi_open_zones_lock);
-	return found;
+	return 0;
 }
 
 static struct xfs_open_zone *
@@ -1177,31 +1180,24 @@ xfs_zone_gc_mount(
 {
 	struct xfs_zone_info	*zi = mp->m_zone_info;
 	struct xfs_zone_gc_data	*data;
-	struct xfs_open_zone	*oz;
 	int			error;
 
 	/*
-	 * If there are no free zones available for GC, pick the open zone with
+	 * If there are no free zones available for GC, or the number of open
+	 * zones has reached the open zone limit, pick the open zone with
 	 * the least used space to GC into.  This should only happen after an
-	 * unclean shutdown near ENOSPC while GC was ongoing.
-	 *
-	 * We also need to do this for the first gc zone allocation if we
-	 * unmounted while at the open limit.
+	 * unclean shutdown while GC was ongoing.  Otherwise a GC zone will
+	 * be selected from the free zone pool on demand.
 	 */
 	if (!xfs_group_marked(mp, XG_TYPE_RTG, XFS_RTG_FREE) ||
-	    zi->zi_nr_open_zones == mp->m_max_open_zones)
-		oz = xfs_zone_gc_steal_open(zi);
-	else
-		oz = xfs_open_zone(mp, WRITE_LIFE_NOT_SET, true);
-	if (!oz) {
-		xfs_warn(mp, "unable to allocate a zone for gc");
-		error = -EIO;
-		goto out;
+	    zi->zi_nr_open_zones >= mp->m_max_open_zones) {
+		error = xfs_zone_gc_steal_open(zi);
+		if (error) {
+			xfs_warn(mp, "unable to steal an open zone for gc");
+			return error;
+		}
 	}
 
-	trace_xfs_zone_gc_target_opened(oz->oz_rtg);
-	zi->zi_open_gc_zone = oz;
-
 	data = xfs_zone_gc_data_alloc(mp);
 	if (!data) {
 		error = -ENOMEM;
@@ -1224,7 +1220,6 @@ xfs_zone_gc_mount(
 	kfree(data);
 out_put_gc_zone:
 	xfs_open_zone_put(zi->zi_open_gc_zone);
-out:
 	return error;
 }
 
-- 
2.47.3

[PATCH 2/7] xfs: add a separate tracepoint for stealing an open zone for GC

[Christoph Hellwig]

The case where we have to reuse an already open zone warrants a different
trace point vs the normal opening of a GC zone.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_trace.h   | 1 +
 fs/xfs/xfs_zone_gc.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 433f84252119..c1aa15973e74 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -394,6 +394,7 @@ DEFINE_ZONE_EVENT(xfs_zone_full);
 DEFINE_ZONE_EVENT(xfs_zone_opened);
 DEFINE_ZONE_EVENT(xfs_zone_reset);
 DEFINE_ZONE_EVENT(xfs_zone_gc_target_opened);
+DEFINE_ZONE_EVENT(xfs_zone_gc_target_stolen);
 
 TRACE_EVENT(xfs_zone_free_blocks,
 	TP_PROTO(struct xfs_rtgroup *rtg, xfs_rgblock_t rgbno,
diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index f076f66c5d57..273c496c2f1c 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -543,7 +543,7 @@ xfs_zone_gc_steal_open(
 		return -EIO;
 	}
 
-	trace_xfs_zone_gc_target_opened(found->oz_rtg);
+	trace_xfs_zone_gc_target_stolen(found->oz_rtg);
 	found->oz_is_gc = true;
 	list_del_init(&found->oz_entry);
 	zi->zi_nr_open_zones--;
-- 
2.47.3

[PATCH 3/7] xfs: put the open zone later xfs_open_zone_put

[Christoph Hellwig]

The open zone is what holds the rtg reference for us.  This doesn't
matter until we support shrinking, and even then is rather theoretical
because we can't shrink away a just filled zone in a tiny race window,
but let's play safe here.

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

diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
index b0a6bc777c36..297a918bf28e 100644
--- a/fs/xfs/xfs_zone_alloc.c
+++ b/fs/xfs/xfs_zone_alloc.c
@@ -182,11 +182,11 @@ xfs_open_zone_mark_full(
 		list_del_init(&oz->oz_entry);
 	}
 	spin_unlock(&zi->zi_open_zones_lock);
-	xfs_open_zone_put(oz);
 
 	wake_up_all(&zi->zi_zone_wait);
 	if (used < rtg_blocks(rtg))
 		xfs_zone_account_reclaimable(rtg, rtg_blocks(rtg) - used);
+	xfs_open_zone_put(oz);
 }
 
 static inline void
-- 
2.47.3

[PATCH 4/7] xfs: rename xfs_zone_gc_iter_next to xfs_zone_gc_iter_irec

[Christoph Hellwig]

This function returns the current iterator position, which makes the
_next postfix a bit misleading.

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

diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 273c496c2f1c..3723f5293a64 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -374,7 +374,7 @@ xfs_zone_gc_query(
 }
 
 static bool
-xfs_zone_gc_iter_next(
+xfs_zone_gc_iter_irec(
 	struct xfs_mount	*mp,
 	struct xfs_zone_gc_iter	*iter,
 	struct xfs_rmap_irec	*chunk_rec,
@@ -691,7 +691,7 @@ xfs_zone_gc_start_chunk(
 	if (xfs_is_shutdown(mp))
 		return false;
 
-	if (!xfs_zone_gc_iter_next(mp, iter, &irec, &ip))
+	if (!xfs_zone_gc_iter_irec(mp, iter, &irec, &ip))
 		return false;
 	oz = xfs_zone_gc_alloc_blocks(data, &irec.rm_blockcount, &daddr,
 			&is_seq);
-- 
2.47.3

[PATCH 5/7] xfs: refactor GC zone selection helpers

[Christoph Hellwig]

Merge xfs_zone_gc_ensure_target into xfs_zone_gc_select_target
to keep all zone selection code together.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_zone_gc.c | 45 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 3723f5293a64..6ce5a0708a9b 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -552,6 +552,9 @@ xfs_zone_gc_steal_open(
 	return 0;
 }
 
+/*
+ * Ensure we have a valid open zone to write to.
+ */
 static struct xfs_open_zone *
 xfs_zone_gc_select_target(
 	struct xfs_mount	*mp)
@@ -559,12 +562,25 @@ xfs_zone_gc_select_target(
 	struct xfs_zone_info	*zi = mp->m_zone_info;
 	struct xfs_open_zone	*oz = zi->zi_open_gc_zone;
 
+	if (oz) {
+		/*
+		 * If we have space available, just keep using the existing
+		 * zone.
+		 */
+		if (oz->oz_allocated < rtg_blocks(oz->oz_rtg))
+			return oz;
+
+		/*
+		 * Wait for all writes to the current zone to finish before
+		 * picking a new one.
+		 */
+		if (oz->oz_written < rtg_blocks(oz->oz_rtg))
+			return NULL;
+	}
+
 	/*
-	 * We need to wait for pending writes to finish.
+	 * Open a new zone when there is none currently in use.
 	 */
-	if (oz && oz->oz_written < rtg_blocks(oz->oz_rtg))
-		return NULL;
-
 	ASSERT(zi->zi_nr_open_zones <=
 		mp->m_max_open_zones - XFS_OPEN_GC_ZONES);
 	oz = xfs_open_zone(mp, WRITE_LIFE_NOT_SET, true);
@@ -576,23 +592,6 @@ xfs_zone_gc_select_target(
 	return oz;
 }
 
-/*
- * Ensure we have a valid open zone to write the GC data to.
- *
- * If the current target zone has space keep writing to it, else first wait for
- * all pending writes and then pick a new one.
- */
-static struct xfs_open_zone *
-xfs_zone_gc_ensure_target(
-	struct xfs_mount	*mp)
-{
-	struct xfs_open_zone	*oz = mp->m_zone_info->zi_open_gc_zone;
-
-	if (!oz || oz->oz_allocated == rtg_blocks(oz->oz_rtg))
-		return xfs_zone_gc_select_target(mp);
-	return oz;
-}
-
 static void
 xfs_zone_gc_end_io(
 	struct bio		*bio)
@@ -615,7 +614,7 @@ xfs_zone_gc_alloc_blocks(
 	struct xfs_mount	*mp = data->mp;
 	struct xfs_open_zone	*oz;
 
-	oz = xfs_zone_gc_ensure_target(mp);
+	oz = xfs_zone_gc_select_target(mp);
 	if (!oz)
 		return NULL;
 
@@ -1019,7 +1018,7 @@ xfs_zone_gc_should_start_new_work(
 	if (!data->scratch_available)
 		return false;
 
-	oz = xfs_zone_gc_ensure_target(data->mp);
+	oz = xfs_zone_gc_select_target(data->mp);
 	if (!oz || oz->oz_allocated == rtg_blocks(oz->oz_rtg))
 		return false;
 
-- 
2.47.3

[PATCH 6/7] xfs: streamline GC zone selection

[Christoph Hellwig]

Currently picking of the GC target zone is done a bit odd, both in the
main "can we start new GC cycles" routine and in the low-level block
allocator for GC.  This was mostly done to work around the rules for
when code in a waitqueue wait loop can sleep.

But there is a trick to check if the process state has been set to
running to discover if the wait loop has to be retried all this becomes
much simpler.  So we can just select a GC zone just before writing,
and bail out of starting new work if we can't find usable zone.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_zone_gc.c | 95 +++++++++++++++++++-------------------------
 1 file changed, 40 insertions(+), 55 deletions(-)

diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 6ce5a0708a9b..24f20f99ae1a 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -383,9 +383,6 @@ xfs_zone_gc_iter_irec(
 	struct xfs_rmap_irec	*irec;
 	int			error;
 
-	if (!iter->victim_rtg)
-		return false;
-
 retry:
 	if (iter->rec_idx == iter->rec_count) {
 		error = xfs_zone_gc_query(mp, iter);
@@ -555,7 +552,7 @@ xfs_zone_gc_steal_open(
 /*
  * Ensure we have a valid open zone to write to.
  */
-static struct xfs_open_zone *
+static bool
 xfs_zone_gc_select_target(
 	struct xfs_mount	*mp)
 {
@@ -568,14 +565,14 @@ xfs_zone_gc_select_target(
 		 * zone.
 		 */
 		if (oz->oz_allocated < rtg_blocks(oz->oz_rtg))
-			return oz;
+			return true;
 
 		/*
 		 * Wait for all writes to the current zone to finish before
 		 * picking a new one.
 		 */
 		if (oz->oz_written < rtg_blocks(oz->oz_rtg))
-			return NULL;
+			return false;
 	}
 
 	/*
@@ -589,7 +586,7 @@ xfs_zone_gc_select_target(
 	spin_lock(&zi->zi_open_zones_lock);
 	zi->zi_open_gc_zone = oz;
 	spin_unlock(&zi->zi_open_zones_lock);
-	return oz;
+	return !!oz;
 }
 
 static void
@@ -604,7 +601,7 @@ xfs_zone_gc_end_io(
 	wake_up_process(data->mp->m_zone_info->zi_gc_thread);
 }
 
-static struct xfs_open_zone *
+static bool
 xfs_zone_gc_alloc_blocks(
 	struct xfs_zone_gc_data	*data,
 	xfs_extlen_t		*count_fsb,
@@ -612,11 +609,7 @@ xfs_zone_gc_alloc_blocks(
 	bool			*is_seq)
 {
 	struct xfs_mount	*mp = data->mp;
-	struct xfs_open_zone	*oz;
-
-	oz = xfs_zone_gc_select_target(mp);
-	if (!oz)
-		return NULL;
+	struct xfs_open_zone	*oz = mp->m_zone_info->zi_open_gc_zone;
 
 	*count_fsb = min(*count_fsb, XFS_B_TO_FSB(mp, data->scratch_available));
 
@@ -638,7 +631,7 @@ xfs_zone_gc_alloc_blocks(
 	spin_unlock(&mp->m_sb_lock);
 
 	if (!*count_fsb)
-		return NULL;
+		return false;
 
 	*daddr = xfs_gbno_to_daddr(rtg_group(oz->oz_rtg), 0);
 	*is_seq = bdev_zone_is_seq(mp->m_rtdev_targp->bt_bdev, *daddr);
@@ -646,7 +639,7 @@ xfs_zone_gc_alloc_blocks(
 		*daddr += XFS_FSB_TO_BB(mp, oz->oz_allocated);
 	oz->oz_allocated += *count_fsb;
 	atomic_inc(&oz->oz_ref);
-	return oz;
+	return true;
 }
 
 static void
@@ -671,6 +664,28 @@ xfs_zone_gc_add_data(
 	} while (len);
 }
 
+static bool
+xfs_zone_gc_can_start_chunk(
+	struct xfs_zone_gc_data	*data)
+{
+
+	if (xfs_is_shutdown(data->mp))
+		return false;
+	if (!data->scratch_available)
+		return false;
+
+	if (!data->iter.victim_rtg) {
+		if (kthread_should_stop() || kthread_should_park())
+			return false;
+		if (!xfs_zoned_need_gc(data->mp))
+			return false;
+		if (!xfs_zone_gc_select_victim(data))
+			return false;
+	}
+
+	return xfs_zone_gc_select_target(data->mp);
+}
+
 static bool
 xfs_zone_gc_start_chunk(
 	struct xfs_zone_gc_data	*data)
@@ -678,7 +693,6 @@ xfs_zone_gc_start_chunk(
 	struct xfs_zone_gc_iter	*iter = &data->iter;
 	struct xfs_mount	*mp = data->mp;
 	struct block_device	*bdev = mp->m_rtdev_targp->bt_bdev;
-	struct xfs_open_zone	*oz;
 	struct xfs_rmap_irec	irec;
 	struct xfs_gc_bio	*chunk;
 	struct xfs_inode	*ip;
@@ -687,14 +701,15 @@ xfs_zone_gc_start_chunk(
 	unsigned int		len;
 	bool			is_seq;
 
-	if (xfs_is_shutdown(mp))
+	if (!xfs_zone_gc_can_start_chunk(data))
 		return false;
 
+	set_current_state(TASK_RUNNING);
 	if (!xfs_zone_gc_iter_irec(mp, iter, &irec, &ip))
 		return false;
-	oz = xfs_zone_gc_alloc_blocks(data, &irec.rm_blockcount, &daddr,
-			&is_seq);
-	if (!oz) {
+
+	if (!xfs_zone_gc_alloc_blocks(data, &irec.rm_blockcount, &daddr,
+			&is_seq)) {
 		xfs_irele(ip);
 		return false;
 	}
@@ -713,7 +728,7 @@ xfs_zone_gc_start_chunk(
 	chunk->new_daddr = daddr;
 	chunk->is_seq = is_seq;
 	chunk->data = data;
-	chunk->oz = oz;
+	chunk->oz = mp->m_zone_info->zi_open_gc_zone;
 	chunk->victim_rtg = iter->victim_rtg;
 	atomic_inc(&rtg_group(chunk->victim_rtg)->xg_active_ref);
 	atomic_inc(&chunk->victim_rtg->rtg_gccount);
@@ -1007,33 +1022,6 @@ xfs_zone_gc_reset_zones(
 	} while (next);
 }
 
-static bool
-xfs_zone_gc_should_start_new_work(
-	struct xfs_zone_gc_data	*data)
-{
-	struct xfs_open_zone	*oz;
-
-	if (xfs_is_shutdown(data->mp))
-		return false;
-	if (!data->scratch_available)
-		return false;
-
-	oz = xfs_zone_gc_select_target(data->mp);
-	if (!oz || oz->oz_allocated == rtg_blocks(oz->oz_rtg))
-		return false;
-
-	if (!data->iter.victim_rtg) {
-		if (kthread_should_stop() || kthread_should_park())
-			return false;
-		if (!xfs_zoned_need_gc(data->mp))
-			return false;
-		if (!xfs_zone_gc_select_victim(data))
-			return false;
-	}
-
-	return true;
-}
-
 /*
  * Handle the work to read and write data for GC and to reset the zones,
  * including handling all completions.
@@ -1083,13 +1071,10 @@ xfs_zone_gc_handle_work(
 	}
 	blk_finish_plug(&plug);
 
-	if (xfs_zone_gc_should_start_new_work(data)) {
-		set_current_state(TASK_RUNNING);
-		blk_start_plug(&plug);
-		while (xfs_zone_gc_start_chunk(data))
-			;
-		blk_finish_plug(&plug);
-	}
+	blk_start_plug(&plug);
+	while (xfs_zone_gc_start_chunk(data))
+		;
+	blk_finish_plug(&plug);
 }
 
 /*
-- 
2.47.3

[PATCH 7/7] xfs: reduce special casing for the open GC zone

[Christoph Hellwig]

Currently the open zone used for garbage collection is a special snow
flake, and it that has been a bit annoying for some further zoned XFS
work I've been doing.

Remove the zi_open_gc_field and instead track the open GC zone in the
zi_open_zones list together with the normal open zones, and keep an extra
pointer and a reference of in the GC thread's data structure.  This means
anything iterating over open zones just has to look at zi_open_zones, and
the life time rules are consistent.  It also helps to add support for
multiple open GC zones if we ever need them, and removes a bit of code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_zone_alloc.c | 19 +++++++----
 fs/xfs/xfs_zone_gc.c    | 71 ++++++++++++++++++++++-------------------
 fs/xfs/xfs_zone_info.c  | 12 +++----
 fs/xfs/xfs_zone_priv.h  | 15 ++-------
 4 files changed, 58 insertions(+), 59 deletions(-)

diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
index 297a918bf28e..db27fdb3a7d3 100644
--- a/fs/xfs/xfs_zone_alloc.c
+++ b/fs/xfs/xfs_zone_alloc.c
@@ -174,16 +174,18 @@ xfs_open_zone_mark_full(
 	WRITE_ONCE(rtg->rtg_open_zone, NULL);
 
 	spin_lock(&zi->zi_open_zones_lock);
-	if (oz->oz_is_gc) {
-		ASSERT(current == zi->zi_gc_thread);
-		zi->zi_open_gc_zone = NULL;
-	} else {
+	if (oz->oz_is_gc)
+		zi->zi_nr_open_gc_zones--;
+	else
 		zi->zi_nr_open_zones--;
-		list_del_init(&oz->oz_entry);
-	}
+	list_del_init(&oz->oz_entry);
 	spin_unlock(&zi->zi_open_zones_lock);
 
-	wake_up_all(&zi->zi_zone_wait);
+	if (oz->oz_is_gc)
+		wake_up_process(zi->zi_gc_thread);
+	else
+		wake_up_all(&zi->zi_zone_wait);
+
 	if (used < rtg_blocks(rtg))
 		xfs_zone_account_reclaimable(rtg, rtg_blocks(rtg) - used);
 	xfs_open_zone_put(oz);
@@ -557,6 +559,9 @@ xfs_try_use_zone(
 	struct xfs_open_zone	*oz,
 	unsigned int		goodness)
 {
+	if (oz->oz_is_gc)
+		return false;
+
 	if (oz->oz_allocated == rtg_blocks(oz->oz_rtg))
 		return false;
 
diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 24f20f99ae1a..8c184127b351 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -125,6 +125,7 @@ struct xfs_zone_gc_iter {
  */
 struct xfs_zone_gc_data {
 	struct xfs_mount		*mp;
+	struct xfs_open_zone		*oz;
 
 	/* bioset used to allocate the gc_bios */
 	struct bio_set			bio_set;
@@ -525,9 +526,10 @@ xfs_zone_gc_select_victim(
 }
 
 static int
-xfs_zone_gc_steal_open(
-	struct xfs_zone_info	*zi)
+xfs_zone_gc_steal_open_zone(
+	struct xfs_zone_gc_data	*data)
 {
+	struct xfs_zone_info	*zi = data->mp->m_zone_info;
 	struct xfs_open_zone	*oz, *found = NULL;
 
 	spin_lock(&zi->zi_open_zones_lock);
@@ -542,10 +544,12 @@ xfs_zone_gc_steal_open(
 
 	trace_xfs_zone_gc_target_stolen(found->oz_rtg);
 	found->oz_is_gc = true;
-	list_del_init(&found->oz_entry);
 	zi->zi_nr_open_zones--;
-	zi->zi_open_gc_zone = found;
+	zi->zi_nr_open_gc_zones++;
 	spin_unlock(&zi->zi_open_zones_lock);
+
+	atomic_inc(&found->oz_ref);
+	data->oz = found;
 	return 0;
 }
 
@@ -554,39 +558,43 @@ xfs_zone_gc_steal_open(
  */
 static bool
 xfs_zone_gc_select_target(
-	struct xfs_mount	*mp)
+	struct xfs_zone_gc_data	*data)
 {
-	struct xfs_zone_info	*zi = mp->m_zone_info;
-	struct xfs_open_zone	*oz = zi->zi_open_gc_zone;
+	struct xfs_zone_info	*zi = data->mp->m_zone_info;
 
-	if (oz) {
+	if (data->oz) {
 		/*
 		 * If we have space available, just keep using the existing
 		 * zone.
 		 */
-		if (oz->oz_allocated < rtg_blocks(oz->oz_rtg))
+		if (data->oz->oz_allocated < rtg_blocks(data->oz->oz_rtg))
 			return true;
 
 		/*
 		 * Wait for all writes to the current zone to finish before
 		 * picking a new one.
 		 */
-		if (oz->oz_written < rtg_blocks(oz->oz_rtg))
+		if (data->oz->oz_written < rtg_blocks(data->oz->oz_rtg))
 			return false;
+
+		xfs_open_zone_put(data->oz);
 	}
 
 	/*
 	 * Open a new zone when there is none currently in use.
 	 */
 	ASSERT(zi->zi_nr_open_zones <=
-		mp->m_max_open_zones - XFS_OPEN_GC_ZONES);
-	oz = xfs_open_zone(mp, WRITE_LIFE_NOT_SET, true);
-	if (oz)
-		trace_xfs_zone_gc_target_opened(oz->oz_rtg);
+		data->mp->m_max_open_zones - XFS_OPEN_GC_ZONES);
+	data->oz = xfs_open_zone(data->mp, WRITE_LIFE_NOT_SET, true);
+	if (!data->oz)
+		return false;
+	trace_xfs_zone_gc_target_opened(data->oz->oz_rtg);
+	atomic_inc(&data->oz->oz_ref);
 	spin_lock(&zi->zi_open_zones_lock);
-	zi->zi_open_gc_zone = oz;
+	zi->zi_nr_open_gc_zones++;
+	list_add_tail(&data->oz->oz_entry, &zi->zi_open_zones);
 	spin_unlock(&zi->zi_open_zones_lock);
-	return !!oz;
+	return true;
 }
 
 static void
@@ -609,7 +617,7 @@ xfs_zone_gc_alloc_blocks(
 	bool			*is_seq)
 {
 	struct xfs_mount	*mp = data->mp;
-	struct xfs_open_zone	*oz = mp->m_zone_info->zi_open_gc_zone;
+	struct xfs_open_zone	*oz = data->oz;
 
 	*count_fsb = min(*count_fsb, XFS_B_TO_FSB(mp, data->scratch_available));
 
@@ -683,7 +691,7 @@ xfs_zone_gc_can_start_chunk(
 			return false;
 	}
 
-	return xfs_zone_gc_select_target(data->mp);
+	return xfs_zone_gc_select_target(data);
 }
 
 static bool
@@ -728,7 +736,7 @@ xfs_zone_gc_start_chunk(
 	chunk->new_daddr = daddr;
 	chunk->is_seq = is_seq;
 	chunk->data = data;
-	chunk->oz = mp->m_zone_info->zi_open_gc_zone;
+	chunk->oz = data->oz;
 	chunk->victim_rtg = iter->victim_rtg;
 	atomic_inc(&rtg_group(chunk->victim_rtg)->xg_active_ref);
 	atomic_inc(&chunk->victim_rtg->rtg_gccount);
@@ -1134,6 +1142,8 @@ xfs_zoned_gcd(
 	}
 	xfs_clear_zonegc_running(mp);
 
+	if (data->oz)
+		xfs_open_zone_put(data->oz);
 	if (data->iter.victim_rtg)
 		xfs_rtgroup_rele(data->iter.victim_rtg);
 
@@ -1166,6 +1176,10 @@ xfs_zone_gc_mount(
 	struct xfs_zone_gc_data	*data;
 	int			error;
 
+	data = xfs_zone_gc_data_alloc(mp);
+	if (!data)
+		return -ENOMEM;
+
 	/*
 	 * If there are no free zones available for GC, or the number of open
 	 * zones has reached the open zone limit, pick the open zone with
@@ -1175,35 +1189,30 @@ xfs_zone_gc_mount(
 	 */
 	if (!xfs_group_marked(mp, XG_TYPE_RTG, XFS_RTG_FREE) ||
 	    zi->zi_nr_open_zones >= mp->m_max_open_zones) {
-		error = xfs_zone_gc_steal_open(zi);
+		error = xfs_zone_gc_steal_open_zone(data);
 		if (error) {
 			xfs_warn(mp, "unable to steal an open zone for gc");
-			return error;
+			goto out_free_gc_data;
 		}
 	}
 
-	data = xfs_zone_gc_data_alloc(mp);
-	if (!data) {
-		error = -ENOMEM;
-		goto out_put_gc_zone;
-	}
-
 	zi->zi_gc_thread = kthread_create(xfs_zoned_gcd, data,
 			"xfs-zone-gc/%s", mp->m_super->s_id);
 	if (IS_ERR(zi->zi_gc_thread)) {
 		xfs_warn(mp, "unable to create zone gc thread");
 		error = PTR_ERR(zi->zi_gc_thread);
-		goto out_free_gc_data;
+		goto out_put_oz;
 	}
 
 	/* xfs_zone_gc_start will unpark for rw mounts */
 	kthread_park(zi->zi_gc_thread);
 	return 0;
 
+out_put_oz:
+	if (data->oz)
+		xfs_open_zone_put(data->oz);
 out_free_gc_data:
 	kfree(data);
-out_put_gc_zone:
-	xfs_open_zone_put(zi->zi_open_gc_zone);
 	return error;
 }
 
@@ -1214,6 +1223,4 @@ xfs_zone_gc_unmount(
 	struct xfs_zone_info	*zi = mp->m_zone_info;
 
 	kthread_stop(zi->zi_gc_thread);
-	if (zi->zi_open_gc_zone)
-		xfs_open_zone_put(zi->zi_open_gc_zone);
 }
diff --git a/fs/xfs/xfs_zone_info.c b/fs/xfs/xfs_zone_info.c
index 8e56181021b3..47b475e21af8 100644
--- a/fs/xfs/xfs_zone_info.c
+++ b/fs/xfs/xfs_zone_info.c
@@ -30,11 +30,12 @@ xfs_show_open_zone(
 	struct seq_file		*m,
 	struct xfs_open_zone	*oz)
 {
-	seq_printf(m, "\t  zone %d, wp %u, written %u, used %u, hint %s\n",
+	seq_printf(m, "\t  zone %d, wp %u, written %u, used %u, hint %s %s\n",
 		rtg_rgno(oz->oz_rtg),
 		oz->oz_allocated, oz->oz_written,
 		rtg_rmap(oz->oz_rtg)->i_used_blocks,
-		xfs_write_hint_to_str(oz->oz_write_hint));
+		xfs_write_hint_to_str(oz->oz_write_hint),
+		oz->oz_is_gc ? "(GC)" : "");
 }
 
 static void
@@ -58,9 +59,8 @@ xfs_show_full_zone_used_distribution(
 	spin_unlock(&zi->zi_used_buckets_lock);
 
 	full = mp->m_sb.sb_rgcount;
-	if (zi->zi_open_gc_zone)
-		full--;
 	full -= zi->zi_nr_open_zones;
+	full -= zi->zi_nr_open_gc_zones;
 	full -= atomic_read(&zi->zi_nr_free_zones);
 	full -= reclaimable;
 
@@ -104,10 +104,6 @@ xfs_zoned_show_stats(
 	seq_puts(m, "\topen zones:\n");
 	list_for_each_entry(oz, &zi->zi_open_zones, oz_entry)
 		xfs_show_open_zone(m, oz);
-	if (zi->zi_open_gc_zone) {
-		seq_puts(m, "\topen gc zone:\n");
-		xfs_show_open_zone(m, zi->zi_open_gc_zone);
-	}
 	spin_unlock(&zi->zi_open_zones_lock);
 	seq_puts(m, "\tused blocks distribution (fully written zones):\n");
 	xfs_show_full_zone_used_distribution(m, mp);
diff --git a/fs/xfs/xfs_zone_priv.h b/fs/xfs/xfs_zone_priv.h
index 8fbf9a52964e..fcb57506d8e6 100644
--- a/fs/xfs/xfs_zone_priv.h
+++ b/fs/xfs/xfs_zone_priv.h
@@ -32,11 +32,7 @@ struct xfs_open_zone {
 	 */
 	enum rw_hint		oz_write_hint;
 
-	/*
-	 * Is this open zone used for garbage collection?  There can only be a
-	 * single open GC zone, which is pointed to by zi_open_gc_zone in
-	 * struct xfs_zone_info.  Constant over the life time of an open zone.
-	 */
+	/* Is this open zone used for garbage collection? */
 	bool			oz_is_gc;
 
 	/*
@@ -68,6 +64,7 @@ struct xfs_zone_info {
 	spinlock_t		zi_open_zones_lock;
 	struct list_head	zi_open_zones;
 	unsigned int		zi_nr_open_zones;
+	unsigned int		zi_nr_open_gc_zones;
 
 	/*
 	 * Free zone search cursor and number of free zones:
@@ -81,15 +78,9 @@ struct xfs_zone_info {
 	wait_queue_head_t	zi_zone_wait;
 
 	/*
-	 * Pointer to the GC thread, and the current open zone used by GC
-	 * (if any).
-	 *
-	 * zi_open_gc_zone is mostly private to the GC thread, but can be read
-	 * for debugging from other threads, in which case zi_open_zones_lock
-	 * must be taken to access it.
+	 * Pointer to the GC thread.
 	 */
 	struct task_struct      *zi_gc_thread;
-	struct xfs_open_zone	*zi_open_gc_zone;
 
 	/*
 	 * List of zones that need a reset:
-- 
2.47.3

Re: [PATCH 1/7] xfs: delay opening the GC zone

[Carlos Maiolino]

On Mon, Mar 30, 2026 at 07:44:11AM +0200, Christoph Hellwig wrote:
> The code that selects a new GC zone when the previous one is full also
> handles a zone not being set at all.  Make use of that to simplify the
> logic in xfs_zone_gc_mount.
> 

I might be wrong, but the subject looks misleading to me, wouldn't be
better to just set it to something like "simplify xfs_zone_gc_mount"?

Not a big deal for me, just kind of confused me while reading the patch.
But changing subject or not, feel free to add:

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

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_gc.c | 45 ++++++++++++++++++++------------------------
>  1 file changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index 0ff710fa0ee7..f076f66c5d57 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -527,7 +527,7 @@ xfs_zone_gc_select_victim(
>  	return true;
>  }
>  
> -static struct xfs_open_zone *
> +static int
>  xfs_zone_gc_steal_open(
>  	struct xfs_zone_info	*zi)
>  {
> @@ -538,15 +538,18 @@ xfs_zone_gc_steal_open(
>  		if (!found || oz->oz_allocated < found->oz_allocated)
>  			found = oz;
>  	}
> -
> -	if (found) {
> -		found->oz_is_gc = true;
> -		list_del_init(&found->oz_entry);
> -		zi->zi_nr_open_zones--;
> +	if (!found) {
> +		spin_unlock(&zi->zi_open_zones_lock);
> +		return -EIO;
>  	}
>  
> +	trace_xfs_zone_gc_target_opened(found->oz_rtg);
> +	found->oz_is_gc = true;
> +	list_del_init(&found->oz_entry);
> +	zi->zi_nr_open_zones--;
> +	zi->zi_open_gc_zone = found;
>  	spin_unlock(&zi->zi_open_zones_lock);
> -	return found;
> +	return 0;
>  }
>  
>  static struct xfs_open_zone *
> @@ -1177,31 +1180,24 @@ xfs_zone_gc_mount(
>  {
>  	struct xfs_zone_info	*zi = mp->m_zone_info;
>  	struct xfs_zone_gc_data	*data;
> -	struct xfs_open_zone	*oz;
>  	int			error;
>  
>  	/*
> -	 * If there are no free zones available for GC, pick the open zone with
> +	 * If there are no free zones available for GC, or the number of open
> +	 * zones has reached the open zone limit, pick the open zone with
>  	 * the least used space to GC into.  This should only happen after an
> -	 * unclean shutdown near ENOSPC while GC was ongoing.
> -	 *
> -	 * We also need to do this for the first gc zone allocation if we
> -	 * unmounted while at the open limit.
> +	 * unclean shutdown while GC was ongoing.  Otherwise a GC zone will
> +	 * be selected from the free zone pool on demand.
>  	 */
>  	if (!xfs_group_marked(mp, XG_TYPE_RTG, XFS_RTG_FREE) ||
> -	    zi->zi_nr_open_zones == mp->m_max_open_zones)
> -		oz = xfs_zone_gc_steal_open(zi);
> -	else
> -		oz = xfs_open_zone(mp, WRITE_LIFE_NOT_SET, true);
> -	if (!oz) {
> -		xfs_warn(mp, "unable to allocate a zone for gc");
> -		error = -EIO;
> -		goto out;
> +	    zi->zi_nr_open_zones >= mp->m_max_open_zones) {
> +		error = xfs_zone_gc_steal_open(zi);
> +		if (error) {
> +			xfs_warn(mp, "unable to steal an open zone for gc");
> +			return error;
> +		}
>  	}
>  
> -	trace_xfs_zone_gc_target_opened(oz->oz_rtg);
> -	zi->zi_open_gc_zone = oz;
> -
>  	data = xfs_zone_gc_data_alloc(mp);
>  	if (!data) {
>  		error = -ENOMEM;
> @@ -1224,7 +1220,6 @@ xfs_zone_gc_mount(
>  	kfree(data);
>  out_put_gc_zone:
>  	xfs_open_zone_put(zi->zi_open_gc_zone);
> -out:
>  	return error;
>  }
>  
> -- 
> 2.47.3
> 
> 

Re: [PATCH 2/7] xfs: add a separate tracepoint for stealing an open zone for GC

[Carlos Maiolino]

On Mon, Mar 30, 2026 at 07:44:12AM +0200, Christoph Hellwig wrote:
> The case where we have to reuse an already open zone warrants a different
> trace point vs the normal opening of a GC zone.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  fs/xfs/xfs_trace.h   | 1 +
>  fs/xfs/xfs_zone_gc.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 433f84252119..c1aa15973e74 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -394,6 +394,7 @@ DEFINE_ZONE_EVENT(xfs_zone_full);
>  DEFINE_ZONE_EVENT(xfs_zone_opened);
>  DEFINE_ZONE_EVENT(xfs_zone_reset);
>  DEFINE_ZONE_EVENT(xfs_zone_gc_target_opened);
> +DEFINE_ZONE_EVENT(xfs_zone_gc_target_stolen);
>  
>  TRACE_EVENT(xfs_zone_free_blocks,
>  	TP_PROTO(struct xfs_rtgroup *rtg, xfs_rgblock_t rgbno,
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index f076f66c5d57..273c496c2f1c 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -543,7 +543,7 @@ xfs_zone_gc_steal_open(
>  		return -EIO;
>  	}
>  
> -	trace_xfs_zone_gc_target_opened(found->oz_rtg);
> +	trace_xfs_zone_gc_target_stolen(found->oz_rtg);
>  	found->oz_is_gc = true;
>  	list_del_init(&found->oz_entry);
>  	zi->zi_nr_open_zones--;
> -- 
> 2.47.3
> 
> 

Re: [PATCH 3/7] xfs: put the open zone later xfs_open_zone_put

[Carlos Maiolino]

On Mon, Mar 30, 2026 at 07:44:13AM +0200, Christoph Hellwig wrote:
> The open zone is what holds the rtg reference for us.  This doesn't
> matter until we support shrinking, and even then is rather theoretical
> because we can't shrink away a just filled zone in a tiny race window,
> but let's play safe here.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

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


> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index b0a6bc777c36..297a918bf28e 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -182,11 +182,11 @@ xfs_open_zone_mark_full(
>  		list_del_init(&oz->oz_entry);
>  	}
>  	spin_unlock(&zi->zi_open_zones_lock);
> -	xfs_open_zone_put(oz);
>  
>  	wake_up_all(&zi->zi_zone_wait);
>  	if (used < rtg_blocks(rtg))
>  		xfs_zone_account_reclaimable(rtg, rtg_blocks(rtg) - used);
> +	xfs_open_zone_put(oz);
>  }
>  
>  static inline void
> -- 
> 2.47.3
> 
> 

Re: [PATCH 4/7] xfs: rename xfs_zone_gc_iter_next to xfs_zone_gc_iter_irec

[Carlos Maiolino]

On Mon, Mar 30, 2026 at 07:44:14AM +0200, Christoph Hellwig wrote:
> This function returns the current iterator position, which makes the
> _next postfix a bit misleading.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  fs/xfs/xfs_zone_gc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index 273c496c2f1c..3723f5293a64 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -374,7 +374,7 @@ xfs_zone_gc_query(
>  }
>  
>  static bool
> -xfs_zone_gc_iter_next(
> +xfs_zone_gc_iter_irec(
>  	struct xfs_mount	*mp,
>  	struct xfs_zone_gc_iter	*iter,
>  	struct xfs_rmap_irec	*chunk_rec,
> @@ -691,7 +691,7 @@ xfs_zone_gc_start_chunk(
>  	if (xfs_is_shutdown(mp))
>  		return false;
>  
> -	if (!xfs_zone_gc_iter_next(mp, iter, &irec, &ip))
> +	if (!xfs_zone_gc_iter_irec(mp, iter, &irec, &ip))
>  		return false;
>  	oz = xfs_zone_gc_alloc_blocks(data, &irec.rm_blockcount, &daddr,
>  			&is_seq);
> -- 
> 2.47.3
> 
> 

Re: [PATCH 5/7] xfs: refactor GC zone selection helpers

[Carlos Maiolino]

On Mon, Mar 30, 2026 at 07:44:15AM +0200, Christoph Hellwig wrote:
> Merge xfs_zone_gc_ensure_target into xfs_zone_gc_select_target
> to keep all zone selection code together.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_gc.c | 45 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 23 deletions(-)

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

> 
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index 3723f5293a64..6ce5a0708a9b 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -552,6 +552,9 @@ xfs_zone_gc_steal_open(
>  	return 0;
>  }
>  
> +/*
> + * Ensure we have a valid open zone to write to.
> + */
>  static struct xfs_open_zone *
>  xfs_zone_gc_select_target(
>  	struct xfs_mount	*mp)
> @@ -559,12 +562,25 @@ xfs_zone_gc_select_target(
>  	struct xfs_zone_info	*zi = mp->m_zone_info;
>  	struct xfs_open_zone	*oz = zi->zi_open_gc_zone;
>  
> +	if (oz) {
> +		/*
> +		 * If we have space available, just keep using the existing
> +		 * zone.
> +		 */
> +		if (oz->oz_allocated < rtg_blocks(oz->oz_rtg))
> +			return oz;
> +
> +		/*
> +		 * Wait for all writes to the current zone to finish before
> +		 * picking a new one.
> +		 */
> +		if (oz->oz_written < rtg_blocks(oz->oz_rtg))
> +			return NULL;
> +	}
> +
>  	/*
> -	 * We need to wait for pending writes to finish.
> +	 * Open a new zone when there is none currently in use.
>  	 */
> -	if (oz && oz->oz_written < rtg_blocks(oz->oz_rtg))
> -		return NULL;
> -
>  	ASSERT(zi->zi_nr_open_zones <=
>  		mp->m_max_open_zones - XFS_OPEN_GC_ZONES);
>  	oz = xfs_open_zone(mp, WRITE_LIFE_NOT_SET, true);
> @@ -576,23 +592,6 @@ xfs_zone_gc_select_target(
>  	return oz;
>  }
>  
> -/*
> - * Ensure we have a valid open zone to write the GC data to.
> - *
> - * If the current target zone has space keep writing to it, else first wait for
> - * all pending writes and then pick a new one.
> - */
> -static struct xfs_open_zone *
> -xfs_zone_gc_ensure_target(
> -	struct xfs_mount	*mp)
> -{
> -	struct xfs_open_zone	*oz = mp->m_zone_info->zi_open_gc_zone;
> -
> -	if (!oz || oz->oz_allocated == rtg_blocks(oz->oz_rtg))
> -		return xfs_zone_gc_select_target(mp);
> -	return oz;
> -}
> -
>  static void
>  xfs_zone_gc_end_io(
>  	struct bio		*bio)
> @@ -615,7 +614,7 @@ xfs_zone_gc_alloc_blocks(
>  	struct xfs_mount	*mp = data->mp;
>  	struct xfs_open_zone	*oz;
>  
> -	oz = xfs_zone_gc_ensure_target(mp);
> +	oz = xfs_zone_gc_select_target(mp);
>  	if (!oz)
>  		return NULL;
>  
> @@ -1019,7 +1018,7 @@ xfs_zone_gc_should_start_new_work(
>  	if (!data->scratch_available)
>  		return false;
>  
> -	oz = xfs_zone_gc_ensure_target(data->mp);
> +	oz = xfs_zone_gc_select_target(data->mp);
>  	if (!oz || oz->oz_allocated == rtg_blocks(oz->oz_rtg))
>  		return false;
>  
> -- 
> 2.47.3
> 
> 

Re: [PATCH 6/7] xfs: streamline GC zone selection

[Carlos Maiolino]

On Mon, Mar 30, 2026 at 07:44:16AM +0200, Christoph Hellwig wrote:
> Currently picking of the GC target zone is done a bit odd, both in the
> main "can we start new GC cycles" routine and in the low-level block
> allocator for GC.  This was mostly done to work around the rules for
> when code in a waitqueue wait loop can sleep.
> 
> But there is a trick to check if the process state has been set to
> running to discover if the wait loop has to be retried all this becomes
> much simpler.

The final part of the sentence looks odd. But I'm no English native
either.

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

> So we can just select a GC zone just before writing,
> and bail out of starting new work if we can't find usable zone.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_gc.c | 95 +++++++++++++++++++-------------------------
>  1 file changed, 40 insertions(+), 55 deletions(-)
> 
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index 6ce5a0708a9b..24f20f99ae1a 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -383,9 +383,6 @@ xfs_zone_gc_iter_irec(
>  	struct xfs_rmap_irec	*irec;
>  	int			error;
>  
> -	if (!iter->victim_rtg)
> -		return false;
> -
>  retry:
>  	if (iter->rec_idx == iter->rec_count) {
>  		error = xfs_zone_gc_query(mp, iter);
> @@ -555,7 +552,7 @@ xfs_zone_gc_steal_open(
>  /*
>   * Ensure we have a valid open zone to write to.
>   */
> -static struct xfs_open_zone *
> +static bool
>  xfs_zone_gc_select_target(
>  	struct xfs_mount	*mp)
>  {
> @@ -568,14 +565,14 @@ xfs_zone_gc_select_target(
>  		 * zone.
>  		 */
>  		if (oz->oz_allocated < rtg_blocks(oz->oz_rtg))
> -			return oz;
> +			return true;
>  
>  		/*
>  		 * Wait for all writes to the current zone to finish before
>  		 * picking a new one.
>  		 */
>  		if (oz->oz_written < rtg_blocks(oz->oz_rtg))
> -			return NULL;
> +			return false;
>  	}
>  
>  	/*
> @@ -589,7 +586,7 @@ xfs_zone_gc_select_target(
>  	spin_lock(&zi->zi_open_zones_lock);
>  	zi->zi_open_gc_zone = oz;
>  	spin_unlock(&zi->zi_open_zones_lock);
> -	return oz;
> +	return !!oz;
>  }
>  
>  static void
> @@ -604,7 +601,7 @@ xfs_zone_gc_end_io(
>  	wake_up_process(data->mp->m_zone_info->zi_gc_thread);
>  }
>  
> -static struct xfs_open_zone *
> +static bool
>  xfs_zone_gc_alloc_blocks(
>  	struct xfs_zone_gc_data	*data,
>  	xfs_extlen_t		*count_fsb,
> @@ -612,11 +609,7 @@ xfs_zone_gc_alloc_blocks(
>  	bool			*is_seq)
>  {
>  	struct xfs_mount	*mp = data->mp;
> -	struct xfs_open_zone	*oz;
> -
> -	oz = xfs_zone_gc_select_target(mp);
> -	if (!oz)
> -		return NULL;
> +	struct xfs_open_zone	*oz = mp->m_zone_info->zi_open_gc_zone;
>  
>  	*count_fsb = min(*count_fsb, XFS_B_TO_FSB(mp, data->scratch_available));
>  
> @@ -638,7 +631,7 @@ xfs_zone_gc_alloc_blocks(
>  	spin_unlock(&mp->m_sb_lock);
>  
>  	if (!*count_fsb)
> -		return NULL;
> +		return false;
>  
>  	*daddr = xfs_gbno_to_daddr(rtg_group(oz->oz_rtg), 0);
>  	*is_seq = bdev_zone_is_seq(mp->m_rtdev_targp->bt_bdev, *daddr);
> @@ -646,7 +639,7 @@ xfs_zone_gc_alloc_blocks(
>  		*daddr += XFS_FSB_TO_BB(mp, oz->oz_allocated);
>  	oz->oz_allocated += *count_fsb;
>  	atomic_inc(&oz->oz_ref);
> -	return oz;
> +	return true;
>  }
>  
>  static void
> @@ -671,6 +664,28 @@ xfs_zone_gc_add_data(
>  	} while (len);
>  }
>  
> +static bool
> +xfs_zone_gc_can_start_chunk(
> +	struct xfs_zone_gc_data	*data)
> +{
> +
> +	if (xfs_is_shutdown(data->mp))
> +		return false;
> +	if (!data->scratch_available)
> +		return false;
> +
> +	if (!data->iter.victim_rtg) {
> +		if (kthread_should_stop() || kthread_should_park())
> +			return false;
> +		if (!xfs_zoned_need_gc(data->mp))
> +			return false;
> +		if (!xfs_zone_gc_select_victim(data))
> +			return false;
> +	}
> +
> +	return xfs_zone_gc_select_target(data->mp);
> +}
> +
>  static bool
>  xfs_zone_gc_start_chunk(
>  	struct xfs_zone_gc_data	*data)
> @@ -678,7 +693,6 @@ xfs_zone_gc_start_chunk(
>  	struct xfs_zone_gc_iter	*iter = &data->iter;
>  	struct xfs_mount	*mp = data->mp;
>  	struct block_device	*bdev = mp->m_rtdev_targp->bt_bdev;
> -	struct xfs_open_zone	*oz;
>  	struct xfs_rmap_irec	irec;
>  	struct xfs_gc_bio	*chunk;
>  	struct xfs_inode	*ip;
> @@ -687,14 +701,15 @@ xfs_zone_gc_start_chunk(
>  	unsigned int		len;
>  	bool			is_seq;
>  
> -	if (xfs_is_shutdown(mp))
> +	if (!xfs_zone_gc_can_start_chunk(data))
>  		return false;
>  
> +	set_current_state(TASK_RUNNING);
>  	if (!xfs_zone_gc_iter_irec(mp, iter, &irec, &ip))
>  		return false;
> -	oz = xfs_zone_gc_alloc_blocks(data, &irec.rm_blockcount, &daddr,
> -			&is_seq);
> -	if (!oz) {
> +
> +	if (!xfs_zone_gc_alloc_blocks(data, &irec.rm_blockcount, &daddr,
> +			&is_seq)) {
>  		xfs_irele(ip);
>  		return false;
>  	}
> @@ -713,7 +728,7 @@ xfs_zone_gc_start_chunk(
>  	chunk->new_daddr = daddr;
>  	chunk->is_seq = is_seq;
>  	chunk->data = data;
> -	chunk->oz = oz;
> +	chunk->oz = mp->m_zone_info->zi_open_gc_zone;
>  	chunk->victim_rtg = iter->victim_rtg;
>  	atomic_inc(&rtg_group(chunk->victim_rtg)->xg_active_ref);
>  	atomic_inc(&chunk->victim_rtg->rtg_gccount);
> @@ -1007,33 +1022,6 @@ xfs_zone_gc_reset_zones(
>  	} while (next);
>  }
>  
> -static bool
> -xfs_zone_gc_should_start_new_work(
> -	struct xfs_zone_gc_data	*data)
> -{
> -	struct xfs_open_zone	*oz;
> -
> -	if (xfs_is_shutdown(data->mp))
> -		return false;
> -	if (!data->scratch_available)
> -		return false;
> -
> -	oz = xfs_zone_gc_select_target(data->mp);
> -	if (!oz || oz->oz_allocated == rtg_blocks(oz->oz_rtg))
> -		return false;
> -
> -	if (!data->iter.victim_rtg) {
> -		if (kthread_should_stop() || kthread_should_park())
> -			return false;
> -		if (!xfs_zoned_need_gc(data->mp))
> -			return false;
> -		if (!xfs_zone_gc_select_victim(data))
> -			return false;
> -	}
> -
> -	return true;
> -}
> -
>  /*
>   * Handle the work to read and write data for GC and to reset the zones,
>   * including handling all completions.
> @@ -1083,13 +1071,10 @@ xfs_zone_gc_handle_work(
>  	}
>  	blk_finish_plug(&plug);
>  
> -	if (xfs_zone_gc_should_start_new_work(data)) {
> -		set_current_state(TASK_RUNNING);
> -		blk_start_plug(&plug);
> -		while (xfs_zone_gc_start_chunk(data))
> -			;
> -		blk_finish_plug(&plug);
> -	}
> +	blk_start_plug(&plug);
> +	while (xfs_zone_gc_start_chunk(data))
> +		;
> +	blk_finish_plug(&plug);
>  }
>  
>  /*
> -- 
> 2.47.3
> 
> 

Re: [PATCH 7/7] xfs: reduce special casing for the open GC zone

[Carlos Maiolino]

On Mon, Mar 30, 2026 at 07:44:17AM +0200, Christoph Hellwig wrote:
> Currently the open zone used for garbage collection is a special snow
> flake, and it that has been a bit annoying for some further zoned XFS
> work I've been doing.
> 
> Remove the zi_open_gc_field and instead track the open GC zone in the
> zi_open_zones list together with the normal open zones, and keep an extra
> pointer and a reference of in the GC thread's data structure.  This means
> anything iterating over open zones just has to look at zi_open_zones, and
> the life time rules are consistent.  It also helps to add support for
> multiple open GC zones if we ever need them, and removes a bit of code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  fs/xfs/xfs_zone_alloc.c | 19 +++++++----
>  fs/xfs/xfs_zone_gc.c    | 71 ++++++++++++++++++++++-------------------
>  fs/xfs/xfs_zone_info.c  | 12 +++----
>  fs/xfs/xfs_zone_priv.h  | 15 ++-------
>  4 files changed, 58 insertions(+), 59 deletions(-)
> 
> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index 297a918bf28e..db27fdb3a7d3 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -174,16 +174,18 @@ xfs_open_zone_mark_full(
>  	WRITE_ONCE(rtg->rtg_open_zone, NULL);
>  
>  	spin_lock(&zi->zi_open_zones_lock);
> -	if (oz->oz_is_gc) {
> -		ASSERT(current == zi->zi_gc_thread);
> -		zi->zi_open_gc_zone = NULL;
> -	} else {
> +	if (oz->oz_is_gc)
> +		zi->zi_nr_open_gc_zones--;
> +	else
>  		zi->zi_nr_open_zones--;
> -		list_del_init(&oz->oz_entry);
> -	}
> +	list_del_init(&oz->oz_entry);
>  	spin_unlock(&zi->zi_open_zones_lock);
>  
> -	wake_up_all(&zi->zi_zone_wait);
> +	if (oz->oz_is_gc)
> +		wake_up_process(zi->zi_gc_thread);
> +	else
> +		wake_up_all(&zi->zi_zone_wait);
> +
>  	if (used < rtg_blocks(rtg))
>  		xfs_zone_account_reclaimable(rtg, rtg_blocks(rtg) - used);
>  	xfs_open_zone_put(oz);
> @@ -557,6 +559,9 @@ xfs_try_use_zone(
>  	struct xfs_open_zone	*oz,
>  	unsigned int		goodness)
>  {
> +	if (oz->oz_is_gc)
> +		return false;
> +
>  	if (oz->oz_allocated == rtg_blocks(oz->oz_rtg))
>  		return false;
>  
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index 24f20f99ae1a..8c184127b351 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -125,6 +125,7 @@ struct xfs_zone_gc_iter {
>   */
>  struct xfs_zone_gc_data {
>  	struct xfs_mount		*mp;
> +	struct xfs_open_zone		*oz;
>  
>  	/* bioset used to allocate the gc_bios */
>  	struct bio_set			bio_set;
> @@ -525,9 +526,10 @@ xfs_zone_gc_select_victim(
>  }
>  
>  static int
> -xfs_zone_gc_steal_open(
> -	struct xfs_zone_info	*zi)
> +xfs_zone_gc_steal_open_zone(
> +	struct xfs_zone_gc_data	*data)
>  {
> +	struct xfs_zone_info	*zi = data->mp->m_zone_info;
>  	struct xfs_open_zone	*oz, *found = NULL;
>  
>  	spin_lock(&zi->zi_open_zones_lock);
> @@ -542,10 +544,12 @@ xfs_zone_gc_steal_open(
>  
>  	trace_xfs_zone_gc_target_stolen(found->oz_rtg);
>  	found->oz_is_gc = true;
> -	list_del_init(&found->oz_entry);
>  	zi->zi_nr_open_zones--;
> -	zi->zi_open_gc_zone = found;
> +	zi->zi_nr_open_gc_zones++;
>  	spin_unlock(&zi->zi_open_zones_lock);
> +
> +	atomic_inc(&found->oz_ref);
> +	data->oz = found;
>  	return 0;
>  }
>  
> @@ -554,39 +558,43 @@ xfs_zone_gc_steal_open(
>   */
>  static bool
>  xfs_zone_gc_select_target(
> -	struct xfs_mount	*mp)
> +	struct xfs_zone_gc_data	*data)
>  {
> -	struct xfs_zone_info	*zi = mp->m_zone_info;
> -	struct xfs_open_zone	*oz = zi->zi_open_gc_zone;
> +	struct xfs_zone_info	*zi = data->mp->m_zone_info;
>  
> -	if (oz) {
> +	if (data->oz) {
>  		/*
>  		 * If we have space available, just keep using the existing
>  		 * zone.
>  		 */
> -		if (oz->oz_allocated < rtg_blocks(oz->oz_rtg))
> +		if (data->oz->oz_allocated < rtg_blocks(data->oz->oz_rtg))
>  			return true;
>  
>  		/*
>  		 * Wait for all writes to the current zone to finish before
>  		 * picking a new one.
>  		 */
> -		if (oz->oz_written < rtg_blocks(oz->oz_rtg))
> +		if (data->oz->oz_written < rtg_blocks(data->oz->oz_rtg))
>  			return false;
> +
> +		xfs_open_zone_put(data->oz);
>  	}
>  
>  	/*
>  	 * Open a new zone when there is none currently in use.
>  	 */
>  	ASSERT(zi->zi_nr_open_zones <=
> -		mp->m_max_open_zones - XFS_OPEN_GC_ZONES);
> -	oz = xfs_open_zone(mp, WRITE_LIFE_NOT_SET, true);
> -	if (oz)
> -		trace_xfs_zone_gc_target_opened(oz->oz_rtg);
> +		data->mp->m_max_open_zones - XFS_OPEN_GC_ZONES);
> +	data->oz = xfs_open_zone(data->mp, WRITE_LIFE_NOT_SET, true);
> +	if (!data->oz)
> +		return false;
> +	trace_xfs_zone_gc_target_opened(data->oz->oz_rtg);
> +	atomic_inc(&data->oz->oz_ref);
>  	spin_lock(&zi->zi_open_zones_lock);
> -	zi->zi_open_gc_zone = oz;
> +	zi->zi_nr_open_gc_zones++;
> +	list_add_tail(&data->oz->oz_entry, &zi->zi_open_zones);
>  	spin_unlock(&zi->zi_open_zones_lock);
> -	return !!oz;
> +	return true;
>  }
>  
>  static void
> @@ -609,7 +617,7 @@ xfs_zone_gc_alloc_blocks(
>  	bool			*is_seq)
>  {
>  	struct xfs_mount	*mp = data->mp;
> -	struct xfs_open_zone	*oz = mp->m_zone_info->zi_open_gc_zone;
> +	struct xfs_open_zone	*oz = data->oz;
>  
>  	*count_fsb = min(*count_fsb, XFS_B_TO_FSB(mp, data->scratch_available));
>  
> @@ -683,7 +691,7 @@ xfs_zone_gc_can_start_chunk(
>  			return false;
>  	}
>  
> -	return xfs_zone_gc_select_target(data->mp);
> +	return xfs_zone_gc_select_target(data);
>  }
>  
>  static bool
> @@ -728,7 +736,7 @@ xfs_zone_gc_start_chunk(
>  	chunk->new_daddr = daddr;
>  	chunk->is_seq = is_seq;
>  	chunk->data = data;
> -	chunk->oz = mp->m_zone_info->zi_open_gc_zone;
> +	chunk->oz = data->oz;
>  	chunk->victim_rtg = iter->victim_rtg;
>  	atomic_inc(&rtg_group(chunk->victim_rtg)->xg_active_ref);
>  	atomic_inc(&chunk->victim_rtg->rtg_gccount);
> @@ -1134,6 +1142,8 @@ xfs_zoned_gcd(
>  	}
>  	xfs_clear_zonegc_running(mp);
>  
> +	if (data->oz)
> +		xfs_open_zone_put(data->oz);
>  	if (data->iter.victim_rtg)
>  		xfs_rtgroup_rele(data->iter.victim_rtg);
>  
> @@ -1166,6 +1176,10 @@ xfs_zone_gc_mount(
>  	struct xfs_zone_gc_data	*data;
>  	int			error;
>  
> +	data = xfs_zone_gc_data_alloc(mp);
> +	if (!data)
> +		return -ENOMEM;
> +
>  	/*
>  	 * If there are no free zones available for GC, or the number of open
>  	 * zones has reached the open zone limit, pick the open zone with
> @@ -1175,35 +1189,30 @@ xfs_zone_gc_mount(
>  	 */
>  	if (!xfs_group_marked(mp, XG_TYPE_RTG, XFS_RTG_FREE) ||
>  	    zi->zi_nr_open_zones >= mp->m_max_open_zones) {
> -		error = xfs_zone_gc_steal_open(zi);
> +		error = xfs_zone_gc_steal_open_zone(data);
>  		if (error) {
>  			xfs_warn(mp, "unable to steal an open zone for gc");
> -			return error;
> +			goto out_free_gc_data;
>  		}
>  	}
>  
> -	data = xfs_zone_gc_data_alloc(mp);
> -	if (!data) {
> -		error = -ENOMEM;
> -		goto out_put_gc_zone;
> -	}
> -
>  	zi->zi_gc_thread = kthread_create(xfs_zoned_gcd, data,
>  			"xfs-zone-gc/%s", mp->m_super->s_id);
>  	if (IS_ERR(zi->zi_gc_thread)) {
>  		xfs_warn(mp, "unable to create zone gc thread");
>  		error = PTR_ERR(zi->zi_gc_thread);
> -		goto out_free_gc_data;
> +		goto out_put_oz;
>  	}
>  
>  	/* xfs_zone_gc_start will unpark for rw mounts */
>  	kthread_park(zi->zi_gc_thread);
>  	return 0;
>  
> +out_put_oz:
> +	if (data->oz)
> +		xfs_open_zone_put(data->oz);
>  out_free_gc_data:
>  	kfree(data);
> -out_put_gc_zone:
> -	xfs_open_zone_put(zi->zi_open_gc_zone);
>  	return error;
>  }
>  
> @@ -1214,6 +1223,4 @@ xfs_zone_gc_unmount(
>  	struct xfs_zone_info	*zi = mp->m_zone_info;
>  
>  	kthread_stop(zi->zi_gc_thread);
> -	if (zi->zi_open_gc_zone)
> -		xfs_open_zone_put(zi->zi_open_gc_zone);
>  }
> diff --git a/fs/xfs/xfs_zone_info.c b/fs/xfs/xfs_zone_info.c
> index 8e56181021b3..47b475e21af8 100644
> --- a/fs/xfs/xfs_zone_info.c
> +++ b/fs/xfs/xfs_zone_info.c
> @@ -30,11 +30,12 @@ xfs_show_open_zone(
>  	struct seq_file		*m,
>  	struct xfs_open_zone	*oz)
>  {
> -	seq_printf(m, "\t  zone %d, wp %u, written %u, used %u, hint %s\n",
> +	seq_printf(m, "\t  zone %d, wp %u, written %u, used %u, hint %s %s\n",
>  		rtg_rgno(oz->oz_rtg),
>  		oz->oz_allocated, oz->oz_written,
>  		rtg_rmap(oz->oz_rtg)->i_used_blocks,
> -		xfs_write_hint_to_str(oz->oz_write_hint));
> +		xfs_write_hint_to_str(oz->oz_write_hint),
> +		oz->oz_is_gc ? "(GC)" : "");
>  }
>  
>  static void
> @@ -58,9 +59,8 @@ xfs_show_full_zone_used_distribution(
>  	spin_unlock(&zi->zi_used_buckets_lock);
>  
>  	full = mp->m_sb.sb_rgcount;
> -	if (zi->zi_open_gc_zone)
> -		full--;
>  	full -= zi->zi_nr_open_zones;
> +	full -= zi->zi_nr_open_gc_zones;
>  	full -= atomic_read(&zi->zi_nr_free_zones);
>  	full -= reclaimable;
>  
> @@ -104,10 +104,6 @@ xfs_zoned_show_stats(
>  	seq_puts(m, "\topen zones:\n");
>  	list_for_each_entry(oz, &zi->zi_open_zones, oz_entry)
>  		xfs_show_open_zone(m, oz);
> -	if (zi->zi_open_gc_zone) {
> -		seq_puts(m, "\topen gc zone:\n");
> -		xfs_show_open_zone(m, zi->zi_open_gc_zone);
> -	}
>  	spin_unlock(&zi->zi_open_zones_lock);
>  	seq_puts(m, "\tused blocks distribution (fully written zones):\n");
>  	xfs_show_full_zone_used_distribution(m, mp);
> diff --git a/fs/xfs/xfs_zone_priv.h b/fs/xfs/xfs_zone_priv.h
> index 8fbf9a52964e..fcb57506d8e6 100644
> --- a/fs/xfs/xfs_zone_priv.h
> +++ b/fs/xfs/xfs_zone_priv.h
> @@ -32,11 +32,7 @@ struct xfs_open_zone {
>  	 */
>  	enum rw_hint		oz_write_hint;
>  
> -	/*
> -	 * Is this open zone used for garbage collection?  There can only be a
> -	 * single open GC zone, which is pointed to by zi_open_gc_zone in
> -	 * struct xfs_zone_info.  Constant over the life time of an open zone.
> -	 */
> +	/* Is this open zone used for garbage collection? */
>  	bool			oz_is_gc;
>  
>  	/*
> @@ -68,6 +64,7 @@ struct xfs_zone_info {
>  	spinlock_t		zi_open_zones_lock;
>  	struct list_head	zi_open_zones;
>  	unsigned int		zi_nr_open_zones;
> +	unsigned int		zi_nr_open_gc_zones;
>  
>  	/*
>  	 * Free zone search cursor and number of free zones:
> @@ -81,15 +78,9 @@ struct xfs_zone_info {
>  	wait_queue_head_t	zi_zone_wait;
>  
>  	/*
> -	 * Pointer to the GC thread, and the current open zone used by GC
> -	 * (if any).
> -	 *
> -	 * zi_open_gc_zone is mostly private to the GC thread, but can be read
> -	 * for debugging from other threads, in which case zi_open_zones_lock
> -	 * must be taken to access it.
> +	 * Pointer to the GC thread.
>  	 */
>  	struct task_struct      *zi_gc_thread;
> -	struct xfs_open_zone	*zi_open_gc_zone;
>  
>  	/*
>  	 * List of zones that need a reset:
> -- 
> 2.47.3
> 
> 

Re: [PATCH 1/7] xfs: delay opening the GC zone

[Christoph Hellwig]

On Mon, Mar 30, 2026 at 02:16:53PM +0200, Carlos Maiolino wrote:
> On Mon, Mar 30, 2026 at 07:44:11AM +0200, Christoph Hellwig wrote:
> > The code that selects a new GC zone when the previous one is full also
> > handles a zone not being set at all.  Make use of that to simplify the
> > logic in xfs_zone_gc_mount.
> > 
> 
> I might be wrong, but the subject looks misleading to me, wouldn't be
> better to just set it to something like "simplify xfs_zone_gc_mount"?
> 
> Not a big deal for me, just kind of confused me while reading the patch.
> But changing subject or not, feel free to add:

Well, both would be correct, but I think the current subject is more
useful as simplify could mean anything.