[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Yang Xu]

Now, tst_dev_bytes_written api can get the whole disk stat correctly(such as /dev/sda),
but can not get partition stat correctly(such as /dev/sda5).
fail as below:
export LTP_DEV=/dev/sda5
tst_device.c:388: CONF: Test device stat file: /sys/block/sda5/stat not found

To fix this, use /proc/diskstats to parse instead of /sys/block/<devices>/stat.
Data format as same as diskstats_show function in kernel genhd.c[1].

Also, add hint when loop driver doesn't support blk-mq[2]. So that, user can export
LTP_DEV to test.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/genhd.c
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b5dd2f60

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 lib/tst_device.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 10f71901d..ad764a38d 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -375,24 +375,22 @@ int tst_umount(const char *path)
 
 unsigned long tst_dev_bytes_written(const char *dev)
 {
-	struct stat st;
-	unsigned long dev_sec_write = 0, dev_bytes_written, io_ticks = 0;
-	char dev_stat_path[1024];
-
-	snprintf(dev_stat_path, sizeof(dev_stat_path), "/sys/block/%s/stat",
-		 strrchr(dev, '/') + 1);
+	unsigned long dev_sec_write = 0, dev_bytes_written;
+	unsigned int io_ticks = 0;
+	char fmt[1024];
 
-	if (stat(dev_stat_path, &st) != 0)
-		tst_brkm(TCONF, NULL, "Test device stat file: %s not found",
-			 dev_stat_path);
+	sprintf(fmt, "%%*4d %%*7d %s %%*lu %%*lu %%*lu %%*u %%*lu %%*lu %%lu %%*u %%*u %%u",
+			strrchr(dev, '/') + 1);
 
-	SAFE_FILE_SCANF(NULL, dev_stat_path,
-			"%*s %*s %*s %*s %*s %*s %lu %*s %*s %lu",
-			&dev_sec_write, &io_ticks);
+	SAFE_FILE_LINES_SCANF(NULL, "/proc/diskstats", fmt, &dev_sec_write, &io_ticks);
 
+	/* If we create block device by attaching a free loop device, loop driver
+	 * needs to support blk-mq(commit b5dd2f6047c), so that kernel can get I/O
+	 * statistics about loop device.
+	 */
 	if (!io_ticks)
-		tst_brkm(TCONF, NULL, "Test device stat file: %s broken",
-			 dev_stat_path);
+		tst_brkm(TCONF, NULL, "io_ticks on test device %s is always 0, "
+				"export LTP_DEV to test", dev);
 
 	dev_bytes_written = (dev_sec_write - prev_dev_sec_write) * 512;
 
-- 
2.18.0

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Sumit Garg]

On Mon, 6 Jan 2020 at 14:00, Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote:
>
> Now, tst_dev_bytes_written api can get the whole disk stat correctly(such as /dev/sda),
> but can not get partition stat correctly(such as /dev/sda5).
> fail as below:
> export LTP_DEV=/dev/sda5
> tst_device.c:388: CONF: Test device stat file: /sys/block/sda5/stat not found
>
> To fix this, use /proc/diskstats to parse instead of /sys/block/<devices>/stat.
> Data format as same as diskstats_show function in kernel genhd.c[1].
>
> Also, add hint when loop driver doesn't support blk-mq[2]. So that, user can export
> LTP_DEV to test.
>
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/genhd.c
> [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b5dd2f60
>
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>  lib/tst_device.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 10f71901d..ad764a38d 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -375,24 +375,22 @@ int tst_umount(const char *path)
>
>  unsigned long tst_dev_bytes_written(const char *dev)
>  {
> -       struct stat st;
> -       unsigned long dev_sec_write = 0, dev_bytes_written, io_ticks = 0;
> -       char dev_stat_path[1024];
> -
> -       snprintf(dev_stat_path, sizeof(dev_stat_path), "/sys/block/%s/stat",
> -                strrchr(dev, '/') + 1);
> +       unsigned long dev_sec_write = 0, dev_bytes_written;
> +       unsigned int io_ticks = 0;
> +       char fmt[1024];
>
> -       if (stat(dev_stat_path, &st) != 0)
> -               tst_brkm(TCONF, NULL, "Test device stat file: %s not found",
> -                        dev_stat_path);
> +       sprintf(fmt, "%%*4d %%*7d %s %%*lu %%*lu %%*lu %%*u %%*lu %%*lu %%lu %%*u %%*u %%u",
> +                       strrchr(dev, '/') + 1);
>
> -       SAFE_FILE_SCANF(NULL, dev_stat_path,
> -                       "%*s %*s %*s %*s %*s %*s %lu %*s %*s %lu",
> -                       &dev_sec_write, &io_ticks);
> +       SAFE_FILE_LINES_SCANF(NULL, "/proc/diskstats", fmt, &dev_sec_write, &io_ticks);
>
> +       /* If we create block device by attaching a free loop device, loop driver
> +        * needs to support blk-mq(commit b5dd2f6047c), so that kernel can get I/O
> +        * statistics about loop device.
> +        */
>         if (!io_ticks)
> -               tst_brkm(TCONF, NULL, "Test device stat file: %s broken",
> -                        dev_stat_path);
> +               tst_brkm(TCONF, NULL, "io_ticks on test device %s is always 0, "
> +                               "export LTP_DEV to test", dev);
>
>         dev_bytes_written = (dev_sec_write - prev_dev_sec_write) * 512;
>
> --
> 2.18.0
>
>
>

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Cyril Hrubis]

Hi!
> Now, tst_dev_bytes_written api can get the whole disk stat correctly(such as /dev/sda),
> but can not get partition stat correctly(such as /dev/sda5).
> fail as below:
> export LTP_DEV=/dev/sda5
> tst_device.c:388: CONF: Test device stat file: /sys/block/sda5/stat not found

It seems that in case of partitions the stat file is available under
/sys/block/sda/sda5/stat.

For NVME it's similar as:

/sys/block/nvme0n1/nvme0n1p1/stat

I do wonder if it wouldn't be easier to try to match the prefix of the
device name first, then try a directory with a full name after that.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Yang Xu]

