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

[Mark Harmstone <mark@harmstone.com>]

On 13/06/2025 11.00 pm, Boris Burkov wrote:
> 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.

It's not obvious here, but this is because of start_block_group_remapping(), which is
added in a later patch. remove_block_group_free_space() gets called when a block group
is remapped, or when a non-remapped block group is removed.

>> +	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
>>
>