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

[PATCH] 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, 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),
+		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, stats, btrfs_devinfo_stats_show);
+
 static struct attribute *devid_attrs[] = {
 	BTRFS_ATTR_PTR(devid, in_fs_metadata),
 	BTRFS_ATTR_PTR(devid, missing),
 	BTRFS_ATTR_PTR(devid, replace_target),
 	BTRFS_ATTR_PTR(devid, scrub_speed_max),
+	BTRFS_ATTR_PTR(devid, stats),
 	BTRFS_ATTR_PTR(devid, writeable),
 	NULL
 };
-- 
2.31.1

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

[David Sterba]

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",
                                  ^
should not be here, uncommited version sent.

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

[Anand Jain]

On 4/6/21 9:20 pm, 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 planned stat here is errors stat.
  So why not rename this to error_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.

  Nice!

Thanks, Anand

> 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),
> +		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, stats, btrfs_devinfo_stats_show);
> +
>   static struct attribute *devid_attrs[] = {
>   	BTRFS_ATTR_PTR(devid, in_fs_metadata),
>   	BTRFS_ATTR_PTR(devid, missing),
>   	BTRFS_ATTR_PTR(devid, replace_target),
>   	BTRFS_ATTR_PTR(devid, scrub_speed_max),
> +	BTRFS_ATTR_PTR(devid, stats),
>   	BTRFS_ATTR_PTR(devid, writeable),
>   	NULL
>   };
> 

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

[David Sterba]

On Fri, Jun 04, 2021 at 09:41:09PM +0800, Anand Jain wrote:
> On 4/6/21 9:20 pm, 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 planned stat here is errors stat.
>   So why not rename this to error_stats?

I think it's commonly called device stats, dev stats, so when it's in
'devinfo' it's like it's the 'stats' for the device. We don't have other
stats, like regarding io but in that case it would make sense to
distnguish the names.

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

[Anand Jain]

On 04/06/2021 22:21, David Sterba wrote:
> On Fri, Jun 04, 2021 at 09:41:09PM +0800, Anand Jain wrote:
>> On 4/6/21 9:20 pm, 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 planned stat here is errors stat.
>>    So why not rename this to error_stats?
> 
> I think it's commonly called device stats, dev stats, so when it's in
> 'devinfo' it's like it's the 'stats' for the device.


> We don't have other
> stats, like regarding io but in that case it would make sense to
> distnguish the names.

My read_policy work (which I suppose is next on your list for review) 
made sense that publishing the io-stat information locally from btrfs is 
a good idea. So that it provides clarity if the IO is skewed to a device 
or balanced. Which is even more essential in the case of mixed device 
types. For now IMHO,  /sys/fs/btrfs/FSID/devinfo/DEVID/error_stats
is harmless.

Thanks, Anand

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

[David Sterba]

On Sat, Jun 05, 2021 at 06:38:16AM +0800, Anand Jain wrote:
> On 04/06/2021 22:21, David Sterba wrote:
> > On Fri, Jun 04, 2021 at 09:41:09PM +0800, Anand Jain wrote:
> >> On 4/6/21 9:20 pm, 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 planned stat here is errors stat.
> >>    So why not rename this to error_stats?
> > 
> > I think it's commonly called device stats, dev stats, so when it's in
> > 'devinfo' it's like it's the 'stats' for the device.
> 
> 
> > We don't have other
> > stats, like regarding io but in that case it would make sense to
> > distnguish the names.
> 
> My read_policy work (which I suppose is next on your list for review) 

Yeah, it's among the next things to merge once the current features
stabilize enough.

> made sense that publishing the io-stat information locally from btrfs is 
> a good idea. So that it provides clarity if the IO is skewed to a device 
> or balanced. Which is even more essential in the case of mixed device 
> types. For now IMHO,  /sys/fs/btrfs/FSID/devinfo/DEVID/error_stats
> is harmless.

