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

[Mark Harmstone <mark@harmstone.com>]

On 16/08/2025 2.02 am, Boris Burkov wrote:
> 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?

This would imply adding a new flag to btrfs balance start, and a new
version of the ioctl, and I'm not sure it's worth it. Happy to argue
the toss though.

Doing btrfs balance start -m already implies -s, so it's not much of
a stretch for to cover REMAP as well.

Possibly it would make more sense for REMAP to be SYSTEM for balancing
purposes rather than METADATA.

>> +	}
>>   
>>   	/* 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
>>