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

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

[Johannes Thumshirn]

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 (*active_bg == block_group) {
+			btrfs_put_block_group(block_group);
+			*active_bg = NULL;
+		}
+		btrfs_zoned_meta_io_unlock(fs_info);
+	}
+
 	clear_and_wake_up_bit(BTRFS_FS_NEED_ZONE_FINISH, &fs_info->flags);
 
 	return 0;
-- 
2.54.0

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

[Miquel Sabaté Solà]

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

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?

> +		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?

> +			*active_bg = NULL;
> +		}
> +		btrfs_zoned_meta_io_unlock(fs_info);
> +	}
> +
>  	clear_and_wake_up_bit(BTRFS_FS_NEED_ZONE_FINISH, &fs_info->flags);
>
>  	return 0;

Thanks,
Miquel

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

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

[Johannes Thumshirn]

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.

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

[Miquel Sabaté Solà]

[-- 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 --]

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

[Johannes Thumshirn]

On 7/3/26 1:41 PM, Miquel Sabaté Solà wrote:
> Points raised by Sashiko aside, thanks for the clarifications, much
> appreciated.

No problem, you're welcome.

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

[Johannes Thumshirn]

On 7/3/26 10:45 AM, Johannes Thumshirn wrote:
> 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 (*active_bg == block_group) {
> +			btrfs_put_block_group(block_group);
> +			*active_bg = NULL;
> +		}
> +		btrfs_zoned_meta_io_unlock(fs_info);
> +	}
> +
>   	clear_and_wake_up_bit(BTRFS_FS_NEED_ZONE_FINISH, &fs_info->flags);
>   
>   	return 0;

I think Sashiko has a point here:

https://sashiko.dev/#/patchset/20260703084559.136605-1-johannes.thumshirn%40wdc.com

check_bg_is_active() should take a reference before calling into 
do_zone_finish() or actually clearing fs_info->active_{meta,system}_bg 
can even be done in check_bg_is_active() after calling do_zone_finish().

That'll then also eliminate Miquel's concerns.

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

[Miquel Sabaté Solà]

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

Johannes Thumshirn @ 2026-07-03 12:01 +02:

> On 7/3/26 10:45 AM, Johannes Thumshirn wrote:
>> 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 (*active_bg == block_group) {
>> +			btrfs_put_block_group(block_group);
>> +			*active_bg = NULL;
>> +		}
>> +		btrfs_zoned_meta_io_unlock(fs_info);
>> +	}
>> +
>>   	clear_and_wake_up_bit(BTRFS_FS_NEED_ZONE_FINISH, &fs_info->flags);
>>     	return 0;
>
> I think Sashiko has a point here:
>
> https://sashiko.dev/#/patchset/20260703084559.136605-1-johannes.thumshirn%40wdc.com
>
> check_bg_is_active() should take a reference before calling into
> do_zone_finish() or actually clearing fs_info->active_{meta,system}_bg can even
> be done in check_bg_is_active() after calling do_zone_finish().
>
> That'll then also eliminate Miquel's concerns.

I'd maybe take the latter to avoid adding more complexity to an already
complex do_zone_finish(). Besides, I see that do_zone_finish() is
already called in many other places throughout btrfs/zoned.c, so I
wonder if having the changes in do_zone_finish() would also "spill" over
there.

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

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

[Johannes Thumshirn]

On 7/3/26 1:46 PM, Miquel Sabaté Solà wrote:
> Johannes Thumshirn @ 2026-07-03 12:01 +02:
>
>> On 7/3/26 10:45 AM, Johannes Thumshirn wrote:
>>> 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 (*active_bg == block_group) {
>>> +			btrfs_put_block_group(block_group);
>>> +			*active_bg = NULL;
>>> +		}
>>> +		btrfs_zoned_meta_io_unlock(fs_info);
>>> +	}
>>> +
>>>    	clear_and_wake_up_bit(BTRFS_FS_NEED_ZONE_FINISH, &fs_info->flags);
>>>      	return 0;
>> I think Sashiko has a point here:
>>
>> https://sashiko.dev/#/patchset/20260703084559.136605-1-johannes.thumshirn%40wdc.com
>>
>> check_bg_is_active() should take a reference before calling into
>> do_zone_finish() or actually clearing fs_info->active_{meta,system}_bg can even
>> be done in check_bg_is_active() after calling do_zone_finish().
>>
>> That'll then also eliminate Miquel's concerns.
> I'd maybe take the latter to avoid adding more complexity to an already
> complex do_zone_finish(). Besides, I see that do_zone_finish() is
> already called in many other places throughout btrfs/zoned.c, so I
> wonder if having the changes in do_zone_finish() would also "spill" over
> there.

Yep that's what I did for v2. But I didn't want to send it out too 
quickly, to give other possible reviewers a chance to eventually spot 
another bug before.