Re: [PATCH] btrfs: sysfs: export dev stats in devinfo directory

[Omar Sandoval <osandov@osandov.com>]

On Wed, Jun 09, 2021 at 08:50:14PM +0200, David Sterba wrote:
> On Wed, Jun 09, 2021 at 11:24:21AM -0700, Omar Sandoval wrote:
> > On Fri, Jun 04, 2021 at 03:20:58PM +0200, David Sterba wrote:
> > > The device stats can be read by ioctl, wrapped by command 'btrfs device
> > > stats'. Provide another source where to read the information in
> > > /sys/fs/btrfs/FSID/devinfo/DEVID/stats . The format is a list of
> > > 'key value' pairs one per line, which is common in other stat files.
> > > The names are the same as used in other device stat outputs.
> > > 
> > > The stats are all in one file as it's the snapshot of all available
> > > stats. The 'one value per file' is not very suitable here. The stats
> > > should be valid right after the stats item is read from disk, shortly
> > > after initializing the device, but in any case also print the validity
> > > status.
> > > 
> > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > ---
> > >  fs/btrfs/sysfs.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> > > index 4b508938e728..3d4c806c4f73 100644
> > > --- a/fs/btrfs/sysfs.c
> > > +++ b/fs/btrfs/sysfs.c
> > > @@ -1495,11 +1495,39 @@ static ssize_t btrfs_devinfo_writeable_show(struct kobject *kobj,
> > >  }
> > >  BTRFS_ATTR(devid, writeable, btrfs_devinfo_writeable_show);
> > >  
> > > +static ssize_t btrfs_devinfo_stats_show(struct kobject *kobj,
> > > +		struct kobj_attribute *a, char *buf)
> > > +{
> > > +	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
> > > +						   devid_kobj);
> > > +
> > > +	/*
> > > +	 * Print all at once so we get a snapshot of all values from the same
> > > +	 * time. Keep them in sync and in order of definition of
> > > +	 * btrfs_dev_stat_values.
> > > +	 */
> > > +	return scnprintf(buf, PAGE_SIZE,
> > > +		"stats_valid %d\n",
> > > +		"write_errs %d\n"
> > > +		"read_errs %d\n"
> > > +		"flush_errs %d\n"
> > > +		"corruption_errs %d\n"
> > > +		"generation_errs %d\n",
> > > +		!!(device->dev_stats_valid),
> > 
> > The ioctl returns ENODEV is !dev_stats_valid, maybe this file should do
> > the same? It seems a little awkward to have a flag that means that the
> > rest of the file is meaningless.
> 
> You mean returning -ENODEV when reading the stats file? Or return 0 but
> the contents is something like 'stats invalid' or similar.

I'd vote for returning -ENODEV when reading the stats file, but I think
either one is fine.