| Message ID | 20211014070311.1595609-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Remove spurious unlock/lock of unused_bgs_lock | expand |
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
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.
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. >
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.
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.
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.
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,
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(-)