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

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

[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 <johannes.thumshirn@wdc.com>
---
 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;
 	}
 	up_read(&dev_replace->rwsem);
 
-	if (!fully_written)
+	if (!ret && !fully_written)
 		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)
-- 
2.55.0

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

[Qu Wenruo]

在 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 <johannes.thumshirn@wdc.com>
> ---
>   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?

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

The branch for bg with FLAG_ZONED_DATA_RELOC case, we return -EAGAIN but 
decrease the bg RO before returning.

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)

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

[Johannes Thumshirn]

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 <johannes.thumshirn@wdc.com>
>> ---
>>   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)
>

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

[Qu Wenruo]

在 2026/8/19 20:14, Johannes Thumshirn 写道:
> 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 <johannes.thumshirn@wdc.com>
>>> ---
>>>   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.

I mean, if we have 3 stripes, and the 2nd stripe failed to be finished.
Should the remaining two be finished, or only the first one be finished?

I guess it doesn't make much difference, as for one call site, we will 
mark the full fs as error, but not all call sites are doing so.

That may be something to consider later though.
> 
> 
>>
>>>       }
>>>       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.

OK, makes sense.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> 
> 
> 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)
>>
>