Re: [PATCH v2 15/16] btrfs: add fully_remapped_bgs list

[Boris Burkov <boris@bur.io>]

On Wed, Aug 13, 2025 at 03:34:57PM +0100, Mark Harmstone wrote:
> Add a fully_remapped_bgs list to struct btrfs_transaction, which holds
> block groups which have just had their last identity remap removed.
> 
> In btrfs_finish_extent_commit() we can then discard their full dev
> extents, as we're also setting their num_stripes to 0. Finally if the BG
> is now empty, i.e. there's neither identity remaps nor normal remaps,
> add it to the unused_bgs list to be taken care of there.
> 
> Signed-off-by: Mark Harmstone <mark@harmstone.com>
> ---
>  fs/btrfs/block-group.c | 26 ++++++++++++++++++++++++++
>  fs/btrfs/block-group.h |  2 ++
>  fs/btrfs/extent-tree.c | 37 ++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/relocation.c  |  2 ++
>  fs/btrfs/transaction.c |  1 +
>  fs/btrfs/transaction.h |  1 +
>  6 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 7a0524138235..7f8707dfd62c 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1803,6 +1803,14 @@ void btrfs_mark_bg_unused(struct btrfs_block_group *bg)
>  	struct btrfs_fs_info *fs_info = bg->fs_info;
>  
>  	spin_lock(&fs_info->unused_bgs_lock);
> +
> +	/* Leave fully remapped block groups on the fully_remapped_bgs list. */
> +	if (bg->flags & BTRFS_BLOCK_GROUP_REMAPPED &&
> +	    bg->identity_remap_count == 0) {
> +		spin_unlock(&fs_info->unused_bgs_lock);
> +		return;
> +	}
> +
>  	if (list_empty(&bg->bg_list)) {
>  		btrfs_get_block_group(bg);
>  		trace_btrfs_add_unused_block_group(bg);
> @@ -4792,3 +4800,21 @@ bool btrfs_block_group_should_use_size_class(const struct btrfs_block_group *bg)
>  		return false;
>  	return true;
>  }
> +
> +void btrfs_mark_bg_fully_remapped(struct btrfs_block_group *bg,
> +				  struct btrfs_trans_handle *trans)
> +{
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +
> +	spin_lock(&fs_info->unused_bgs_lock);
> +
> +	if (!list_empty(&bg->bg_list))
> +		list_del(&bg->bg_list);
> +	else
> +		btrfs_get_block_group(bg);
> +
> +	list_add_tail(&bg->bg_list, &trans->transaction->fully_remapped_bgs);
> +
> +	spin_unlock(&fs_info->unused_bgs_lock);
> +
> +}

Why does the fully remapped bg list takeover from other lists rather
than use the link function?

What protection is in place to ensure that we never mark it fully
remapped while it is on the new_bgs list (as with the unused list)?

I suspect such a block group won't ever be reclaimed even with explicit
balances, but it is important to check and be sure.

If this *is* strictly necessary, I would like to see an extension to
btrfs_link_bg_list that can handle the list_move_tail variant.

Another option is to generalize this one together with mark_unused()
and just check the NEW flag here.

> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index 0433b0127ed8..025ea2c6f8a8 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -408,5 +408,7 @@ int btrfs_use_block_group_size_class(struct btrfs_block_group *bg,
>  				     enum btrfs_block_group_size_class size_class,
>  				     bool force_wrong_size_class);
>  bool btrfs_block_group_should_use_size_class(const struct btrfs_block_group *bg);
> +void btrfs_mark_bg_fully_remapped(struct btrfs_block_group *bg,
> +				  struct btrfs_trans_handle *trans);
>  
>  #endif /* BTRFS_BLOCK_GROUP_H */
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index b02e99b41553..157a032df128 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2853,7 +2853,7 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
>  {
>  	struct btrfs_fs_info *fs_info = trans->fs_info;
>  	struct btrfs_block_group *block_group, *tmp;
> -	struct list_head *deleted_bgs;
> +	struct list_head *deleted_bgs, *fully_remapped_bgs;
>  	struct extent_io_tree *unpin = &trans->transaction->pinned_extents;
>  	struct extent_state *cached_state = NULL;
>  	u64 start;
> @@ -2951,6 +2951,41 @@ int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
>  		}
>  	}
>  

