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

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

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