Re: [PATCH v2] btrfs: block-group: Fix free-space bitmap threshould

[Filipe Manana]

On Fri, Aug 21, 2020 at 2:43 PM Marcos Paulo de Souza
<marcos@xxxxxxxxxxxxx> wrote:
>
> From: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
>
> [BUG]
> After commit 9afc66498a0b ("btrfs: block-group: refactor how we read one
> block group item"), cache->length is being assigned after calling
> btrfs_create_block_group_cache. This causes a problem since
> set_free_space_tree_thresholds is calculate the free-space threshould to
> decide is the free-space tree should convert from extents to bitmaps.
>
> The current code calls set_free_space_tree_thresholds with cache->length
> being 0, which then makes cache->bitmap_high_thresh being zero. This
> implies the system will always use bitmap instead of extents, which is
> not desired if the block group is not fragmented.
>
> This behavior can be seen by a test that expects to repair systems
> with FREE_SPACE_EXTENT and FREE_SPACE_BITMAP, but the current code only
> created FREE_SPACE_BITMAP.
>
> [FIX]
> Call set_free_space_tree_thresholds after setting cache->length.
>
> Link: https://github.com/kdave/btrfs-progs/issues/251
> Fixes: 9afc66498a0b ("btrfs: block-group: refactor how we read one block group item")
> CC: stable@xxxxxxxxxxxxxxx # 5.8+
> Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> ---
>
>  Changes from v1:
>  * Add a warning in set_free_space_tree_thresholds when bg->length is zero (Qu)
>
>  fs/btrfs/block-group.c     | 4 +++-
>  fs/btrfs/free-space-tree.c | 3 +++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 44fdfa2eeb2e..01e8ba1da1d3 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1798,7 +1798,6 @@ static struct btrfs_block_group *btrfs_create_block_group_cache(
>
>         cache->fs_info = fs_info;
>         cache->full_stripe_len = btrfs_full_stripe_len(fs_info, start);
> -       set_free_space_tree_thresholds(cache);
>
>         cache->discard_index = BTRFS_DISCARD_INDEX_UNUSED;
>
> @@ -1908,6 +1907,8 @@ static int read_one_block_group(struct btrfs_fs_info *info,
>
>         read_block_group_item(cache, path, key);
>
> +       set_free_space_tree_thresholds(cache);
> +
>         if (need_clear) {
>                 /*
>                  * When we mount with old space cache, we need to
> @@ -2128,6 +2129,7 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
>                 return -ENOMEM;
>
>         cache->length = size;
> +       set_free_space_tree_thresholds(cache);
>         cache->used = bytes_used;
>         cache->flags = type;
>         cache->last_byte_to_unpin = (u64)-1;
> diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> index 8b1f5c8897b7..1d191fbc754b 100644
> --- a/fs/btrfs/free-space-tree.c
> +++ b/fs/btrfs/free-space-tree.c
> @@ -22,6 +22,9 @@ void set_free_space_tree_thresholds(struct btrfs_block_group *cache)
>         size_t bitmap_size;
>         u64 num_bitmaps, total_bitmap_size;
>
> +       if (cache->length == 0)
> +               btrfs_warn(cache->fs_info, "block group length is zero");

This alone is not very useful.
With something like:

if (WARN_ON(cache->length) == 0)
  .... (and the message including the block group's logical address
too, the ->start field)

Such a bug is much easier to spot. If a test case from fstests
triggers it, it will be reported as a test failure.

Why not an ASSERT() instead? Though I don't have a strong preference
between the two for this case.
Either option will make it easy to spot with fstests.

As for the rest, the fix itself looks good to me.
You can later add,

Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>

Thanks.

> +
>         /*
>          * We convert to bitmaps when the disk space required for using extents
>          * exceeds that required for using bitmaps.
> --
> 2.28.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”