Re: [PATCH v2 16/16] btrfs: allow balancing remap tree

[Boris Burkov <boris@bur.io>]

On Wed, Aug 13, 2025 at 03:34:58PM +0100, Mark Harmstone wrote:
> Balancing the REMAP chunk, i.e. the chunk in which the remap tree lives,
> is a special case.
> 
> We can't use the remap tree itself for this, as then we'd have no way to
> boostrap it on mount. And we can't use the pre-remap tree code for this
> as it relies on walking the extent tree, and we're not creating backrefs
> for REMAP chunks.
> 
> So instead, if a balance would relocate any REMAP block groups, mark
> those block groups as readonly and COW every leaf of the remap tree.
> 
> There's more sophisticated ways of doing this, such as only COWing nodes
> within a block group that's to be relocated, but they're fiddly and with
> lots of edge cases. Plus it's not anticipated that a) the number of
> REMAP chunks is going to be particularly large, or b) that users will
> want to only relocate some of these chunks - the main use case here is
> to unbreak RAID conversion and device removal.
> 
> Signed-off-by: Mark Harmstone <mark@harmstone.com>
> ---
>  fs/btrfs/volumes.c | 161 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 157 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e13f16a7a904..dc535ed90ae0 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4011,8 +4011,11 @@ static bool should_balance_chunk(struct extent_buffer *leaf, struct btrfs_chunk
>  	struct btrfs_balance_args *bargs = NULL;
>  	u64 chunk_type = btrfs_chunk_type(leaf, chunk);
>  
> -	if (chunk_type & BTRFS_BLOCK_GROUP_REMAP)
> -		return false;
> +	/* treat REMAP chunks as METADATA */
> +	if (chunk_type & BTRFS_BLOCK_GROUP_REMAP) {
> +		chunk_type &= ~BTRFS_BLOCK_GROUP_REMAP;
> +		chunk_type |= BTRFS_BLOCK_GROUP_METADATA;

why not honor the REMAP chunk type where appropriate?

> +	}
>  
>  	/* type filter */
>  	if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
> @@ -4095,6 +4098,113 @@ static bool should_balance_chunk(struct extent_buffer *leaf, struct btrfs_chunk
>  	return true;
>  }
>  
> +struct remap_chunk_info {
> +	struct list_head list;
> +	u64 offset;
> +	struct btrfs_block_group *bg;
> +	bool made_ro;
> +};
> +
> +static int cow_remap_tree(struct btrfs_trans_handle *trans,
> +			  struct btrfs_path *path)
> +{
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	struct btrfs_key key = { 0 };
> +	int ret;
> +
> +	ret = btrfs_search_slot(trans, fs_info->remap_root, &key, path, 0, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	while (true) {
> +		ret = btrfs_next_leaf(fs_info->remap_root, path);
> +		if (ret < 0) {
> +			return ret;
> +		} else if (ret > 0) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +
> +		btrfs_release_path(path);
> +
> +		ret = btrfs_search_slot(trans, fs_info->remap_root, &key, path,
> +					0, 1);
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int balance_remap_chunks(struct btrfs_fs_info *fs_info,
> +				struct btrfs_path *path,
> +				struct list_head *chunks)
> +{
> +	struct remap_chunk_info *rci, *tmp;
> +	struct btrfs_trans_handle *trans;
> +	int ret;
> +
> +	list_for_each_entry_safe(rci, tmp, chunks, list) {
> +		rci->bg = btrfs_lookup_block_group(fs_info, rci->offset);
> +		if (!rci->bg) {
> +			list_del(&rci->list);
> +			kfree(rci);
> +			continue;
> +		}
> +
> +		ret = btrfs_inc_block_group_ro(rci->bg, false);

Just thinking out loud, what happens if we concurrently attempt a
balance that would need to use the remap tree? Is something structurally
blocking that at a higher level? Or will it fail? How will that failure
be handled? Does the answer hold for btrfs-internal background reclaim
rather than explicit balancing?

> +		if (ret)
> +			goto end;
> +
> +		rci->made_ro = true;
> +	}
> +
> +	if (list_empty(chunks))
> +		return 0;
> +
> +	trans = btrfs_start_transaction(fs_info->remap_root, 0);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto end;
> +	}
> +
> +	mutex_lock(&fs_info->remap_mutex);
> +
> +	ret = cow_remap_tree(trans, path);
> +
> +	btrfs_release_path(path);
> +
> +	mutex_unlock(&fs_info->remap_mutex);
> +
> +	btrfs_commit_transaction(trans);
> +
> +end:
> +	while (!list_empty(chunks)) {
> +		bool unused;
> +
> +		rci = list_first_entry(chunks, struct remap_chunk_info, list);
> +
> +		spin_lock(&rci->bg->lock);
> +		unused = !btrfs_is_block_group_used(rci->bg);
> +		spin_unlock(&rci->bg->lock);
> +
> +		if (unused)
> +			btrfs_mark_bg_unused(rci->bg);
> +
> +		if (rci->made_ro)
> +			btrfs_dec_block_group_ro(rci->bg);
> +
> +		btrfs_put_block_group(rci->bg);
> +
> +		list_del(&rci->list);
> +		kfree(rci);
> +	}
> +
> +	return ret;
> +}
> +
>  static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  {
>  	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
> @@ -4117,6 +4227,9 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  	u32 count_meta = 0;
>  	u32 count_sys = 0;
>  	int chunk_reserved = 0;
> +	struct remap_chunk_info *rci;
> +	unsigned int num_remap_chunks = 0;
> +	LIST_HEAD(remap_chunks);
>  
>  	path = btrfs_alloc_path();
>  	if (!path) {
> @@ -4215,7 +4328,8 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  				count_data++;
>  			else if (chunk_type & BTRFS_BLOCK_GROUP_SYSTEM)
>  				count_sys++;
> -			else if (chunk_type & BTRFS_BLOCK_GROUP_METADATA)
> +			else if (chunk_type & (BTRFS_BLOCK_GROUP_METADATA |
> +					       BTRFS_BLOCK_GROUP_REMAP))
>  				count_meta++;
>  
>  			goto loop;
> @@ -4235,6 +4349,30 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  			goto loop;
>  		}
>  
> +		/*
> +		 * Balancing REMAP chunks takes place separately - add the
> +		 * details to a list so it can be processed later.
> +		 */
> +		if (chunk_type & BTRFS_BLOCK_GROUP_REMAP) {
> +			mutex_unlock(&fs_info->reclaim_bgs_lock);
> +
> +			rci = kmalloc(sizeof(struct remap_chunk_info),
> +				      GFP_NOFS);
> +			if (!rci) {
> +				ret = -ENOMEM;
> +				goto error;
> +			}
> +
> +			rci->offset = found_key.offset;
> +			rci->bg = NULL;
> +			rci->made_ro = false;
> +			list_add_tail(&rci->list, &remap_chunks);
> +
> +			num_remap_chunks++;
> +
> +			goto loop;
> +		}
> +
>  		if (!chunk_reserved) {
>  			/*
>  			 * We may be relocating the only data chunk we have,
> @@ -4274,11 +4412,26 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  		key.offset = found_key.offset - 1;
>  	}
>  
> +	btrfs_release_path(path);
> +
>  	if (counting) {
> -		btrfs_release_path(path);
>  		counting = false;
>  		goto again;
>  	}
> +
> +	if (!list_empty(&remap_chunks)) {
> +		ret = balance_remap_chunks(fs_info, path, &remap_chunks);
> +		if (ret == -ENOSPC)
> +			enospc_errors++;
> +
> +		if (!ret) {
> +			btrfs_delete_unused_bgs(fs_info);

Why is this necessary here?

> +
> +			spin_lock(&fs_info->balance_lock);
> +			bctl->stat.completed += num_remap_chunks;
> +			spin_unlock(&fs_info->balance_lock);
> +		}
> +	}
>  error:
>  	btrfs_free_path(path);
>  	if (enospc_errors) {
> -- 
> 2.49.1
>