On 17.07.20 г. 13:05 ч., Anand Jain wrote:
> Martin reported the following test case which reproduces lockdep
> warning on his machine.
>
> modprobe scsi_debug dev_size_mb=1024 num_parts=2
> sleep 3
> mkfs.btrfs /dev/sda1
> mount /dev/sda1 /mnt
> cp -v /bin/ls /mnt
> umount /mnt
> btrfstune -S 1 /dev/sda1
> mount /dev/sda1 /mnt
> btrfs dev add /dev/sda2 /mnt
> umount /mnt
> mount /dev/sda2 /mnt
> <splat>
>
> kernel: ======================================================
> kernel: WARNING: possible circular locking dependency detected
> kernel: 5.8.0-rc1+ #575 Not tainted
> kernel: ------------------------------------------------------
> kernel: mount/1024 is trying to acquire lock:
> kernel: ffff888065e0a4e0 (&fs_devs->device_list_mutex){+.+.}-{3:3}, at: clone_fs_devices+0x46/0x1f0 [btrfs]
> kernel: #012but task is already holding lock:
> kernel: ffff8880610508d0 (&fs_info->chunk_mutex){+.+.}-{3:3}, at: btrfs_read_chunk_tree+0xd1/0x390 [btrfs]
> kernel: #012which lock already depends on the new lock.
> kernel: #012the existing dependency chain (in reverse order) is:
> kernel: #012-> #1 (&fs_info->chunk_mutex){+.+.}-{3:3}:
> kernel: __lock_acquire+0x798/0xe50
> kernel: lock_acquire+0x15a/0x4d0
> kernel: __mutex_lock+0x116/0xbd0
> kernel: btrfs_remove_chunk+0x769/0xaa0 [btrfs]
> kernel: btrfs_delete_unused_bgs+0xa2c/0xfe0 [btrfs]
> kernel: cleaner_kthread+0x27c/0x2a0 [btrfs]
> kernel: kthread+0x1d6/0x200
> kernel: ret_from_fork+0x22/0x30
> kernel: #012-> #0 (&fs_devs->device_list_mutex){+.+.}-{3:3}:
> kernel: check_prev_add+0xf5/0xf50
> kernel: validate_chain+0xca7/0x1920
> kernel: __lock_acquire+0x798/0xe50
> kernel: lock_acquire+0x15a/0x4d0
> kernel: __mutex_lock+0x116/0xbd0
> kernel: clone_fs_devices+0x46/0x1f0 [btrfs]
> kernel: read_one_dev+0x15f/0x930 [btrfs]
> kernel: btrfs_read_chunk_tree+0x333/0x390 [btrfs]
> kernel: open_ctree+0xa72/0x15a6 [btrfs]
> kernel: btrfs_mount_root.cold+0xe/0xf1 [btrfs]
> kernel: legacy_get_tree+0x82/0xd0
> kernel: vfs_get_tree+0x4c/0x140
> kernel: fc_mount+0xf/0x60
> kernel: vfs_kern_mount.part.0+0x71/0x90
> kernel: btrfs_mount+0x1d7/0x610 [btrfs]
> kernel: legacy_get_tree+0x82/0xd0
> kernel: vfs_get_tree+0x4c/0x140
> kernel: do_mount+0xad1/0xe70
> kernel: __x64_sys_mount+0xbe/0x100
> kernel: do_syscall_64+0x56/0xa0
> kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9
> kernel: #012other info that might help us debug this:
> kernel: Possible unsafe locking scenario:
> kernel: CPU0 CPU1
> kernel: ---- ----
> kernel: lock(&fs_info->chunk_mutex);
> kernel: lock(&fs_devs->device_list_mutex);
> kernel: lock(&fs_info->chunk_mutex);
> kernel: lock(&fs_devs->device_list_mutex);
> kernel: #012 *** DEADLOCK ***
> ================================================
>
> Lockdep warning is complaining about the violation of the lock order of
> device_list_mutex and chunk_mutex[1]. And, lockdep warning isn't entirely
> correct, as it appears that it can't understand the different filesystem
> instances. Here, chunk_mutex was held by the mounting sprout filesystem,
> and device_list_mutex was held belongs to the seed filesystem as the sprout
> does not want the seed device to be freed.
>
> [1]
> open_ctree <== mount sprout
> btrfs_read_chunk_tree()
> mutex_lock(&uuid_mutex) <== global fsid lock
> mutex_lock(&fs_info->chunk_mutex) <== sprout fs
> read_one_dev()
> open_seed_devices()
> clone_fs_devices()
> mutex_lock(&orig->device_list_mutex) <== seed fs_devices
open_seed_devices doesn't delete the seed device
>
> There are two function stacks [1] and [2] leading to clone_fs_devices().
>
> [2]
> btrfs_init_new_device()
> mutex_lock(&uuid_mutex);
> btrfs_prepare_sprout()
> lockdep_assert_held(&uuid_mutex);
> clone_fs_devices()
and neither does prepare_sprout. So why are you mentioning them in the
context of freeing seed devices? By the way clone_fs_devices also
doesn't free the seed.
>
> They both hold the uuid_mutex which is sufficient to protect from
> freeing the seed device. That's because a seed device can not be
As such the first sentence is false, because holding the uuid_mutex in
those 2 paths has absolutely no relevance to freeing the seed devices.
> freed while it is mounted because it is read-only and an unmounted
> seed device (but registered) can be freed only by the command forget
> or making it stale. Which is handled by the function
> btrfs_free_stale_devices() which also needs uuid_mutex.
Both of those cases seem to be handled by btrfs_forget_devices which
indeed holds the uuid_mutex so this is correct.
>
> So remove the unnecessary seed->device_list_mutex in clone_fs_devices.
>
> Reported-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> Tested-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
> ---
> The above test case is similar to fstests btrfs/161 so no new
> test case will be required.
>
> 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 c35603b5595a..9dc3b826be0d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -561,6 +561,8 @@ static int btrfs_free_stale_devices(const char *path,
> struct btrfs_device *device, *tmp_device;
> int ret = 0;
>
> + lockdep_assert_held(&uuid_mutex);
> +
> if (path)
> ret = -ENOENT;
>
> @@ -985,7 +987,6 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
> if (IS_ERR(fs_devices))
> return fs_devices;
>
> - mutex_lock(&orig->device_list_mutex);
> fs_devices->total_devices = orig->total_devices;
>
> list_for_each_entry(orig_dev, &orig->devices, dev_list) {
> @@ -1017,10 +1018,8 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
> device->fs_devices = fs_devices;
> fs_devices->num_devices++;
> }
> - mutex_unlock(&orig->device_list_mutex);
> return fs_devices;
> error:
> - mutex_unlock(&orig->device_list_mutex);
> free_fs_devices(fs_devices);
> return ERR_PTR(ret);
> }
>
