On Fri, Jul 17, 2020 at 03:12:27PM -0400, Josef Bacik wrote:
> Fix this by not holding the ->device_list_mutex at this point. The
> device_list_mutex exists to protect us from modifying the device list
> while the file system is running.
>
> However it can also be modified by doing a scan on a device. But this
> action is specifically protected by the uuid_mutex, which we are holding
> here. We cannot race with opening at this point because we have the
> ->s_mount lock held during the mount. Not having the
> ->device_list_mutex here is perfectly safe as we're not going to change
> the devices at this point.
Agreed, the uuid_mutex is sufficient here, since 81ffd56b574 ("btrfs:
fix mount and ioctl device scan ioctl race") that excludes the critical
parts of mount and scan.
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
> fs/btrfs/volumes.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ce01e44f8134..20295441251a 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -258,6 +258,9 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
> * may be used to exclude some operations from running concurrently without any
> * modifications to the list (see write_all_supers)
> *
> + * Is not required at mount and close times, because our device list is
> + * protected by the uuid_mutex at that point.
This is correct, however there's one comment a few lines above about
unid_mutex
"does not protect: manipulation of the fs_devices::devices list!"
so I'll update it means 'not in general but there are exceptions like
mount context'.
> + *
> * balance_mutex
> * -------------
> * protects balance structures (status, state) and context accessed from
> @@ -602,6 +605,11 @@ static int btrfs_free_stale_devices(const char *path,
> return ret;
> }
>
> +/*
> + * This is only used on mount, and we are protected from competing things
> + * messing with our fs_devices by the uuid_mutex, thus we do not need the
> + * fs_devices->device_list_mutex here.
> + */
> static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
> struct btrfs_device *device, fmode_t flags,
> void *holder)
> @@ -1230,7 +1238,6 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>
> lockdep_assert_held(&uuid_mutex);
>
> - mutex_lock(&fs_devices->device_list_mutex);
I'll leave a comment here as the device list is clearly modified
(list_sort).
> if (fs_devices->opened) {
> fs_devices->opened++;
> ret = 0;
> @@ -1238,7 +1245,6 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
> list_sort(NULL, &fs_devices->devices, devid_cmp);
> ret = open_fs_devices(fs_devices, flags, holder);
> }
> - mutex_unlock(&fs_devices->device_list_mutex);
>
> return ret;
> }
> --
> 2.24.1
