Re: [PATCH v1 2/3] btrfs: expose chunk size in sysfs.

[David Sterba <dsterba@suse.cz>]

On Tue, Feb 08, 2022 at 11:31:21AM -0800, Stefan Roesch wrote:
> This adds a new sysfs entry in /sys/fs/btrfs/<uuid>/allocation/<block
> type>/chunk_size. This allows to query the chunk size and also set the
> chunk size.
> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> ---
>  fs/btrfs/sysfs.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index beb7f72d50b8..ca337117f15b 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -21,6 +21,7 @@
>  #include "space-info.h"
>  #include "block-group.h"
>  #include "qgroup.h"
> +#include "misc.h"
>  
>  /*
>   * Structure name                       Path
> @@ -92,6 +93,7 @@ static struct btrfs_feature_attr btrfs_attr_features_##_name = {	     \
>  
>  static inline struct btrfs_fs_info *to_fs_info(struct kobject *kobj);
>  static inline struct btrfs_fs_devices *to_fs_devs(struct kobject *kobj);
> +static inline struct kobject *get_btrfs_kobj(struct kobject *kobj);
>  
>  static struct btrfs_feature_attr *to_btrfs_feature_attr(struct kobj_attribute *a)
>  {
> @@ -708,6 +710,81 @@ static ssize_t btrfs_space_info_show_##field(struct kobject *kobj,	\
>  }									\
>  BTRFS_ATTR(space_info, field, btrfs_space_info_show_##field)
>  
> +/*
> + * Return space info chunk size.
> + */

Comment not necessary.

> +static ssize_t btrfs_chunk_size_show(struct kobject *kobj,
> +				     struct kobj_attribute *a, char *buf)
> +{
> +	struct btrfs_space_info *sinfo = to_space_info(kobj);
> +
> +	return sysfs_emit(buf, "%llu\n", atomic64_read(&sinfo->chunk_size));
> +}
> +
> +/*
> + * Store new user supplied chunk size in space info.
> + *
> + * Note: If the new chunk size value is larger than 10% of free space it is
> + *       reduced to match that limit.
> + */
> +static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
> +				      struct kobj_attribute *a,
> +				      const char *buf, size_t len)
> +{
> +	struct btrfs_space_info *space_info = to_space_info(kobj);
> +	struct btrfs_fs_info *fs_info = to_fs_info(get_btrfs_kobj(kobj));
> +	u64 val;
> +	int ret;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (!fs_info) {
> +		pr_err("couldn't get fs_info\n");

What's the reason for such error message? I would end up in the system
log without any context so it does not bring any value to user nor does
it tell what wen wrong.

Checking other functions, there's no such check in most of them and I
don't think it's needed, the filesystem can't be unmounted with open
file references to sysfs.

> +		return -EPERM;
> +	}
> +
> +	if (sb_rdonly(fs_info->sb))
> +		return -EROFS;
> +
> +	if (!fs_info->fs_devices)
> +		return -EINVAL;
> +
> +	if (btrfs_is_zoned(fs_info))
> +		return -EINVAL;
> +
> +	if (!space_info) {
> +		btrfs_err(fs_info, "couldn't get space_info\n");

Same as the error message above.

Also I'm not sure there could be a NULL space_info, it's one of the core
data structures used all the time.

> +		return -EPERM;
> +	}
> +
> +	/* System block type must not be changed. */
> +	if (space_info->flags & BTRFS_BLOCK_GROUP_SYSTEM)
> +		return -EINVAL;

This would probably be EPERM, EINVAL is for invalid values (like out of
range).

> +
> +	ret = kstrtoull(buf, 10, &val);

As this is supposed to be used by humans, it's better to use memparse
that also translates the K/M/G suffix so it's easy to write '256M' etc.

> +	if (ret)
> +		return ret;
> +
> +	val = min(val, BTRFS_MAX_DATA_CHUNK_SIZE);
> +
> +	/*
> +	 * Limit stripe size to 10% of available space.
> +	 */
> +	val = min(div_factor(fs_info->fs_devices->total_rw_bytes, 1), val);
> +
> +	/* Must be multiple of 256M. */
> +	val &= ~(SZ_256M - 1);

Where does this requirement come from?

> +
> +	/* Must be at least 256M. */
> +	if (val < SZ_256M)
> +		return -EINVAL;
> +
> +	btrfs_update_space_info_chunk_size(space_info, val);
> +
> +	return val;
> +}
> +
>  SPACE_INFO_ATTR(flags);
>  SPACE_INFO_ATTR(total_bytes);
>  SPACE_INFO_ATTR(bytes_used);
> @@ -718,6 +795,8 @@ SPACE_INFO_ATTR(bytes_readonly);
>  SPACE_INFO_ATTR(bytes_zone_unusable);
>  SPACE_INFO_ATTR(disk_used);
>  SPACE_INFO_ATTR(disk_total);
> +BTRFS_ATTR_RW(space_info, chunk_size, btrfs_chunk_size_show,
> +	      btrfs_chunk_size_store);
>  
>  /*
>   * Allocation information about block group types.
> @@ -735,6 +814,7 @@ static struct attribute *space_info_attrs[] = {
>  	BTRFS_ATTR_PTR(space_info, bytes_zone_unusable),
>  	BTRFS_ATTR_PTR(space_info, disk_used),
>  	BTRFS_ATTR_PTR(space_info, disk_total),
> +	BTRFS_ATTR_PTR(space_info, chunk_size),
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(space_info);
> @@ -1099,6 +1179,20 @@ static inline struct btrfs_fs_info *to_fs_info(struct kobject *kobj)
>  	return to_fs_devs(kobj)->fs_info;
>  }
>  
> +/*
> + * Get btrfs sysfs kobject.
> + */
> +static inline struct kobject *get_btrfs_kobj(struct kobject *kobj)
> +{
> +	while (kobj) {
> +		if (kobj->ktype == &btrfs_ktype)
> +			return kobj;
> +		kobj = kobj->parent;
> +	}
> +
> +	return NULL;
> +}

There's the to_fs_info helper to get fs_info from any kobj, why is
get_btrfs_kobj needed to traverse back?