Re: [PATCH] btrfs: zoned: finish active block group cleanup if call_zone_finish() fails

On 8/19/26 12:36 PM, Qu Wenruo wrote:
>
>
> 在 2026/8/19 19:56, Johannes Thumshirn 写道:
>> do_zone_finish() clears BLOCK_GROUP_FLAG_ZONE_IS_ACTIVE before finishing
>> the zones. If call_zone_finish() then fails it returned early, 
>> leaving the
>> now inactive block group on fs_info->zone_active_bgs, leaking its
>> reference, the BTRFS_FS_NEED_ZONE_FINISH waiters are never woken, and as
>> its alloc_offset equals the zone capacity btrfs_zone_finish_one_bg() 
>> keeps
>> selecting it, spinning btrfs_zoned_activate_one_bg().
>>
>> Fall through to the cleanup on failure too and return the error, but 
>> keep
>> the block group read-only as its zones are left inconsistent.
>>
>> Fixes: d70cbdda75da ("btrfs: zoned: consolidate zone finish functions")
>> Link: 
>> https://sashiko.dev/#/patchset/20260818100037.1366563-1-johannes.thumshirn%40wdc.com
>> Signed-off-by: Johannes Thumshirn <[email protected]>
>> ---
>>   fs/btrfs/zoned.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
>> index 82c2394dd43c..9d448cdd60c4 100644
>> --- a/fs/btrfs/zoned.c
>> +++ b/fs/btrfs/zoned.c
>> @@ -2628,14 +2628,12 @@ static int do_zone_finish(struct 
>> btrfs_block_group *block_group, bool fully_writ
>>       for (i = 0; i < map->num_stripes; i++) {
>>             ret = call_zone_finish(block_group, &map->stripes[i]);
>> -        if (ret) {
>> -            up_read(&dev_replace->rwsem);
>> -            return ret;
>> -        }
>> +        if (ret)
>> +            break;
>
> I'm wondering should we continue to finish the other stripes instead?

But then one stripe would stay active, leaking that HW resource.


>
>>       }
>>       up_read(&dev_replace->rwsem);
>>   -    if (!fully_written)
>> +    if (!ret && !fully_written)
>
> Not familiar with zoned code, but if we hit an error, the block group 
> will stay RO.
>
> Is that the expected behavior?
Yes. If zone finish fails for some reason it was probably a hardware 
error on that zone. So my take was to leave it RO to be safe.
>
> The branch for bg with FLAG_ZONED_DATA_RELOC case, we return -EAGAIN 
> but decrease the bg RO before returning.
>
But in that case call_zone_finish() hasn't been called yet, so we don't 
know if we have an error from the block-layer/hw side.


Hope that makes sense.


> So it looks like we should always decrease the bg RO even for error case?
>
> Thanks,
> Qu
>
>> btrfs_dec_block_group_ro(block_group);
>>         spin_lock(&fs_info->zone_active_bgs_lock);
>> @@ -2648,7 +2646,7 @@ static int do_zone_finish(struct 
>> btrfs_block_group *block_group, bool fully_writ
>>         clear_and_wake_up_bit(BTRFS_FS_NEED_ZONE_FINISH, 
>> &fs_info->flags);
>>   -    return 0;
>> +    return ret;
>>   }
>>     int btrfs_zone_finish(struct btrfs_block_group *block_group)
>