Agreed, I thought about the same, gathering some regular io stats, so
the error_stats makes sense. There's still one open question whether to
do it all in one file or in a subdirectory error_stats/ . The sysfs way
is one value per file but for the stats I'm more inclined to follow what
/proc/ stats do. It's more convenient to monitor stats in one file read
than having to do 'cat error_stats/*' or with filenames as 'grep ^
error_stats/*'.

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

[Anand Jain]

On 8/6/21 2:55 am, David Sterba wrote:
> On Sat, Jun 05, 2021 at 06:38:16AM +0800, Anand Jain wrote:
>> On 04/06/2021 22:21, David Sterba wrote:
>>> On Fri, Jun 04, 2021 at 09:41:09PM +0800, Anand Jain wrote:
>>>> On 4/6/21 9:20 pm, 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 planned stat here is errors stat.
>>>>     So why not rename this to error_stats?
>>>
>>> I think it's commonly called device stats, dev stats, so when it's in
>>> 'devinfo' it's like it's the 'stats' for the device.
>>
>>
>>> We don't have other
>>> stats, like regarding io but in that case it would make sense to
>>> distnguish the names.
>>
>> My read_policy work (which I suppose is next on your list for review)
> 
> Yeah, it's among the next things to merge once the current features
> stabilize enough.
> 
>> made sense that publishing the io-stat information locally from btrfs is
>> a good idea. So that it provides clarity if the IO is skewed to a device
>> or balanced. Which is even more essential in the case of mixed device
>> types. For now IMHO,  /sys/fs/btrfs/FSID/devinfo/DEVID/error_stats
>> is harmless.
> 
> Agreed, I thought about the same, gathering some regular io stats, so
> the error_stats makes sense.  > There's still one open question whether to
> do it all in one file or in a subdirectory error_stats/ . > The sysfs way
> is one value per file but for the stats


> I'm more inclined to follow what
> /proc/ stats do. It's more convenient to monitor stats in one file read
> than having to do 'cat error_stats/*' or with filenames as 'grep ^
> error_stats/*'.

  Agreed. I prefer one file from the convenience pov. Also, block dev has
  one file, the reason to represent it invariably at a given time [1]
    [1] https://www.kernel.org/doc/Documentation/block/stat.txt
  IMO the same applies to btrfs too.

Thanks, Anand

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

[David Sterba]

On Wed, Jun 09, 2021 at 03:43:44PM +0800, Anand Jain wrote:
> > I'm more inclined to follow what
> > /proc/ stats do. It's more convenient to monitor stats in one file read
> > than having to do 'cat error_stats/*' or with filenames as 'grep ^
> > error_stats/*'.
> 
>   Agreed. I prefer one file from the convenience pov. Also, block dev has
>   one file, the reason to represent it invariably at a given time [1]
>     [1] https://www.kernel.org/doc/Documentation/block/stat.txt
>   IMO the same applies to btrfs too.

Ah nice, same reason for us to use the file then. I'd argue that having
the names of the values is more convenient than just an array of raw
numbers though.

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]

Hi David,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20210604]
[cannot apply to v5.13-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/David-Sterba/btrfs-sysfs-export-dev-stats-in-devinfo-directory/20210604-212445
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: riscv-randconfig-s032-20210604 (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/8a58ea51305ace4835c7abc51e46b7b64e25b793
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review David-Sterba/btrfs-sysfs-export-dev-stats-in-devinfo-directory/20210604-212445
        git checkout 8a58ea51305ace4835c7abc51e46b7b64e25b793
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/btrfs/sysfs.c: In function 'btrfs_devinfo_stats_show':
>> fs/btrfs/sysfs.c:1510:17: warning: format '%d' expects argument of type 'int', but argument 4 has type 'char *' [-Wformat=]
    1510 |   "stats_valid %d\n",
         |                ~^
         |                 |
         |                 int
         |                %s
    1511 |   "write_errs %d\n"
         |   ~~~~~~~~~~~~~~~~~
         |   |
         |   char *
>> fs/btrfs/sysfs.c:1510:3: warning: too many arguments for format [-Wformat-extra-args]
    1510 |   "stats_valid %d\n",
         |   ^~~~~~~~~~~~~~~~~~


vim +1510 fs/btrfs/sysfs.c

  1497	
  1498	static ssize_t btrfs_devinfo_stats_show(struct kobject *kobj,
  1499			struct kobj_attribute *a, char *buf)
  1500	{
  1501		struct btrfs_device *device = container_of(kobj, struct btrfs_device,
  1502							   devid_kobj);
  1503	
  1504		/*
  1505		 * Print all at once so we get a snapshot of all values from the same
  1506		 * time. Keep them in sync and in order of definition of
  1507		 * btrfs_dev_stat_values.
  1508		 */
  1509		return scnprintf(buf, PAGE_SIZE,
> 1510			"stats_valid %d\n",
  1511			"write_errs %d\n"
  1512			"read_errs %d\n"
  1513			"flush_errs %d\n"
  1514			"corruption_errs %d\n"
  1515			"generation_errs %d\n",
  1516			!!(device->dev_stats_valid),
  1517			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_WRITE_ERRS),
  1518			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_READ_ERRS),
  1519			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_FLUSH_ERRS),
  1520			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_CORRUPTION_ERRS),
  1521			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_GENERATION_ERRS));
  1522	}
  1523	BTRFS_ATTR(devid, stats, btrfs_devinfo_stats_show);
  1524	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38658 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3524 bytes --]

