disk_io_counters() - linux: mimic iostat behavior

[giampaolo]

Re: #1244, #1305, #1312.

[giampaolo]

@wiggin15 this is a competing PR re. your #1312. The fundamental difference is that:

• it doesn't rely on /disk/partitions anymore, it just reads /proc/diskstats
• it mimics is_device() function of iostat https://github.com/sysstat/sysstat/blob/97912938cd476645b267280069e83b1c8dc0e1c7/common.c#L208-L221

This is what iostat does basically.

With that said, I'm still not completely sure this is the desired behavior as we're losing information (the partitions). For example, on my machine:

$ iostat
...
loop7             0,02         0,02         0,00      19161          0
nvme0n1           7,44        46,64       419,57   44357716  399041042
loop8             0,00         0,00         0,00       1820          0
...

$ iostat -p
...
loop7             0,02         0,02         0,00      19161          0
nvme0n1           7,44        46,64       419,56   44357728  399044530
nvme0n1p1         0,00         0,01         0,00      13168          1
nvme0n1p2         0,85         3,88         3,82    3686381    3631544
nvme0n1p3         5,73        34,83       314,54   33122864  299158432
nvme0n1p4         0,75         7,91       101,20    7527576   96254553
loop8             0,00         0,00         0,00       1820          0
...

So far psutil used to show both base disks and partitions, mimicking iostat -p. With this change in place and #1312 it will not, and I'm not sure this is what we want as we're losing info which can potentially be useful. I think the best compromise is to include partitions when perdisk=True is passed (more available info) else skip them in order to avoid counting disk + partition(s) twice.

@wiggin15 does it make sense to you?

PS: another aspect is that we're also returning virtual partitions (e.g. "loop8"). One may argue there should be a switch to skip them but I don't want to complicate disk_io_counters() API by adding a parameter which only applies to Linux.

[wiggin15]

With this change in place and #1312 it will not

Actually, my patch retains the partition information (still more like iostat -p). The problem with #1312 is that it always ignores parent devices if they have partitions.

So, this PR always strips partition information, and #1312 always strips parent devices, regardless of perdisk.

Indeed, the best thing to do is probably return the partition information and if perdisk is False then skip devices that have partitions. This is pretty easy with #1312, but for that we need to pass perdisk to the platform modules - all disk_io_counters will have to get the perdisk parameter.

If we don't want to do that then it's your call - #1312 will return more information (more like today, except without the wrong multiple counting), and this PR will always strip partitions (but this PR is far simpler in terms of code).

[giampaolo]

Actually, my patch retains the partition information (still more like iostat -p). The problem with #1312 is that it always ignores parent devices if they have partitions. So, this PR always strips partition information, and #1312 always strips parent devices, regardless of perdisk.

Correct, sorry about the confusion.

Indeed, the best thing to do is probably return the partition information and if perdisk is False then skip devices that have partitions. This is pretty easy with #1312, but for that we need to pass perdisk to the platform modules - all disk_io_counters will have to get the perdisk parameter.

We have to pass perdisk parameter also with this PR. Not a big deal, it will just be ignored on other platforms.

If we don't want to do that then it's your call - #1312 will return more information (more like today, except without the wrong multiple counting), and this PR will always strip partitions (but this PR is far simpler in terms of code).

Yes it appears this one is simpler so I prefer to go along with it. Thanks for #1312 though, I appreciate it.