On 22.10.21 г. 17:14, David Sterba wrote:

On Fri, Oct 22, 2021 at 09:12:11AM +0300, Nikolay Borisov wrote:

quoted

On 21.10.21 г. 20:04, David Sterba wrote:

quoted

On Thu, Oct 14, 2021 at 10:03:11AM +0300, Nikolay Borisov wrote:

quoted

Since both unused block groups and reclaim bgs lists are protected by
unused_bgs_lock then free them in the same critical section without
doing an extra unlock/lock pair.

Signed-off-by: Nikolay Borisov <redacted>
---
 fs/btrfs/block-group.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index e790ea0798c7..308b8e92c70e 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c

@@ -3873,9 +3873,7 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info)
 		list_del_init(&block_group->bg_list);
 		btrfs_put_block_group(block_group);
 	}
-	spin_unlock(&info->unused_bgs_lock);
 
-	spin_lock(&info->unused_bgs_lock);

That looks correct, I'm not sure about one thing. The calls to
btrfs_put_block_group can be potentaily taking some time if the last
reference is dropped and we need to call btrfs_discard_cancel_work and
several kfree()s. Indirectly there's eg. cancel_delayed_work_sync and
btrfs_discard_schedule_work, so calling all that under unused_bgs_lock
seems quite heavy.

btrfs_free_block_groups is called from 2 contexts only:

1. If we error out during mount
2. One of the last things we do during unmount, when all worker threads
are stopped.

IMO doing the cond_resched_lock would be a premature optimisation and
I'd aim for simplicity.

I'm not optimizing anything but rather preventing problems, cond_resched
is lightweight and one of the things that's nice to the rest of the
system.

But my patch doesn't change that, even without the patch the problem you
are hinting at (which I think is moot) can still occur because we the
final put is still done under the lock. So at the very least it should
be a different patch.