1. This will block the next transaction waiting on TRANS_STATE_COMPLETED

2. This is not compatible with the spirit and purpose of async discard,
which is our default and best discard mode.

3. This doesn't check discard mode at all, it just defaults to
DISCARD_SYNC style behavior, so it doesn't respect NODISCARD either.

> +	fully_remapped_bgs = &trans->transaction->fully_remapped_bgs;
> +	list_for_each_entry_safe(block_group, tmp, fully_remapped_bgs, bg_list) {
> +		struct btrfs_chunk_map *map;
> +
> +		if (!TRANS_ABORTED(trans))
> +			ret = btrfs_discard_extent(fs_info, block_group->start,
> +						   block_group->length, NULL,
> +						   false);
> +
> +		map = btrfs_get_chunk_map(fs_info, block_group->start, 1);
> +		if (IS_ERR(map))
> +			return PTR_ERR(map);
> +
> +		/*
> +		 * Set num_stripes to 0, so that btrfs_remove_dev_extents()
> +		 * won't run a second time.
> +		 */
> +		map->num_stripes = 0;
> +
> +		btrfs_free_chunk_map(map);
> +
> +		if (block_group->used == 0 && block_group->remap_bytes == 0) {
> +			spin_lock(&fs_info->unused_bgs_lock);
> +			list_move_tail(&block_group->bg_list,
> +				       &fs_info->unused_bgs);
> +			spin_unlock(&fs_info->unused_bgs_lock);

Please use the helpers, it's important for ensuring correct ref counting
in the long run. I also think that the previous patch had some
discussion for more standardized integration with unused_bgs so I sort
of hope this code goes away entirely.

> +		} else {
> +			spin_lock(&fs_info->unused_bgs_lock);
> +			list_del_init(&block_group->bg_list);
> +			spin_unlock(&fs_info->unused_bgs_lock);
> +
> +			btrfs_put_block_group(block_group);
> +		}
> +	}
> +
>  	return unpin_error;
>  }
>  
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 84ff59866e96..0745a3d1c867 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4819,6 +4819,8 @@ static int last_identity_remap_gone(struct btrfs_trans_handle *trans,
>  	if (ret)
>  		return ret;
>  
> +	btrfs_mark_bg_fully_remapped(bg, trans);
> +
>  	return 0;
>  }
>  
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 64b9c427af6a..7c308d33e767 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -381,6 +381,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
>  	mutex_init(&cur_trans->cache_write_mutex);
>  	spin_lock_init(&cur_trans->dirty_bgs_lock);
>  	INIT_LIST_HEAD(&cur_trans->deleted_bgs);
> +	INIT_LIST_HEAD(&cur_trans->fully_remapped_bgs);
>  	spin_lock_init(&cur_trans->dropped_roots_lock);
>  	list_add_tail(&cur_trans->list, &fs_info->trans_list);
>  	btrfs_extent_io_tree_init(fs_info, &cur_trans->dirty_pages,
> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> index 9f7c777af635..b362915288b5 100644
> --- a/fs/btrfs/transaction.h
> +++ b/fs/btrfs/transaction.h
> @@ -109,6 +109,7 @@ struct btrfs_transaction {
>  	spinlock_t dirty_bgs_lock;
>  	/* Protected by spin lock fs_info->unused_bgs_lock. */
>  	struct list_head deleted_bgs;
> +	struct list_head fully_remapped_bgs;
>  	spinlock_t dropped_roots_lock;
>  	struct btrfs_delayed_ref_root delayed_refs;
>  	struct btrfs_fs_info *fs_info;
> -- 
> 2.49.1
>