Re: [PATCH 1/2] btrfs: Introduce the virtual_fsid feature

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/7/6 06:52, Guilherme G. Piccoli wrote:
> On 05/05/2023 04:21, Qu Wenruo wrote:
>> [...]
>> I would prefer a much simpler but more explicit method.
>>
>> Just introduce a new compat_ro feature, maybe call it SINGLE_DEV.
>>
>> By this, we can avoid multiple meanings of the same super member, nor
>> need any special mount option.
>> Remember, mount option is never a good way to enable/disable a new feature.
>>
>> The better method to enable/disable a feature should be mkfs and btrfstune.
>>
>> Then go mostly the same of your patch, but maybe with something extra:
>>
>> - Disbale multi-dev code
>>     Include device add/replace/removal, this is already done in your
>>     patch.
>>
>> - Completely skip device scanning
>>     I see no reason to keep btrfs with SINGLE_DEV feature to be added to
>>     the device list at all.
>>     It only needs to be scanned at mount time, and never be kept in the
>>     in-memory device list.
>>
>
> Hi Qu, I'm implementing this compat_ro idea of yours, but I'd like to
> ask your input in some "design decisions" I'm facing here.
>
> (a) I've skipped the device_list_add() step of adding the recent created
> fs_devices struct to fs_uuids list, but I kept the btrfs_device creation
> step. With that, the mount of two filesystems with same fsid fails..at
> sysfs directory creation!
>
> Of course - because it tries to add the same folder name to
> /sys/fs/btrfs/ !!! I have some options here:
>
> (I) Should I keep using a random generated fsid for single_dev devices,
> in order we can mount many of them while not messing too much with the
> code? I'd continue "piggybacking" on metadata_uuid idea if (I) is the
> proper choice.
>
> (II) Or maybe track down all fsid usage in the code (like this sysfs
> case) and deal with that? In the sysfs case, we could change that folder
> name to some other format, like fsid.NUM for single_dev devices, whereas
> NUM is an incremental value for devices mounted with same fsid.
>
> I'm not too fond of this alternative due to its complexity and "API"
> breakage - userspace already expects /sys/fs/btrfs/ entries to be the fsid.
>
> (III) Should we hide the filesystem from sysfs (and other potential
> conflicts that come from same fsid mounting points)? Seems a hideous
> approach, due to API breakage and bug potentials.

Personally speaking, I would go one of the following solution:

- Keep the sysfs, but adds a refcount to the sysfs related structures
   If we still go register the sysfs interface, then we have to keep a
   refcount, or any of the same fsid got unmounted, we would remove the
   whole sysfs entry.

- Skip the sysfs entry completely for any fs with the new compat_ro flag
   This would your idea (III), but the sysfs interface itself is not that
   critical (we add and remove entries from time to time), so I believe
   it's feasible to hide the sysfs for certain features.

>
> Maybe there are other choices, better than mine - lemme know if you have
> some ideas!
>
> Also, one last question/confirmation: you mentioned that "The better
> method to enable/disable a feature should be mkfs" - you mean the same
> way mkfs could be used to set features like "raid56" or "no-holes"?

Yes.

>
> By checking "mkfs.btrfs -O list-all", I don't see metadata_uuid for
> example, which is confined to btrfstune it seems. I'm already modifying
> btrfs-progs/mkfs, but since I'm emailing you, why not confirm, right? heh

I'm not familiar with metadata_uuid, but there are similar features like
seeding, which is only available in btrfstune, but not in mkfs.

It's not that uncommon, but yeah, you have found something we should
improve on.

Thanks,
Qu

>
> Thanks again for the advice and suggestions - much appreciated!
> Cheers,
>
>
> Guilherme