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

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

[David Sterba]

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.

In case the stats are not yet valid, print just 'invalid' as the file
contents.

Signed-off-by: David Sterba <dsterba@suse.com>
---

v2:
- print 'invalid' separtely and not among the values
- rename file name to 'error_stats' to leave 'stats' available for any
  other kind of stats we'd like in the future

 fs/btrfs/sysfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 4b508938e728..ebde1d09e686 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1495,7 +1495,36 @@ static ssize_t btrfs_devinfo_writeable_show(struct kobject *kobj,
 }
 BTRFS_ATTR(devid, writeable, btrfs_devinfo_writeable_show);
 
+static ssize_t btrfs_devinfo_error_stats_show(struct kobject *kobj,
+		struct kobj_attribute *a, char *buf)
+{
+	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
+						   devid_kobj);
+
+	if (!device->dev_stats_valid)
+		return scnprintf(buf, PAGE_SIZE, "invalid\n");
+
+	/*
+	 * 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,
+		"write_errs %d\n"
+		"read_errs %d\n"
+		"flush_errs %d\n"
+		"corruption_errs %d\n"
+		"generation_errs %d\n",
+		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_WRITE_ERRS),
+		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_READ_ERRS),
+		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_FLUSH_ERRS),
+		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_CORRUPTION_ERRS),
+		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_GENERATION_ERRS));
+}
+BTRFS_ATTR(devid, error_stats, btrfs_devinfo_error_stats_show);
+
 static struct attribute *devid_attrs[] = {
+	BTRFS_ATTR_PTR(devid, error_stats),
 	BTRFS_ATTR_PTR(devid, in_fs_metadata),
 	BTRFS_ATTR_PTR(devid, missing),
 	BTRFS_ATTR_PTR(devid, replace_target),
-- 
2.31.1

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

[Holger Hoffstätte]

On 2021-06-11 15:36, 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
                                    ^error_stats :)

Otherwise a very welcome improvement, thank you!

cheers
Holger

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

[Anand Jain]

> 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.
> 
> In case the stats are not yet valid, print just 'invalid' as the file
> contents.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> 
> v2:
> - print 'invalid' separtely and not among the values
> - rename file name to 'error_stats' to leave 'stats' available for any
>    other kind of stats we'd like in the future
> 
>   fs/btrfs/sysfs.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 4b508938e728..ebde1d09e686 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1495,7 +1495,36 @@ static ssize_t btrfs_devinfo_writeable_show(struct kobject *kobj,
>   }
>   BTRFS_ATTR(devid, writeable, btrfs_devinfo_writeable_show);
>   
> +static ssize_t btrfs_devinfo_error_stats_show(struct kobject *kobj,
> +		struct kobj_attribute *a, char *buf)
> +{
> +	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
> +						   devid_kobj);
> +
> +	if (!device->dev_stats_valid)
> +		return scnprintf(buf, PAGE_SIZE, "invalid\n");

and

Nit:
Instead of invalid, IMO, not yet valid is closer to what this error 
implies. And matches with the ioctl Warning message.

7743         btrfs_warn(fs_info, "get dev_stats failed, not yet valid");


> +
> +	/*
> +	 * 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,
> +		"write_errs %d\n"
> +		"read_errs %d\n"
> +		"flush_errs %d\n"
> +		"corruption_errs %d\n"
> +		"generation_errs %d\n",
> +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_WRITE_ERRS),
> +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_READ_ERRS),
> +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_FLUSH_ERRS),
> +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_CORRUPTION_ERRS),
> +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_GENERATION_ERRS));
> +}
> +BTRFS_ATTR(devid, error_stats, btrfs_devinfo_error_stats_show);
> +
>   static struct attribute *devid_attrs[] = {
> +	BTRFS_ATTR_PTR(devid, error_stats),
>   	BTRFS_ATTR_PTR(devid, in_fs_metadata),
>   	BTRFS_ATTR_PTR(devid, missing),
>   	BTRFS_ATTR_PTR(devid, replace_target),
> 


write_errs 0
read_errs 0
flush_errs 0
corruption_errs 1
generation_errs 0


Another option was to print all errors in a single line. A single line
will help continuous monitoring of the error_stats. But it is not a big
deal.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

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

[David Sterba]

On Mon, Jun 14, 2021 at 09:00:50PM +0800, Anand Jain wrote:
> 
> > 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.
> > 
> > In case the stats are not yet valid, print just 'invalid' as the file
> > contents.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> > 
> > v2:
> > - print 'invalid' separtely and not among the values
> > - rename file name to 'error_stats' to leave 'stats' available for any
> >    other kind of stats we'd like in the future
> > 
> >   fs/btrfs/sysfs.c | 29 +++++++++++++++++++++++++++++
> >   1 file changed, 29 insertions(+)
> > 
> > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> > index 4b508938e728..ebde1d09e686 100644
> > --- a/fs/btrfs/sysfs.c
> > +++ b/fs/btrfs/sysfs.c
> > @@ -1495,7 +1495,36 @@ static ssize_t btrfs_devinfo_writeable_show(struct kobject *kobj,
> >   }
> >   BTRFS_ATTR(devid, writeable, btrfs_devinfo_writeable_show);
> >   
> > +static ssize_t btrfs_devinfo_error_stats_show(struct kobject *kobj,
> > +		struct kobj_attribute *a, char *buf)
> > +{
> > +	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
> > +						   devid_kobj);
> > +
> > +	if (!device->dev_stats_valid)
> > +		return scnprintf(buf, PAGE_SIZE, "invalid\n");
> 
> and
> 
> Nit:
> Instead of invalid, IMO, not yet valid is closer to what this error 
> implies. And matches with the ioctl Warning message.
> 
> 7743         btrfs_warn(fs_info, "get dev_stats failed, not yet valid");

Yeah I was not sure about that, 'invalid' does not have the same
meaning.  I'd like something that's short and easily parseable so no
long sentences or multi-word value like 'not yet valid'.

It's trivial to change if somebody has a better idea or convince me with
some variation on not-yet-valid.

> > +
> > +	/*
> > +	 * 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,
> > +		"write_errs %d\n"
> > +		"read_errs %d\n"
> > +		"flush_errs %d\n"
> > +		"corruption_errs %d\n"
> > +		"generation_errs %d\n",
> > +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_WRITE_ERRS),
> > +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_READ_ERRS),
> > +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_FLUSH_ERRS),
> > +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_CORRUPTION_ERRS),
> > +		btrfs_dev_stat_read(device, BTRFS_DEV_STAT_GENERATION_ERRS));
> > +}
> > +BTRFS_ATTR(devid, error_stats, btrfs_devinfo_error_stats_show);
> > +
> >   static struct attribute *devid_attrs[] = {
> > +	BTRFS_ATTR_PTR(devid, error_stats),
> >   	BTRFS_ATTR_PTR(devid, in_fs_metadata),
> >   	BTRFS_ATTR_PTR(devid, missing),
> >   	BTRFS_ATTR_PTR(devid, replace_target),
> > 
> 
> 
> write_errs 0
> read_errs 0
> flush_errs 0
> corruption_errs 1
> generation_errs 0
> 
> 
> Another option was to print all errors in a single line. A single line
> will help continuous monitoring of the error_stats. But it is not a big
> deal.

I think that's just a matter of transforming the data to the convenient
form. Line based stats values is more common and grep-friendly. For a
monitoring tool/scripts that want it in one line it's perhaps a simple

  cat devinfo/error_sats | tr '\n' ' '

Converting from one line to multiple lines is not that trivial.