diff --git a/HISTORY.rst b/HISTORY.rst index 602ce3d76..3dcafa3c2 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -24,6 +24,8 @@ XXXX-XX-XX MemoryError. (patch by sylvainduchesne) - 1309_: [Linux] Process.status() is unable to recognie "idle" and "parked" statuses (returns '?'). +- 1313_: [Linux] disk_io_counters() can report inflated IO counters due to + erroneously counting base disk device and its partition(s) twice. 5.4.6 ===== diff --git a/docs/index.rst b/docs/index.rst index a64448e3d..097d828dc 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -421,6 +421,8 @@ Disks cache. On Windows it may be ncessary to issue ``diskperf -y`` command from cmd.exe first in order to enable IO counters. + On diskless machines this function will return ``None`` or ``{}`` if + *perdisk* is ``True``. >>> import psutil >>> psutil.disk_io_counters() @@ -474,6 +476,8 @@ Network numbers will always be increasing or remain the same, but never decrease. ``net_io_counters.cache_clear()`` can be used to invalidate the *nowrap* cache. + On machines with no network iterfaces this function will return ``None`` or + ``{}`` if *pernic* is ``True``. >>> import psutil >>> psutil.net_io_counters() diff --git a/psutil/__init__.py b/psutil/__init__.py index 299fc1ea8..291d0d16a 100644 --- a/psutil/__init__.py +++ b/psutil/__init__.py @@ -2013,7 +2013,8 @@ def disk_io_counters(perdisk=False, nowrap=True): On recent Windows versions 'diskperf -y' command may need to be executed first otherwise this function won't find any disk. """ - rawdict = _psplatform.disk_io_counters() + kwargs = dict(perdisk=perdisk) if LINUX else {} + rawdict = _psplatform.disk_io_counters(**kwargs) if not rawdict: return {} if perdisk else None if nowrap: diff --git a/psutil/_pslinux.py b/psutil/_pslinux.py index ffa3a8a32..0aa507bd2 100644 --- a/psutil/_pslinux.py +++ b/psutil/_pslinux.py @@ -252,12 +252,12 @@ def file_flags_to_mode(flags): return mode -def get_sector_size(partition): - """Return the sector size of a partition. - Used by disk_io_counters(). - """ +def get_sector_size(name): + """Return the sector size of a storage device.""" + # Some devices may have a slash in their name (e.g. cciss/c0d0...). + name = name.replace('/', '!') try: - with open("/sys/block/%s/queue/hw_sector_size" % partition, "rt") as f: + with open("/sys/block/%s/queue/hw_sector_size" % name, "rt") as f: return int(f.read()) except (IOError, ValueError): # man iostat states that sectors are equivalent with blocks and @@ -265,6 +265,25 @@ def get_sector_size(partition): return SECTOR_SIZE_FALLBACK +def is_storage_device(name): + """Return True if the given name refers to a physical (e.g. "sda", + "nvme0n1") or virtual (e.g. "loop1", "ram") storage device. + In case name refers to a device's partition (e.g. "sda1", "nvme0n1p1") + this is supposed to return False. + """ + # Readapted from iostat source code, see: + # https://github.com/sysstat/sysstat/blob/ + # 97912938cd476645b267280069e83b1c8dc0e1c7/common.c#L208 + # Some devices may have a slash in their name (e.g. cciss/c0d0...). + name = name.replace('/', '!') + including_virtual = True + if including_virtual: + path = "/sys/block/%s" % name + else: + path = "/sys/block/%s/device" % name + return os.access(path, os.F_OK) + + @memoize def set_scputimes_ntuple(procfs_path): """Set a namedtuple of variable fields depending on the CPU times @@ -1028,32 +1047,11 @@ def net_if_stats(): disk_usage = _psposix.disk_usage -def disk_io_counters(): +def disk_io_counters(perdisk=False): """Return disk I/O statistics for every disk installed on the system as a dict of raw tuples. """ - # determine partitions we want to look for - def get_partitions(): - partitions = [] - with open_text("%s/partitions" % get_procfs_path()) as f: - lines = f.readlines()[2:] - for line in reversed(lines): - _, _, _, name = line.split() - if name[-1].isdigit(): - # we're dealing with a partition (e.g. 'sda1'); 'sda' will - # also be around but we want to omit it - partitions.append(name) - else: - if not partitions or not partitions[-1].startswith(name): - # we're dealing with a disk entity for which no - # partitions have been defined (e.g. 'sda' but - # 'sda1' was not around), see: - # https://github.com/giampaolo/psutil/issues/338 - partitions.append(name) - return partitions - retdict = {} - partitions = get_partitions() with open_text("%s/diskstats" % get_procfs_path()) as f: lines = f.readlines() for line in lines: @@ -1091,12 +1089,26 @@ def get_partitions(): else: raise ValueError("not sure how to interpret line %r" % line) - if name in partitions: - ssize = get_sector_size(name) - rbytes *= ssize - wbytes *= ssize - retdict[name] = (reads, writes, rbytes, wbytes, rtime, wtime, - reads_merged, writes_merged, busy_time) + if not perdisk and not is_storage_device(name): + # perdisk=False means we want to calculate totals so we skip + # partitions (e.g. 'sda1', 'nvme0n1p1') and only include + # base disk devices (e.g. 'sda', 'nvme0n1'). Base disks + # include a total of all their partitions + some extra size + # of their own: + # $ cat /proc/diskstats + # 259 0 sda 10485760 ... + # 259 1 sda1 5186039 ... + # 259 1 sda2 5082039 ... + # See: + # https://github.com/giampaolo/psutil/pull/1313 + continue + + ssize = get_sector_size(name) + rbytes *= ssize + wbytes *= ssize + retdict[name] = (reads, writes, rbytes, wbytes, rtime, wtime, + reads_merged, writes_merged, busy_time) + return retdict diff --git a/psutil/tests/test_linux.py b/psutil/tests/test_linux.py index 9ea59b617..761f79047 100755 --- a/psutil/tests/test_linux.py +++ b/psutil/tests/test_linux.py @@ -986,15 +986,10 @@ def test_disk_io_counters_kernel_2_4_mocked(self): # Tests /proc/diskstats parsing format for 2.4 kernels, see: # https://github.com/giampaolo/psutil/issues/767 with mock_open_content( - '/proc/partitions', - textwrap.dedent("""\ - major minor #blocks name - - 8 0 488386584 hda - """)): - with mock_open_content( - '/proc/diskstats', - " 3 0 1 hda 2 3 4 5 6 7 8 9 10 11 12"): + '/proc/diskstats', + " 3 0 1 hda 2 3 4 5 6 7 8 9 10 11 12"): + with mock.patch('psutil._pslinux.is_storage_device', + return_value=True): ret = psutil.disk_io_counters(nowrap=False) self.assertEqual(ret.read_count, 1) self.assertEqual(ret.read_merged_count, 2) @@ -1011,15 +1006,10 @@ def test_disk_io_counters_kernel_2_6_full_mocked(self): # lines reporting all metrics: # https://github.com/giampaolo/psutil/issues/767 with mock_open_content( - '/proc/partitions', - textwrap.dedent("""\ - major minor #blocks name - - 8 0 488386584 hda - """)): - with mock_open_content( - '/proc/diskstats', - " 3 0 hda 1 2 3 4 5 6 7 8 9 10 11"): + '/proc/diskstats', + " 3 0 hda 1 2 3 4 5 6 7 8 9 10 11"): + with mock.patch('psutil._pslinux.is_storage_device', + return_value=True): ret = psutil.disk_io_counters(nowrap=False) self.assertEqual(ret.read_count, 1) self.assertEqual(ret.read_merged_count, 2) @@ -1038,15 +1028,10 @@ def test_disk_io_counters_kernel_2_6_limited_mocked(self): # (instead of a disk). See: # https://github.com/giampaolo/psutil/issues/767 with mock_open_content( - '/proc/partitions', - textwrap.dedent("""\ - major minor #blocks name - - 8 0 488386584 hda - """)): - with mock_open_content( - '/proc/diskstats', - " 3 1 hda 1 2 3 4"): + '/proc/diskstats', + " 3 1 hda 1 2 3 4"): + with mock.patch('psutil._pslinux.is_storage_device', + return_value=True): ret = psutil.disk_io_counters(nowrap=False) self.assertEqual(ret.read_count, 1) self.assertEqual(ret.read_bytes, 2 * SECTOR_SIZE) @@ -1059,6 +1044,56 @@ def test_disk_io_counters_kernel_2_6_limited_mocked(self): self.assertEqual(ret.write_time, 0) self.assertEqual(ret.busy_time, 0) + def test_disk_io_counters_include_partitions(self): + # Make sure that when perdisk=True disk partitions are returned, + # see: + # https://github.com/giampaolo/psutil/pull/1313#issuecomment-408626842 + with mock_open_content( + '/proc/diskstats', + textwrap.dedent("""\ + 3 0 nvme0n1 1 2 3 4 5 6 7 8 9 10 11 + 3 0 nvme0n1p1 1 2 3 4 5 6 7 8 9 10 11 + """)): + with mock.patch('psutil._pslinux.is_storage_device', + return_value=False): + ret = psutil.disk_io_counters(perdisk=True, nowrap=False) + self.assertEqual(len(ret), 2) + self.assertEqual(ret['nvme0n1'].read_count, 1) + self.assertEqual(ret['nvme0n1p1'].read_count, 1) + self.assertEqual(ret['nvme0n1'].write_count, 5) + self.assertEqual(ret['nvme0n1p1'].write_count, 5) + + def test_disk_io_counters_exclude_partitions(self): + # Make sure that when perdisk=False partitions (e.g. 'sda1', + # 'nvme0n1p1') are skipped and not included in the total count. + # https://github.com/giampaolo/psutil/pull/1313#issuecomment-408626842 + with mock_open_content( + '/proc/diskstats', + textwrap.dedent("""\ + 3 0 nvme0n1 1 2 3 4 5 6 7 8 9 10 11 + 3 0 nvme0n1p1 1 2 3 4 5 6 7 8 9 10 11 + """)): + with mock.patch('psutil._pslinux.is_storage_device', + return_value=False): + ret = psutil.disk_io_counters(perdisk=False, nowrap=False) + self.assertIsNone(ret) + + # + def is_storage_device(name): + return name == 'nvme0n1' + + with mock_open_content( + '/proc/diskstats', + textwrap.dedent("""\ + 3 0 nvme0n1 1 2 3 4 5 6 7 8 9 10 11 + 3 0 nvme0n1p1 1 2 3 4 5 6 7 8 9 10 11 + """)): + with mock.patch('psutil._pslinux.is_storage_device', + create=True, side_effect=is_storage_device): + ret = psutil.disk_io_counters(perdisk=False, nowrap=False) + self.assertEqual(ret.read_count, 1) + self.assertEqual(ret.write_count, 5) + # ===================================================================== # --- misc