On 2017年12月08日 20:41, Anand Jain wrote:
>
>
> On 12/08/2017 08:02 PM, Qu Wenruo wrote:
>>
>>
>> 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.
>>
>
> I have worked quite a lot on defining the problem statements before
> btrfs. Here the user has to be blamed to have forgotten to update
> the fstab. I don't understand why should we workaround in btrfs for
> the reason that user may miss to update fstab. We don't design
> a stuff that like that, we design for the purpose, here backup SB
> is for the purpose that we all know, if we don't use backup SB, then
> its an incomplete design.
Then implement something like ext*, using explicit mount option sb=n.
And since it's already called "backup", it's something used for
recovery, not for normal operation.
We already have rescue tool to recover sb from backup, so it's not a
incomplete design.
Personally speaking, I prefer the current way and leave backup just as
backup.
You can my example of a user fault, but the real world needs us to
handle user's fault.
Or there will be no need for mkfs -f options or rm --no-preserve-root
options at all.
Thanks,
Qu
>
>>>
>>>>
>>>>>
>>>>>> 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.
>
> There is no problem is user does the correct thing that is to
> update the fstab. OR using a complete mount command. If user
> using -t btrfs OR unupdated btrfs then it indicates his intentions.
>
> Thanks, Anand
>
>> 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@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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@xxxxxxxxxxxxxxx
>>>>> 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@xxxxxxxxxxxxxxx
>>> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: OpenPGP digital signature
