Re: [PATCH] btrfs: Remove spurious unlock/lock of unused_bgs_lock

[Nikolay Borisov <nborisov@suse.com>]

On 22.10.21 г. 17:14, David Sterba wrote:
> On Fri, Oct 22, 2021 at 09:12:11AM +0300, Nikolay Borisov wrote:
>> On 21.10.21 г. 20:04, David Sterba wrote:
>>> On Thu, Oct 14, 2021 at 10:03:11AM +0300, Nikolay Borisov wrote:
>>>> 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 <nborisov@suse.com>
>>>> ---
>>>>  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.