Hi David,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20210604]
[cannot apply to v5.13-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/David-Sterba/btrfs-sysfs-export-dev-stats-in-devinfo-directory/20210604-212445
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: riscv-randconfig-s032-20210604 (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/8a58ea51305ace4835c7abc51e46b7b64e25b793
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review David-Sterba/btrfs-sysfs-export-dev-stats-in-devinfo-directory/20210604-212445
        git checkout 8a58ea51305ace4835c7abc51e46b7b64e25b793
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/btrfs/sysfs.c: In function 'btrfs_devinfo_stats_show':
>> fs/btrfs/sysfs.c:1510:17: warning: format '%d' expects argument of type 'int', but argument 4 has type 'char *' [-Wformat=]
    1510 |   "stats_valid %d\n",
         |                ~^
         |                 |
         |                 int
         |                %s
    1511 |   "write_errs %d\n"
         |   ~~~~~~~~~~~~~~~~~
         |   |
         |   char *
>> fs/btrfs/sysfs.c:1510:3: warning: too many arguments for format [-Wformat-extra-args]
    1510 |   "stats_valid %d\n",
         |   ^~~~~~~~~~~~~~~~~~


vim +1510 fs/btrfs/sysfs.c

  1497	
  1498	static ssize_t btrfs_devinfo_stats_show(struct kobject *kobj,
  1499			struct kobj_attribute *a, char *buf)
  1500	{
  1501		struct btrfs_device *device = container_of(kobj, struct btrfs_device,
  1502							   devid_kobj);
  1503	
  1504		/*
  1505		 * Print all at once so we get a snapshot of all values from the same
  1506		 * time. Keep them in sync and in order of definition of
  1507		 * btrfs_dev_stat_values.
  1508		 */
  1509		return scnprintf(buf, PAGE_SIZE,
> 1510			"stats_valid %d\n",
  1511			"write_errs %d\n"
  1512			"read_errs %d\n"
  1513			"flush_errs %d\n"
  1514			"corruption_errs %d\n"
  1515			"generation_errs %d\n",
  1516			!!(device->dev_stats_valid),
  1517			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_WRITE_ERRS),
  1518			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_READ_ERRS),
  1519			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_FLUSH_ERRS),
  1520			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_CORRUPTION_ERRS),
  1521			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_GENERATION_ERRS));
  1522	}
  1523	BTRFS_ATTR(devid, stats, btrfs_devinfo_stats_show);
  1524	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38658 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3287 bytes --]

Hi David,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20210604]
[cannot apply to v5.13-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/David-Sterba/btrfs-sysfs-export-dev-stats-in-devinfo-directory/20210604-212445
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: arm-randconfig-r004-20210604 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 5c0d1b2f902aa6a9cf47cc7e42c5b83bb2217cf9)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/8a58ea51305ace4835c7abc51e46b7b64e25b793
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review David-Sterba/btrfs-sysfs-export-dev-stats-in-devinfo-directory/20210604-212445
        git checkout 8a58ea51305ace4835c7abc51e46b7b64e25b793
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/btrfs/sysfs.c:1511:3: warning: format specifies type 'int' but the argument has type 'char *' [-Wformat]
                   "write_errs %d\n"
                   ^~~~~~~~~~~~~~~~~
>> fs/btrfs/sysfs.c:1516:3: warning: data argument not used by format string [-Wformat-extra-args]
                   !!(device->dev_stats_valid),
                   ^
   2 warnings generated.


vim +1511 fs/btrfs/sysfs.c

  1497	
  1498	static ssize_t btrfs_devinfo_stats_show(struct kobject *kobj,
  1499			struct kobj_attribute *a, char *buf)
  1500	{
  1501		struct btrfs_device *device = container_of(kobj, struct btrfs_device,
  1502							   devid_kobj);
  1503	
  1504		/*
  1505		 * Print all at once so we get a snapshot of all values from the same
  1506		 * time. Keep them in sync and in order of definition of
  1507		 * btrfs_dev_stat_values.
  1508		 */
  1509		return scnprintf(buf, PAGE_SIZE,
  1510			"stats_valid %d\n",
> 1511			"write_errs %d\n"
  1512			"read_errs %d\n"
  1513			"flush_errs %d\n"
  1514			"corruption_errs %d\n"
  1515			"generation_errs %d\n",
> 1516			!!(device->dev_stats_valid),
  1517			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_WRITE_ERRS),
  1518			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_READ_ERRS),
  1519			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_FLUSH_ERRS),
  1520			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_CORRUPTION_ERRS),
  1521			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_GENERATION_ERRS));
  1522	}
  1523	BTRFS_ATTR(devid, stats, btrfs_devinfo_stats_show);
  1524	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23482 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3364 bytes --]

