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

["Dongjiang Zhu" <zhudongjiang@fygo.io>]

在 2026/8/24 22:42, Johannes Thumshirn 写道:
> 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?
> 

Yes, I can work on a separate fstest. To make sure I understand the
suggestion correctly, do you have in mind a zoned-device test that
creates a metadata-heavy workload, syncs, fully unmounts and mounts the
filesystem repeatedly, and then verifies that further writes and syncs
still succeed?

>> 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.
> 

Agreed, I will remove it.

>>   	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?
> 
> [...]
> 
> Same here, don't you need the block_group->lock?

Both accesses only happen during open_ctree(), after all block groups
have been loaded and before the background threads are started. Following
other mount-time initialization code, I assumed that block_group->lock
was not necessary because the allocation state cannot change concurrently.

On the other hand, taking the lock would make the usual protection of
alloc_offset explicit and avoid relying on the mount-time context for
future maintenance. Would you prefer that I add the locking around both
reads in v2?

Thanks,
Dongjiang Zhu