Re: [PATCH 1/4] btrfs: zoned: flush active metadata block group at btree_writepages() start

On Wed, Jul 22, 2026 at 10:08:35AM -0700, Boris Burkov wrote:
> On Wed, Jul 22, 2026 at 01:30:13PM +0200, Johannes Thumshirn wrote:
> > On a zoned filesystem btree_writepages() writes metadata in ascending
> 
> wording nit: isn't this true on non-zoned too? Maybe something like:
> btree_writepages() writes metadata in ascending logical order. On a
> zoned filsystem, which keeps a single .... <something terrible happens>
> 
> > logical address order and keeps a single active metadata/system block group,
> > pivoting it via btrfs_check_meta_write_pointer() -> check_bg_is_active() as
> > writeback moves between block groups.
> > 
> > If the active block group is at a higher address than another block group
> > that also has dirty metadata, the walk reaches the lower one first and the
> > pivot must finish the active block group. It cannot finish one that still has
> > unsent IO, and refuses to wait for it during WB_SYNC_ALL commit writeback, so
> 
> Can you add some more detail on why it refuses? Or why the pivot can't
> be the one to do these submissions? I think that would help fully
> motivate this change.
> 
> > btrfs_check_meta_write_pointer() returns -EAGAIN and the transaction is
> > aborted, forcing the filesystem read-only. This happens intermittently under
> > metadata-heavy relocation (e.g. fstests btrfs/187).
> > 
> > Flush the active metadata and system block groups at the start of
> > btree_writepages(), under the fs_info->zoned_meta_io_lock it already holds, so
> > they have no unsent IO and the later pivot can finish them.
> > 
> > Fixes: 13bb483d32ab ("btrfs: zoned: activate metadata block group on write time")
> > Assisted-by: LLM (debugging, commit message)
> > Signed-off-by: Johannes Thumshirn <[email protected]>
> > ---
> >  fs/btrfs/extent_io.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 97bd18d515af..bac3edabe7c9 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -2383,6 +2383,52 @@ void btrfs_btree_wait_writeback_range(struct btrfs_fs_info *fs_info, u64 start,
> >  	}
> >  }
> >  
> > +/* Write out the dirty metadata extent buffers of a single block group. */
> 
> This comment is quite generic, and not zoned specific, but the function
> name is much more zoned focused (and we assert zoned)
> 
> I am also not a huge fan of this code mostly duplicating the loop in
> btree_writepages while just leaving that spot open coded. Is that an
> intentional decision because of important distinctions between the two?
> Is there a variant we could lift to "write a range" and make
> flush_active_meta_bg() and btree_writepages share it? (like the while
> loop basically? or maybe the per-eb bit?)
> 
> note the missing call to btrfs_schedule_zone_finish_bg() I noted inline
> as a concrete reason it's helpful to not duplicate.
> 
> > +static void flush_active_meta_bg(struct address_space *mapping,
> > +				 struct writeback_control *wbc,
> > +				 struct btrfs_eb_write_context *ctx,
> > +				 struct btrfs_block_group *bg)
> > +{
> > +	struct btrfs_fs_info *fs_info = inode_to_fs_info(mapping->host);
> > +	unsigned long index = bg->start >> fs_info->nodesize_bits;
> > +	unsigned long end = (btrfs_block_group_end(bg) - 1) >> fs_info->nodesize_bits;
> > +	struct eb_batch batch;
> > +	unsigned int nr_ebs;
> > +
> > +	ASSERT(btrfs_is_zoned(fs_info));
> > +	lockdep_assert_held(&fs_info->zoned_meta_io_lock);
> > +
> > +	eb_batch_init(&batch);
> > +	while (index <= end &&
> > +	       (nr_ebs = buffer_tree_get_ebs_tag(fs_info, &index, end,
> > +						 PAGECACHE_TAG_DIRTY, &batch))) {
> > +		struct extent_buffer *eb;
> > +
> > +		while ((eb = eb_batch_next(&batch)) != NULL) {
> > +			ctx->eb = eb;
> > +
> > +			/*
> > +			 * Best effort: if the eb is not writable at the write
> > +			 * pointer (e.g. a hole), stop flushing this bg and let
> > +			 * the main walk deal with it.
> > +			 */
> > +			if (btrfs_check_meta_write_pointer(eb->fs_info, ctx)) {
> > +				eb_batch_release(&batch);
> > +				return;
> > +			}
> > +
> > +			if (!lock_extent_buffer_for_io(eb, wbc))
> > +				continue;
> > +
> > +			if (ctx->zoned_bg)
> 
> the btree_writepages code also calls btrfs_schedule_zone_finish_bg()
> here, is that intentionally omitted?
> 
> > +				ctx->zoned_bg->meta_write_pointer += eb->len;
> > +			write_one_eb(eb, wbc);
> > +		}
> > +		eb_batch_release(&batch);
> > +		cond_resched();
> > +	}
> > +}
> > +
> >  int btree_writepages(struct address_space *mapping, struct writeback_control *wbc)
> >  {
> >  	struct btrfs_eb_write_context ctx = { .wbc = wbc };
> > @@ -2418,6 +2464,22 @@ int btree_writepages(struct address_space *mapping, struct writeback_control *wb
> >  	else
> >  		tag = PAGECACHE_TAG_DIRTY;
> >  	btrfs_zoned_meta_io_lock(fs_info);
> > +
> > +	/*
> > +	 * On a zoned filesystem, flush the currently active metadata/system
> > +	 * block group(s) first, under this same lock, so the ascending-address
> > +	 * walk below can pivot the active block group instead of aborting the
> > +	 * commit with -EAGAIN.
> > +	 */
> > +	if (btrfs_is_zoned(fs_info) && wbc->sync_mode == WB_SYNC_ALL &&
> > +	    !wbc->for_sync) {
> > +		if (fs_info->active_meta_bg)
> > +			flush_active_meta_bg(mapping, wbc, &ctx,
> > +					     fs_info->active_meta_bg);
> > +		if (fs_info->active_system_bg)
> > +			flush_active_meta_bg(mapping, wbc, &ctx,
> > +					     fs_info->active_system_bg);
> > +	}
> 
> I think this placement makes sense, but I also wouldn't mind just having
> a more generic "eb ordering" concept that zoned could use to plug the
> active ebs to the front of the list then fallback to the normal order,
> or something. Not a deal breaker for me, just food for thought.
> 

Furthermore, I feel like this is actually quite redundant with the whole
EBUSY pivot logic.

So for random reclaim writeback, we walk the ebs in index order and skip
any not in the active bg if the active bg has dirty ebs. Then we reach
the active bg and flush it, submitting everything. Subsequent tries to
writeback the non-active will now wait on that submitted IO and
ultimately win and get to become active, and do more submission.

But we could save the bother and make things more direct and simple if
we just had btree_writepages() always write out the active bg first then
the rest. I don't know if we could fully delete the EBUSY, though, maybe
there is some racy case where it would still happen a bit, even if
mostly it didn't?

> I think fewer "tricks" for zoned writeback and more "it falls out
> correctly from a generic form" would make it easier to maintain in the
> long run. Obviously there are real, important differences so this kind
> of thing is never free...
> 
> Thanks,
> Boris
> 
> >  retry:
> >  	if (wbc->sync_mode == WB_SYNC_ALL)
> >  		buffer_tree_tag_for_writeback(fs_info, index, end);
> > -- 
> > 2.54.0
> >