Re: [PATCH RFC] btrfs: pick device with lowest devt for show_devname

[David Sterba <dsterba@suse.cz>]

On Thu, Nov 02, 2023 at 07:10:48PM +0800, Anand Jain wrote:
> In a non-single-device Btrfs filesystem, if Btrfs is already mounted and
> if you run the command 'mount -a,' it will fail and the command
> 'umount <device>' also fails. As below:
> 
> ----------------
> $ cat /etc/fstab | grep btrfs
> UUID=12345678-1234-1234-1234-123456789abc /btrfs btrfs defaults,nofail 0 0
> 
> $ mkfs.btrfs -qf --uuid 12345678-1234-1234-1234-123456789abc /dev/sda2 /dev/sda1
> $ mount --verbose -a
> /                        : ignored
> /btrfs                   : successfully mounted
> 
> $ ls -l /dev/disk/by-uuid | grep 12345678-1234-1234-1234-123456789abc
> lrwxrwxrwx 1 root root 10 Nov  2 17:43 12345678-1234-1234-1234-123456789abc -> ../../sda1
> 
> $ cat /proc/self/mounts | grep btrfs
> /dev/sda2 /btrfs btrfs rw,relatime,space_cache=v2,subvolid=5,subvol=/ 0 0
> 
> $ findmnt --df /btrfs
> SOURCE    FSTYPE SIZE  USED AVAIL USE% TARGET
> /dev/sda2 btrfs    2G  5.8M  1.8G   0% /btrfs
> 
> $ mount --verbose -a
> /                        : ignored
> mount: /btrfs: /dev/sda1 already mounted or mount point busy.
> $echo $?
> 32
> 
> $ umount /dev/sda1
> umount: /dev/sda1: not mounted.
> $ echo $?
> 32
> ----------------
> 
> I assume (RFC) this is because '/dev/disk/by-uuid,' '/proc/self/mounts,'
> and 'findmnt' do not all reference the same device, resulting in the
> 'mount -a' and 'umount' failures. However, an empirically found solution
> is to align them using a rule, such as the disk with the lowest 'devt,'
> for a multi-device Btrfs filesystem.
> 
> I'm not yet sure (RFC) how to create a udev rule to point to the disk with
> the lowest 'devt,' as this kernel patch does, and I believe it is
> possible.
> 
> And this would ensure that '/proc/self/mounts,' 'findmnt,' and
> '/dev/disk/by-uuid' all reference the same device.
> 
> After applying this patch, the above test passes. Unfortunately,
> /dev/disk/by-uuid also points to the lowest 'devt' by chance, even though
> no rule has been set as of now. As shown below.
> 
> ----------------
> $ mkfs.btrfs -qf --uuid 12345678-1234-1234-1234-123456789abc /dev/sda2 /dev/sda1
> 
> $ mount --verbose -a
> /                        : ignored
> /btrfs                   : successfully mounted
> 
> $ ls -l /dev/disk/by-uuid | grep 12345678-1234-1234-1234-123456789abc
> lrwxrwxrwx 1 root root 10 Nov  2 17:53 12345678-1234-1234-1234-123456789abc -> ../../sda1
> 
> $ cat /proc/self/mounts | grep btrfs
> /dev/sda1 /btrfs btrfs rw,relatime,space_cache=v2,subvolid=5,subvol=/ 0 0
> 
> $ findmnt --df /btrfs
> SOURCE    FSTYPE SIZE  USED AVAIL USE% TARGET
> /dev/sda1 btrfs    2G  5.8M  1.8G   0% /btrfs
> 
> $ mount --verbose -a
> /                        : ignored
> /btrfs                   : already mounted
> $echo $?
> 0
> 
> $ umount /dev/sda1
> $echo $?
> 0
> ----------------
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/super.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 66bdb6fd83bd..d768917cc5cc 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2301,7 +2301,18 @@ static int btrfs_unfreeze(struct super_block *sb)
>  
>  static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
>  {
> -	struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb);
> +	struct btrfs_fs_devices *fs_devices = btrfs_sb(root->d_sb)->fs_devices;
> +	struct btrfs_device *device;
> +	struct btrfs_device *first_device = NULL;
> +
> +	list_for_each_entry(device, &fs_devices->devices, dev_list) {
> +		if (first_device == NULL) {
> +			first_device = device;
> +			continue;
> +		}
> +		if (first_device->devt > device->devt)
> +			first_device = device;
> +	}

I think we agree in principle that the devt can be used to determine the
device to print in show_devname, however I'd like to avoid iterating the
device list here. We used to have it, first with mutex protection, then
RCU. That we can simply print the latest_dev is quite convenient.

I suggest to either add a separate fs_info variable to set the desired
device and only print it here, or reuse latest_dev for that.

Adding a separate variable for that should be safer as latest_dev is
used in various way and I can't say it's always clear.