| Message ID | 1578299418-4961-1-git-send-email-xuyang2018.jy@cn.fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] tst_dev_bytes_written: parsing /proc/diskstats instead of sys file | expand |
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 > > >
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.
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 >
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 > > > >
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.
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
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...
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
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;
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(-)