Re: [PATCH v2 08/13] btrfs: introduce btrfs_space_info sub-group

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

On Fri Mar 21, 2025 at 1:11 AM JST, Johannes Thumshirn wrote:
> On 19.03.25 07:17, Naohiro Aota wrote:
>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>> index 56c3aa0e7fe2..f5f0485d37b6 100644
>> --- a/fs/btrfs/block-group.c
>> +++ b/fs/btrfs/block-group.c
>> @@ -4537,6 +4537,12 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info)
>>   					struct btrfs_space_info,
>>   					list);
>>   
>> +		for (int i = 0; i < BTRFS_SPACE_INFO_SUB_GROUP_MAX; i++) {
>> +			if (space_info->sub_group[i]) {
>> +				check_removing_space_info(space_info->sub_group[i]);
>> +				kfree(space_info->sub_group[i]);
>> +			}
>> +		}
>>   		check_removing_space_info(space_info);
>
> you could move the loop into check_removing_space_info(). As long as 
> sub_groups don't have sub_groups the recursion won't be too bad for the 
> kernel's stack constraints.
>
>>   		list_del(&space_info->list);
>>   		btrfs_sysfs_remove_space_info(space_info);
>> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
>> index c421161f4237..53eea3cd2bf3 100644
>> --- a/fs/btrfs/space-info.c
>> +++ b/fs/btrfs/space-info.c
>> @@ -249,6 +249,7 @@ static void init_space_info(struct btrfs_fs_info *info,
>>   	INIT_LIST_HEAD(&space_info->priority_tickets);
>>   	space_info->clamp = 1;
>>   	btrfs_update_space_info_chunk_size(space_info, calc_chunk_size(info, flags));
>> +	space_info->subgroup_id = SUB_GROUP_PRIMARY;
>>   
>>   	if (btrfs_is_zoned(info))
>>   		space_info->bg_reclaim_threshold = BTRFS_DEFAULT_ZONED_RECLAIM_THRESH;
>> @@ -266,6 +267,22 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>>   
>>   	init_space_info(info, space_info, flags);
>>   
>> +	if (btrfs_is_zoned(info)) {
>> +		if (flags & BTRFS_BLOCK_GROUP_DATA) {
>> +			struct btrfs_space_info *reloc = kzalloc(sizeof(*reloc), GFP_NOFS);
>> +
>> +			if (!reloc)
>> +				return -ENOMEM;
>
> I'd prefer:
>
> 			struct btrfs_space_info *reloc;
>
> 			reloc = kzalloc(sizeof(*reloc), GFP_NOFS);
> 			if (!reloc)
> 				return -ENOMEM;
>
> Also maybe add as the next patch also adds a case for METADATA, maybe 
> factor that out into:
>
> 	if (btrfs_is_zoned(info)) {
> 		ret = btrfs_create_zone_sub_space_info(info);
> 		/* or create_sub_info_for_zoned? */
> 		if (ret)
> 			return ret;
> 	}

Indeed. I'll factor out the code into a function.

>
>> +			init_space_info(info, reloc, flags);
>> +			space_info->sub_group[SUB_GROUP_DATA_RELOC] = reloc;
>> +			reloc->parent = space_info;
>> +			reloc->subgroup_id = SUB_GROUP_DATA_RELOC;
>> +
>> +			ret = btrfs_sysfs_add_space_info_type(info, reloc);
>> +			ASSERT(!ret);
>> +		}
>> +	}
>> +