On 17/03/2021 16:26, Filipe Manana wrote:
>> +void btrfs_reclaim_bgs(struct btrfs_fs_info *fs_info)
>> +{
>> +       struct btrfs_block_group *bg;
>> +       struct btrfs_space_info *space_info;
>> +       int ret = 0;
>> +
>> +       if (!test_bit(BTRFS_FS_OPEN, &fs_info->flags))
>> +               return;
>> +
>> +       if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_BALANCE))
>> +               return;
>> +
>> +       mutex_lock(&fs_info->delete_unused_bgs_mutex);
>> +       mutex_lock(&fs_info->reclaim_bgs_lock);
> 
> Could we just use delete_unused_bgs_mutex? I think you are using it
> only because btrfs_relocate_chunk() asserts it's being held.
> 
> Just renaming delete_unused_bgs_mutex to a more generic name like
> reclaim_bgs_lock, and just use that should work.

Do I understand you correctly, that btrfs_delete_unused_bgs and 
btrfs_reclaim_bgs
should use the same mutex? I was thinking about that but then didn't want to
have one mutex protecting two lists.

> 
>> +       while (!list_empty(&fs_info->reclaim_bgs)) {
>> +               bg = list_first_entry(&fs_info->reclaim_bgs,
>> +                                     struct btrfs_block_group,
>> +                                     bg_list);
>> +               list_del_init(&bg->bg_list);
>> +
>> +               space_info = bg->space_info;
>> +               mutex_unlock(&fs_info->reclaim_bgs_lock);
>> +
>> +               down_write(&space_info->groups_sem);
> 
> Having a comment on why we lock groups_sem would be good to have, just
> like at btrfs_delete_unused_bgs().
> 

OK

>> +
>> +               spin_lock(&bg->lock);
>> +               if (bg->reserved || bg->pinned || bg->ro ||
>> +                   list_is_singular(&bg->list)) {
>> +                       /*
>> +                        * We want to bail if we made new allocations or have
>> +                        * outstanding allocations in this block group.  We 
>> do
>> +                        * the ro check in case balance is currently acting 
>> on
>> +                        * this block group.
> 
> Why is the list_is_singular() check there?
> 
> This was copied from the empty block group removal case -
> btrfs_delete_unused_bgs(), but here I don't see why it's needed, since
> we relocate rather than remove block groups.
> The list_is_singular() from btrfs_delete_unused_bgs() is there to
> prevent losing the raid profile for data when the last data block
> group is removed (which implies not using space cache v1).
> 
> Other than that, overall it looks good.

Ah OK, yes this was a copy.


>> +               /*
>> +                * Reclaim block groups in the reclaim_bgs list after we 
>> deleted
>> +                * all unused block_groups. This possibly gives us some more 
>> free
>> +                * space.
>> +                */
>> +               btrfs_reclaim_bgs(fs_info);
> 
> Now with the cleaner kthread doing relocation, which can be a very
> slow operation, we end up delaying iputs and other work delegated to
> it for a very long time.
> Not saying it needs to be fixed right away, perhaps it's not that bad,
> I'm just not sure at the moment. We already do subvolume deletion in
> this kthread, which is also a slow operation.

Noted.

Reply via email to