Re: [PATCH RFC] btrfs: self heal from SB fail

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6983 bytes --]



On 2017年12月08日 19:48, Anand Jain wrote:
> 
> 
> On 12/08/2017 07:01 PM, Qu Wenruo wrote:
>>
>>
>> On 2017年12月08日 18:39, Anand Jain wrote:
>>>
>>>
>>> On 12/08/2017 04:17 PM, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2017年12月08日 15:57, Anand Jain wrote:
>>>>> -EXPERIMENTAL-
>>>>> As of now when primary SB fails we won't self heal and would fail
>>>>> mount,
>>>>> this is an experimental patch which thinks why not go and read backup
>>>>> copy.
>>>>
>>>> Just curious about in which real world case that backup super block can
>>>> help.
>>>> At least from what I see in mail list, only few cases where backup
>>>> super
>>>> helps.
>>>
>>>   Theoretical design helps. I ended up in this situation though. And
>>>   ext4 has -o sb flag to manage this part. When we can expect EIO on
>>>   any part of the disk block why not on the LBA which contains primary
>>>   SB. And should we fail the mount for that reason ? No.
>>
>> And how do you ensure it's a btrfs?
> 
>  Hmm. You mean outside of btrfs ? I did experiment with wipe and then
>  using /etc/fstab to mount, and it did lead to btrfs, is that your
>  concern that it shouldn't have been. That looked surprising to me as
>  well, but then problem points at wipefs instead.

It's closer, but still doesn't reach the point.
It can be a mkfs, other than wipefs.

It's can be another case like:

One user used btrfs for a while
But bugs made him/her unhappy, and he/she turned to use xfs (whatever
the fs is) instead.

While he/she forgot to change its fstab, and rebooted the system.

And suddenly, it's btrfs again!
What a surprise.


> 
>>
>>>
>>>> Despite that self super heal seems good, although I agree with
>>>> David, we
>>>> need a weaker but necessary check (magic and fsid from primary super?)
>>>> to ensure it's a valid btrfs before we use the backup supers.
>>>
>>>   Of course, we already have btrfs_check_super_valid() to verify the SB,
>>>   I don't understand why - how do we verify the SB should be the
>>> point of
>>>   concern here, at all.
>>
>> The point here is, to distinguish an old and invalid btrfs (some other
>> valid fs mkfs beyond the old fs) from a valid btrfs with corrupted
>> primary fs.
> 
>  Ok. When you check all the SBs you would pick the one which has passed
>  btrfs_check_super_valid() and has highest generation. Did I ans your
>  concern ? If a SB does not pass btrfs_check_super_valid() its not a
>  valid btrfs SB at all.

No, not really.

What if the first SB is a XFS one or even a fs you didn't ever hear?

Skip it and use the 2nd one?
This filesystem is not even btrfs anymore.

Thanks,
Qu

> 
> 
>> This the main concern here.
>> The difference between recovery and recognizing a bad btrfs is quite
>> important.
> 
>  btrfs_check_super_valid() is already doing that right ? The point here
>  is, should we use the backup SB when btrfs_check_super_valid() fails on
>  primary SB.
> 
> Thanks, Anand
> 
>> Thanks,
>> Qu
>>
>>>
>>> Thanks, Anand
>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>>> ---
>>>>>    fs/btrfs/disk-io.c |  8 +++++++-
>>>>>    fs/btrfs/volumes.c | 10 +++++++---
>>>>>    2 files changed, 14 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>>> index 9b20c1f3563b..a791b8dfe8a8 100644
>>>>> --- a/fs/btrfs/disk-io.c
>>>>> +++ b/fs/btrfs/disk-io.c
>>>>> @@ -3190,7 +3190,7 @@ struct buffer_head *btrfs_read_dev_super(struct
>>>>> block_device *bdev)
>>>>>         * So, we need to add a special mount option to scan for
>>>>>         * later supers, using BTRFS_SUPER_MIRROR_MAX instead
>>>>>         */
>>>>> -    for (i = 0; i < 1; i++) {
>>>>> +    for (i = 0; i < BTRFS_SUPER_MIRROR_MAX; i++) {
>>>>>            ret = btrfs_read_dev_one_super(bdev, i, &bh);
>>>>>            if (ret)
>>>>>                continue;
>>>>> @@ -4015,11 +4015,17 @@ static int btrfs_check_super_valid(struct
>>>>> btrfs_fs_info *fs_info)
>>>>>            ret = -EINVAL;
>>>>>        }
>>>>>    +#if 0
>>>>> +    /*
>>>>> +     * Need a way to check for any copy of SB, as its not a
>>>>> +     * strong check, just ignore this for now.
>>>>> +     */
>>>>>        if (btrfs_super_bytenr(sb) != BTRFS_SUPER_INFO_OFFSET) {
>>>>>            btrfs_err(fs_info, "super offset mismatch %llu != %u",
>>>>>                  btrfs_super_bytenr(sb), BTRFS_SUPER_INFO_OFFSET);
>>>>>            ret = -EINVAL;
>>>>>        }
>>>>> +#endif
>>>>>          /*
>>>>>         * Obvious sys_chunk_array corruptions, it must hold at least
>>>>> one key
>>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>>> index 9fa2539a8493..f368db94d62b 100644
>>>>> --- a/fs/btrfs/volumes.c
>>>>> +++ b/fs/btrfs/volumes.c
>>>>> @@ -1369,7 +1369,7 @@ int btrfs_scan_one_device(const char *path,
>>>>> fmode_t flags, void *holder,
>>>>>    {
>>>>>        struct btrfs_super_block *disk_super;
>>>>>        struct block_device *bdev;
>>>>> -    struct page *page;
>>>>> +    struct buffer_head *sb_bh;
>>>>>        int ret = -EINVAL;
>>>>>        u64 devid;
>>>>>        u64 transid;
>>>>> @@ -1392,8 +1392,12 @@ int btrfs_scan_one_device(const char *path,
>>>>> fmode_t flags, void *holder,
>>>>>            goto error;
>>>>>        }
>>>>>    -    if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super))
>>>>> +    sb_bh = btrfs_read_dev_super(bdev);
>>>>> +    if (IS_ERR(sb_bh)) {
>>>>> +        ret = PTR_ERR(sb_bh);
>>>>>            goto error_bdev_put;
>>>>> +    }
>>>>> +    disk_super = (struct btrfs_super_block *) sb_bh->b_data;
>>>>>          devid = btrfs_stack_device_id(&disk_super->dev_item);
>>>>>        transid = btrfs_super_generation(disk_super);
>>>>> @@ -1413,7 +1417,7 @@ int btrfs_scan_one_device(const char *path,
>>>>> fmode_t flags, void *holder,
>>>>>        if (!ret && fs_devices_ret)
>>>>>            (*fs_devices_ret)->total_devices = total_devices;
>>>>>    -    btrfs_release_disk_super(page);
>>>>> +    brelse(sb_bh);
>>>>>      error_bdev_put:
>>>>>        blkdev_put(bdev, flags);
>>>>>
>>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe
>>> linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]