Re: [PATCH v4 1/4] btrfs: store chunk size in space-info struct.

[Stefan Roesch <shr@fb.com>]

On 11/5/21 1:52 AM, Nikolay Borisov wrote:
> 
> 
> On 29.10.21 г. 21:39, Stefan Roesch wrote:
>> The chunk size is stored in the btrfs_space_info structure.
>> It is initialized at the start and is then used.
>>
>> Two api's are added to get the current value and one to update
>> the value.
> 
> There is just a single API added to update the size, there is no api to
> get the value, one has to directly read default_chunk_size. Additionally
> if it's going to be changed then does the "default_" prefix really mean
> anything?
>

I changed the name to chunk_size.
 
>>
>> These api's will be used to be able to expose the chunk_size
>> as a sysfs setting.
>>
>> Signed-off-by: Stefan Roesch <shr@fb.com>
>> ---
>>  fs/btrfs/space-info.c | 51 +++++++++++++++++++++++++++++++++++++++++++
>>  fs/btrfs/space-info.h |  3 +++
>>  fs/btrfs/volumes.c    | 28 ++++++++----------------
>>  3 files changed, 63 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
>> index 48d77f360a24..7370c152ce8a 100644
>> --- a/fs/btrfs/space-info.c
>> +++ b/fs/btrfs/space-info.c
>> @@ -181,6 +181,56 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info *info)
>>  		found->full = 0;
>>  }
>>  
>> +/*
>> + * Compute chunk size depending on block type for regular volumes.
>> + */
>> +static u64 compute_chunk_size_regular(struct btrfs_fs_info *info, u64 flags)
>> +{
>> +	ASSERT(flags & BTRFS_BLOCK_GROUP_TYPE_MASK);
>> +
>> +	if (flags & BTRFS_BLOCK_GROUP_DATA)
>> +		return SZ_1G;
>> +	else if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
>> +		return SZ_32M;
>> +
>> +	/* Handle BTRFS_BLOCK_GROUP_METADATA */
>> +	if (info->fs_devices->total_rw_bytes > 50ULL * SZ_1G)
>> +		return SZ_1G;
>> +
>> +	return SZ_256M;
>> +}
>> +
>> +/*
>> + * Compute chunk size for zoned volumes.
>> + */
>> +static u64 compute_chunk_size_zoned(struct btrfs_fs_info *info)
>> +{
>> +	return info->zone_size;
>> +}
> 
> nit: This is trivial and so can simply be open-coded in
> compute_chunk_size, yes it's static and will likely be compiled out but
> it adds a bit of cognitive load when reading the code. In any case I'd
> leave this to David to decide whether to leave the function or not.
> 

I removed the function.

>> +
>> +/*
>> + * Compute chunk size depending on volume type.
>> + */
> 
> <snip>
> 
> 
>>  static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>>  {
>>  
>> @@ -202,6 +252,7 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>>  	INIT_LIST_HEAD(&space_info->tickets);
>>  	INIT_LIST_HEAD(&space_info->priority_tickets);
>>  	space_info->clamp = 1;
>> +	space_info->default_chunk_size = compute_chunk_size(info, flags);
>>  
>>  	ret = btrfs_sysfs_add_space_info_type(info, space_info);
>>  	if (ret)
> 
> <snip>
> 
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 546bf1146b2d..563e5b30060d 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -5063,26 +5063,16 @@ static void init_alloc_chunk_ctl_policy_regular(
>>  				struct btrfs_fs_devices *fs_devices,
>>  				struct alloc_chunk_ctl *ctl)
>>  {
>> -	u64 type = ctl->type;
>> +	struct btrfs_space_info *space_info;
>>  
>> -	if (type & BTRFS_BLOCK_GROUP_DATA) {
>> -		ctl->max_stripe_size = SZ_1G;
>> -		ctl->max_chunk_size = BTRFS_MAX_DATA_CHUNK_SIZE;
>> -	} else if (type & BTRFS_BLOCK_GROUP_METADATA) {
>> -		/* For larger filesystems, use larger metadata chunks */
>> -		if (fs_devices->total_rw_bytes > 50ULL * SZ_1G)
>> -			ctl->max_stripe_size = SZ_1G;
>> -		else
>> -			ctl->max_stripe_size = SZ_256M;
>> -		ctl->max_chunk_size = ctl->max_stripe_size;
>> -	} else if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
>> -		ctl->max_stripe_size = SZ_32M;
>> -		ctl->max_chunk_size = 2 * ctl->max_stripe_size;
>> -		ctl->devs_max = min_t(int, ctl->devs_max,
>> -				      BTRFS_MAX_DEVS_SYS_CHUNK);
>> -	} else {
>> -		BUG();
>> -	}
>> +	space_info = btrfs_find_space_info(fs_devices->fs_info, ctl->type);
>> +	ASSERT(space_info);
>> +
>> +	ctl->max_chunk_size = space_info->default_chunk_size;
>> +	ctl->max_stripe_size = space_info->default_chunk_size;
> 
> Those are racy accesses, no ? Chunk allocation happens under
> chunk_mutex, not the space_info lock ? Perhaps it could be turned into
> an atomic?
> 

Good catch. I replaced it with an atomic.

>> +
>> +	if (ctl->type & BTRFS_BLOCK_GROUP_SYSTEM)
>> +		ctl->devs_max = min_t(int, ctl->devs_max, BTRFS_MAX_DEVS_SYS_CHUNK);
>>  
>>  	/* We don't want a chunk larger than 10% of writable space */
>>  	ctl->max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 1),
>>