Hi Anand, thanks for the feedback / review, much appreciated!
You're definitely helping a lot with this patch =)
On 06/09/2023 13:14, Anand Jain wrote:
> [...]
>> Anand: the distinction of fsid/metadata_uuid is indeed required on
>> btrfs_validate_super() - since we don't write the virtual/rand fsid to
>> the disk, and such function operates on the superblock that is read
>> from the disk, it fails for the single_dev case unless we condition check
>> there - thanks for noticing that though, was interesting to experiment
>> and validate =)
>
> Yep, that makes sense. Thanks. I have added cases 1 and 2 in an upcoming
> patch, and as part of this patch, you could add case 3 as below. Case 4
> is just for discussion.
>
>
> 1. Normally
>
> fs_devices->fsid == fs_devices->metadata_uuid == sb->fsid;
> sb->metadata_uuid == 0;
>
> 2. BTRFS_FEATURE_INCOMPAT_METADATA_UUID
>
> fs_devices->fsid == sb->fsid;
> fs_devices->metadata_uuid == sb->metadata_uuid;
>
>
> 3. BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV
>
> fs_devices->fsid == random();
> fs_devices->metadata_uuid = sb->fsid;
> sb->metadata_uuid == 0;
>
>
>
> For future development: (ignore for now)
>
> 4. BTRFS_FEATURE_INCOMPAT_METADATA_UUID |\
> BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV
>
> fs_devices->fsid == random();
> sb->fsid == actual_fsid (unused);
> fs_devices->metadata_uuid == sb->metadata_uuid;
>
This is a very good way of expressing the differences, quite good as
documentation! Thanks for that, it makes sense for me.
What do you mean by "you could add case 3 as below"? I'm already doing
that in the code, correct? Or do you mean somehow document that? I guess
this could be kinda copy/paste as a comment or in the wiki, for example.
[Looking in the list I think I found a patch from you adding these
comments to volumes.h =) ]
> [...]
>> + btrfs_err(fs_info,
>> + "device removal is unsupported on SINGLE_DEV devices\n");
>
> No \n at the end. btrfs_err() already adds one.
>
>> + btrfs_err(fs_info,
>> + "device removal is unsupported on SINGLE_DEV devices\n");
>
> here too.
>> + btrfs_err(fs_info,
>> + "device replace is unsupported on SINGLE_DEV devices\n");
>
> and here.
>
Good catch, will fix that!
> [...]
>> @@ -889,7 +889,7 @@ static int btrfs_parse_device_options(const char *options, blk_mode_t flags)
>> error = -ENOMEM;
>> goto out;
>> }
>> - device = btrfs_scan_one_device(device_name, flags);
>
>> + device = btrfs_scan_one_device(device_name, flags, true);
>
> Why do we have to pass 'true' in btrfs_scan_one_device() here? It is
> single device and I don't see any special handle for the seed device.
>> case BTRFS_IOC_SCAN_DEV:
>> mutex_lock(&uuid_mutex);
>> - device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ);
>> + device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ, false);
>> ret = PTR_ERR_OR_ZERO(device);
>> mutex_unlock(&uuid_mutex);
>> break;
>> @@ -2210,7 +2210,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>> break;
>> case BTRFS_IOC_DEVICES_READY:
>> mutex_lock(&uuid_mutex);
>> - device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ);
>> + device = btrfs_scan_one_device(vol->name, BLK_OPEN_READ, false);
>> if (IS_ERR(device)) {
>> mutex_unlock(&uuid_mutex);
>> ret = PTR_ERR(device);
>
> With this patch, command 'btrfs device scan' and 'btrfs device ready'
> returns -EINVAL for the single-device? Some os distributions checks
> the status using these commands during boot. Instead, it is ok to
> just return success here.
These are related things. So, regarding the
btrfs_parse_device_options(), as per my understanding this a mount
option that causes a device scan - so, it is a mount-time operation
somehow, correct? But I'm glad to s/true/false and prevent such
scanning, if you think it's more appropriate.
Now, about "just return success" on device scans, just return 0 then? OK
for me...
> [...]
>> + if (single_dev) {
>> + if (has_metadata_uuid || fsid_change_in_progress) {
>> + btrfs_err(NULL,
>> + "SINGLE_DEV devices don't support the metadata_uuid feature\n");
>> + return ERR_PTR(-EINVAL);
>
> It could right?
In theory, yes. But notice that we have a special situation with
SINGLE_DEV - we make use of the metadata_uuid infrastructure
*partially*; the in-memory structures are affected, but the superblock
is not touched. Now, to support both metadata_uuid and SINGLE_DEV, it
means for example that the user wants to mount two identical devices
(with metadata_uuid enabled) at same time, which is not currently
supported. But then SINGLE_DEV can't use metadata_uuid for that, since
this infrastructure is in use already, we'd need to have like a third
fsid entity, IIUC.
I think this could be achieved but I'm not sure the value for that, and
specially in the first iteration of the patch. I'd vote to merge this as
simple as possible and maybe extend that in the future to support
co-existing metadata_uuid, if that makes sense for some users. OK for you?
> [...]
>> @@ -1402,6 +1444,16 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags)
>> goto error_bdev_put;
>> }
>>
>> + single_dev = btrfs_super_compat_ro_flags(disk_super) &
>> + BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV;
>> +
>> + if (!mounting && single_dev) {
>> + pr_info("BTRFS: skipped non-mount scan on SINGLE_DEV device %s\n",
>> + path);
>> + btrfs_release_disk_super(disk_super);
>
> leaks bdev?
>
Ugh, apparently yes, thanks for noticing this!
>> + return ERR_PTR(-EINVAL);
>
> We need to let seed device scan even for the single device.
>
> In fact we can make no-scan required for the any fs with the total_devs
> == 1.
>
> I wrote a patch send it out for the review. So no special handling for
> single-device will be required.
This one?
https://lore.kernel.org/linux-btrfs/b0e0240254557461c137cd9b943f00b0d5048083.1693959204.git.anand.jain@xxxxxxxxxx/
OK, seems it does directly affect my patch, if yours is merged I can
remove part of the code I'm proposing, which is nice. I'll wait David /
Josef feedback on your patch to move on from here, if that's accepted,
I'll incorporate yours in my next iteration.
--
If possible, could you CC me in your patches related to metadata_uuid
and fsid for now, since I'm also working on that? This helps me to be
aware of the stuff.
For example, looking in the list, I just found:
https://lore.kernel.org/linux-btrfs/0b71460e3a52cf77cd0f7d533e28d2502e285c11.1693820430.git.anand.jain@xxxxxxxxxx/
This is the perfect place to add the comment above, related to fsid's
right? I'll do that in my next version.
Thanks,
Guilherme
