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

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

[Nikolay Borisov]

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);
 	while (!list_empty(&info->reclaim_bgs)) {
 		block_group = list_first_entry(&info->reclaim_bgs,
 					       struct btrfs_block_group,
-- 
2.25.1

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

[Josef Bacik]

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>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

[David Sterba]

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.

OTOH, this is in btrfs_free_block_groups so it's called at the end of
mount so we don't care about performance. There could be a problem with
a lot of queued work and doing that in one locking section can cause
warnings or other stalls.

That the lock is unlocked and locked at least inserts one opportunity to
schedule. Alternatively to be totally scheduler friendly, the loops
could follow pattern


	spin_lock(lock);
	while (!list_empty()) {
		entry = list_first();

		list_del_init(entry);
		btrfs_put_block_group(entry->bg);
		cond_resched_lock(lock);
	}
	spin_unlock(lock);

So with the cond_resched_lock inside the whole lock scope can be done as
you suggest.

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

[Nikolay Borisov]

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.

> 
> OTOH, this is in btrfs_free_block_groups so it's called at the end of
> mount so we don't care about performance. There could be a problem with
> a lot of queued work and doing that in one locking section can cause
> warnings or other stalls.
> 
> That the lock is unlocked and locked at least inserts one opportunity to
> schedule. Alternatively to be totally scheduler friendly, the loops
> could follow pattern
> 
> 
> 	spin_lock(lock);
> 	while (!list_empty()) {
> 		entry = list_first();
> 
> 		list_del_init(entry);
> 		btrfs_put_block_group(entry->bg);
> 		cond_resched_lock(lock);
> 	}
> 	spin_unlock(lock);
> 
> So with the cond_resched_lock inside the whole lock scope can be done as
> you suggest.
> 

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

[David Sterba]

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.

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

[Nikolay Borisov]

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.

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

[David Sterba]

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>

Added to misc-next. The cond_resched in the loops still makes sense but
is for another patch.