Re: [PATCH 13/13] xfs: convert xfs_sysfs attrs to use ->seq_show

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Sep 13, 2021 at 07:41:21AM +0200, Christoph Hellwig wrote:
> Trivial conversion to the seq_file based sysfs attributes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_stats.c | 24 +++++-------
>  fs/xfs/xfs_stats.h |  2 +-
>  fs/xfs/xfs_sysfs.c | 96 +++++++++++++++++++++++-----------------------
>  3 files changed, 58 insertions(+), 64 deletions(-)
> 
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index 20e0534a772c9..71e7a84ba0403 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -16,10 +16,9 @@ static int counter_val(struct xfsstats __percpu *stats, int idx)
>  	return val;
>  }
>  
> -int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
> +void xfs_stats_format(struct xfsstats __percpu *stats, struct seq_file *sf)
>  {
>  	int		i, j;
> -	int		len = 0;
>  	uint64_t	xs_xstrat_bytes = 0;
>  	uint64_t	xs_write_bytes = 0;
>  	uint64_t	xs_read_bytes = 0;
> @@ -58,13 +57,12 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  	/* Loop over all stats groups */
>  
>  	for (i = j = 0; i < ARRAY_SIZE(xstats); i++) {
> -		len += scnprintf(buf + len, PATH_MAX - len, "%s",
> -				xstats[i].desc);
> +		seq_printf(sf, "%s", xstats[i].desc);
> +
>  		/* inner loop does each group */
>  		for (; j < xstats[i].endpoint; j++)
> -			len += scnprintf(buf + len, PATH_MAX - len, " %u",
> -					counter_val(stats, j));
> -		len += scnprintf(buf + len, PATH_MAX - len, "\n");
> +			seq_printf(sf, " %u", counter_val(stats, j));
> +		seq_printf(sf, "\n");
>  	}
>  	/* extra precision counters */
>  	for_each_possible_cpu(i) {
> @@ -74,18 +72,14 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  		defer_relog += per_cpu_ptr(stats, i)->s.defer_relog;
>  	}
>  
> -	len += scnprintf(buf + len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
> +	seq_printf(sf, "xpc %Lu %Lu %Lu\n",
>  			xs_xstrat_bytes, xs_write_bytes, xs_read_bytes);
> -	len += scnprintf(buf + len, PATH_MAX-len, "defer_relog %llu\n",
> -			defer_relog);
> -	len += scnprintf(buf + len, PATH_MAX-len, "debug %u\n",
> +	seq_printf(sf, "defer_relog %llu\n", defer_relog);
>  #if defined(DEBUG)
> -		1);
> +	seq_printf(sf, "debug 1\n");
>  #else
> -		0);
> +	seq_printf(sf, "debug 0\n");
>  #endif
> -
> -	return len;
>  }

That is a sysfs file?  What happened to the "one value per file" rule
here?

Ugh.

Anyway, I like the idea, but as you can see here, it could lead to even
more abuse of sysfs files.  We are just now getting people to use
sysfs_emit() and that is showing us where people have been abusing the
api in bad ways.

Is there any way that sysfs can keep the existing show functionality and
just do the seq_printf() for the buffer returned by the attribute file
inside of the sysfs core?  That would allow us to keep all of the
existing attribute file functions as-is, and still get rid of the sysfs
core usage here?

thanks,

greg k-h