Re: [PATCH] btrfs: zoned: reset active_meta_bg on zone finish

On 7/3/26 11:31 AM, Miquel Sabaté Solà wrote:
> Hi,
>
> If you don't mind, a couple of questions from a newcomer that is trying
> to grok this part of the code :)
>
> Johannes Thumshirn @ 2026-07-03 10:45 +02:
>
>> do_zone_finish() clears BLOCK_GROUP_FLAG_ZONE_IS_ACTIVE and removes the
>> block group from zone_active_bgs, but only the path in
>> check_bg_is_active() resets fs_info->active_meta_bg / active_system_bg.
>> Any other finish path leaves active_meta_bg / active_system_bg pointing
>> at an inactive, fully written block group.
>>
>> Reset the corresponding active_{meta,system}_bg pointer in do_zone_finish()
>> so it can never go stale.
>>
>> Fixes: 13bb483d32ab ("btrfs: zoned: activate metadata block group on write time")
>> Cc: [email protected]
>> Signed-off-by: Johannes Thumshirn <[email protected]>
>> ---
>>   fs/btrfs/zoned.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
>> index 44a13ed6b8b2..c8c850de1702 100644
>> --- a/fs/btrfs/zoned.c
>> +++ b/fs/btrfs/zoned.c
>> @@ -2539,6 +2539,7 @@ static int do_zone_finish(struct btrfs_block_group *block_group, bool fully_writ
>>   	const bool is_metadata = (block_group->flags &
>>   			(BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_SYSTEM));
>>   	struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
>> +	struct btrfs_block_group **active_bg = NULL;
>>   	int ret = 0;
>>   	int i;
>>
>> @@ -2636,6 +2637,20 @@ static int do_zone_finish(struct btrfs_block_group *block_group, bool fully_writ
>>   	/* For active_bg_list */
>>   	btrfs_put_block_group(block_group);
>>
>> +	if (block_group->flags & BTRFS_BLOCK_GROUP_SYSTEM)
>> +		active_bg = &fs_info->active_system_bg;
>> +	else if (block_group->flags & BTRFS_BLOCK_GROUP_METADATA)
>> +		active_bg = &fs_info->active_meta_bg;
>> +
>> +	if (active_bg) {
>> +		btrfs_zoned_meta_io_lock(fs_info);
> If you need to lock/unlock in order call btrfs_put_block_group() and
> then reset *active_bg, couldn't the previous if statement be written
> like so?
>
> if (active_bg && (*active_bg == block_group)) {
>
> This would then only lock/unlock just in the case we really want to
> touch this 'block_group', no?


Yes it could be simplified, but I'm thinking what it would buy us. For 
sure we would not take the lock when finishing a DATA block-group here. 
Note the lock is not protecting a data structure but is for serializing 
metadata writes, so we do a QD=1 write to the drive for METADATA/SYSTEM 
block-groups as we cannot use REQ_OP_ZONE_APPEND on these.


>> +		if (*active_bg == block_group) {
>> +			btrfs_put_block_group(block_group);
> Also, hasn't 'block_group' already been put before your patch? Won't
> this try to double-free this pointer? Or it is about decreasing the
> reference twice for this block group?
The put before is for the reference on the active_bgs_list, so we should 
still have a reference left.