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

[Anand Jain <anand.jain@oracle.com>]

On 12/08/2017 08:12 PM, Nikolay Borisov wrote:
> 
> 
> On  8.12.2017 12:33, Anand Jain wrote:
>>
>>
>> On 12/08/2017 04:40 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On  8.12.2017 09: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.
>>>>
>>>> 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);
>>>
>>> This patch prompts another question: why do we have a page-based and a
>>> bufferhead-based interface to reading the super block?
>>
>>   Right. we need to know that. Sorry I just saw this.
> 
> FWIW unless we explicitly need a per-block state tracking (which I don't
> think we do) we should ideally switch to page-based mechanism. Buffer
> heads are considered deprecated. Also the only reason why we do have
> btrfsic_submit_bh is for the superblock bufer head. So potentially by
> removing the only requirement in the kernel where bh are used we can
> simplify the btrfsic code as well . Just something to keep in mind.

  Thanks. These are related to the other email thread [1]
  [1]
  [Question] Two variants of SB reads can we move into bio read instead ?
  We can follow up there. ? Could we ? That cleanup is due for a long
  time now.

Thanks, Anand

>>
>>   I have a very old patch to converge them and clean this up, but haven't
>>   sent it because there are some missing information on why it ended up
>>   like that in the first place.
>>
>> Thanks, Anand
>>
>>
>>> I did prototype
>>> switching the bufferheads to page based but the resulting code wasn't
>>> any cleaner. I believe there is also open the question what happens when
>>> btrfs is run on a 64k page machine. I.e. we are going to read a single
>>> page and the sb is going to be in the first 4k but what about the rest
>>> 60, they could potentially contain other metadata. The page will have to
>>> be freed asap so as not to peg the neighboring metadata?
>>
>>
>>>
>>>> +    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
>>