Re: [PATCH] btrfs: sysfs: set / query btrfs stripe size

[Stefan Roesch <shr@fb.com>]

On 10/26/21 11:51 PM, Damien Le Moal wrote:
> On 2021/10/27 15:28, Johannes Thumshirn wrote:
>> On 26/10/2021 18:59, Stefan Roesch wrote:
>>
>> [...]
>>
>>> +/*
>>> + * Compute stripe size depending on block type.
>>> + */
>>> +static u64 compute_stripe_size(struct btrfs_fs_info *info, u64 flags)
>>> +{
>>> +	if (flags & BTRFS_BLOCK_GROUP_DATA) {
>>> +		return SZ_1G;
>>> +	} else if (flags & BTRFS_BLOCK_GROUP_METADATA) {
>>> +		/* For larger filesystems, use larger metadata chunks */
>>> +		return info->fs_devices->total_rw_bytes > 50ULL * SZ_1G
>>> +			? 5ULL * SZ_1G
>>> +			: SZ_256M;
>>> +	} else if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>>> +		return SZ_32M;
>>> +	}
>>> +
>>> +	BUG();
>>> +}
>>> +
>>> +/*
>>> + * Compute chunk size depending on block type and stripe size.
>>> + */
>>> +static u64 compute_chunk_size(u64 flags, u64 max_stripe_size)
>>> +{
>>> +	if (flags & BTRFS_BLOCK_GROUP_DATA)
>>> +		return BTRFS_MAX_DATA_CHUNK_SIZE;
>>> +	else if (flags & BTRFS_BLOCK_GROUP_METADATA)
>>> +		return max_stripe_size;
>>> +	else if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
>>> +		return 2 * max_stripe_size;
>>> +
>>> +	BUG();
>>> +}
>>> +
>>> +/*
>>> + * Update maximum stripe size and chunk size.
>>> + *
>>> + */
>>> +void btrfs_update_space_info_max_alloc_sizes(struct btrfs_space_info *space_info,
>>> +					     u64 flags, u64 max_stripe_size)
>>> +{
>>> +	spin_lock(&space_info->lock);
>>> +	space_info->max_stripe_size = max_stripe_size;
>>> +	space_info->max_chunk_size = compute_chunk_size(flags,
>>> +						space_info->max_stripe_size);
>>> +	spin_unlock(&space_info->lock);
>>> +}
>>> +
>>>  static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>>>  {
>>>  
>>> @@ -203,6 +251,10 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>>>  	INIT_LIST_HEAD(&space_info->priority_tickets);
>>>  	space_info->clamp = 1;
>>>  
>>> +	space_info->max_stripe_size = compute_stripe_size(info, flags);
>>> +	space_info->max_chunk_size = compute_chunk_size(flags,
>>> +						space_info->max_stripe_size);
>>> +
>>>  	ret = btrfs_sysfs_add_space_info_type(info, space_info);
>>>  	if (ret)
>>>  		return ret;
>>
>> [...]
>>
>>   
>>> +/*
>>> + * Return space info stripe size.
>>> + */
>>> +static ssize_t btrfs_stripe_size_show(struct kobject *kobj,
>>> +				      struct kobj_attribute *a, char *buf)
>>> +{
>>> +	struct btrfs_space_info *sinfo = to_space_info(kobj);
>>> +
>>> +	return btrfs_show_u64(&sinfo->max_stripe_size, &sinfo->lock, buf);
>>> +}
>>
>>
>> This will return the wrong values for a fs on a zoned device.
>> What you could do is:
>>
>> static ssize_t btrfs_stripe_size_show(struct kobject *kobj,
>> 				struct kobj_attribute *a, char *buf)
>>
>> {
>> 	struct btrfs_space_info *sinfo = to_space_info(kobj);
>> 	struct btrfs_fs_info *fs_info = to_fs_info(get_btrfs_kobj(kobj));
>> 	u64 max_stripe_size;
>>
>> 	spin_lock(&sinfo->lock);
>> 	if (btrfs_is_zoned(fs_info))
>> 		max_stripe_size = fs_info->zone_size;
>> 	else
>> 		max_stripe_size = sinfo->max_stripe_size;
>> 	spin_unlock(&sinfo->lock);
> 
> This will not work once we have stripped zoned volume though, won't it ?
> Why is not max_stripe_size set to zone size for a simple zoned btrfs volume ?
> 

My intention was to not support zoned volumes with this patch. However I missed 
the correct check in the function btrfs_stripe_size_show. The intent was to return
-EINVAL for zoned volumes. 

Any thoughts?

>>
>> 	return btrfs_show_u64(&max_stripe_size, NULL, buf);
>> }
>>
>> [...]
>>
>>> +	if (fs_info->fs_devices->chunk_alloc_policy == BTRFS_CHUNK_ALLOC_ZONED)
>>> +		return -EINVAL;
>>
>> Nit: As we can't mix zoned and non-zoned devices 'if (btrfs_is_zoned(fs_info))'
>> should do the trick here as well and is way more readable. 
>>
>>
> 
>