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

[Qu Wenruo]

On 2020/8/21 上午10:42, Marcos Paulo de Souza 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+
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>

Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>

It would be even nicer if you could add some warning or self-test on
cache->length to prevent such problem from happening again.

Thanks,
Qu
> ---
>  fs/btrfs/block-group.c | 4 +++-
>  1 file changed, 3 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;
>

Attachment: signature.asc
Description: OpenPGP digital signature