Re: [PATCH] btrfs: skip reclaim if block_group is empty

[Filipe Manana <fdmanana@kernel.org>]

On Mon, Oct 10, 2022 at 8:25 PM Boris Burkov <boris@bur.io> wrote:
>
> As we delete extents from a block group, at some deletion we cross below
> the reclaim threshold. It is possible we are still in the middle of
> deleting more extents and might soon hit 0. If that occurs, we would
> leave the block group on the reclaim list, not in the care of unused
> deletion or async discard.
>
> It is pointless and wasteful to relocate empty block groups, so if we do

Hum? Why pointless and wasteful?
Relocating an empty block group results in deleting it.

In fact, before we tracked unused block groups and had the cleaner
kthread remove them, that was the only
way to delete unused block groups - trigger relocation from user space.

btrfs_relocate_chunk() explicitly calls btrfs_remove_chunk() at the
end, and the relocation itself
will do nothing except:

1) commit the current transaction when it starts
2) search the extent tree for extents in this block group - here it
will not find anything, and therefore do nothing.
3) commit another transaction

So I don't quite understand what this patch is trying to accomplish.

At the very least the changelog needs to be more detailed.

As it is, it gives the wrong idea that the relocation will leave the
block group around.
If your goal is to avoid the 2 transaction commits and the search on
the extent tree, then please be explicit in
the changelog.

Thanks.

> notice that case (we might not if the reclaim worker runs *before* we
> finish emptying it), don't bother with relocating the block group.
>
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/block-group.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 11fd52657b76..c3ea627d2457 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1608,6 +1608,25 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
>                         up_write(&space_info->groups_sem);
>                         goto next;
>                 }
> +               if (bg->used == 0) {
> +                       /*
> +                        * It is possible that we trigger relocation on a block
> +                        * group as its extents are deleted and it first goes
> +                        * below the threshold, then shortly goes empty. In that
> +                        * case, we will do relocation, even though we could
> +                        * more cheaply just delete the unused block group. Try
> +                        * to catch that case here, though of course it is
> +                        * possible there is a delete still coming the future,
> +                        * so we can't avoid needless relocation of this sort
> +                        * altogether. We can at least avoid relocating empty
> +                        * block groups.
> +                        */
> +                       if (!btrfs_test_opt(fs_info, DISCARD_ASYNC))
> +                               btrfs_mark_bg_unused(bg);
> +                       spin_unlock(&bg->lock);
> +                       up_write(&space_info->groups_sem);
> +                       goto next;
> +               }
>                 spin_unlock(&bg->lock);
>
>                 /* Get out fast, in case we're unmounting the filesystem */
> --
> 2.37.2
>