Re: [PATCH v2 07/13] btrfs: pass space_info for block group creation

[Naohiro Aota <Naohiro.Aota@wdc.com>]

On Thu Mar 20, 2025 at 9:21 PM JST, Johannes Thumshirn wrote:
> On 19.03.25 07:17, Naohiro Aota wrote:
>> Add btrfs_space_info parameter to btrfs_make_block_group(), its related
>> functions and related struct. Passed space_info will have a new block
>> group. If NULL is passed, it uses the default space_info.
>> 
>> The parameter is used in a later commit and the behavior is unchanged now.
>> 
>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>> ---
>>   fs/btrfs/block-group.c | 15 ++++++++-------
>>   fs/btrfs/block-group.h |  2 +-
>>   fs/btrfs/volumes.c     | 16 +++++++++++-----
>>   fs/btrfs/volumes.h     |  2 +-
>>   4 files changed, 21 insertions(+), 14 deletions(-)
>> 
>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>> index fa08d7b67b1f..56c3aa0e7fe2 100644
>> --- a/fs/btrfs/block-group.c
>> +++ b/fs/btrfs/block-group.c
>> @@ -2868,7 +2868,7 @@ static u64 calculate_global_root_id(const struct btrfs_fs_info *fs_info, u64 off
>>   }
>>   
>>   struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *trans,
>> -						 u64 type,
>> +						 struct btrfs_space_info *space_info, u64 type,
>>   						 u64 chunk_offset, u64 size)
>>   {
>
> Please move u64 type to the next line.
>
> [...]
>
>> -static struct btrfs_block_group *do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
>> +static struct btrfs_block_group *do_chunk_alloc(struct btrfs_trans_handle *trans,
>> +						struct btrfs_space_info *space_info, u64 flags)
>
>
> Same for flags here (or shift struct btrfs_space_info to the left).
>
>
>> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
>> index c01f3af726a1..cb9b0405172c 100644
>> --- a/fs/btrfs/block-group.h
>> +++ b/fs/btrfs/block-group.h
>> @@ -326,7 +326,7 @@ void btrfs_reclaim_bgs(struct btrfs_fs_info *fs_info);
>>   void btrfs_mark_bg_to_reclaim(struct btrfs_block_group *bg);
>>   int btrfs_read_block_groups(struct btrfs_fs_info *info);
>>   struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *trans,
>> -						 u64 type,
>> +						 struct btrfs_space_info *space_info, u64 type,
>
> Same here.
>
>> @@ -5618,7 +5620,7 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
>>   		return ERR_PTR(ret);
>>   	}
>>   
>> -	block_group = btrfs_make_block_group(trans, type, start, ctl->chunk_size);
>> +	block_group = btrfs_make_block_group(trans, ctl->space_info, type, start, ctl->chunk_size);
>
> ctl->chunk_size is at 99 here, please move it to a new line as well.
>
>>   	if (IS_ERR(block_group)) {
>>   		btrfs_remove_chunk_map(info, map);
>>   		return block_group;
>> @@ -5644,7 +5646,7 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
>>   }
>>   
>>   struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
>> -					    u64 type)
>> +					     struct btrfs_space_info *space_info, u64 type)
>
> Same for type. space_info is already over 80, so type should go to a new 
> line.
>

Thanks, I will update these.

>>   {
>>   	struct btrfs_fs_info *info = trans->fs_info;
>>   	struct btrfs_fs_devices *fs_devices = info->fs_devices;
>> @@ -5672,8 +5674,12 @@ struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> +	if (!space_info)
>> +		space_info = btrfs_find_space_info(info, type);
>> +	ASSERT(space_info);
>
> space_info == NULL should never happen, so an ASSERT() is justified. But 
> can we make a graceful return in case CONFIG_BTRFS_ASSERT=n? Like with 
> the BTRFS_BLOCK_GROUP_TYPE_MASK check above:
>
> 	if (!space_info) {
> 		ASSERT(0);
> 		return ERR_PTR(-EINVAL);
> 	}

Apparently, space_info == NULL is going to be checked with ASSERT() in
btrfs_make_block_group() too. But, anyway, adding proper error handling
would be better here. I'll do that.