Re: [PATCH 0/4] Userspace support for FSID change

[Nikolay Borisov <nborisov@suse.com>]

On 29.08.2018 15:47, Qu Wenruo wrote:
> 
> 
> On 2018/8/29 下午8:33, Nikolay Borisov wrote:
>>
>>
>> On 29.08.2018 15:09, Qu Wenruo wrote:
>>>
>>>
>>> On 2018/8/29 下午4:35, Nikolay Borisov wrote:
>>>> Here is the userspace tooling support for utilising the new metadata_uuid field, 
>>>> enabling the change of fsid without having to rewrite every metadata block. This
>>>> patchset consists of adding support for the new field to various tools and 
>>>> files (Patch 1). The actual implementation of the new -m|-M options (which are 
>>>> described in more detail in Patch 2). A new misc-tests testcasei (Patch 3) which
>>>> exercises the new options and verifies certain invariants hold (these are also
>>>> described in Patch2). Patch 4 is more or less copy of the kernel conuterpart 
>>>> just reducing some duplication between btrfs_fs_info and btrfs_fs_devices 
>>>> structures.
>>>
>>> So to my understand, now we have another layer of UUID.
>>>
>>> Before we have one fsid, both used in superblock and tree blocks.
>>>
>>> Now we have 2 fsid, the one used in tree blocks are kept the same, but
>>> changed its name to metadata_uuid in superblock.
>>> And superblock::fsid will become a new field, and although they are the
>>> same at mkfs time, they could change several times during its operation.
>>>
>>> This indeed makes uuid change super fast, only needs to update all
>>> superblocks of the fs, instead of all tree blocks.
>>>
>>> However I have one nitpick of the design. Unlike XFS, btrfs supports
>>> multiple devices.
>>> If we have a raid10 fs with 4 devices, and it has already gone through
>>> several UUID change (so its metadata uuid is already different from fsid).
>>>
>>> And during another UUID change procedure, we lost power while only
>>> updated 2 super blocks, what will happen for kernel device assembly?
>>>
>>> (Although considering how fast the UUID change would happen, such case
>>> should be super niche)
>>
>> Then I guess you will be fucked. I'm all ears for suggestion how to
>> rectify this without skyrocketing the complexity. The current UUID
>> rewrite method sets a flag int he superblock that FSID change is in
>> progress and clears it once every metadatablock has been rewritten. I
>> can piggyback on this mechanism but I'm not sure it provides 100%
>> guarantee. Because by the some token you can set this flag, start
>> writing the super blocks then lose power and then only some of the
>> superblocks could have this flag set so we back at square 1.
> 
> Well, forget it, considering how fast the new method is, such case
> should be really rare.
> 
>>
>>>
>>>>
>>>> The intended usecase of this feature is to give the sysadmin the ability to 
>>>> create copies of filesystesm, change their uuid quickly and mount them alongside
>>>> the original filesystem for, say, forensic purposes. 
>>>>
>>>> One thing which still hasn't been set in stone is whether the new options 
>>>> will remain as -m|-M or whether they should subsume the current -u|-U - from 
>>>> the point of view of users nothing should change.
>>>
>>> Well, user would be surprised by how fast the new -m is, thus there is
>>> still something changed :)
>>>
>>> I prefer to subsume current -u/-U, and use the new one if the incompat
>>> feature is already set. Or fall back to original behavior.
>>>
>>> But I'm not a fan of using INCOMPAT flags as an indicator of changed
>>> fsid/metadata uuid.
>>> INCOMPAT feature should not change so easily nor acts as an indicator.
>>>
>>> That's to say, the flag should only be set at mkfs time, and then never
>>> change unlike the 2nd patch (I don't even like btrfstune to change
>>> incompat flags).
>>>
>>> E.g.
>>> mkfs.btrfs -O metadata_uuid <device>, then we could use the new way to
>>> change fsid without touching metadata uuid.
>>> Or we could only use the old method.
>>
>> I disagree, I don't see any benefit in this but only added complexity.
>> Can you elaborate more ?
> 
> Well, the incompat feature is really introducing some incompatible
> on-disk format change.
> So if you're introducing the new metadata_uuid field, no matter if it
> differs from fsid or not, it's a new field, and the incompat flag should
> be set.
> 
> To me, older kernel could recognize the new format when fsid matches
> metadata uuid (since there in your current patchset, such case will not
> have incompat flag set) is a little dangerous.
> 
> What will happen if such old kernel/btrfs-progs tries to change fsid?
> Older btrfs-progs doesn't know there is a new field, it will not touch
> the metadata uuid field, just changing the fsid field along with all
> tree blocks.
> 
> This will cause a fs whose fsid doesn't match metadata uuid and has no
> incompat flag set. This is definitely leading to compatibility problem.

Nope, when both fsid/metadata_uuid match and the INCOMPAT flag is not
set then on-disk we never save the metadata_uuid value and this value is
set only in-memory, the only thing on-disk is fsid. In this case if one
decides to rewrite the fsuid then this is fine - btrfs-progs disallow
using -m|-M when fsid rewrite is in progress. So when the fsid/metadata
is not set to old progs the kernel continue working as-is. When it's
changed then the incompat flags will prevent us from doing harmful
modifications.

> 
> So we need to follow the incompat flags rule strictly, if on-disk format
> is changed, we need the new incompat flag.
> 
> Thanks,
> Qu
>>
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>> So this is something which 
>>>> I'd like to hear from the community. Of course the alternative of rewriting 
>>>> the metadata blocks will be assigne new options - perhaps -m|M ?
>>>>
>>>> I've tested this with multiple xfstest runs with the new tools installed as 
>>>> well as running btrfs-progs test and have observed no regressions. 
>>>>
>>>> Nikolay Borisov (4):
>>>>   btrfs-progs: Add support for metadata_uuid field.
>>>>   btrfstune: Add support for changing the user uuid
>>>>   btrfs-progs: tests: Add tests for changing fsid feature
>>>>   btrfs-progs: Remove fsid/metdata_uuid fields from fs_info
>>>>
>>>>  btrfstune.c                                | 174 ++++++++++++++++++++++++-----
>>>>  check/main.c                               |   2 +-
>>>>  chunk-recover.c                            |  17 ++-
>>>>  cmds-filesystem.c                          |   2 +
>>>>  cmds-inspect-dump-super.c                  |  22 +++-
>>>>  convert/common.c                           |   2 +
>>>>  ctree.c                                    |  15 +--
>>>>  ctree.h                                    |   8 +-
>>>>  disk-io.c                                  |  62 ++++++++--
>>>>  image/main.c                               |  25 +++--
>>>>  tests/misc-tests/033-metadata-uuid/test.sh | 137 +++++++++++++++++++++++
>>>>  volumes.c                                  |  37 ++++--
>>>>  volumes.h                                  |   1 +
>>>>  13 files changed, 431 insertions(+), 73 deletions(-)
>>>>  create mode 100755 tests/misc-tests/033-metadata-uuid/test.sh
>>>>
>>>