Avoid counting disk io twice for NVMe SSD block device

[spacewander]

The first NVMe block device will be named as nvme0n1,
and its partitions are nvme0n1p1, nvme0n1p2, etc.
Therefore we could not use the last letter to identify if the name is a
partition or not.

[spacewander]

Here is the output from current disk_io_counters:

In [2]: psutil.disk_io_counters(perdisk=True)
Out[2]: 
{'nvme0n1': sdiskio(read_count=59871, write_count=26114, read_bytes=1706764800, write_bytes=993231872, read_time=21832, write_time=29972, read_merged_count=43, write_merged_count=38182, busy_time=19128),
 'nvme0n1p1': sdiskio(read_count=669, write_count=2, read_bytes=6141440, write_bytes=1024, read_time=20, write_time=0, read_merged_count=0, write_merged_count=0, busy_time=20),
 'nvme0n1p2': sdiskio(read_count=32, write_count=0, read_bytes=581632, write_bytes=0, read_time=4, write_time=0, read_merged_count=0, write_merged_count=0, busy_time=4),
 'nvme0n1p3': sdiskio(read_count=42, write_count=0, read_bytes=2121728, write_bytes=0, read_time=8, write_time=0, read_merged_count=0, write_merged_count=0, busy_time=8),
 'nvme0n1p4': sdiskio(read_count=42, write_count=0, read_bytes=2121728, write_bytes=0, read_time=20, write_time=0, read_merged_count=0, write_merged_count=0, busy_time=12),
 'nvme0n1p5': sdiskio(read_count=42, write_count=0, read_bytes=2121728, write_bytes=0, read_time=24, write_time=0, read_merged_count=0, write_merged_count=0, busy_time=16),
 'nvme0n1p6': sdiskio(read_count=58958, write_count=25940, read_bytes=1691456512, write_bytes=993230848, read_time=21748, write_time=29808, read_merged_count=43, write_merged_count=38182, busy_time=18948)}

[spacewander]

Any suggestion about this pull request?

[giampaolo]

Sorry, been busy. I briefly took a look at this and it seems more complex. Looking for a number is not enough, see:
#338

major minor  #blocks  name

 202        0   19922944 xvda
 202       16    1048576 xvdb 

In the case above xvdb should be taken into account. As such it appears a logic based on the partition
name alone is not enough. We probably want to play with minor field instead. The info that I've found online about /proc/partitions is pretty scarce but I suspect we may want to collect all names with a minor number > 0. I will have to dig more in order to confirm this though.

[spacewander]

@giampaolo
Thanks for your quick reply!

I dived into the Linux's source code tonight, and found the /proc/partitions is displayed by show_partition:
https://elixir.bootlin.com/linux/v3.10.108/source/block/genhd.c#L845
Each disk/partition is printed by disk_name:
https://elixir.bootlin.com/linux/v3.10.108/source/block/partition-generic.c#L34
I have also checked the source from a higher version, the code is almost unchanged.

There are three combinations of the name part in /proc/partitions:

1. disk_name, if partition_no is None
2. disk_name + 'p' + partition_no, if disk_name ends with a digit
3. disk_name + partition_no

I have updated my code as a result of this new discovery.

[kportertx]

I believe this incorrectly assumes the last disk it a partition.

[spacewander]

@kportertx
get_partitions returns both partitions and disk without any partitions. If the last disk is not a partition, it will be a disk without partition.

[kportertx]

Wouldn't this still match nvme0n1? It is looking for 0 or more 'p's followed by one or more digits.

[spacewander]

@kportertx
The partition_suffix is used to match partition suffix, not the whole partition name. Certainly, it will be better to write r'^p*\d+$' to avoid confusing. I will change it in the new commit.

[giampaolo]

Note: #1305 (comment) states that we can have read/write loads also for disks with no partition, which complicates things even more.

[giampaolo]

Update: it turns out iostat skips some disks/partitions (and psutil doesn't). That's what we should do. I'm trying to skim through iostat source code in order to understand where the "skip" of certain occurs:
https://github.com/sysstat/sysstat/blob/master/iostat.c
Any help is welcome. ;)

[giampaolo]

I filed a bug on iostat project asking for help:
sysstat/sysstat#185

[wiggin15]

Any help is welcome. ;)

My 2 cents: I think the magic you're looking for is here: https://github.com/sysstat/sysstat/blob/master/iostat.c#L719
This "continue" skips partitions (unless you run iostat -p - then DISPLAY_PARTITIONS(flags) will be True). From what I understand, according to iostat, actual devices will have an entry in /sys/block while partitions will not.

(Note 1: in https://github.com/giampaolo/psutil/blob/master/psutil/_pslinux.py#L1076 psutil says if fields_len is 14 then the entry in "referring to a disk" and if it is 7 then it is "referring to a partition", but iostat says 14 is "Device or partition" and 7 is "Partition without extended statistics")
(Note 2: we already look in /sys/block to find device's sector size. For partitions, this path, apparently, does not exist and we fall back to default size of 512, but maybe we can search for the partition's device and look there).

[giampaolo]

@wiggin15 thanks a lot man, that's exactly what I was looking for

[wiggin15]

@giampaolo FYI I started working on a new (bigger) patch here:
https://github.com/Infinidat/psutil/commits/issue1244
just so we won't do the work twice