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
>>>>
>>>
