Re: [PATCH 1/3] btrfs: fix lockdep splat in open_fs_devices

[David Sterba <dsterba@suse.cz>]

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@toxicpanda.com>
> ---
>  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