Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

Anand Jain
2018-04-21T05:16:08Z

On 04/21/2018 10:43 AM, Qu Wenruo wrote:
>
>
> On 2018年04月21日 10:38, Anand Jain wrote:
>>
>>
>> On 04/20/2018 11:15 PM, David Sterba wrote:
>>> On Fri, Apr 20, 2018 at 10:32:03PM +0800, Anand Jain wrote:
>>>>
>>>>
>>>> On 04/19/2018 05:38 PM, Qu Wenruo wrote:
>>>>> Although we have already checked incompat flags manually before really
>>>>> mounting it, we could still enhance btrfs_check_super_valid() to check
>>>>> incompat flags for later write time super block validation check.
>>>>>
>>>>> This patch adds such incompat flags check for
>>>>> btrfs_check_super_valid(),
>>>>> currently it won't be triggered, but provides the basis for later write
>>>>> time check.
>>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>> ---
>>>>>    fs/btrfs/disk-io.c | 13 +++++++++++++
>>>>>    1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>>> index 60caa68c3618..ec123158f051 100644
>>>>> --- a/fs/btrfs/disk-io.c
>>>>> +++ b/fs/btrfs/disk-io.c
>>>>> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct
>>>>> btrfs_fs_info *fs_info)
>>>>>            ret = -EINVAL;
>>>>>        }
>>>>>    +    /*
>>>>> +     * Before calling btrfs_check_super_valid() we have already
>>>>> checked
>>>>> +     * incompat flags. So if we developr new incompat flags, it's
>>>>> must be
>>>>> +     * some corruption.
>>>>> +     */
>>>>> +    if (btrfs_super_incompat_flags(sb) &
>>>>> ~BTRFS_FEATURE_INCOMPAT_SUPP) {
>>>>> +        btrfs_err(fs_info,
>>>>> +        "corrupted incompat flags detected 0x%llx, supported 0x%llx",
>>>
>>>> 2707         features = btrfs_super_incompat_flags(disk_super) &
>>>> 2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;
>>>> 2709         if (features) {
>>>> 2710                 btrfs_err(fs_info,
>>>> 2711                     "cannot mount because of unsupported
>>>> optional features (%llx)",
>>>> 2712                     features);
>>>> 2713                 err = -EINVAL;
>>>> 2714                 goto fail_alloc;
>>>> 2715         }
>>>
>>> So there's a "user experience" change, now that you pointed out the
>>> other check.
>>
>>  Its a regression.
>>
>>> If a filesystem with incompat bits set will be mounted,
>>> this will say 'you have corrupted filesystem', which is not IMO what we
>>> want to tell.
>>>
>>> Tough the extended btrfs_check_super_valid could catch the corrupted
>>> incompat bits, what we need at mount time is wording from the 2nd
>>> message ("cannot mount unsuppported features").
>>>
>>> I think that there should be a separate function that does the
>>> pre-commit checks, calls btrfs_check_super_valid and also validates the
>>> incompat bit.
>>
>>  We can still print it as 'unsupported optional features', which would
>>  imply corruption in the non-mount context.
>
> In that case such wording is not obvious enough to info it's super block
> corruption.

If the device is already mounted. Then its a corruption.

Thanks, Anand

> So I still prefer current way of checking incompact flags and csum types
> manually at mount time before btrfs_validate_super().
>
> David's idea of a separate function to do mount time check and gives
> better prompt is pretty good.
> Especially for incompat features, as incompat features will increase in
> the future and for older kernel info such feature as corruption is not
> acceptable.
>
> But currently, there are only 2 members we need to check at mount time
> (csum_type and incompat features), and only one caller, current code
> looks good enough for its purpose.
>
> Thanks,
> Qu
>
>>
>> Thanks, Anand
>>
>>> --
>>> 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
>