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

["Miquel Sabaté Solà" <mssola@mssola.com>]

[-- Attachment #1: Type: text/plain, Size: 3220 bytes --]

Johannes Thumshirn @ 2026-07-03 11:50 +02:

> 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: stable@vger.kernel.org
>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>> ---
>>>   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.

Points raised by Sashiko aside, thanks for the clarifications, much
appreciated.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]