From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2120.oracle.com ([156.151.31.85]:58482 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752470AbdLHNDX (ORCPT ); Fri, 8 Dec 2017 08:03:23 -0500 Subject: Re: [PATCH RFC] btrfs: self heal from SB fail To: Nikolay Borisov , linux-btrfs@vger.kernel.org References: <20171208075705.23462-1-anand.jain@oracle.com> <4d5ebff9-8556-a7d5-82ee-03b69776ddd1@suse.com> From: Anand Jain Message-ID: <2ac0fc6d-9053-6275-2e79-28dd33602bba@oracle.com> Date: Fri, 8 Dec 2017 21:02:47 +0800 MIME-Version: 1.0 In-Reply-To: <4d5ebff9-8556-a7d5-82ee-03b69776ddd1@suse.com> Content-Type: text/plain; charset=utf-8; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: 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 >>>> --- >>>>   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 >>