Re: [PATCH v2 1/2] btrfs: add support for SUPER_FLAG_CHANGING_FSID in btrfs.ko

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Anand Jain <anand.jain@oracle.com>, linux-btrfs@vger.kernel.org
Cc: wqu@suse.com
Subject: Re: [PATCH v2 1/2] btrfs: add support for SUPER_FLAG_CHANGING_FSID in btrfs.ko
Date: Mon, 8 Jan 2018 15:50:49 +0800	[thread overview]
Message-ID: <9f2ed3bd-4166-ee8f-d2a9-1c4f1df74c94@gmx.com> (raw)
In-Reply-To: <939c1f2d-1858-7b8e-9ad8-3e23937550d5@oracle.com>

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

On 2018年01月08日 13:58, Anand Jain wrote:
> 
> 
> On 01/08/2018 01:25 PM, Qu Wenruo wrote:
>>
>>
>> On 2018年01月08日 13:13, Anand Jain wrote:
>>>
>>>
>>> On 01/08/2018 01:08 PM, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2018年01月08日 13:04, Anand Jain wrote:
>>>>> Userland sets SUPER_FLAG_CHANGING_FSID and resets it only when
>>>>> changing
>>>>> fsid is complete. Its not a good idea to mount the device anything in
>>>>> between, so this patch fails the mount if SB SUPER_FLAG_CHANGING_FSID
>>>>> is set.
>>>>>
>>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>>>     cc: wqu@suse.com
>>>>> ---
>>>>> v1->v2: Oops. Fix cut and paste error, remove ~. My bad.
>>>>>
>>>>>    fs/btrfs/disk-io.c              | 7 ++++++-
>>>>>    include/uapi/linux/btrfs_tree.h | 1 +
>>>>>    2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>>> index a69e5944dc08..0dd215258ff9 100644
>>>>> --- a/fs/btrfs/disk-io.c
>>>>> +++ b/fs/btrfs/disk-io.c
>>>>> @@ -61,7 +61,8 @@
>>>>>                     BTRFS_HEADER_FLAG_RELOC |\
>>>>>                     BTRFS_SUPER_FLAG_ERROR |\
>>>>>                     BTRFS_SUPER_FLAG_SEEDING |\
>>>>> -                 BTRFS_SUPER_FLAG_METADUMP)
>>>>> +                 BTRFS_SUPER_FLAG_METADUMP |\
>>>>> +                 BTRFS_SUPER_FLAG_CHANGING_FSID)
>>>>
>>>> Still the same problem here.
>>>>
>>>> Since that super flag is excluded in btrfs_check_super_valid(), I
>>>> didn't
>>>> see any meaning including it into BTRFS_SUPER_FLAG_SUPP, and it will
>>>> confuse later BTRFS_SUPER_FLAG_SUPP users.
>>>
>>>
>>>   Anyway BTRFS_SUPER_FLAG_SUPP as such does not stop to mount, it
>>>   just warns. So when there is a new flag which is not supported
>>>   we get this warning. As in the case with BTRFS_SUPER_FLAG_METADUMP_V2.
>>>   BTRFS_SUPER_FLAG_METADUMP_V2 in kernel is defined but not supported.
>>>   However since we are supporting BTRFS_SUPER_FLAG_CHANGING_FSID, so
>>>   adding to BTRFS_SUPER_FLAG_SUPP make sense to me. ?
>>
>> We still refuse to mount the fs if CHANGING_FSID is set, which I believe
>> should be called "unsupported"
>>
>> On the other hand it would be better to include METADUMP_V2 into SUPP,
>> as we allow it to be mounted and now kernel knows that flag, output a
>> warning doesn't seems necessary now.
>>
>> It would be even better to change the later SUPP flags checks, into
>> something not only output some meaningless numeric flags, but also
>> output the human readable flags if it's recognized but unsupported. (For
>> CHANGIND_FSID).
> 
>  Originally [1] looks like there isn't any design specific reason not to
>  fail the mount (which I didn't want to alter to fail) but just to warn.
>  Will change it to fail.
> 
> [1]
>  commit 319e4d0661e5323c9f9945f0f8fb5905e5fe74c3
>     btrfs: Enhance super validation check

My fault, I should add the missing super flags at that time, and fail
the mount.

Thanks,
Qu

> 
> Thanks, Anand
> 
>> Thanks,
>> Qu
>>
>>>
>>> Thanks, Anand
>>>
>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>>      static const struct extent_io_ops btree_extent_io_ops;
>>>>>    static void end_workqueue_fn(struct btrfs_work *work);
>>>>> @@ -3906,6 +3907,10 @@ static int btrfs_check_super_valid(struct
>>>>> btrfs_fs_info *fs_info)
>>>>>            btrfs_err(fs_info, "no valid FS found");
>>>>>            ret = -EINVAL;
>>>>>        }
>>>>> +    if (btrfs_super_flags(sb) & BTRFS_SUPER_FLAG_CHANGING_FSID) {
>>>>> +        btrfs_err(fs_info, "SUPER_FLAG_CHANGING_FSID is set");
>>>>> +        ret = -EINVAL;
>>>>> +    }
>>>>>        if (btrfs_super_flags(sb) & ~BTRFS_SUPER_FLAG_SUPP)
>>>>>            btrfs_warn(fs_info, "unrecognized super flag: %llu",
>>>>>                    btrfs_super_flags(sb) & ~BTRFS_SUPER_FLAG_SUPP);
>>>>> diff --git a/include/uapi/linux/btrfs_tree.h
>>>>> b/include/uapi/linux/btrfs_tree.h
>>>>> index 38ab0e06259a..aff1356c2bb8 100644
>>>>> --- a/include/uapi/linux/btrfs_tree.h
>>>>> +++ b/include/uapi/linux/btrfs_tree.h
>>>>> @@ -457,6 +457,7 @@ struct btrfs_free_space_header {
>>>>>    #define BTRFS_SUPER_FLAG_SEEDING    (1ULL << 32)
>>>>>    #define BTRFS_SUPER_FLAG_METADUMP    (1ULL << 33)
>>>>>    #define BTRFS_SUPER_FLAG_METADUMP_V2    (1ULL << 34)
>>>>> +#define BTRFS_SUPER_FLAG_CHANGING_FSID    (1ULL << 35)
>>>>>        /*
>>>>>
>>>>
>>
> -- 
> 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 --]