Re: [PATCH 2/7] btrfs: extend critical section when scanning a new device

[Anand Jain <anand.jain@oracle.com>]

On 06/21/2018 01:51 AM, David Sterba wrote:
> The stale device list removal needs to be protected by device_list_mutex
> too as this could delete from the list and could race with another list
> modification and cause crash.
> 
> The device needs to be fully initialized before it's added to the list
> so the fs_devices also need to be set under the mutex.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/volumes.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1da162928d1a..02246f9af0a3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -791,12 +791,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   		rcu_assign_pointer(device->name, name);
>   
>   		mutex_lock(&fs_devices->device_list_mutex);
> +		device->fs_devices = fs_devices;
>   		list_add_rcu(&device->dev_list, &fs_devices->devices);
>   		fs_devices->num_devices++;
> -		mutex_unlock(&fs_devices->device_list_mutex);
> -
> -		device->fs_devices = fs_devices;
>   		btrfs_free_stale_devices(path, device);


This is not correct.

btrfs_free_stale_devices need the per fs_devices local device_list_mutex
lock as it traverses through the fs_uuids list. Holding just the lock of
the %fs_devices which is being scanned or mounted is not correct.

In my workspace I have replaced this patch with ..

  4130724ec926 btrfs: fix btrfs_free_stale_devices() with needed locks
  90fedec0c200 btrfs: btrfs_free_stale_devices() rename local variables
  439eec4e943c btrfs: fix device_list_add() missing device_list_mutex()
  417ecc91e223 btrfs: do btrfs_free_stale_devices() outside of 
device_list_add()

which are in the ML as well.

Thanks, Anand


> +		mutex_unlock(&fs_devices->device_list_mutex);
>   
>   		if (disk_super->label[0])
>   			pr_info("BTRFS: device label %s devid %llu transid %llu %s\n",
>