Re: [PATCH v4 2/2] btrfs: Introduce the temp-fsid feature

[Anand Jain <anand.jain@oracle.com>]

On 21/09/2023 02:37, David Sterba wrote:
> On Wed, Sep 20, 2023 at 09:16:02AM -0300, Guilherme G. Piccoli wrote:
>> On 19/09/2023 02:01, Anand Jain wrote:
>>> [...]
>>> This must successfully pass the remaining Btrfs fstests test cases with
>>> the MKFS_OPTION="-O temp-fsid" configuration option, or it should call
>>> not run for the incompatible feature.
>>
>> I kinda disagree here - this feature is not compatible with anything
>> else, so I don't think it's fair to expect mounting with temp-fsid will
>> just pass all other tests, specially for things like (the real)
>> metadata_uuid or extra devices, like device removal...
> 
> Yeah, fstests are not in general ready for enabling some feature from
> the outside (mkfs, or mount options). Some of them work as long as
> they're orthogonal but some tests need to detect that and skip. In this
> case all multidevice tests would fail.
> 
> For test coverage there should be at lest one test that verifies known
> set of compatible features or usecases we care about in comibnation with
> the temp-fsid.
> 

If this patch had undergone verification using fstests for a single
device, it might have already resolved the data corruption issue I
recently reported. While it could be an isolated bug, having fstests
confirm that 'yes, everything else is verified except for multi-device
support' would be helpful.

The MKFS_OPTION="-O temp_fsid" config would help create temp_fsid in
fstests, it may not be too difficult to verify.

Also, as commented in v3, btrfs-progs needs to support listing single
devices with conflicting fsids while they are unmounted.

Thanks, Anand


>>> I have observed that the following test case is failing with this patch:
>>>
>>>    $ mkfs.btrfs -fq /dev/sdb1 :0
>>>    $ btrfstune --convert-to-temp-fsid /dev/sdb1 :0
>>>    $ mount /dev/sdb1 /btrfs :0
>>>
>>> Mount /dev/sdb1 again at a different mount point and look for the copied
>>> file 'messages':
>>>
>>>    $ cp /var/log/messages /btrfs :0
>>>
>>>    $ mount /dev/sdb1 /btrfs1 :0
>>>    $ ls -l /btrfs1 :0
>>>    total 0   <-- empty
>>>
>>> The copied file is missing because we consider each mount as a new fsid.
>>> This means subvolume mounts are also not working. Some operating systems
>>> mount $HOME as a subvolume, so those won't work either.
>>>
>>> To resolve this, we can use devt to match in the device list and find
>>> the matching fs_devices or NULL.
>>
>> Ugh, this one is ugly. Thanks for noticing that, I think this needs
>> fixing indeed.
>>
>> I've tried here, mounted the same temp-fsid btrfs device in 2 different
>> mount points, and wrote two different files on each. The mount A can
>> only see the file A, mount B can only see file B. Then after unmouting
>> both, I cannot mount anymore with errors in ctree, so it got corrupted.
>>
>> The way I think we could resolve this is by forbidding mounting a
>> temp-fsid twice - after the random uuid generation, we could check for
>> all fs_devices present and if any of it has the same metadata_uuid, we
>> check if it's the same dev_t and bail.
>>
>> The purpose of the feature is for having the same filesystem in
>> different devices able to mount at the same time, but on different mount
>> points. WDYT?
> 
> The subvolume mount is a common use case and I hope it continues to
> work. Currently it does not seem so as said above, for correctness we
> may need to prevent it. We might find more and this should be known or
> fixed before final release.