tst_dev_bytes_written: parsing /proc/diskstats instead of sys file
diff mbox series

Message ID 1578289125-2491-1-git-send-email-xuyang2018.jy@cn.fujitsu.com
State Superseded
Headers show
Series
  • tst_dev_bytes_written: parsing /proc/diskstats instead of sys file
Related show

Commit Message

Yang Xu Jan. 6, 2020, 5:38 a.m. UTC
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(-)

Comments

Sumit Garg Jan. 6, 2020, 7:57 a.m. UTC | #1
On Mon, 6 Jan 2020 at 11:09, 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(-)
>

Overall patch looks good to me but with minor comments below.

> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 10f71901d..58877c810 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);
> +       FILE_LINES_SCANF(NULL, "/proc/diskstats", fmt, &dev_sec_write, &io_ticks);

Shouldn't we use SAFE_FILE_LINES_SCANF() here?

>
> +       /* 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, "Test device %s io_ticks is always 0, "
> +                               "exporting LTP_DEV to test", dev);

s/exporting/export/

-Sumit

>
>         dev_bytes_written = (dev_sec_write - prev_dev_sec_write) * 512;
>
> --
> 2.18.0
>
>
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
Yang Xu Jan. 6, 2020, 8:15 a.m. UTC | #2
on 2020/01/06 15:57, Sumit Garg wrote:
> On Mon, 6 Jan 2020 at 11:09, 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(-)
>>
> 
> Overall patch looks good to me but with minor comments below.
Thanks for your review.
> 
>> diff --git a/lib/tst_device.c b/lib/tst_device.c
>> index 10f71901d..58877c810 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);
>> +       FILE_LINES_SCANF(NULL, "/proc/diskstats", fmt, &dev_sec_write, &io_ticks);
> 
> Shouldn't we use SAFE_FILE_LINES_SCANF() here?
Yes. I will use SAFE_FILE_LINES_SCANF() in v2.
> 
>>
>> +       /* 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, "Test device %s io_ticks is always 0, "
>> +                               "exporting LTP_DEV to test", dev);
> 
> s/exporting/export/
...
> 
> -Sumit
> 
>>
>>          dev_bytes_written = (dev_sec_write - prev_dev_sec_write) * 512;
>>
>> --
>> 2.18.0
>>
>>
>>
>>
>> --
>> Mailing list info: https://lists.linux.it/listinfo/ltp
> 
>

Patch
diff mbox series

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 10f71901d..58877c810 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);
+	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, "Test device %s io_ticks is always 0, "
+				"exporting LTP_DEV to test", dev);
 
 	dev_bytes_written = (dev_sec_write - prev_dev_sec_write) * 512;