[PATCH] btrfs: repair super block num_devices automatically

[PATCH] btrfs: repair super block num_devices automatically

[Qu Wenruo]

[BUG]
There is a report that a btrfs has a bad super block num devices.

This makes btrfs to reject the fs completely.

[CAUSE]
It's pretty straightforward, if super_num_devices doesn't match the
deviecs we found in chunk tree, there must be some wrong with the fs
thus we can reject the fs.

But on the other hand, super_num_devices is not determinant counter,
especially when we already have a correct counter after iterating the
chunk tree.

[FIX]
Make the super_num_devices check less strict, converting it from a hard
error to a warning, and reset the value to a correct one for the current
or next transaction commitment.

Reported-by: Luca Béla Palkovics <luca.bela.palkovics@gmail.com>
Link: https://lore.kernel.org/linux-btrfs/CA+8xDSpvdm_U0QLBAnrH=zqDq_cWCOH5TiV46CKmp3igr44okQ@mail.gmail.com/
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 74c8024d8f96..d0ba3ff21920 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7682,12 +7682,12 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
 	 * do another round of validation checks.
 	 */
 	if (total_dev != fs_info->fs_devices->total_devices) {
-		btrfs_err(fs_info,
-	   "super_num_devices %llu mismatch with num_devices %llu found here",
+		btrfs_warn(fs_info,
+	   "super_num_devices %llu mismatch with num_devices %llu found here, will be repaired on next transaction commitment",
 			  btrfs_super_num_devices(fs_info->super_copy),
 			  total_dev);
-		ret = -EINVAL;
-		goto error;
+		fs_info->fs_devices->total_devices = total_dev;
+		btrfs_set_super_num_devices(fs_info->super_copy, total_dev);
 	}
 	if (btrfs_super_total_bytes(fs_info->super_copy) <
 	    fs_info->fs_devices->total_rw_bytes) {
-- 
2.35.1

Re: [PATCH] btrfs: repair super block num_devices automatically

[Anand Jain]

On 28/02/2022 07:51, Qu Wenruo wrote:
> [BUG]
> There is a report that a btrfs has a bad super block num devices.
> 
> This makes btrfs to reject the fs completely.
> 

> [CAUSE]
> It's pretty straightforward, if super_num_devices doesn't match the
> deviecs we found in chunk tree, there must be some wrong with the fs
> thus we can reject the fs.
> 
> But on the other hand, super_num_devices is not determinant counter,
> especially when we already have a correct counter after iterating the
> chunk tree.

Cause analysis is incomplete, given that SB write is the last. The root
(and thus chunk tree) and super_num_devices will be consistent always.
Do we know how the miss-match happened?

Thanks, Anand


> [FIX]
> Make the super_num_devices check less strict, converting it from a hard
> error to a warning, and reset the value to a correct one for the current
> or next transaction commitment.
> 
> Reported-by: Luca Béla Palkovics <luca.bela.palkovics@gmail.com>
> Link: https://lore.kernel.org/linux-btrfs/CA+8xDSpvdm_U0QLBAnrH=zqDq_cWCOH5TiV46CKmp3igr44okQ@mail.gmail.com/
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/volumes.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 74c8024d8f96..d0ba3ff21920 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7682,12 +7682,12 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
>   	 * do another round of validation checks.
>   	 */
>   	if (total_dev != fs_info->fs_devices->total_devices) {
> -		btrfs_err(fs_info,
> -	   "super_num_devices %llu mismatch with num_devices %llu found here",
> +		btrfs_warn(fs_info,
> +	   "super_num_devices %llu mismatch with num_devices %llu found here, will be repaired on next transaction commitment",
>   			  btrfs_super_num_devices(fs_info->super_copy),
>   			  total_dev);
> -		ret = -EINVAL;
> -		goto error;
> +		fs_info->fs_devices->total_devices = total_dev;
> +		btrfs_set_super_num_devices(fs_info->super_copy, total_dev);
>   	}
>   	if (btrfs_super_total_bytes(fs_info->super_copy) <
>   	    fs_info->fs_devices->total_rw_bytes) {

Re: [PATCH] btrfs: repair super block num_devices automatically

[Qu Wenruo]

On 2022/2/28 11:08, Anand Jain wrote:
> On 28/02/2022 07:51, Qu Wenruo wrote:
>> [BUG]
>> There is a report that a btrfs has a bad super block num devices.
>>
>> This makes btrfs to reject the fs completely.
>>
>
>> [CAUSE]
>> It's pretty straightforward, if super_num_devices doesn't match the
>> deviecs we found in chunk tree, there must be some wrong with the fs
>> thus we can reject the fs.
>>
>> But on the other hand, super_num_devices is not determinant counter,
>> especially when we already have a correct counter after iterating the
>> chunk tree.
>
> Cause analysis is incomplete, given that SB write is the last. The root
> (and thus chunk tree) and super_num_devices will be consistent always.
> Do we know how the miss-match happened?

Sorry, I should provide a full analyse on this.

In fact there is a window in device remove path that we first remove
device item in chunk tree, and COMMIT transaction, then decrease the
device counter (without commit transaction immediately).

In fact, there is already a TODO comment in btrfs_rm_dev_item() call
inside btrfs_rm_device() saying exactly the same thing.

Thus if we got a power loss/reboot, like what the reporter is doing, it
will cause such mismatch.

Thanks,
Qu

>
> Thanks, Anand
>
>
>> [FIX]
>> Make the super_num_devices check less strict, converting it from a hard
>> error to a warning, and reset the value to a correct one for the current
>> or next transaction commitment.
>>
>> Reported-by: Luca Béla Palkovics <luca.bela.palkovics@gmail.com>
>> Link:
>> https://lore.kernel.org/linux-btrfs/CA+8xDSpvdm_U0QLBAnrH=zqDq_cWCOH5TiV46CKmp3igr44okQ@mail.gmail.com/
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/volumes.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 74c8024d8f96..d0ba3ff21920 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -7682,12 +7682,12 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info
>> *fs_info)
>>        * do another round of validation checks.
>>        */
>>       if (total_dev != fs_info->fs_devices->total_devices) {
>> -        btrfs_err(fs_info,
>> -       "super_num_devices %llu mismatch with num_devices %llu found
>> here",
>> +        btrfs_warn(fs_info,
>> +       "super_num_devices %llu mismatch with num_devices %llu found
>> here, will be repaired on next transaction commitment",
>>                 btrfs_super_num_devices(fs_info->super_copy),
>>                 total_dev);
>> -        ret = -EINVAL;
>> -        goto error;
>> +        fs_info->fs_devices->total_devices = total_dev;
>> +        btrfs_set_super_num_devices(fs_info->super_copy, total_dev);
>>       }
>>       if (btrfs_super_total_bytes(fs_info->super_copy) <
>>           fs_info->fs_devices->total_rw_bytes) {
>

Re: [PATCH] btrfs: repair super block num_devices automatically

[Anand Jain]

On 28/02/2022 11:17, Qu Wenruo wrote:
> 
> 
> On 2022/2/28 11:08, Anand Jain wrote:
>> On 28/02/2022 07:51, Qu Wenruo wrote:
>>> [BUG]
>>> There is a report that a btrfs has a bad super block num devices.
>>>
>>> This makes btrfs to reject the fs completely.
>>>
>>
>>> [CAUSE]
>>> It's pretty straightforward, if super_num_devices doesn't match the
>>> deviecs we found in chunk tree, there must be some wrong with the fs
>>> thus we can reject the fs.
>>>
>>> But on the other hand, super_num_devices is not determinant counter,
>>> especially when we already have a correct counter after iterating the
>>> chunk tree.
>>
>> Cause analysis is incomplete, given that SB write is the last. The root
>> (and thus chunk tree) and super_num_devices will be consistent always.
>> Do we know how the miss-match happened?
> 
> Sorry, I should provide a full analyse on this.
> 
> In fact there is a window in device remove path that we first remove
> device item in chunk tree, and COMMIT transaction, then decrease the
> device counter (without commit transaction immediately).
> 
> In fact, there is already a TODO comment in btrfs_rm_dev_item() call
> inside btrfs_rm_device() saying exactly the same thing.
> 
> Thus if we got a power loss/reboot, like what the reporter is doing, it
> will cause such mismatch.


If sb write commit failed. Why isn't root read from the superblock
pointing to the old chunk tree with 3device items?

Thanks, Anand

> Thanks,
> Qu
> 
>>
>> Thanks, Anand
>>
>>
>>> [FIX]
>>> Make the super_num_devices check less strict, converting it from a hard
>>> error to a warning, and reset the value to a correct one for the current
>>> or next transaction commitment.
>>>
>>> Reported-by: Luca Béla Palkovics <luca.bela.palkovics@gmail.com>
>>> Link:
>>> https://lore.kernel.org/linux-btrfs/CA+8xDSpvdm_U0QLBAnrH=zqDq_cWCOH5TiV46CKmp3igr44okQ@mail.gmail.com/ 
>>>
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   fs/btrfs/volumes.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 74c8024d8f96..d0ba3ff21920 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -7682,12 +7682,12 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info
>>> *fs_info)
>>>        * do another round of validation checks.
>>>        */
>>>       if (total_dev != fs_info->fs_devices->total_devices) {
>>> -        btrfs_err(fs_info,
>>> -       "super_num_devices %llu mismatch with num_devices %llu found
>>> here",
>>> +        btrfs_warn(fs_info,
>>> +       "super_num_devices %llu mismatch with num_devices %llu found
>>> here, will be repaired on next transaction commitment",
>>>                 btrfs_super_num_devices(fs_info->super_copy),
>>>                 total_dev);
>>> -        ret = -EINVAL;
>>> -        goto error;
>>> +        fs_info->fs_devices->total_devices = total_dev;
>>> +        btrfs_set_super_num_devices(fs_info->super_copy, total_dev);
>>>       }
>>>       if (btrfs_super_total_bytes(fs_info->super_copy) <
>>>           fs_info->fs_devices->total_rw_bytes) {
>>

Re: [PATCH] btrfs: repair super block num_devices automatically

[Qu Wenruo]

On 2022/2/28 13:49, Anand Jain wrote:
> On 28/02/2022 11:17, Qu Wenruo wrote:
>>
>>
>> On 2022/2/28 11:08, Anand Jain wrote:
>>> On 28/02/2022 07:51, Qu Wenruo wrote:
>>>> [BUG]
>>>> There is a report that a btrfs has a bad super block num devices.
>>>>
>>>> This makes btrfs to reject the fs completely.
>>>>
>>>
>>>> [CAUSE]
>>>> It's pretty straightforward, if super_num_devices doesn't match the
>>>> deviecs we found in chunk tree, there must be some wrong with the fs
>>>> thus we can reject the fs.
>>>>
>>>> But on the other hand, super_num_devices is not determinant counter,
>>>> especially when we already have a correct counter after iterating the
>>>> chunk tree.
>>>
>>> Cause analysis is incomplete, given that SB write is the last. The root
>>> (and thus chunk tree) and super_num_devices will be consistent always.
>>> Do we know how the miss-match happened?
>>
>> Sorry, I should provide a full analyse on this.
>>
>> In fact there is a window in device remove path that we first remove
>> device item in chunk tree, and COMMIT transaction, then decrease the
>> device counter (without commit transaction immediately).
>>
>> In fact, there is already a TODO comment in btrfs_rm_dev_item() call
>> inside btrfs_rm_device() saying exactly the same thing.
>>
>> Thus if we got a power loss/reboot, like what the reporter is doing, it
>> will cause such mismatch.
> 
> 
> If sb write commit failed. Why isn't root read from the superblock
> pointing to the old chunk tree with 3device items?

The failed trans is not the trans committed in btrfs_rm_dev_item().

> 
> Thanks, Anand
> 
>> Thanks,
>> Qu
>>
>>>
>>> Thanks, Anand
>>>
>>>
>>>> [FIX]
>>>> Make the super_num_devices check less strict, converting it from a hard
>>>> error to a warning, and reset the value to a correct one for the 
>>>> current
>>>> or next transaction commitment.
>>>>
>>>> Reported-by: Luca Béla Palkovics <luca.bela.palkovics@gmail.com>
>>>> Link:
>>>> https://lore.kernel.org/linux-btrfs/CA+8xDSpvdm_U0QLBAnrH=zqDq_cWCOH5TiV46CKmp3igr44okQ@mail.gmail.com/ 
>>>>
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>   fs/btrfs/volumes.c | 8 ++++----
>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 74c8024d8f96..d0ba3ff21920 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -7682,12 +7682,12 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info
>>>> *fs_info)
>>>>        * do another round of validation checks.
>>>>        */
>>>>       if (total_dev != fs_info->fs_devices->total_devices) {
>>>> -        btrfs_err(fs_info,
>>>> -       "super_num_devices %llu mismatch with num_devices %llu found
>>>> here",
>>>> +        btrfs_warn(fs_info,
>>>> +       "super_num_devices %llu mismatch with num_devices %llu found
>>>> here, will be repaired on next transaction commitment",
>>>>                 btrfs_super_num_devices(fs_info->super_copy),
>>>>                 total_dev);
>>>> -        ret = -EINVAL;
>>>> -        goto error;
>>>> +        fs_info->fs_devices->total_devices = total_dev;
>>>> +        btrfs_set_super_num_devices(fs_info->super_copy, total_dev);
>>>>       }
>>>>       if (btrfs_super_total_bytes(fs_info->super_copy) <
>>>>           fs_info->fs_devices->total_rw_bytes) {
>>>
> 

Re: [PATCH] btrfs: repair super block num_devices automatically

[Anand Jain]

On 28/02/2022 14:33, Qu Wenruo wrote:
> 
> 
> On 2022/2/28 13:49, Anand Jain wrote:
>> On 28/02/2022 11:17, Qu Wenruo wrote:
>>>
>>>
>>> On 2022/2/28 11:08, Anand Jain wrote:
>>>> On 28/02/2022 07:51, Qu Wenruo wrote:
>>>>> [BUG]
>>>>> There is a report that a btrfs has a bad super block num devices.
>>>>>
>>>>> This makes btrfs to reject the fs completely.
>>>>>
>>>>
>>>>> [CAUSE]
>>>>> It's pretty straightforward, if super_num_devices doesn't match the
>>>>> deviecs we found in chunk tree, there must be some wrong with the fs
>>>>> thus we can reject the fs.
>>>>>
>>>>> But on the other hand, super_num_devices is not determinant counter,
>>>>> especially when we already have a correct counter after iterating the
>>>>> chunk tree.
>>>>
>>>> Cause analysis is incomplete, given that SB write is the last. The root
>>>> (and thus chunk tree) and super_num_devices will be consistent always.
>>>> Do we know how the miss-match happened?
>>>
>>> Sorry, I should provide a full analyse on this.
>>>
>>> In fact there is a window in device remove path that we first remove
>>> device item in chunk tree, and COMMIT transaction, then decrease the
>>> device counter (without commit transaction immediately).
>>>
>>> In fact, there is already a TODO comment in btrfs_rm_dev_item() call
>>> inside btrfs_rm_device() saying exactly the same thing.
>>>
>>> Thus if we got a power loss/reboot, like what the reporter is doing, it
>>> will cause such mismatch.
>>
>>
>> If sb write commit failed. Why isn't root read from the superblock
>> pointing to the old chunk tree with 3device items?
> 
> The failed trans is not the trans committed in btrfs_rm_dev_item().

Ah. I got it. So why not fix the TODO instead of fixing debris due to
missed write?


>> Thanks, Anand
>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> Thanks, Anand
>>>>
>>>>
>>>>> [FIX]
>>>>> Make the super_num_devices check less strict, converting it from a 
>>>>> hard
>>>>> error to a warning, and reset the value to a correct one for the 
>>>>> current
>>>>> or next transaction commitment.
>>>>>
>>>>> Reported-by: Luca Béla Palkovics <luca.bela.palkovics@gmail.com>
>>>>> Link:
>>>>> https://lore.kernel.org/linux-btrfs/CA+8xDSpvdm_U0QLBAnrH=zqDq_cWCOH5TiV46CKmp3igr44okQ@mail.gmail.com/ 
>>>>>
>>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>> ---
>>>>>   fs/btrfs/volumes.c | 8 ++++----
>>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>>> index 74c8024d8f96..d0ba3ff21920 100644
>>>>> --- a/fs/btrfs/volumes.c
>>>>> +++ b/fs/btrfs/volumes.c
>>>>> @@ -7682,12 +7682,12 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info
>>>>> *fs_info)
>>>>>        * do another round of validation checks.
>>>>>        */
>>>>>       if (total_dev != fs_info->fs_devices->total_devices) {
>>>>> -        btrfs_err(fs_info,
>>>>> -       "super_num_devices %llu mismatch with num_devices %llu found
>>>>> here",
>>>>> +        btrfs_warn(fs_info,
>>>>> +       "super_num_devices %llu mismatch with num_devices %llu found
>>>>> here, will be repaired on next transaction commitment",
>>>>>                 btrfs_super_num_devices(fs_info->super_copy),
>>>>>                 total_dev);
>>>>> -        ret = -EINVAL;
>>>>> -        goto error;
>>>>> +        fs_info->fs_devices->total_devices = total_dev;
>>>>> +        btrfs_set_super_num_devices(fs_info->super_copy, total_dev);
>>>>>       }
>>>>>       if (btrfs_super_total_bytes(fs_info->super_copy) <
>>>>>           fs_info->fs_devices->total_rw_bytes) {
>>>>
>>
>