On 27/09/2021 23:32, Josef Bacik wrote:

On 8/24/21 10:00 PM, Anand Jain wrote:

quoted

On 28/07/2021 05:01, Josef Bacik wrote:

quoted

For device removal and replace we call btrfs_find_device_by_devspec,
which if we give it a device path and nothing else will call
btrfs_find_device_by_path, which opens the block device and reads the
super block and then looks up our device based on that.

However this is completely unnecessary because we have the path stored
in our device on our fsdevices.  All we need to do if we're given a path
is look through the fs_devices on our file system and use that device if
we find it, reading the super block is just silly.

The device path as stored in our fs_devices can differ from the path
provided by the user for the same device (for example, dm, lvm).

btrfs-progs sanitize the device path but, others (for example, an ioctl
test case) might not. And the path lookup would fail.

Also, btrfs dev scan <path> can update the device path anytime, even
after it is mounted. Fixing that failed the subsequent subvolume mounts
(if I remember correctly).

This is a good point, that's kind of a big deal from a UX perspective.

quoted

quoted

This fixes the case where we end up with our sb write "lock" getting the
dependency of the block device ->open_mutex, which resulted in the
following lockdep splat

Can we do..

btrfs_exclop_start()
  ::
find device part (read sb)
  ::
mnt_want_write_file()?

I looked into this, but we'd have to re-order the exclop_start to above 
the mnt_want_write_file() part everywhere to be consistent, and this is 
mostly OK except for balance.  Balance the exclop is tied to the 
lifetime of the balance ctl, which can exist past the task running 
balance because we could pause the balance.

Could we get around this?  Sure, but in my head exclop == lock.  This 
means we have something akin to

exclop_start
mnt_want_write_file()

pause balance
mnt_drop_write()

resume balance

exclop_start magic stuff in balance to resume without doing the exclop
mnt_want_write_file()
<do balance>
exclop_finish
mnt_drop_write()

If we're OK with this then we can definitely do that.

This is getting complex. IMO.

The other option is simply to make userspace do the superblock read and 
use the devid thing for us.  Then we just eat the UX problem for older 
tools where you want to do btrfs rm device /dev/mapper/whatever and we 
have the pathname as /dev/dm-#.

Both options are unattractive in their own way. 

I agree.

I think the first 
option is only annoying to us, and maintains the UX expectations.  But I 
want more than me to make this decision, so if you and Dave are OK with 
that I'll go with re-ordering exclop+mnt_want_write_file(), and then put 
the device lookup between the two of them for device removal.  Thanks,

There is a 3rd option.

Here, the root of the problem is about reading superblock after 
mnt_drop_write().

So to avoid this, can we read the sb -> devid before mnt_drop_write()? 
and use the devid later on?

But when we read the sb we have neither mnt_drop_write() nor 
exclop_start..().

If the devid we read is stale, at a later stage the btrfs_rm_device() 
will still verify it.

I experimented this option, the diff is here [1]. This change needs a 
lot of clean up and did not copy the same logic to btrfs_ioctl_rm_dev() 
or tried to merge it. This diff passed the -g volume test cases with no 
new regressions. But I wasn't able to reproduce the original issue for 
which we wrote this patch.



[1]

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9eb0c1eb568e..e9c6bd05abf9 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c

@@ -3164,15 +3164,13 @@ static long btrfs_ioctl_rm_dev_v2(struct file 

*file, void __user *arg)
         struct block_device *bdev = NULL;
         fmode_t mode;
         int ret;
+       bool cancel_or_missing = false;
         bool cancel = false;
+       u64 devid;

         if (!capable(CAP_SYS_ADMIN))
                 return -EPERM;

-       ret = mnt_want_write_file(file);
-       if (ret)
-               return ret;
-
         vol_args = memdup_user(arg, sizeof(*vol_args));
         if (IS_ERR(vol_args)) {
                 ret = PTR_ERR(vol_args);

@@ -3184,9 +3182,32 @@ static long btrfs_ioctl_rm_dev_v2(struct file 

*file, void __user *arg)
                 goto out;
         }
         vol_args->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
-       if (!(vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID) &&
-           strcmp("cancel", vol_args->name) == 0)
-               cancel = true;
+       if (vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID)
+               devid = vol_args->devid;
+       else {
+               if (strcmp("cancel", vol_args->name) == 0) {
+                       cancel_or_missing = true;
+                       cancel = true;
+               } else if (!strcmp("missing", vol_args->name))
+                       cancel_or_missing = true;
+               else {
+                       struct block_device *bdev;
+                       struct btrfs_super_block *disk_super;
+
+                       ret = btrfs_get_bdev_and_sb(vol_args->name, 
FMODE_READ,
+                                                   fs_info->bdev_holder, 0,
+                                                   &bdev, &disk_super);
+                       if (ret)
+                               goto out;
+                       devid = 
btrfs_stack_device_id(&disk_super->dev_item);
+                       btrfs_release_disk_super(disk_super);
+                       blkdev_put(bdev, FMODE_READ);
+               }
+       }
+
+       ret = mnt_want_write_file(file);
+       if (ret)
+               goto out;

         ret = exclop_start_or_cancel_reloc(fs_info, 
BTRFS_EXCLOP_DEV_REMOVE,
                                            cancel);

@@ -3194,10 +3215,10 @@ static long btrfs_ioctl_rm_dev_v2(struct file 

*file, void __user *arg)
                 goto out;
         /* Exclusive operation is now claimed */

-       if (vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID)
-               ret = btrfs_rm_device(fs_info, NULL, vol_args->devid, 
&bdev, &mode);
-       else
+       if (cancel_or_missing)
                 ret = btrfs_rm_device(fs_info, vol_args->name, 0, 
&bdev, &mode);
+       else
+               ret = btrfs_rm_device(fs_info, NULL, devid, &bdev, &mode);

         btrfs_exclop_finish(fs_info);

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6ade80bae3a5..85ae7294cea2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c

@@ -493,7 +493,7 @@ static struct btrfs_fs_devices 

*find_fsid_with_metadata_uuid(
  }


-static int
+int
  btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void 
*holder,
                       int flush, struct block_device **bdev,
                       struct btrfs_super_block **disk_super)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index c7ac43d8a7e8..fa1d1faa70d4 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h

@@ -502,6 +502,9 @@ void btrfs_close_devices(struct btrfs_fs_devices 

*fs_devices);
  void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices);
  void btrfs_assign_next_active_device(struct btrfs_device *device,
                                      struct btrfs_device *this_dev);
+int btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void 
*holder,
+                         int flush, struct block_device **bdev,
+                         struct btrfs_super_block **disk_super);
  struct btrfs_device *btrfs_find_device_by_devspec(struct btrfs_fs_info 
*fs_info,
                                                   u64 devid,
                                                   const char *devpath);



Thanks, Anand