Hi
> Hi!
>> Now, tst_dev_bytes_written api can get the whole disk stat correctly(such as /dev/sda),
>> but can not get partition stat correctly(such as /dev/sda5).
>> fail as below:
>> export LTP_DEV=/dev/sda5
>> tst_device.c:388: CONF: Test device stat file: /sys/block/sda5/stat not found
> 
> It seems that in case of partitions the stat file is available under
> /sys/block/sda/sda5/stat.
> 
> For NVME it's similar as:
> 
> /sys/block/nvme0n1/nvme0n1p1/stat
> 
> I do wonder if it wouldn't be easier to try to match the prefix of the
> device name first, then try a directory with a full name after that.
Maybe, I am fine with your way(I tested it). I only  refer to kernel 
documention [1] .sysfs and proc file  use the same source for the 
information and so should not differ. So I use fixed format as kernel 
function to exact.  Also, I want to listen advise from Sumit.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/iostats.rst

Best Regards
Yang Xu
> 

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Sumit Garg]

Hi Cyril, Yang,

On Thu, 9 Jan 2020 at 14:05, Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote:
>
> Hi
> > Hi!
> >> Now, tst_dev_bytes_written api can get the whole disk stat correctly(such as /dev/sda),
> >> but can not get partition stat correctly(such as /dev/sda5).
> >> fail as below:
> >> export LTP_DEV=/dev/sda5
> >> tst_device.c:388: CONF: Test device stat file: /sys/block/sda5/stat not found
> >
> > It seems that in case of partitions the stat file is available under
> > /sys/block/sda/sda5/stat.
> >
> > For NVME it's similar as:
> >
> > /sys/block/nvme0n1/nvme0n1p1/stat
> >
> > I do wonder if it wouldn't be easier to try to match the prefix of the
> > device name first, then try a directory with a full name after that.

I did checked earlier that we could use partition specific
sub-directory for "stat" file but I thought it would be complex in
comparison to referring a single file. But your implementation doesn't
look that complex.

> Maybe, I am fine with your way(I tested it). I only  refer to kernel
> documention [1] .sysfs and proc file  use the same source for the
> information and so should not differ. So I use fixed format as kernel
> function to exact.  Also, I want to listen advise from Sumit.
>

TBH, I think it's just a matter of taste. I am fine with either approach.

-Sumit

> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/iostats.rst
>
> Best Regards
> Yang Xu
> >
>
>

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Cyril Hrubis]

Hi!
> > Maybe, I am fine with your way(I tested it). I only  refer to kernel
> > documention [1] .sysfs and proc file  use the same source for the
> > information and so should not differ. So I use fixed format as kernel
> > function to exact.  Also, I want to listen advise from Sumit.
> >
> 
> TBH, I think it's just a matter of taste. I am fine with either approach.

I dot not like that much that we generate the scanf format string on the
fly in the version that parses /proc/diskstat.

In the end we have to merge one of them, I slightly prefer mine, but I
do not have a strong feelings about this.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Petr Vorel]

Hi,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > > Maybe, I am fine with your way(I tested it). I only  refer to kernel
> > > documention [1] .sysfs and proc file  use the same source for the
> > > information and so should not differ. So I use fixed format as kernel
> > > function to exact.  Also, I want to listen advise from Sumit.


> > TBH, I think it's just a matter of taste. I am fine with either approach.

> I dot not like that much that we generate the scanf format string on the
> fly in the version that parses /proc/diskstat.
+1

> In the end we have to merge one of them, I slightly prefer mine, but I
> do not have a strong feelings about this.
+1

If we merge Cyril's, I'd be for adding the hint about LTP_DEV (maybe as a
separate commit).

Thank you both for working on this fix.

Kind regards,
Petr

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Cyril Hrubis]

Hi!
> If we merge Cyril's, I'd be for adding the hint about LTP_DEV (maybe as a
> separate commit).

Maybe it would be a bit better to print short TINFO message in the case
of failure instead of adding comments into the source code...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file

[Petr Vorel]

Hi,

> > If we merge Cyril's, I'd be for adding the hint about LTP_DEV (maybe as a
> > separate commit).

> Maybe it would be a bit better to print short TINFO message in the case
> of failure instead of adding comments into the source code...
There is extended TCONF message in the patch. Yep, maybe just append the commend
to that message as well.

Kind regards,
Petr