linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Austin S. Hemmelgarn" <ahferroin7@gmail.com>
To: Nikolay Borisov <nborisov@suse.com>,
	Qu Wenruo <quwenruo.btrfs@gmx.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 0/4] Userspace support for FSID change
Date: Wed, 29 Aug 2018 08:43:50 -0400	[thread overview]
Message-ID: <b22a3df4-b65c-f069-4129-b405808c3188@gmail.com> (raw)
In-Reply-To: <cd1572a2-883b-48ce-0212-3b35373d8ef3@suse.com>

On 2018-08-29 08: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.
> 
>>
>>>
>>> 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 ?
Same here, I see essentially zero benefit to this, and one _big_ 
drawback, namely that you can't convert an existing volume to use this 
approach if it's a feature that can only be set at mkfs time.

That one drawback means that this is effectively useless for all 
existing BTRFS volumes, which is a pretty big limitation.

I also do think an INCOMPAT feature bit is appropriate here.  Volumes 
with this feature will potentially be enumerated with the wrong UUID on 
older kernels, which is a pretty big behavioral issue (on the level of 
completely breaking boot on some systems, keep in mind that almost all 
major distros use volume UUID's to identify volumes in /etc/fstab).
> 
> 
>>
>> 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
>>>
>>

  reply	other threads:[~2018-08-29 16:40 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-29  8:35 [PATCH 0/4] Userspace support for FSID change Nikolay Borisov
2018-08-29  8:35 ` [PATCH 1/4] btrfs-progs: Add support for metadata_uuid field Nikolay Borisov
2018-08-29  8:35 ` [PATCH 2/4] btrfstune: Add support for changing the user uuid Nikolay Borisov
2018-08-29  8:35 ` [PATCH 3/4] btrfs-progs: tests: Add tests for changing fsid feature Nikolay Borisov
2018-08-29  8:35 ` [PATCH 4/4] btrfs-progs: Remove fsid/metdata_uuid fields from fs_info Nikolay Borisov
2018-08-29 12:09 ` [PATCH 0/4] Userspace support for FSID change Qu Wenruo
2018-08-29 12:33   ` Nikolay Borisov
2018-08-29 12:43     ` Austin S. Hemmelgarn [this message]
2018-08-29 12:47     ` Qu Wenruo
2018-09-02  9:56       ` Nikolay Borisov
2018-09-02 23:50         ` Qu Wenruo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b22a3df4-b65c-f069-4129-b405808c3188@gmail.com \
    --to=ahferroin7@gmail.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=nborisov@suse.com \
    --cc=quwenruo.btrfs@gmx.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).