Skip to content

Commit

Permalink
disk_io_counters() - linux: mimic iostat behavior (#1313)
Browse files Browse the repository at this point in the history 
Fix #1244, #1305, #1312. disk_io_counters(perdisk=False) on Linux was counting disk device + partitions(s) twice
  • Loading branch information
@giampaolo
giampaolo committed Jul 28, 2018
1 parent 196dcb2 commit 45c0ebc
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 61 deletions.
2 changes: 2 additions & 0 deletions HISTORY.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
=====
Expand Down
4 changes: 4 additions & 0 deletions docs/index.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
3 changes: 2 additions & 1 deletion psutil/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
78 changes: 45 additions & 33 deletions psutil/_pslinux.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,19 +252,38 @@ 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
# have a size of 512 bytes since 2.4 kernels.
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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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


Expand Down
89 changes: 62 additions & 27 deletions psutil/tests/test_linux.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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
Expand Down

0 comments on commit 45c0ebc

Please sign in to comment.