* [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
@ 2021-03-03 18:10 Anand Jain
2021-03-06 8:37 ` Anand Jain
2021-06-21 15:52 ` Anand Jain
0 siblings, 2 replies; 10+ messages in thread
From: Anand Jain @ 2021-03-03 18:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: Anand Jain, Su Yue
Following test case reproduces lockdep warning.
Test case:
DEV1=/dev/vdb
DEV2=/dev/vdc
umount /btrfs
run mkfs.btrfs -f $DEV1
run btrfstune -S 1 $DEV1
run mount $DEV1 /btrfs
run btrfs device add $DEV2 /btrfs -f
run umount /btrfs
run mount $DEV2 /btrfs
run umount /btrfs
The warning claims a possible ABBA deadlock between the threads initiated by
[#1] btrfs device add and [#0] the mount.
======================================================
[ 540.743122] WARNING: possible circular locking dependency detected
[ 540.743129] 5.11.0-rc7+ #5 Not tainted
[ 540.743135] ------------------------------------------------------
[ 540.743142] mount/2515 is trying to acquire lock:
[ 540.743149] ffffa0c5544c2ce0 (&fs_devs->device_list_mutex){+.+.}-{4:4}, at: clone_fs_devices+0x6d/0x210 [btrfs]
[ 540.743458] but task is already holding lock:
[ 540.743461] ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at: __btrfs_tree_read_lock+0x32/0x200 [btrfs]
[ 540.743541] which lock already depends on the new lock.
[ 540.743543] the existing dependency chain (in reverse order) is:
[ 540.743546] -> #1 (btrfs-chunk-00){++++}-{4:4}:
[ 540.743566] down_read_nested+0x48/0x2b0
[ 540.743585] __btrfs_tree_read_lock+0x32/0x200 [btrfs]
[ 540.743650] btrfs_read_lock_root_node+0x70/0x200 [btrfs]
[ 540.743733] btrfs_search_slot+0x6c6/0xe00 [btrfs]
[ 540.743785] btrfs_update_device+0x83/0x260 [btrfs]
[ 540.743849] btrfs_finish_chunk_alloc+0x13f/0x660 [btrfs] <--- device_list_mutex
[ 540.743911] btrfs_create_pending_block_groups+0x18d/0x3f0 [btrfs]
[ 540.743982] btrfs_commit_transaction+0x86/0x1260 [btrfs]
[ 540.744037] btrfs_init_new_device+0x1600/0x1dd0 [btrfs]
[ 540.744101] btrfs_ioctl+0x1c77/0x24c0 [btrfs]
[ 540.744166] __x64_sys_ioctl+0xe4/0x140
[ 540.744170] do_syscall_64+0x4b/0x80
[ 540.744174] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 540.744180] -> #0 (&fs_devs->device_list_mutex){+.+.}-{4:4}:
[ 540.744184] __lock_acquire+0x155f/0x2360
[ 540.744188] lock_acquire+0x10b/0x5c0
[ 540.744190] __mutex_lock+0xb1/0xf80
[ 540.744193] mutex_lock_nested+0x27/0x30
[ 540.744196] clone_fs_devices+0x6d/0x210 [btrfs]
[ 540.744270] btrfs_read_chunk_tree+0x3c7/0xbb0 [btrfs]
[ 540.744336] open_ctree+0xf6e/0x2074 [btrfs]
[ 540.744406] btrfs_mount_root.cold.72+0x16/0x127 [btrfs]
[ 540.744472] legacy_get_tree+0x38/0x90
[ 540.744475] vfs_get_tree+0x30/0x140
[ 540.744478] fc_mount+0x16/0x60
[ 540.744482] vfs_kern_mount+0x91/0x100
[ 540.744484] btrfs_mount+0x1e6/0x670 [btrfs]
[ 540.744536] legacy_get_tree+0x38/0x90
[ 540.744537] vfs_get_tree+0x30/0x140
[ 540.744539] path_mount+0x8d8/0x1070
[ 540.744541] do_mount+0x8d/0xc0
[ 540.744543] __x64_sys_mount+0x125/0x160
[ 540.744545] do_syscall_64+0x4b/0x80
[ 540.744547] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 540.744551] other info that might help us debug this:
[ 540.744552] Possible unsafe locking scenario:
[ 540.744553] CPU0 CPU1
[ 540.744554] ---- ----
[ 540.744555] lock(btrfs-chunk-00);
[ 540.744557] lock(&fs_devs->device_list_mutex);
[ 540.744560] lock(btrfs-chunk-00);
[ 540.744562] lock(&fs_devs->device_list_mutex);
[ 540.744564]
*** DEADLOCK ***
[ 540.744565] 3 locks held by mount/2515:
[ 540.744567] #0: ffffa0c56bf7a0e0 (&type->s_umount_key#42/1){+.+.}-{4:4}, at: alloc_super.isra.16+0xdf/0x450
[ 540.744574] #1: ffffffffc05a9628 (uuid_mutex){+.+.}-{4:4}, at: btrfs_read_chunk_tree+0x63/0xbb0 [btrfs]
[ 540.744640] #2: ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at: __btrfs_tree_read_lock+0x32/0x200 [btrfs]
[ 540.744708]
stack backtrace:
[ 540.744712] CPU: 2 PID: 2515 Comm: mount Not tainted 5.11.0-rc7+ #5
But the device_list_mutex in clone_fs_devices() is redundant, as explained
below.
Two threads [1] and [2] (below) could lead to clone_fs_device().
[1]
open_ctree <== mount sprout fs
btrfs_read_chunk_tree()
mutex_lock(&uuid_mutex) <== global lock
read_one_dev()
open_seed_devices()
clone_fs_devices() <== seed fs_devices
mutex_lock(&orig->device_list_mutex) <== seed fs_devices
[2]
btrfs_init_new_device() <== sprouting
mutex_lock(&uuid_mutex); <== global lock
btrfs_prepare_sprout()
lockdep_assert_held(&uuid_mutex)
clone_fs_devices(seed_fs_device) <== seed fs_devices
Both of these threads hold uuid_mutex which is sufficient to protect
getting the seed device(s) freed while we are trying to clone it for
sprouting [2] or mounting a sprout [1] (as above). A mounted seed
device can not free/write/replace because it is read-only. An unmounted
seed device can free by btrfs_free_stale_devices(), but it needs uuid_mutex.
So this patch removes the unnecessary device_list_mutex in clone_fs_devices().
And adds a lockdep_assert_held(&uuid_mutex) in clone_fs_devices().
Reported-by: Su Yue <l@damenly.su>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: Remove Martin's Reported-by and Tested-by.
Add Su's Reported-by.
Add lockdep_assert_held check.
Update the changelog, make it relevant to the current misc-next
fs/btrfs/volumes.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bc3b33efddc5..4188edbad2ef 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -570,6 +570,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;
@@ -1000,11 +1002,12 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
struct btrfs_device *orig_dev;
int ret = 0;
+ lockdep_assert_held(&uuid_mutex);
+
fs_devices = alloc_fs_devices(orig->fsid, NULL);
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) {
@@ -1036,10 +1039,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);
}
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-03-03 18:10 [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs Anand Jain
@ 2021-03-06 8:37 ` Anand Jain
2021-04-05 8:38 ` Anand Jain
2021-06-21 15:52 ` Anand Jain
1 sibling, 1 reply; 10+ messages in thread
From: Anand Jain @ 2021-03-06 8:37 UTC (permalink / raw)
To: dsterba; +Cc: Su Yue, linux-btrfs
David,
Ping?
Thanks, Anand
On 04/03/2021 02:10, Anand Jain wrote:
> Following test case reproduces lockdep warning.
>
> Test case:
> DEV1=/dev/vdb
> DEV2=/dev/vdc
> umount /btrfs
> run mkfs.btrfs -f $DEV1
> run btrfstune -S 1 $DEV1
> run mount $DEV1 /btrfs
> run btrfs device add $DEV2 /btrfs -f
> run umount /btrfs
> run mount $DEV2 /btrfs
> run umount /btrfs
>
> The warning claims a possible ABBA deadlock between the threads initiated by
> [#1] btrfs device add and [#0] the mount.
>
> ======================================================
> [ 540.743122] WARNING: possible circular locking dependency detected
> [ 540.743129] 5.11.0-rc7+ #5 Not tainted
> [ 540.743135] ------------------------------------------------------
> [ 540.743142] mount/2515 is trying to acquire lock:
> [ 540.743149] ffffa0c5544c2ce0 (&fs_devs->device_list_mutex){+.+.}-{4:4}, at: clone_fs_devices+0x6d/0x210 [btrfs]
> [ 540.743458] but task is already holding lock:
> [ 540.743461] ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at: __btrfs_tree_read_lock+0x32/0x200 [btrfs]
> [ 540.743541] which lock already depends on the new lock.
> [ 540.743543] the existing dependency chain (in reverse order) is:
>
> [ 540.743546] -> #1 (btrfs-chunk-00){++++}-{4:4}:
> [ 540.743566] down_read_nested+0x48/0x2b0
> [ 540.743585] __btrfs_tree_read_lock+0x32/0x200 [btrfs]
> [ 540.743650] btrfs_read_lock_root_node+0x70/0x200 [btrfs]
> [ 540.743733] btrfs_search_slot+0x6c6/0xe00 [btrfs]
> [ 540.743785] btrfs_update_device+0x83/0x260 [btrfs]
> [ 540.743849] btrfs_finish_chunk_alloc+0x13f/0x660 [btrfs] <--- device_list_mutex
> [ 540.743911] btrfs_create_pending_block_groups+0x18d/0x3f0 [btrfs]
> [ 540.743982] btrfs_commit_transaction+0x86/0x1260 [btrfs]
> [ 540.744037] btrfs_init_new_device+0x1600/0x1dd0 [btrfs]
> [ 540.744101] btrfs_ioctl+0x1c77/0x24c0 [btrfs]
> [ 540.744166] __x64_sys_ioctl+0xe4/0x140
> [ 540.744170] do_syscall_64+0x4b/0x80
> [ 540.744174] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [ 540.744180] -> #0 (&fs_devs->device_list_mutex){+.+.}-{4:4}:
> [ 540.744184] __lock_acquire+0x155f/0x2360
> [ 540.744188] lock_acquire+0x10b/0x5c0
> [ 540.744190] __mutex_lock+0xb1/0xf80
> [ 540.744193] mutex_lock_nested+0x27/0x30
> [ 540.744196] clone_fs_devices+0x6d/0x210 [btrfs]
> [ 540.744270] btrfs_read_chunk_tree+0x3c7/0xbb0 [btrfs]
> [ 540.744336] open_ctree+0xf6e/0x2074 [btrfs]
> [ 540.744406] btrfs_mount_root.cold.72+0x16/0x127 [btrfs]
> [ 540.744472] legacy_get_tree+0x38/0x90
> [ 540.744475] vfs_get_tree+0x30/0x140
> [ 540.744478] fc_mount+0x16/0x60
> [ 540.744482] vfs_kern_mount+0x91/0x100
> [ 540.744484] btrfs_mount+0x1e6/0x670 [btrfs]
> [ 540.744536] legacy_get_tree+0x38/0x90
> [ 540.744537] vfs_get_tree+0x30/0x140
> [ 540.744539] path_mount+0x8d8/0x1070
> [ 540.744541] do_mount+0x8d/0xc0
> [ 540.744543] __x64_sys_mount+0x125/0x160
> [ 540.744545] do_syscall_64+0x4b/0x80
> [ 540.744547] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [ 540.744551] other info that might help us debug this:
> [ 540.744552] Possible unsafe locking scenario:
>
> [ 540.744553] CPU0 CPU1
> [ 540.744554] ---- ----
> [ 540.744555] lock(btrfs-chunk-00);
> [ 540.744557] lock(&fs_devs->device_list_mutex);
> [ 540.744560] lock(btrfs-chunk-00);
> [ 540.744562] lock(&fs_devs->device_list_mutex);
> [ 540.744564]
> *** DEADLOCK ***
>
> [ 540.744565] 3 locks held by mount/2515:
> [ 540.744567] #0: ffffa0c56bf7a0e0 (&type->s_umount_key#42/1){+.+.}-{4:4}, at: alloc_super.isra.16+0xdf/0x450
> [ 540.744574] #1: ffffffffc05a9628 (uuid_mutex){+.+.}-{4:4}, at: btrfs_read_chunk_tree+0x63/0xbb0 [btrfs]
> [ 540.744640] #2: ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at: __btrfs_tree_read_lock+0x32/0x200 [btrfs]
> [ 540.744708]
> stack backtrace:
> [ 540.744712] CPU: 2 PID: 2515 Comm: mount Not tainted 5.11.0-rc7+ #5
>
>
> But the device_list_mutex in clone_fs_devices() is redundant, as explained
> below.
> Two threads [1] and [2] (below) could lead to clone_fs_device().
>
> [1]
> open_ctree <== mount sprout fs
> btrfs_read_chunk_tree()
> mutex_lock(&uuid_mutex) <== global lock
> read_one_dev()
> open_seed_devices()
> clone_fs_devices() <== seed fs_devices
> mutex_lock(&orig->device_list_mutex) <== seed fs_devices
>
> [2]
> btrfs_init_new_device() <== sprouting
> mutex_lock(&uuid_mutex); <== global lock
> btrfs_prepare_sprout()
> lockdep_assert_held(&uuid_mutex)
> clone_fs_devices(seed_fs_device) <== seed fs_devices
>
> Both of these threads hold uuid_mutex which is sufficient to protect
> getting the seed device(s) freed while we are trying to clone it for
> sprouting [2] or mounting a sprout [1] (as above). A mounted seed
> device can not free/write/replace because it is read-only. An unmounted
> seed device can free by btrfs_free_stale_devices(), but it needs uuid_mutex.
> So this patch removes the unnecessary device_list_mutex in clone_fs_devices().
> And adds a lockdep_assert_held(&uuid_mutex) in clone_fs_devices().
>
> Reported-by: Su Yue <l@damenly.su>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: Remove Martin's Reported-by and Tested-by.
> Add Su's Reported-by.
> Add lockdep_assert_held check.
> Update the changelog, make it relevant to the current misc-next
>
> fs/btrfs/volumes.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bc3b33efddc5..4188edbad2ef 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -570,6 +570,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;
>
> @@ -1000,11 +1002,12 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
> struct btrfs_device *orig_dev;
> int ret = 0;
>
> + lockdep_assert_held(&uuid_mutex);
> +
> fs_devices = alloc_fs_devices(orig->fsid, NULL);
> 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) {
> @@ -1036,10 +1039,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);
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-03-06 8:37 ` Anand Jain
@ 2021-04-05 8:38 ` Anand Jain
2021-04-05 9:18 ` Su Yue
0 siblings, 1 reply; 10+ messages in thread
From: Anand Jain @ 2021-04-05 8:38 UTC (permalink / raw)
To: dsterba; +Cc: Su Yue, linux-btrfs
Ping again.
Thanks, Anand
On 06/03/2021 16:37, Anand Jain wrote:
>
> David,
>
> Ping?
>
> Thanks, Anand
>
>
> On 04/03/2021 02:10, Anand Jain wrote:
>> Following test case reproduces lockdep warning.
>>
>> Test case:
>> DEV1=/dev/vdb
>> DEV2=/dev/vdc
>> umount /btrfs
>> run mkfs.btrfs -f $DEV1
>> run btrfstune -S 1 $DEV1
>> run mount $DEV1 /btrfs
>> run btrfs device add $DEV2 /btrfs -f
>> run umount /btrfs
>> run mount $DEV2 /btrfs
>> run umount /btrfs
>>
>> The warning claims a possible ABBA deadlock between the threads
>> initiated by
>> [#1] btrfs device add and [#0] the mount.
>>
>> ======================================================
>> [ 540.743122] WARNING: possible circular locking dependency detected
>> [ 540.743129] 5.11.0-rc7+ #5 Not tainted
>> [ 540.743135] ------------------------------------------------------
>> [ 540.743142] mount/2515 is trying to acquire lock:
>> [ 540.743149] ffffa0c5544c2ce0
>> (&fs_devs->device_list_mutex){+.+.}-{4:4}, at:
>> clone_fs_devices+0x6d/0x210 [btrfs]
>> [ 540.743458] but task is already holding lock:
>> [ 540.743461] ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at:
>> __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>> [ 540.743541] which lock already depends on the new lock.
>> [ 540.743543] the existing dependency chain (in reverse order) is:
>>
>> [ 540.743546] -> #1 (btrfs-chunk-00){++++}-{4:4}:
>> [ 540.743566] down_read_nested+0x48/0x2b0
>> [ 540.743585] __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>> [ 540.743650] btrfs_read_lock_root_node+0x70/0x200 [btrfs]
>> [ 540.743733] btrfs_search_slot+0x6c6/0xe00 [btrfs]
>> [ 540.743785] btrfs_update_device+0x83/0x260 [btrfs]
>> [ 540.743849] btrfs_finish_chunk_alloc+0x13f/0x660 [btrfs] <---
>> device_list_mutex
>> [ 540.743911] btrfs_create_pending_block_groups+0x18d/0x3f0 [btrfs]
>> [ 540.743982] btrfs_commit_transaction+0x86/0x1260 [btrfs]
>> [ 540.744037] btrfs_init_new_device+0x1600/0x1dd0 [btrfs]
>> [ 540.744101] btrfs_ioctl+0x1c77/0x24c0 [btrfs]
>> [ 540.744166] __x64_sys_ioctl+0xe4/0x140
>> [ 540.744170] do_syscall_64+0x4b/0x80
>> [ 540.744174] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> [ 540.744180] -> #0 (&fs_devs->device_list_mutex){+.+.}-{4:4}:
>> [ 540.744184] __lock_acquire+0x155f/0x2360
>> [ 540.744188] lock_acquire+0x10b/0x5c0
>> [ 540.744190] __mutex_lock+0xb1/0xf80
>> [ 540.744193] mutex_lock_nested+0x27/0x30
>> [ 540.744196] clone_fs_devices+0x6d/0x210 [btrfs]
>> [ 540.744270] btrfs_read_chunk_tree+0x3c7/0xbb0 [btrfs]
>> [ 540.744336] open_ctree+0xf6e/0x2074 [btrfs]
>> [ 540.744406] btrfs_mount_root.cold.72+0x16/0x127 [btrfs]
>> [ 540.744472] legacy_get_tree+0x38/0x90
>> [ 540.744475] vfs_get_tree+0x30/0x140
>> [ 540.744478] fc_mount+0x16/0x60
>> [ 540.744482] vfs_kern_mount+0x91/0x100
>> [ 540.744484] btrfs_mount+0x1e6/0x670 [btrfs]
>> [ 540.744536] legacy_get_tree+0x38/0x90
>> [ 540.744537] vfs_get_tree+0x30/0x140
>> [ 540.744539] path_mount+0x8d8/0x1070
>> [ 540.744541] do_mount+0x8d/0xc0
>> [ 540.744543] __x64_sys_mount+0x125/0x160
>> [ 540.744545] do_syscall_64+0x4b/0x80
>> [ 540.744547] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> [ 540.744551] other info that might help us debug this:
>> [ 540.744552] Possible unsafe locking scenario:
>>
>> [ 540.744553] CPU0 CPU1
>> [ 540.744554] ---- ----
>> [ 540.744555] lock(btrfs-chunk-00);
>> [ 540.744557] lock(&fs_devs->device_list_mutex);
>> [ 540.744560] lock(btrfs-chunk-00);
>> [ 540.744562] lock(&fs_devs->device_list_mutex);
>> [ 540.744564]
>> *** DEADLOCK ***
>>
>> [ 540.744565] 3 locks held by mount/2515:
>> [ 540.744567] #0: ffffa0c56bf7a0e0
>> (&type->s_umount_key#42/1){+.+.}-{4:4}, at:
>> alloc_super.isra.16+0xdf/0x450
>> [ 540.744574] #1: ffffffffc05a9628 (uuid_mutex){+.+.}-{4:4}, at:
>> btrfs_read_chunk_tree+0x63/0xbb0 [btrfs]
>> [ 540.744640] #2: ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at:
>> __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>> [ 540.744708]
>> stack backtrace:
>> [ 540.744712] CPU: 2 PID: 2515 Comm: mount Not tainted 5.11.0-rc7+ #5
>>
>>
>> But the device_list_mutex in clone_fs_devices() is redundant, as
>> explained
>> below.
>> Two threads [1] and [2] (below) could lead to clone_fs_device().
>>
>> [1]
>> open_ctree <== mount sprout fs
>> btrfs_read_chunk_tree()
>> mutex_lock(&uuid_mutex) <== global lock
>> read_one_dev()
>> open_seed_devices()
>> clone_fs_devices() <== seed fs_devices
>> mutex_lock(&orig->device_list_mutex) <== seed fs_devices
>>
>> [2]
>> btrfs_init_new_device() <== sprouting
>> mutex_lock(&uuid_mutex); <== global lock
>> btrfs_prepare_sprout()
>> lockdep_assert_held(&uuid_mutex)
>> clone_fs_devices(seed_fs_device) <== seed fs_devices
>>
>> Both of these threads hold uuid_mutex which is sufficient to protect
>> getting the seed device(s) freed while we are trying to clone it for
>> sprouting [2] or mounting a sprout [1] (as above). A mounted seed
>> device can not free/write/replace because it is read-only. An unmounted
>> seed device can free by btrfs_free_stale_devices(), but it needs
>> uuid_mutex.
>> So this patch removes the unnecessary device_list_mutex in
>> clone_fs_devices().
>> And adds a lockdep_assert_held(&uuid_mutex) in clone_fs_devices().
>>
>> Reported-by: Su Yue <l@damenly.su>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2: Remove Martin's Reported-by and Tested-by.
>> Add Su's Reported-by.
>> Add lockdep_assert_held check.
>> Update the changelog, make it relevant to the current misc-next
>>
>> fs/btrfs/volumes.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index bc3b33efddc5..4188edbad2ef 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -570,6 +570,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;
>> @@ -1000,11 +1002,12 @@ static struct btrfs_fs_devices
>> *clone_fs_devices(struct btrfs_fs_devices *orig)
>> struct btrfs_device *orig_dev;
>> int ret = 0;
>> + lockdep_assert_held(&uuid_mutex);
>> +
>> fs_devices = alloc_fs_devices(orig->fsid, NULL);
>> 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) {
>> @@ -1036,10 +1039,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);
>> }
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-04-05 8:38 ` Anand Jain
@ 2021-04-05 9:18 ` Su Yue
2021-04-05 9:30 ` Anand Jain
2021-04-06 16:48 ` David Sterba
0 siblings, 2 replies; 10+ messages in thread
From: Su Yue @ 2021-04-05 9:18 UTC (permalink / raw)
To: Anand Jain; +Cc: dsterba, linux-btrfs
On Mon 05 Apr 2021 at 16:38, Anand Jain <anand.jain@oracle.com>
wrote:
> Ping again.
>
It's already queued in misc-next.
commit 441737bb30f83914bb8517f52088c0130138d74b (misc-next)
Author: Anand Jain <anand.jain@oracle.com>
Date: Fri Jul 17 18:05:25 2020 +0800
btrfs: fix lockdep warning while mounting sprout fs
Martin reported the following test case which reproduces
lockdep
--
Su
> Thanks, Anand
>
> On 06/03/2021 16:37, Anand Jain wrote:
>>
>> David,
>>
>> Ping?
>>
>> Thanks, Anand
>>
>>
>> On 04/03/2021 02:10, Anand Jain wrote:
>>> Following test case reproduces lockdep warning.
>>>
>>> Test case:
>>> DEV1=/dev/vdb
>>> DEV2=/dev/vdc
>>> umount /btrfs
>>> run mkfs.btrfs -f $DEV1
>>> run btrfstune -S 1 $DEV1
>>> run mount $DEV1 /btrfs
>>> run btrfs device add $DEV2 /btrfs -f
>>> run umount /btrfs
>>> run mount $DEV2 /btrfs
>>> run umount /btrfs
>>>
>>> The warning claims a possible ABBA deadlock between the
>>> threads
>>> initiated by
>>> [#1] btrfs device add and [#0] the mount.
>>>
>>> ======================================================
>>> [ 540.743122] WARNING: possible circular locking dependency
>>> detected
>>> [ 540.743129] 5.11.0-rc7+ #5 Not tainted
>>> [ 540.743135]
>>> ------------------------------------------------------
>>> [ 540.743142] mount/2515 is trying to acquire lock:
>>> [ 540.743149] ffffa0c5544c2ce0
>>> (&fs_devs->device_list_mutex){+.+.}-{4:4}, at:
>>> clone_fs_devices+0x6d/0x210 [btrfs]
>>> [ 540.743458] but task is already holding lock:
>>> [ 540.743461] ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4},
>>> at:
>>> __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>>> [ 540.743541] which lock already depends on the new lock.
>>> [ 540.743543] the existing dependency chain (in reverse order)
>>> is:
>>>
>>> [ 540.743546] -> #1 (btrfs-chunk-00){++++}-{4:4}:
>>> [ 540.743566] down_read_nested+0x48/0x2b0
>>> [ 540.743585] __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>>> [ 540.743650] btrfs_read_lock_root_node+0x70/0x200 [btrfs]
>>> [ 540.743733] btrfs_search_slot+0x6c6/0xe00 [btrfs]
>>> [ 540.743785] btrfs_update_device+0x83/0x260 [btrfs]
>>> [ 540.743849] btrfs_finish_chunk_alloc+0x13f/0x660 [btrfs]
>>> <---
>>> device_list_mutex
>>> [ 540.743911] btrfs_create_pending_block_groups+0x18d/0x3f0
>>> [btrfs]
>>> [ 540.743982] btrfs_commit_transaction+0x86/0x1260 [btrfs]
>>> [ 540.744037] btrfs_init_new_device+0x1600/0x1dd0 [btrfs]
>>> [ 540.744101] btrfs_ioctl+0x1c77/0x24c0 [btrfs]
>>> [ 540.744166] __x64_sys_ioctl+0xe4/0x140
>>> [ 540.744170] do_syscall_64+0x4b/0x80
>>> [ 540.744174] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>
>>> [ 540.744180] -> #0 (&fs_devs->device_list_mutex){+.+.}-{4:4}:
>>> [ 540.744184] __lock_acquire+0x155f/0x2360
>>> [ 540.744188] lock_acquire+0x10b/0x5c0
>>> [ 540.744190] __mutex_lock+0xb1/0xf80
>>> [ 540.744193] mutex_lock_nested+0x27/0x30
>>> [ 540.744196] clone_fs_devices+0x6d/0x210 [btrfs]
>>> [ 540.744270] btrfs_read_chunk_tree+0x3c7/0xbb0 [btrfs]
>>> [ 540.744336] open_ctree+0xf6e/0x2074 [btrfs]
>>> [ 540.744406] btrfs_mount_root.cold.72+0x16/0x127 [btrfs]
>>> [ 540.744472] legacy_get_tree+0x38/0x90
>>> [ 540.744475] vfs_get_tree+0x30/0x140
>>> [ 540.744478] fc_mount+0x16/0x60
>>> [ 540.744482] vfs_kern_mount+0x91/0x100
>>> [ 540.744484] btrfs_mount+0x1e6/0x670 [btrfs]
>>> [ 540.744536] legacy_get_tree+0x38/0x90
>>> [ 540.744537] vfs_get_tree+0x30/0x140
>>> [ 540.744539] path_mount+0x8d8/0x1070
>>> [ 540.744541] do_mount+0x8d/0xc0
>>> [ 540.744543] __x64_sys_mount+0x125/0x160
>>> [ 540.744545] do_syscall_64+0x4b/0x80
>>> [ 540.744547] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>
>>> [ 540.744551] other info that might help us debug this:
>>> [ 540.744552] Possible unsafe locking scenario:
>>>
>>> [ 540.744553] CPU0 CPU1
>>> [ 540.744554] ---- ----
>>> [ 540.744555] lock(btrfs-chunk-00);
>>> [ 540.744557]
>>> lock(&fs_devs->device_list_mutex);
>>> [ 540.744560] lock(btrfs-chunk-00);
>>> [ 540.744562] lock(&fs_devs->device_list_mutex);
>>> [ 540.744564]
>>> *** DEADLOCK ***
>>>
>>> [ 540.744565] 3 locks held by mount/2515:
>>> [ 540.744567] #0: ffffa0c56bf7a0e0
>>> (&type->s_umount_key#42/1){+.+.}-{4:4}, at:
>>> alloc_super.isra.16+0xdf/0x450
>>> [ 540.744574] #1: ffffffffc05a9628 (uuid_mutex){+.+.}-{4:4},
>>> at:
>>> btrfs_read_chunk_tree+0x63/0xbb0 [btrfs]
>>> [ 540.744640] #2: ffffa0c54a7932b8
>>> (btrfs-chunk-00){++++}-{4:4}, at:
>>> __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>>> [ 540.744708]
>>> stack backtrace:
>>> [ 540.744712] CPU: 2 PID: 2515 Comm: mount Not tainted
>>> 5.11.0-rc7+ #5
>>>
>>>
>>> But the device_list_mutex in clone_fs_devices() is redundant,
>>> as
>>> explained
>>> below.
>>> Two threads [1] and [2] (below) could lead to
>>> clone_fs_device().
>>>
>>> [1]
>>> open_ctree <== mount sprout fs
>>> btrfs_read_chunk_tree()
>>> mutex_lock(&uuid_mutex) <== global lock
>>> read_one_dev()
>>> open_seed_devices()
>>> clone_fs_devices() <== seed fs_devices
>>> mutex_lock(&orig->device_list_mutex) <== seed fs_devices
>>>
>>> [2]
>>> btrfs_init_new_device() <== sprouting
>>> mutex_lock(&uuid_mutex); <== global lock
>>> btrfs_prepare_sprout()
>>> lockdep_assert_held(&uuid_mutex)
>>> clone_fs_devices(seed_fs_device) <== seed fs_devices
>>>
>>> Both of these threads hold uuid_mutex which is sufficient to
>>> protect
>>> getting the seed device(s) freed while we are trying to clone
>>> it for
>>> sprouting [2] or mounting a sprout [1] (as above). A mounted
>>> seed
>>> device can not free/write/replace because it is read-only. An
>>> unmounted
>>> seed device can free by btrfs_free_stale_devices(), but it
>>> needs
>>> uuid_mutex.
>>> So this patch removes the unnecessary device_list_mutex in
>>> clone_fs_devices().
>>> And adds a lockdep_assert_held(&uuid_mutex) in
>>> clone_fs_devices().
>>>
>>> Reported-by: Su Yue <l@damenly.su>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>> v2: Remove Martin's Reported-by and Tested-by.
>>> Add Su's Reported-by.
>>> Add lockdep_assert_held check.
>>> Update the changelog, make it relevant to the current
>>> misc-next
>>>
>>> fs/btrfs/volumes.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index bc3b33efddc5..4188edbad2ef 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -570,6 +570,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;
>>> @@ -1000,11 +1002,12 @@ static struct btrfs_fs_devices
>>> *clone_fs_devices(struct btrfs_fs_devices *orig)
>>> struct btrfs_device *orig_dev;
>>> int ret = 0;
>>> + lockdep_assert_held(&uuid_mutex);
>>> +
>>> fs_devices = alloc_fs_devices(orig->fsid, NULL);
>>> 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)
>>> {
>>> @@ -1036,10 +1039,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);
>>> }
>>>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-04-05 9:18 ` Su Yue
@ 2021-04-05 9:30 ` Anand Jain
2021-04-06 16:48 ` David Sterba
1 sibling, 0 replies; 10+ messages in thread
From: Anand Jain @ 2021-04-05 9:30 UTC (permalink / raw)
To: Su Yue; +Cc: dsterba, linux-btrfs
On 05/04/2021 17:18, Su Yue wrote:
>
> On Mon 05 Apr 2021 at 16:38, Anand Jain <anand.jain@oracle.com> wrote:
>
>> Ping again.
>>
> It's already queued in misc-next.
>
Oh thanks.
Thanks David.
-Anand
> commit 441737bb30f83914bb8517f52088c0130138d74b (misc-next)
> Author: Anand Jain <anand.jain@oracle.com>
> Date: Fri Jul 17 18:05:25 2020 +0800
>
> btrfs: fix lockdep warning while mounting sprout fs
>
> Martin reported the following test case which reproduces lockdep
>
> --
> Su
>> Thanks, Anand
>>
>> On 06/03/2021 16:37, Anand Jain wrote:
>>>
>>> David,
>>>
>>> Ping?
>>>
>>> Thanks, Anand
>>>
>>>
>>> On 04/03/2021 02:10, Anand Jain wrote:
>>>> Following test case reproduces lockdep warning.
>>>>
>>>> Test case:
>>>> DEV1=/dev/vdb
>>>> DEV2=/dev/vdc
>>>> umount /btrfs
>>>> run mkfs.btrfs -f $DEV1
>>>> run btrfstune -S 1 $DEV1
>>>> run mount $DEV1 /btrfs
>>>> run btrfs device add $DEV2 /btrfs -f
>>>> run umount /btrfs
>>>> run mount $DEV2 /btrfs
>>>> run umount /btrfs
>>>>
>>>> The warning claims a possible ABBA deadlock between the threads
>>>> initiated by
>>>> [#1] btrfs device add and [#0] the mount.
>>>>
>>>> ======================================================
>>>> [ 540.743122] WARNING: possible circular locking dependency detected
>>>> [ 540.743129] 5.11.0-rc7+ #5 Not tainted
>>>> [ 540.743135] ------------------------------------------------------
>>>> [ 540.743142] mount/2515 is trying to acquire lock:
>>>> [ 540.743149] ffffa0c5544c2ce0
>>>> (&fs_devs->device_list_mutex){+.+.}-{4:4}, at:
>>>> clone_fs_devices+0x6d/0x210 [btrfs]
>>>> [ 540.743458] but task is already holding lock:
>>>> [ 540.743461] ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at:
>>>> __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>>>> [ 540.743541] which lock already depends on the new lock.
>>>> [ 540.743543] the existing dependency chain (in reverse order) is:
>>>>
>>>> [ 540.743546] -> #1 (btrfs-chunk-00){++++}-{4:4}:
>>>> [ 540.743566] down_read_nested+0x48/0x2b0
>>>> [ 540.743585] __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>>>> [ 540.743650] btrfs_read_lock_root_node+0x70/0x200 [btrfs]
>>>> [ 540.743733] btrfs_search_slot+0x6c6/0xe00 [btrfs]
>>>> [ 540.743785] btrfs_update_device+0x83/0x260 [btrfs]
>>>> [ 540.743849] btrfs_finish_chunk_alloc+0x13f/0x660 [btrfs] <---
>>>> device_list_mutex
>>>> [ 540.743911] btrfs_create_pending_block_groups+0x18d/0x3f0 [btrfs]
>>>> [ 540.743982] btrfs_commit_transaction+0x86/0x1260 [btrfs]
>>>> [ 540.744037] btrfs_init_new_device+0x1600/0x1dd0 [btrfs]
>>>> [ 540.744101] btrfs_ioctl+0x1c77/0x24c0 [btrfs]
>>>> [ 540.744166] __x64_sys_ioctl+0xe4/0x140
>>>> [ 540.744170] do_syscall_64+0x4b/0x80
>>>> [ 540.744174] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>>
>>>> [ 540.744180] -> #0 (&fs_devs->device_list_mutex){+.+.}-{4:4}:
>>>> [ 540.744184] __lock_acquire+0x155f/0x2360
>>>> [ 540.744188] lock_acquire+0x10b/0x5c0
>>>> [ 540.744190] __mutex_lock+0xb1/0xf80
>>>> [ 540.744193] mutex_lock_nested+0x27/0x30
>>>> [ 540.744196] clone_fs_devices+0x6d/0x210 [btrfs]
>>>> [ 540.744270] btrfs_read_chunk_tree+0x3c7/0xbb0 [btrfs]
>>>> [ 540.744336] open_ctree+0xf6e/0x2074 [btrfs]
>>>> [ 540.744406] btrfs_mount_root.cold.72+0x16/0x127 [btrfs]
>>>> [ 540.744472] legacy_get_tree+0x38/0x90
>>>> [ 540.744475] vfs_get_tree+0x30/0x140
>>>> [ 540.744478] fc_mount+0x16/0x60
>>>> [ 540.744482] vfs_kern_mount+0x91/0x100
>>>> [ 540.744484] btrfs_mount+0x1e6/0x670 [btrfs]
>>>> [ 540.744536] legacy_get_tree+0x38/0x90
>>>> [ 540.744537] vfs_get_tree+0x30/0x140
>>>> [ 540.744539] path_mount+0x8d8/0x1070
>>>> [ 540.744541] do_mount+0x8d/0xc0
>>>> [ 540.744543] __x64_sys_mount+0x125/0x160
>>>> [ 540.744545] do_syscall_64+0x4b/0x80
>>>> [ 540.744547] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>>
>>>> [ 540.744551] other info that might help us debug this:
>>>> [ 540.744552] Possible unsafe locking scenario:
>>>>
>>>> [ 540.744553] CPU0 CPU1
>>>> [ 540.744554] ---- ----
>>>> [ 540.744555] lock(btrfs-chunk-00);
>>>> [ 540.744557] lock(&fs_devs->device_list_mutex);
>>>> [ 540.744560] lock(btrfs-chunk-00);
>>>> [ 540.744562] lock(&fs_devs->device_list_mutex);
>>>> [ 540.744564]
>>>> *** DEADLOCK ***
>>>>
>>>> [ 540.744565] 3 locks held by mount/2515:
>>>> [ 540.744567] #0: ffffa0c56bf7a0e0
>>>> (&type->s_umount_key#42/1){+.+.}-{4:4}, at:
>>>> alloc_super.isra.16+0xdf/0x450
>>>> [ 540.744574] #1: ffffffffc05a9628 (uuid_mutex){+.+.}-{4:4}, at:
>>>> btrfs_read_chunk_tree+0x63/0xbb0 [btrfs]
>>>> [ 540.744640] #2: ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at:
>>>> __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>>>> [ 540.744708]
>>>> stack backtrace:
>>>> [ 540.744712] CPU: 2 PID: 2515 Comm: mount Not tainted 5.11.0-rc7+ #5
>>>>
>>>>
>>>> But the device_list_mutex in clone_fs_devices() is redundant, as
>>>> explained
>>>> below.
>>>> Two threads [1] and [2] (below) could lead to clone_fs_device().
>>>>
>>>> [1]
>>>> open_ctree <== mount sprout fs
>>>> btrfs_read_chunk_tree()
>>>> mutex_lock(&uuid_mutex) <== global lock
>>>> read_one_dev()
>>>> open_seed_devices()
>>>> clone_fs_devices() <== seed fs_devices
>>>> mutex_lock(&orig->device_list_mutex) <== seed fs_devices
>>>>
>>>> [2]
>>>> btrfs_init_new_device() <== sprouting
>>>> mutex_lock(&uuid_mutex); <== global lock
>>>> btrfs_prepare_sprout()
>>>> lockdep_assert_held(&uuid_mutex)
>>>> clone_fs_devices(seed_fs_device) <== seed fs_devices
>>>>
>>>> Both of these threads hold uuid_mutex which is sufficient to protect
>>>> getting the seed device(s) freed while we are trying to clone it for
>>>> sprouting [2] or mounting a sprout [1] (as above). A mounted seed
>>>> device can not free/write/replace because it is read-only. An unmounted
>>>> seed device can free by btrfs_free_stale_devices(), but it needs
>>>> uuid_mutex.
>>>> So this patch removes the unnecessary device_list_mutex in
>>>> clone_fs_devices().
>>>> And adds a lockdep_assert_held(&uuid_mutex) in clone_fs_devices().
>>>>
>>>> Reported-by: Su Yue <l@damenly.su>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>> v2: Remove Martin's Reported-by and Tested-by.
>>>> Add Su's Reported-by.
>>>> Add lockdep_assert_held check.
>>>> Update the changelog, make it relevant to the current
>>>> misc-next
>>>>
>>>> fs/btrfs/volumes.c | 7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index bc3b33efddc5..4188edbad2ef 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -570,6 +570,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;
>>>> @@ -1000,11 +1002,12 @@ static struct btrfs_fs_devices
>>>> *clone_fs_devices(struct btrfs_fs_devices *orig)
>>>> struct btrfs_device *orig_dev;
>>>> int ret = 0;
>>>> + lockdep_assert_held(&uuid_mutex);
>>>> +
>>>> fs_devices = alloc_fs_devices(orig->fsid, NULL);
>>>> 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) {
>>>> @@ -1036,10 +1039,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);
>>>> }
>>>>
>>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-04-05 9:18 ` Su Yue
2021-04-05 9:30 ` Anand Jain
@ 2021-04-06 16:48 ` David Sterba
2021-04-06 23:01 ` Anand Jain
2021-04-07 8:24 ` Su Yue
1 sibling, 2 replies; 10+ messages in thread
From: David Sterba @ 2021-04-06 16:48 UTC (permalink / raw)
To: Su Yue; +Cc: Anand Jain, dsterba, linux-btrfs
On Mon, Apr 05, 2021 at 05:18:32PM +0800, Su Yue wrote:
>
> On Mon 05 Apr 2021 at 16:38, Anand Jain <anand.jain@oracle.com>
> wrote:
>
> > Ping again.
> >
> It's already queued in misc-next.
> commit 441737bb30f83914bb8517f52088c0130138d74b (misc-next)
> Author: Anand Jain <anand.jain@oracle.com>
> Date: Fri Jul 17 18:05:25 2020 +0800
No it's not, you must have checked some very old snapshot of misc-next,
I don't even have 441737bb30f83914bb8517f52088c0130138d74b in my stale
commit objects so it's been 'git gc'ed already.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-04-06 16:48 ` David Sterba
@ 2021-04-06 23:01 ` Anand Jain
2021-04-07 8:24 ` Su Yue
1 sibling, 0 replies; 10+ messages in thread
From: Anand Jain @ 2021-04-06 23:01 UTC (permalink / raw)
To: dsterba, Su Yue, dsterba, linux-btrfs
On 07/04/2021 00:48, David Sterba wrote:
> On Mon, Apr 05, 2021 at 05:18:32PM +0800, Su Yue wrote:
>>
>> On Mon 05 Apr 2021 at 16:38, Anand Jain <anand.jain@oracle.com>
>> wrote:
>>
>>> Ping again.
>>>
>> It's already queued in misc-next.
>
>> commit 441737bb30f83914bb8517f52088c0130138d74b (misc-next)
>> Author: Anand Jain <anand.jain@oracle.com>
>> Date: Fri Jul 17 18:05:25 2020 +0800
>
> No it's not, you must have checked some very old snapshot of misc-next,
> I don't even have 441737bb30f83914bb8517f52088c0130138d74b in my stale
> commit objects so it's been 'git gc'ed already.
What is holding this patch from the integration?
Thanks, Anand
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-04-06 16:48 ` David Sterba
2021-04-06 23:01 ` Anand Jain
@ 2021-04-07 8:24 ` Su Yue
1 sibling, 0 replies; 10+ messages in thread
From: Su Yue @ 2021-04-07 8:24 UTC (permalink / raw)
To: dsterba, Su Yue, Anand Jain, dsterba, linux-btrfs
On Wed, Apr 7, 2021 at 3:24 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Mon, Apr 05, 2021 at 05:18:32PM +0800, Su Yue wrote:
> >
> > On Mon 05 Apr 2021 at 16:38, Anand Jain <anand.jain@oracle.com>
> > wrote:
> >
> > > Ping again.
> > >
> > It's already queued in misc-next.
>
> > commit 441737bb30f83914bb8517f52088c0130138d74b (misc-next)
> > Author: Anand Jain <anand.jain@oracle.com>
> > Date: Fri Jul 17 18:05:25 2020 +0800
>
> No it's not, you must have checked some very old snapshot of misc-next,
> I don't even have 441737bb30f83914bb8517f52088c0130138d74b in my stale
> commit objects so it's been 'git gc'ed already.
Indeed. Sorry for the wrong info.
--
Su
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-03-03 18:10 [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs Anand Jain
2021-03-06 8:37 ` Anand Jain
@ 2021-06-21 15:52 ` Anand Jain
2021-08-23 11:00 ` Anand Jain
1 sibling, 1 reply; 10+ messages in thread
From: Anand Jain @ 2021-06-21 15:52 UTC (permalink / raw)
To: dsterba; +Cc: Su Yue, linux-btrfs
David,
Ping. I can reproduce this on 5.13.0-rc6+. The patch still applies
conflict-free on the current misc-next as of now.
Thanks, Anand
On 04/03/2021 02:10, Anand Jain wrote:
> Following test case reproduces lockdep warning.
>
> Test case:
> DEV1=/dev/vdb
> DEV2=/dev/vdc
> umount /btrfs
> run mkfs.btrfs -f $DEV1
> run btrfstune -S 1 $DEV1
> run mount $DEV1 /btrfs
> run btrfs device add $DEV2 /btrfs -f
> run umount /btrfs
> run mount $DEV2 /btrfs
> run umount /btrfs
>
> The warning claims a possible ABBA deadlock between the threads initiated by
> [#1] btrfs device add and [#0] the mount.
>
> ======================================================
> [ 540.743122] WARNING: possible circular locking dependency detected
> [ 540.743129] 5.11.0-rc7+ #5 Not tainted
> [ 540.743135] ------------------------------------------------------
> [ 540.743142] mount/2515 is trying to acquire lock:
> [ 540.743149] ffffa0c5544c2ce0 (&fs_devs->device_list_mutex){+.+.}-{4:4}, at: clone_fs_devices+0x6d/0x210 [btrfs]
> [ 540.743458] but task is already holding lock:
> [ 540.743461] ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at: __btrfs_tree_read_lock+0x32/0x200 [btrfs]
> [ 540.743541] which lock already depends on the new lock.
> [ 540.743543] the existing dependency chain (in reverse order) is:
>
> [ 540.743546] -> #1 (btrfs-chunk-00){++++}-{4:4}:
> [ 540.743566] down_read_nested+0x48/0x2b0
> [ 540.743585] __btrfs_tree_read_lock+0x32/0x200 [btrfs]
> [ 540.743650] btrfs_read_lock_root_node+0x70/0x200 [btrfs]
> [ 540.743733] btrfs_search_slot+0x6c6/0xe00 [btrfs]
> [ 540.743785] btrfs_update_device+0x83/0x260 [btrfs]
> [ 540.743849] btrfs_finish_chunk_alloc+0x13f/0x660 [btrfs] <--- device_list_mutex
> [ 540.743911] btrfs_create_pending_block_groups+0x18d/0x3f0 [btrfs]
> [ 540.743982] btrfs_commit_transaction+0x86/0x1260 [btrfs]
> [ 540.744037] btrfs_init_new_device+0x1600/0x1dd0 [btrfs]
> [ 540.744101] btrfs_ioctl+0x1c77/0x24c0 [btrfs]
> [ 540.744166] __x64_sys_ioctl+0xe4/0x140
> [ 540.744170] do_syscall_64+0x4b/0x80
> [ 540.744174] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [ 540.744180] -> #0 (&fs_devs->device_list_mutex){+.+.}-{4:4}:
> [ 540.744184] __lock_acquire+0x155f/0x2360
> [ 540.744188] lock_acquire+0x10b/0x5c0
> [ 540.744190] __mutex_lock+0xb1/0xf80
> [ 540.744193] mutex_lock_nested+0x27/0x30
> [ 540.744196] clone_fs_devices+0x6d/0x210 [btrfs]
> [ 540.744270] btrfs_read_chunk_tree+0x3c7/0xbb0 [btrfs]
> [ 540.744336] open_ctree+0xf6e/0x2074 [btrfs]
> [ 540.744406] btrfs_mount_root.cold.72+0x16/0x127 [btrfs]
> [ 540.744472] legacy_get_tree+0x38/0x90
> [ 540.744475] vfs_get_tree+0x30/0x140
> [ 540.744478] fc_mount+0x16/0x60
> [ 540.744482] vfs_kern_mount+0x91/0x100
> [ 540.744484] btrfs_mount+0x1e6/0x670 [btrfs]
> [ 540.744536] legacy_get_tree+0x38/0x90
> [ 540.744537] vfs_get_tree+0x30/0x140
> [ 540.744539] path_mount+0x8d8/0x1070
> [ 540.744541] do_mount+0x8d/0xc0
> [ 540.744543] __x64_sys_mount+0x125/0x160
> [ 540.744545] do_syscall_64+0x4b/0x80
> [ 540.744547] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [ 540.744551] other info that might help us debug this:
> [ 540.744552] Possible unsafe locking scenario:
>
> [ 540.744553] CPU0 CPU1
> [ 540.744554] ---- ----
> [ 540.744555] lock(btrfs-chunk-00);
> [ 540.744557] lock(&fs_devs->device_list_mutex);
> [ 540.744560] lock(btrfs-chunk-00);
> [ 540.744562] lock(&fs_devs->device_list_mutex);
> [ 540.744564]
> *** DEADLOCK ***
>
> [ 540.744565] 3 locks held by mount/2515:
> [ 540.744567] #0: ffffa0c56bf7a0e0 (&type->s_umount_key#42/1){+.+.}-{4:4}, at: alloc_super.isra.16+0xdf/0x450
> [ 540.744574] #1: ffffffffc05a9628 (uuid_mutex){+.+.}-{4:4}, at: btrfs_read_chunk_tree+0x63/0xbb0 [btrfs]
> [ 540.744640] #2: ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at: __btrfs_tree_read_lock+0x32/0x200 [btrfs]
> [ 540.744708]
> stack backtrace:
> [ 540.744712] CPU: 2 PID: 2515 Comm: mount Not tainted 5.11.0-rc7+ #5
>
>
> But the device_list_mutex in clone_fs_devices() is redundant, as explained
> below.
> Two threads [1] and [2] (below) could lead to clone_fs_device().
>
> [1]
> open_ctree <== mount sprout fs
> btrfs_read_chunk_tree()
> mutex_lock(&uuid_mutex) <== global lock
> read_one_dev()
> open_seed_devices()
> clone_fs_devices() <== seed fs_devices
> mutex_lock(&orig->device_list_mutex) <== seed fs_devices
>
> [2]
> btrfs_init_new_device() <== sprouting
> mutex_lock(&uuid_mutex); <== global lock
> btrfs_prepare_sprout()
> lockdep_assert_held(&uuid_mutex)
> clone_fs_devices(seed_fs_device) <== seed fs_devices
>
> Both of these threads hold uuid_mutex which is sufficient to protect
> getting the seed device(s) freed while we are trying to clone it for
> sprouting [2] or mounting a sprout [1] (as above). A mounted seed
> device can not free/write/replace because it is read-only. An unmounted
> seed device can free by btrfs_free_stale_devices(), but it needs uuid_mutex.
> So this patch removes the unnecessary device_list_mutex in clone_fs_devices().
> And adds a lockdep_assert_held(&uuid_mutex) in clone_fs_devices().
>
> Reported-by: Su Yue <l@damenly.su>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2: Remove Martin's Reported-by and Tested-by.
> Add Su's Reported-by.
> Add lockdep_assert_held check.
> Update the changelog, make it relevant to the current misc-next
>
> fs/btrfs/volumes.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bc3b33efddc5..4188edbad2ef 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -570,6 +570,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;
>
> @@ -1000,11 +1002,12 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
> struct btrfs_device *orig_dev;
> int ret = 0;
>
> + lockdep_assert_held(&uuid_mutex);
> +
> fs_devices = alloc_fs_devices(orig->fsid, NULL);
> 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) {
> @@ -1036,10 +1039,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);
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
2021-06-21 15:52 ` Anand Jain
@ 2021-08-23 11:00 ` Anand Jain
0 siblings, 0 replies; 10+ messages in thread
From: Anand Jain @ 2021-08-23 11:00 UTC (permalink / raw)
To: dsterba; +Cc: Su Yue, linux-btrfs
Ping.
On 21/06/2021 23:52, Anand Jain wrote:
> David,
>
> Ping. I can reproduce this on 5.13.0-rc6+. The patch still applies
> conflict-free on the current misc-next as of now.
>
> Thanks, Anand
>
> On 04/03/2021 02:10, Anand Jain wrote:
>> Following test case reproduces lockdep warning.
>>
>> Test case:
>> DEV1=/dev/vdb
>> DEV2=/dev/vdc
>> umount /btrfs
>> run mkfs.btrfs -f $DEV1
>> run btrfstune -S 1 $DEV1
>> run mount $DEV1 /btrfs
>> run btrfs device add $DEV2 /btrfs -f
>> run umount /btrfs
>> run mount $DEV2 /btrfs
>> run umount /btrfs
>>
>> The warning claims a possible ABBA deadlock between the threads
>> initiated by
>> [#1] btrfs device add and [#0] the mount.
>>
>> ======================================================
>> [ 540.743122] WARNING: possible circular locking dependency detected
>> [ 540.743129] 5.11.0-rc7+ #5 Not tainted
>> [ 540.743135] ------------------------------------------------------
>> [ 540.743142] mount/2515 is trying to acquire lock:
>> [ 540.743149] ffffa0c5544c2ce0
>> (&fs_devs->device_list_mutex){+.+.}-{4:4}, at:
>> clone_fs_devices+0x6d/0x210 [btrfs]
>> [ 540.743458] but task is already holding lock:
>> [ 540.743461] ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at:
>> __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>> [ 540.743541] which lock already depends on the new lock.
>> [ 540.743543] the existing dependency chain (in reverse order) is:
>>
>> [ 540.743546] -> #1 (btrfs-chunk-00){++++}-{4:4}:
>> [ 540.743566] down_read_nested+0x48/0x2b0
>> [ 540.743585] __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>> [ 540.743650] btrfs_read_lock_root_node+0x70/0x200 [btrfs]
>> [ 540.743733] btrfs_search_slot+0x6c6/0xe00 [btrfs]
>> [ 540.743785] btrfs_update_device+0x83/0x260 [btrfs]
>> [ 540.743849] btrfs_finish_chunk_alloc+0x13f/0x660 [btrfs] <---
>> device_list_mutex
>> [ 540.743911] btrfs_create_pending_block_groups+0x18d/0x3f0 [btrfs]
>> [ 540.743982] btrfs_commit_transaction+0x86/0x1260 [btrfs]
>> [ 540.744037] btrfs_init_new_device+0x1600/0x1dd0 [btrfs]
>> [ 540.744101] btrfs_ioctl+0x1c77/0x24c0 [btrfs]
>> [ 540.744166] __x64_sys_ioctl+0xe4/0x140
>> [ 540.744170] do_syscall_64+0x4b/0x80
>> [ 540.744174] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> [ 540.744180] -> #0 (&fs_devs->device_list_mutex){+.+.}-{4:4}:
>> [ 540.744184] __lock_acquire+0x155f/0x2360
>> [ 540.744188] lock_acquire+0x10b/0x5c0
>> [ 540.744190] __mutex_lock+0xb1/0xf80
>> [ 540.744193] mutex_lock_nested+0x27/0x30
>> [ 540.744196] clone_fs_devices+0x6d/0x210 [btrfs]
>> [ 540.744270] btrfs_read_chunk_tree+0x3c7/0xbb0 [btrfs]
>> [ 540.744336] open_ctree+0xf6e/0x2074 [btrfs]
>> [ 540.744406] btrfs_mount_root.cold.72+0x16/0x127 [btrfs]
>> [ 540.744472] legacy_get_tree+0x38/0x90
>> [ 540.744475] vfs_get_tree+0x30/0x140
>> [ 540.744478] fc_mount+0x16/0x60
>> [ 540.744482] vfs_kern_mount+0x91/0x100
>> [ 540.744484] btrfs_mount+0x1e6/0x670 [btrfs]
>> [ 540.744536] legacy_get_tree+0x38/0x90
>> [ 540.744537] vfs_get_tree+0x30/0x140
>> [ 540.744539] path_mount+0x8d8/0x1070
>> [ 540.744541] do_mount+0x8d/0xc0
>> [ 540.744543] __x64_sys_mount+0x125/0x160
>> [ 540.744545] do_syscall_64+0x4b/0x80
>> [ 540.744547] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> [ 540.744551] other info that might help us debug this:
>> [ 540.744552] Possible unsafe locking scenario:
>>
>> [ 540.744553] CPU0 CPU1
>> [ 540.744554] ---- ----
>> [ 540.744555] lock(btrfs-chunk-00);
>> [ 540.744557] lock(&fs_devs->device_list_mutex);
>> [ 540.744560] lock(btrfs-chunk-00);
>> [ 540.744562] lock(&fs_devs->device_list_mutex);
>> [ 540.744564]
>> *** DEADLOCK ***
>>
>> [ 540.744565] 3 locks held by mount/2515:
>> [ 540.744567] #0: ffffa0c56bf7a0e0
>> (&type->s_umount_key#42/1){+.+.}-{4:4}, at:
>> alloc_super.isra.16+0xdf/0x450
>> [ 540.744574] #1: ffffffffc05a9628 (uuid_mutex){+.+.}-{4:4}, at:
>> btrfs_read_chunk_tree+0x63/0xbb0 [btrfs]
>> [ 540.744640] #2: ffffa0c54a7932b8 (btrfs-chunk-00){++++}-{4:4}, at:
>> __btrfs_tree_read_lock+0x32/0x200 [btrfs]
>> [ 540.744708]
>> stack backtrace:
>> [ 540.744712] CPU: 2 PID: 2515 Comm: mount Not tainted 5.11.0-rc7+ #5
>>
>>
>> But the device_list_mutex in clone_fs_devices() is redundant, as
>> explained
>> below.
>> Two threads [1] and [2] (below) could lead to clone_fs_device().
>>
>> [1]
>> open_ctree <== mount sprout fs
>> btrfs_read_chunk_tree()
>> mutex_lock(&uuid_mutex) <== global lock
>> read_one_dev()
>> open_seed_devices()
>> clone_fs_devices() <== seed fs_devices
>> mutex_lock(&orig->device_list_mutex) <== seed fs_devices
>>
>> [2]
>> btrfs_init_new_device() <== sprouting
>> mutex_lock(&uuid_mutex); <== global lock
>> btrfs_prepare_sprout()
>> lockdep_assert_held(&uuid_mutex)
>> clone_fs_devices(seed_fs_device) <== seed fs_devices
>>
>> Both of these threads hold uuid_mutex which is sufficient to protect
>> getting the seed device(s) freed while we are trying to clone it for
>> sprouting [2] or mounting a sprout [1] (as above). A mounted seed
>> device can not free/write/replace because it is read-only. An unmounted
>> seed device can free by btrfs_free_stale_devices(), but it needs
>> uuid_mutex.
>> So this patch removes the unnecessary device_list_mutex in
>> clone_fs_devices().
>> And adds a lockdep_assert_held(&uuid_mutex) in clone_fs_devices().
>>
>> Reported-by: Su Yue <l@damenly.su>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2: Remove Martin's Reported-by and Tested-by.
>> Add Su's Reported-by.
>> Add lockdep_assert_held check.
>> Update the changelog, make it relevant to the current misc-next
>>
>> fs/btrfs/volumes.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index bc3b33efddc5..4188edbad2ef 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -570,6 +570,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;
>> @@ -1000,11 +1002,12 @@ static struct btrfs_fs_devices
>> *clone_fs_devices(struct btrfs_fs_devices *orig)
>> struct btrfs_device *orig_dev;
>> int ret = 0;
>> + lockdep_assert_held(&uuid_mutex);
>> +
>> fs_devices = alloc_fs_devices(orig->fsid, NULL);
>> 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) {
>> @@ -1036,10 +1039,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);
>> }
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-08-23 11:01 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 18:10 [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs Anand Jain
2021-03-06 8:37 ` Anand Jain
2021-04-05 8:38 ` Anand Jain
2021-04-05 9:18 ` Su Yue
2021-04-05 9:30 ` Anand Jain
2021-04-06 16:48 ` David Sterba
2021-04-06 23:01 ` Anand Jain
2021-04-07 8:24 ` Su Yue
2021-06-21 15:52 ` Anand Jain
2021-08-23 11:00 ` Anand Jain
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.