Re: [PATCH 2/4] btrfs: zoned: recover active non-data block group roles on mount

[Johannes Thumshirn <johannes.thumshirn@wdc.com>]

On Mon, Aug 24, 2026 at 02:49:54PM +0800, Dongjiang Zhu wrote:
> [BUG]
> After a remount, a later write can activate a new metadata or system
> block group without finishing the recovered one.  Repeating this across
> remounts consumes additional active-zone slots and reservations, and can
> eventually exhaust all active-zone slots.
> 
> Once no slot is available, activation of a metadata or system block group
> fails.  During synchronous transaction writeback this propagates as
> -EAGAIN, aborting the transaction and forcing the filesystem read-only.
> 
> A remount/fsync/remount reproducer on an HC620 left two partially used
> sequential metadata block groups active while active_meta_bg remained
> NULL:
> 
>   bg=343865819136 alloc=176095232 cap=268435456
>   bg=344402690048 alloc=3260416   cap=268435456
> 

Can you submit a fstest for that please? Maybe even in generic/ so XFS and
F2FS also get coverage?

> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index 10e15a319b93..aa3d6b66495e 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -931,6 +931,10 @@ struct btrfs_fs_info {
>  	u64 data_reloc_bg;
>  	struct mutex zoned_data_reloc_io_lock;
>  
> +	/*
> +	 * Active block groups for normal metadata and system writes. Mount
> +	 * recovery restores one block group for each role and finishes any extras.
> +	 */

I don't think this comments adds any value.

>  	struct btrfs_block_group *active_meta_bg;
>  	struct btrfs_block_group *active_system_bg;
>  
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index bd079d3b31ba..7d91c074a24a 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -3050,22 +3050,73 @@ int btrfs_zoned_activate_one_bg(struct btrfs_space_info *space_info, bool do_fin
>  	return 0;
>  }
>  
> +static int finish_extra_active_nondata_bgs(struct btrfs_fs_info *fs_info)
> +{
> +	struct btrfs_block_group *block_group;
> +	struct btrfs_block_group *next;
> +	u64 tail_unusable;
> +	int ret;
> +
> +	list_for_each_entry_safe(block_group, next, &fs_info->zone_active_bgs,
> +				 active_bg_list) {
> +		if (!(block_group->flags &
> +		      (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_SYSTEM)))
> +			continue;
> +
> +		if (block_group == fs_info->active_meta_bg ||
> +		    block_group == fs_info->active_system_bg)
> +			continue;
> +
> +		btrfs_get_block_group(block_group);
> +		tail_unusable = block_group->zone_capacity - block_group->alloc_offset;

Shouldn't these be accessed under the block_group->lock?

> -		if (!(block_group->flags &
> -		      (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_SYSTEM)))
> +		if (block_group->flags & BTRFS_BLOCK_GROUP_METADATA) {
> +			active_bg = &active_meta_bg;
> +			active_avail = &active_meta_avail;
> +		} else if (block_group->flags & BTRFS_BLOCK_GROUP_SYSTEM) {
> +			active_bg = &active_system_bg;
> +			active_avail = &active_system_avail;
> +		} else {
>  			continue;
> +		}
> +
> +		avail = block_group->zone_capacity - block_group->alloc_offset;

Same here, don't you need the block_group->lock?