Re: [PATCH 04/12] btrfs: remove remapped block groups from the free-space tree

[Boris Burkov <boris@bur.io>]

On Thu, Jun 05, 2025 at 05:23:34PM +0100, Mark Harmstone wrote:
> No new allocations can be done from block groups that have the REMAPPED flag
> set, so there's no value in their having entries in the free-space tree.
> 
> Prevent a search through the free-space tree being scheduled for such a
> block group, and prevent discard being run for a fully-remapped block
> group.
> 
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
>  fs/btrfs/block-group.c | 21 ++++++++++++++++-----
>  fs/btrfs/discard.c     |  9 +++++++++
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 5b0cb04b2b93..9b3b5358f1ba 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -920,6 +920,13 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, bool wait)
>  	if (btrfs_is_zoned(fs_info))
>  		return 0;
>  
> +	/*
> +	 * No allocations can be done from remapped block groups, so they have
> +	 * no entries in the free-space tree.
> +	 */
> +	if (cache->flags & BTRFS_BLOCK_GROUP_REMAPPED)
> +		return 0;
> +
>  	caching_ctl = kzalloc(sizeof(*caching_ctl), GFP_NOFS);
>  	if (!caching_ctl)
>  		return -ENOMEM;
> @@ -1235,9 +1242,11 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>  	 * another task to attempt to create another block group with the same
>  	 * item key (and failing with -EEXIST and a transaction abort).
>  	 */
> -	ret = remove_block_group_free_space(trans, block_group);
> -	if (ret)
> -		goto out;

nit: it feels nicer to hide the check inside the function.

> +	if (!(block_group->flags & BTRFS_BLOCK_GROUP_REMAPPED)) {
> +		ret = remove_block_group_free_space(trans, block_group);
> +		if (ret)
> +			goto out;
> +	}
>  
>  	ret = remove_block_group_item(trans, path, block_group);
>  	if (ret < 0)
> @@ -2457,10 +2466,12 @@ static int read_one_block_group(struct btrfs_fs_info *info,
>  	if (btrfs_chunk_writeable(info, cache->start)) {
>  		if (cache->used == 0) {
>  			ASSERT(list_empty(&cache->bg_list));
> -			if (btrfs_test_opt(info, DISCARD_ASYNC))
> +			if (btrfs_test_opt(info, DISCARD_ASYNC) &&

I asked this on the previous patch, but I guess this means we will never
discard these blocks? Is that desirable? Or are we discarding them at
some other point in the life-cycle?

> +			    !(cache->flags && BTRFS_BLOCK_GROUP_REMAPPED)) {
>  				btrfs_discard_queue_work(&info->discard_ctl, cache);
> -			else
> +			} else {
>  				btrfs_mark_bg_unused(cache);
> +			}
>  		}
>  	} else {
>  		inc_block_group_ro(cache, 1);
> diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
> index 89fe85778115..1015a4d37fb2 100644
> --- a/fs/btrfs/discard.c
> +++ b/fs/btrfs/discard.c
> @@ -698,6 +698,15 @@ void btrfs_discard_punt_unused_bgs_list(struct btrfs_fs_info *fs_info)
>  	/* We enabled async discard, so punt all to the queue */
>  	list_for_each_entry_safe(block_group, next, &fs_info->unused_bgs,
>  				 bg_list) {
> +		/* Fully remapped BGs have nothing to discard */

Same question. If we simply *don't* discard them, I feel like this
comment is misleadingly worded.

> +		spin_lock(&block_group->lock);
> +		if (block_group->flags & BTRFS_BLOCK_GROUP_REMAPPED &&
> +		    !btrfs_is_block_group_used(block_group)) {
> +			spin_unlock(&block_group->lock);
> +			continue;
> +		}
> +		spin_unlock(&block_group->lock);
> +
>  		list_del_init(&block_group->bg_list);
>  		btrfs_discard_queue_work(&fs_info->discard_ctl, block_group);
>  		/*
> -- 
> 2.49.0
>