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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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 ?

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


-- 
Damien Le Moal
Western Digital Research