Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize

[Anand Jain <anand.jain@oracle.com>]

On 10/03/2021 17:08, Qu Wenruo wrote:
> Add extra sysfs interface features/supported_ro_sectorsize and
> features/supported_rw_sectorsize to indicate subpage support.
> 
> Currently for supported_rw_sectorsize all architectures only have their
> PAGE_SIZE listed.
> 
> While for supported_ro_sectorsize, for systems with 64K page size, 4K
> sectorsize is also supported.
> 
> This new sysfs interface would help mkfs.btrfs to do more accurate
> warning.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---

Changes looks good. Nit below...
And maybe it is a good idea to wait for other comments before reroll.

>   fs/btrfs/sysfs.c | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 6eb1c50fa98c..3ef419899472 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -360,11 +360,45 @@ static ssize_t supported_rescue_options_show(struct kobject *kobj,
>   BTRFS_ATTR(static_feature, supported_rescue_options,
>   	   supported_rescue_options_show);
>   
> +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj,
> +					    struct kobj_attribute *a,
> +					    char *buf)
> +{
> +	ssize_t ret = 0;
> +	int i = 0;

  Drop variable i, as ret can be used instead of 'i'.

> +
> +	/* For 64K page size, 4K sector size is supported */
> +	if (PAGE_SIZE == SZ_64K) {
> +		ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K);
> +		i++;
> +	}
> +	/* Other than above subpage, only support PAGE_SIZE as sectorsize yet */
> +	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n",

> +			 (i ? " " : ""), PAGE_SIZE);
                           ^ret

> +	return ret;
> +}
> +BTRFS_ATTR(static_feature, supported_ro_sectorsize,
> +	   supported_ro_sectorsize_show);
> +
> +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj,
> +					    struct kobj_attribute *a,
> +					    char *buf)
> +{
> +	ssize_t ret = 0;
> +
> +	/* Only PAGE_SIZE as sectorsize is supported */
> +	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE);
> +	return ret;
> +}
> +BTRFS_ATTR(static_feature, supported_rw_sectorsize,
> +	   supported_rw_sectorsize_show);

  Why not merge supported_ro_sectorsize and supported_rw_sectorsize
  and show both in two lines...
  For example:
    cat supported_sectorsizes
    ro: 4096 65536
    rw: 65536



>   static struct attribute *btrfs_supported_static_feature_attrs[] = {
>   	BTRFS_ATTR_PTR(static_feature, rmdir_subvol),
>   	BTRFS_ATTR_PTR(static_feature, supported_checksums),
>   	BTRFS_ATTR_PTR(static_feature, send_stream_version),
>   	BTRFS_ATTR_PTR(static_feature, supported_rescue_options),
> +	BTRFS_ATTR_PTR(static_feature, supported_ro_sectorsize),
> +	BTRFS_ATTR_PTR(static_feature, supported_rw_sectorsize),
>   	NULL
>   };
>   
>