Re: [PATCH 01/17] btrfs: drop path before adding new uuid tree entry

Filipe Manana
2020-08-10T16:30:21Z

On Mon, Aug 10, 2020 at 5:28 PM Filipe Manana <fdmanana@gmail.com> wrote:
>
> On Mon, Aug 10, 2020 at 4:44 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > With the conversion to the rwsemaphore I got the following lockdep splat
> >
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 5.8.0-rc7-00167-g0d7ba0c5b375-dirty #925 Not tainted
> > ------------------------------------------------------
> > btrfs-uuid/7955 is trying to acquire lock:
> > ffff88bfbafec0f8 (btrfs-root-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x39/0x180
> >
> > but task is already holding lock:
> > ffff88bfbafef2a8 (btrfs-uuid-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x39/0x180
> >
> > which lock already depends on the new lock.
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 (btrfs-uuid-00){++++}-{3:3}:
> > down_read_nested+0x3e/0x140
> > __btrfs_tree_read_lock+0x39/0x180
> > __btrfs_read_lock_root_node+0x3a/0x50
> > btrfs_search_slot+0x4bd/0x990
> > btrfs_uuid_tree_add+0x89/0x2d0
> > btrfs_uuid_scan_kthread+0x330/0x390
> > kthread+0x133/0x150
> > ret_from_fork+0x1f/0x30
> >
> > -> #0 (btrfs-root-00){++++}-{3:3}:
> > __lock_acquire+0x1272/0x2310
> > lock_acquire+0x9e/0x360
> > down_read_nested+0x3e/0x140
> > __btrfs_tree_read_lock+0x39/0x180
> > __btrfs_read_lock_root_node+0x3a/0x50
> > btrfs_search_slot+0x4bd/0x990
> > btrfs_find_root+0x45/0x1b0
> > btrfs_read_tree_root+0x61/0x100
> > btrfs_get_root_ref.part.50+0x143/0x630
> > btrfs_uuid_tree_iterate+0x207/0x314
> > btrfs_uuid_rescan_kthread+0x12/0x50
> > kthread+0x133/0x150
> > ret_from_fork+0x1f/0x30
> >
> > other info that might help us debug this:
> >
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(btrfs-uuid-00);
> > lock(btrfs-root-00);
> > lock(btrfs-uuid-00);
> > lock(btrfs-root-00);
> >
> > *** DEADLOCK ***
> >
> > 1 lock held by btrfs-uuid/7955:
> > #0: ffff88bfbafef2a8 (btrfs-uuid-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x39/0x180
> >
> > stack backtrace:
> > CPU: 73 PID: 7955 Comm: btrfs-uuid Kdump: loaded Not tainted 5.8.0-rc7-00167-g0d7ba0c5b375-dirty #925
> > Hardware name: Quanta Tioga Pass Single Side 01-0030993006/Tioga Pass Single Side, BIOS F08_3A18 12/20/2018
> > Call Trace:
> > dump_stack+0x78/0xa0
> > check_noncircular+0x165/0x180
> > __lock_acquire+0x1272/0x2310
> > lock_acquire+0x9e/0x360
> > ? __btrfs_tree_read_lock+0x39/0x180
> > ? btrfs_root_node+0x1c/0x1d0
> > down_read_nested+0x3e/0x140
> > ? __btrfs_tree_read_lock+0x39/0x180
> > __btrfs_tree_read_lock+0x39/0x180
> > __btrfs_read_lock_root_node+0x3a/0x50
> > btrfs_search_slot+0x4bd/0x990
> > btrfs_find_root+0x45/0x1b0
> > btrfs_read_tree_root+0x61/0x100
> > btrfs_get_root_ref.part.50+0x143/0x630
> > btrfs_uuid_tree_iterate+0x207/0x314
> > ? btree_readpage+0x20/0x20
> > btrfs_uuid_rescan_kthread+0x12/0x50
> > kthread+0x133/0x150
> > ? kthread_create_on_node+0x60/0x60
> > ret_from_fork+0x1f/0x30
> >
> > This problem exists because we have two different rescan threads,
> > btrfs_uuid_scan_kthread which creates the uuid tree, and
> > btrfs_uuid_tree_iterate that goes through and updates or deletes any out
> > of date roots. The problem is they both do things in different order.
> > btrfs_uuid_scan_kthread() reads the tree_root, and then inserts entries
> > into the uuid_root. btrfs_uuid_tree_iterate() scans the uuid_root, but
> > then does a btrfs_get_fs_root() which can read from the tree_root.
>
> Ok, the get_fs_root() is through btrfs_check_uuid_tree_entry().
>
> >
> > It's actually easy enough to not be holding the path in
> > btrfs_uuid_scan_kthread() when we add a uuid entry, as we already drop
> > it further down and re-start the search when we loop. So simply move
> > the path release before we add our entry to the uuid tree.
> >
> > This also fixes a problem where we're holding a path open after we do
> > btrfs_end_transaction(), which has it's own problems.
> >
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> > fs/btrfs/volumes.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index d7670e2a9f39..3ac44dad58bb 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -4462,6 +4462,7 @@ int btrfs_uuid_scan_kthread(void *data)
> > goto skip;
> > }
> > update_tree:
> > + btrfs_release_path(path);
>
> We can actually do the release right after reading from the eb, so we
> avoid 3 path releases in different places.
> The end result would be:
>
> https://pastebin.com/2gH54HBw

Well, it should be https://pastebin.com/t5nDBhVR

>
> Either way, it looks good to me.
>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>
> Thanks.
>
> > if (!btrfs_is_empty_uuid(root_item.uuid)) {
> > ret = btrfs_uuid_tree_add(trans, root_item.uuid,
> > BTRFS_UUID_KEY_SUBVOL,
> > @@ -4486,6 +4487,7 @@ int btrfs_uuid_scan_kthread(void *data)
> > }
> >
> > skip:
> > + btrfs_release_path(path);
> > if (trans) {
> > ret = btrfs_end_transaction(trans);
> > trans = NULL;
> > @@ -4493,7 +4495,6 @@ int btrfs_uuid_scan_kthread(void *data)
> > break;
> > }
> >
> > - btrfs_release_path(path);
> > if (key.offset < (u64)-1) {
> > key.offset++;
> > } else if (key.type < BTRFS_ROOT_ITEM_KEY) {
> > --
> > 2.24.1
> >
>
>
> --
> Filipe David Manana,
>
> “Whether you think you can, or you think you can't — you're right.”

--
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”