Hi David,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20210604]
[cannot apply to v5.13-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/David-Sterba/btrfs-sysfs-export-dev-stats-in-devinfo-directory/20210604-212445
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: arm-randconfig-r004-20210604 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 5c0d1b2f902aa6a9cf47cc7e42c5b83bb2217cf9)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/8a58ea51305ace4835c7abc51e46b7b64e25b793
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review David-Sterba/btrfs-sysfs-export-dev-stats-in-devinfo-directory/20210604-212445
        git checkout 8a58ea51305ace4835c7abc51e46b7b64e25b793
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/btrfs/sysfs.c:1511:3: warning: format specifies type 'int' but the argument has type 'char *' [-Wformat]
                   "write_errs %d\n"
                   ^~~~~~~~~~~~~~~~~
>> fs/btrfs/sysfs.c:1516:3: warning: data argument not used by format string [-Wformat-extra-args]
                   !!(device->dev_stats_valid),
                   ^
   2 warnings generated.


vim +1511 fs/btrfs/sysfs.c

  1497	
  1498	static ssize_t btrfs_devinfo_stats_show(struct kobject *kobj,
  1499			struct kobj_attribute *a, char *buf)
  1500	{
  1501		struct btrfs_device *device = container_of(kobj, struct btrfs_device,
  1502							   devid_kobj);
  1503	
  1504		/*
  1505		 * Print all at once so we get a snapshot of all values from the same
  1506		 * time. Keep them in sync and in order of definition of
  1507		 * btrfs_dev_stat_values.
  1508		 */
  1509		return scnprintf(buf, PAGE_SIZE,
  1510			"stats_valid %d\n",
> 1511			"write_errs %d\n"
  1512			"read_errs %d\n"
  1513			"flush_errs %d\n"
  1514			"corruption_errs %d\n"
  1515			"generation_errs %d\n",
> 1516			!!(device->dev_stats_valid),
  1517			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_WRITE_ERRS),
  1518			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_READ_ERRS),
  1519			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_FLUSH_ERRS),
  1520			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_CORRUPTION_ERRS),
  1521			btrfs_dev_stat_read(device, BTRFS_DEV_STAT_GENERATION_ERRS));
  1522	}
  1523	BTRFS_ATTR(devid, stats, btrfs_devinfo_stats_show);
  1524	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 23482 bytes --]

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

[Omar Sandoval]

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.

> +		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, stats, btrfs_devinfo_stats_show);
> +
>  static struct attribute *devid_attrs[] = {
>  	BTRFS_ATTR_PTR(devid, in_fs_metadata),
>  	BTRFS_ATTR_PTR(devid, missing),
>  	BTRFS_ATTR_PTR(devid, replace_target),
>  	BTRFS_ATTR_PTR(devid, scrub_speed_max),
> +	BTRFS_ATTR_PTR(devid, stats),
>  	BTRFS_ATTR_PTR(devid, writeable),
>  	NULL
>  };
> -- 
> 2.31.1
> 

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

[Omar Sandoval]

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.

Typo, I meant to say "the ioctl returns ENODEV _if_ !dev_stats_valid".

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

[David Sterba]

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.

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

[Omar Sandoval]

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.

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

[David Sterba]

On Wed, Jun 09, 2021 at 05:55:05PM -0700, Omar Sandoval wrote:
> > > 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.

Hm so I think this should reflect how the sysfs files are used. They all
contain textual information, and errors are returned when eg. there are
no permissions.

In a shell script it's IMHO more convenient to do

stats=$(cat $devicepath/stats)

and then validate contents of $stats rather then catching the error
value and deciding based on that what happend. Not to say that this
would also print an error message. I've found this in
admin-guide/sysfs-rules.rst that's perhaps closest to a recommendation
we could follow:

172 - When reading and writing sysfs device attribute files, avoid dependency
173     on specific error codes wherever possible. This minimizes coupling to
174     the error handling implementation within the kernel.

So I take it as that error codes belong to the sysfs layer and the
validity of the contents is up to the sysfs user, ie. btrfs in this
case.

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

[Omar Sandoval]

On Thu, Jun 10, 2021 at 06:37:01PM +0200, David Sterba wrote:
> On Wed, Jun 09, 2021 at 05:55:05PM -0700, Omar Sandoval wrote:
> > > > 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.
> 
> Hm so I think this should reflect how the sysfs files are used. They all
> contain textual information, and errors are returned when eg. there are
> no permissions.
> 
> In a shell script it's IMHO more convenient to do
> 
> stats=$(cat $devicepath/stats)
> 
> and then validate contents of $stats rather then catching the error
> value and deciding based on that what happend. Not to say that this
> would also print an error message. I've found this in
> admin-guide/sysfs-rules.rst that's perhaps closest to a recommendation
> we could follow:
> 
> 172 - When reading and writing sysfs device attribute files, avoid dependency
> 173     on specific error codes wherever possible. This minimizes coupling to
> 174     the error handling implementation within the kernel.
> 
> So I take it as that error codes belong to the sysfs layer and the
> validity of the contents is up to the sysfs user, ie. btrfs in this
> case.

Good point. "Real" programs will just use the ioctl, so it makes sense
to cater this to shell scripts.