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

[Josef Bacik <josef@toxicpanda.com>]

On 8/10/20 12:28 PM, Filipe Manana 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
> 
> Either way, it looks good to me.

We need it at the skip: spot because we'll go straight to skip in some cases and 
not release the path, and then deadlock.  Fun things I discovered in testing ;).

Josef