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