[nfs-utils PATCH 0/8] mountstats/nfsiostat: bugfixes for iostat

[nfs-utils PATCH 0/8] mountstats/nfsiostat: bugfixes for iostat

[Frank Sorenson]

This patchset is intended to fix several bugs in nfsiostat and
'mountstats iostat', in particular when an <interval> is specified.

Specifically, the patches address the following issues when printing
multiple times:

* both nfsiostat and 'mountstats iostat':
   - if <count> is also specified, the scripts sleep an additional
     <interval> after printing <count> times, and parse mountstats
     unnecessarily before checking the <count>

   - if no nfs mounts are present when printing, the scripts output
     a message that there are no nfs mounts, and exit immediately.

     However, if multiple intervals are indicated, it makes more sense
     to output the message and sleep until the next iteration; there
     may be nfs mounts the next time through.

   - if mountpoints are specified on the command-line, but are not
     present during an iteration, the scripts crash.


 * 'mountstats iostat':
   - if an nfs filesystem is unmounted between iterations, the
     script will crash attempting to reference the non-existent
     mountpoint in the new mountstats file (or if another filesystem
     type is mounted at that location, will instead crash while
     comparing old and new stats)

   - new nfs mounts are not detected


 * nfsiostat:
   - if a new nfs filesystem is mounted between iterations, the
     script will crash attempting to reference the non-existent
     mountpoint in the old mountstats file

   - if a mountpoint is specified on the command-line, but topmost
     mount there is not nfs, the script crashes while trying to
     compare old and new stats


To address these issues, the patches do the following:
 * when printing diff stats from previous iteration:
   - verify that a device exists in the old mountstats before referencing it
   - verify that both old and new fstypes are the same

 * when filtering the current mountstats file:
   - verify the device exists in the mountstats file before referencing it
   - filter the list of nfs mountpoints each iostat iteration

 * check for empty device list and output the 'No NFS mount points found'
    message, but don't immediately exit the script

 * merge the infinite loop and counted loop, and (for the counted loop)
    decrement and check the count before sleeping and parsing the mountstats
    file


Frank Sorenson (8):
  mountstats/nfsiostat: add a function to return the fstype
  mountstats: when printing iostats, verify that old and new types are
    the same
  nfsiostat: mirror how mountstats iostat prints the stats
  nfsiostat: fix crash when filtering mountstats after unmount
  nfsiostat: make comment explain mount/unmount more broadly
  mountstats: filter for nfs mounts in a function, each iostat iteration
  mountstats/nfsiostat: Move the checks for empty mountpoint list into
    the print function
  mountstats/nfsiostat: merge and rework the infinite and counted loops

 tools/mountstats/mountstats.py | 102 ++++++++++++++++--------------
 tools/nfs-iostat/nfs-iostat.py | 110 +++++++++++++--------------------
 2 files changed, 100 insertions(+), 112 deletions(-)

-- 
2.47.1

[nfs-utils PATCH 1/8] mountstats/nfsiostat: add a function to return the fstype

[Frank Sorenson]

We'll need to get the DeviceData fstype when printing iostats
with multiple iterations, so we can verify that old and new
mountpoints are the same.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 tools/mountstats/mountstats.py | 5 +++++
 tools/nfs-iostat/nfs-iostat.py | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index 8e129c83..00d1ac7e 100755
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -333,6 +333,11 @@ class DeviceData:
             found = True
             self.__parse_rpc_line(words)
 
+    def fstype(self):
+        """Return the fstype for the mountpoint
+        """
+        return self.__nfs_data['fstype']
+
     def is_nfs_mountpoint(self):
         """Return True if this is an NFS or NFSv4 mountpoint,
         otherwise return False
diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
index 31587370..95176a4b 100755
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -187,6 +187,11 @@ class DeviceData:
             found = True
             self.__parse_rpc_line(words)
 
+    def fstype(self):
+        """Return the fstype for the mountpoint
+        """
+        return self.__nfs_data['fstype']
+
     def is_nfs_mountpoint(self):
         """Return True if this is an NFS or NFSv4 mountpoint,
         otherwise return False
-- 
2.48.1

[nfs-utils PATCH 2/8] mountstats: when printing iostats, verify that old and new types are the same

[Frank Sorenson]

It's not sufficient to verify that old and new are not autofs; both
should be the same fstype, in order to cover other potential
mismatches.  This prevents crashes when a path is a mountpoint, but
not nfs or autofs.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 tools/mountstats/mountstats.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index 00d1ac7e..59139ccc 100755
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -961,14 +961,15 @@ def print_iostat_summary(old, new, devices, time):
     for device in devices:
         stats = DeviceData()
         stats.parse_stats(new[device])
-        if not old or device not in old:
+        if old and device in old:
+            old_stats = DeviceData()
+            old_stats.parse_stats(old[device])
+            if stats.fstype() == old_stats.fstype():
+                stats.compare_iostats(old_stats).display_iostats(time)
+            else: # device is in old, but fstypes are different
+                stats.display_iostats(time)
+        else: # device is only in new
             stats.display_iostats(time)
-        else:
-            if ("fstype autofs" not in str(old[device])) and ("fstype autofs" not in str(new[device])):
-                old_stats = DeviceData()
-                old_stats.parse_stats(old[device])
-                diff_stats = stats.compare_iostats(old_stats)
-                diff_stats.display_iostats(time)
 
 def iostat_command(args):
     """iostat-like command for NFS mount points
-- 
2.48.1

[nfs-utils PATCH 3/8] nfsiostat: mirror how mountstats iostat prints the stats

[Frank Sorenson]

Currently, nfsiostat assumes that if old mountstats are provided,
then 'old' will contain all the new devices as well, and will
therefore crash if outputting multiple iterations and a new nfs
mount occurs between iterations.

'mountstats iostat' covers the new-nfs-mount situation by checking
that the old mountstats contains the device before referencing it.
It also verifies both old and new mountpoints are the same type.
Therefore, make the nfsiostat output function like mountstats.

However nfsiostat also has to allow sorting, so we can't just
output the full/diff stats as we go.  Instead, put the stats to
output into a list to display, sort the list if necessary, then
output the stats at the end.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 tools/nfs-iostat/nfs-iostat.py | 42 ++++++++++++----------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
index 95176a4b..bf5eead9 100755
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -476,41 +476,27 @@ def parse_stats_file(filename):
     return ms_dict
 
 def print_iostat_summary(old, new, devices, time, options):
-    stats = {}
-    diff_stats = {}
-    devicelist = []
-    if old:
-        # Trim device list to only include intersection of old and new data,
-        # this addresses umounts due to autofs mountpoints
-        for device in devices:
-            if "fstype autofs" not in str(old[device]):
-                devicelist.append(device)
-    else:
-        devicelist = devices
+    display_stats = {}
 
-    for device in devicelist:
-        stats[device] = DeviceData()
-        stats[device].parse_stats(new[device])
-        if old:
+    for device in devices:
+        stats = DeviceData()
+        stats.parse_stats(new[device])
+        if old and device in old:
             old_stats = DeviceData()
             old_stats.parse_stats(old[device])
-            diff_stats[device] = stats[device].compare_iostats(old_stats)
+            if stats.fstype() == old_stats.fstype():
+                display_stats[device] = stats.compare_iostats(old_stats)
+            else: # device is in old, but fstypes are different
+                display_stats[device] = stats
+        else: # device is only in new
+            display_stats[device] = stats
 
     if options.sort:
-        if old:
-            # We now have compared data and can print a comparison
-            # ordered by mountpoint ops per second
-            devicelist.sort(key=lambda x: diff_stats[x].ops(time), reverse=True)
-        else:
-            # First iteration, just sort by newly parsed ops/s
-            devicelist.sort(key=lambda x: stats[x].ops(time), reverse=True)
+        devices.sort(key=lambda x: display_stats[x].ops(time), reverse=True)
 
     count = 1
-    for device in devicelist:
-        if old:
-            diff_stats[device].display_iostats(time, options.which)
-        else:
-            stats[device].display_iostats(time, options.which)
+    for device in devices:
+        display_stats[device].display_iostats(time, options.which)
 
         count += 1
         if (count > options.list):
-- 
2.48.1

[nfs-utils PATCH 4/8] nfsiostat: fix crash when filtering mountstats after unmount

[Frank Sorenson]

If an unmount occurs between iterations, the filter function will
crash when referencing the 'device' in the new mountstats.  Verify
it exists before trying to access it.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 tools/nfs-iostat/nfs-iostat.py | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
index bf5eead9..08b827c0 100755
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -511,10 +511,11 @@ def list_nfs_mounts(givenlist, mountstats):
     devicelist = []
     if len(givenlist) > 0:
         for device in givenlist:
-            stats = DeviceData()
-            stats.parse_stats(mountstats[device])
-            if stats.is_nfs_mountpoint():
-                devicelist += [device]
+            if device in mountstats:
+                stats = DeviceData()
+                stats.parse_stats(mountstats[device])
+                if stats.is_nfs_mountpoint():
+                    devicelist += [device]
     else:
         for device, descr in mountstats.items():
             stats = DeviceData()
-- 
2.48.1

[nfs-utils PATCH 5/8] nfsiostat: make comment explain mount/unmount more broadly

[Frank Sorenson]

The comment explaining the need to recheck the devices list
suggests that nfs mounts/unmounts may occur when automount
is involved, but they can happen for other reasons as well.

Make the comment explain the issue more broadly.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 tools/nfs-iostat/nfs-iostat.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
index 08b827c0..1502b431 100755
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -631,8 +631,8 @@ client are listed.
             time.sleep(interval)
             sample_time = interval
             mountstats = parse_stats_file('/proc/self/mountstats')
-            # automount mountpoints add and drop, if automount is involved
-            # we need to recheck the devices list when reparsing
+            # nfs mountpoints may appear or disappear, so we need to
+            # recheck the devices list each time we parse mountstats
             devices = list_nfs_mounts(origdevices,mountstats)
             if len(devices) == 0:
                 print('No NFS mount points were found')
@@ -645,8 +645,8 @@ client are listed.
             time.sleep(interval)
             sample_time = interval
             mountstats = parse_stats_file('/proc/self/mountstats')
-            # automount mountpoints add and drop, if automount is involved
-            # we need to recheck the devices list when reparsing
+            # nfs mountpoints may appear or disappear, so we need to
+            # recheck the devices list each time we parse mountstats
             devices = list_nfs_mounts(origdevices,mountstats)
             if len(devices) == 0:
                 print('No NFS mount points were found')
-- 
2.48.1

[nfs-utils PATCH 6/8] mountstats: filter for nfs mounts in a function, each iostat iteration

[Frank Sorenson]

Currently, 'mountstats iostat' filters for nfs mounts when first
parsing mountstats, and never re-verifies the list when printing
multiple iterations.  As a result, new nfs mountpoints are never
detected, and unmounts result in a crash.

nfsiostat covers both new mounts and unmounts by filtering the
list of devices in a function during each iteration.

Align the scripts by copying the nfsiostat filtering function,
and filter each iteration.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 tools/mountstats/mountstats.py | 53 ++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index 59139ccc..e640642a 100755
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -971,11 +971,32 @@ def print_iostat_summary(old, new, devices, time):
         else: # device is only in new
             stats.display_iostats(time)
 
+def list_nfs_mounts(givenlist, mountstats):
+    """return a list of NFS mounts given a list to validate or
+       return a full list if the given list is empty -
+       may return an empty list if none found
+    """
+    devicelist = []
+    if len(givenlist) > 0:
+        for device in givenlist:
+            if device in mountstats:
+                stats = DeviceData()
+                stats.parse_stats(mountstats[device])
+                if stats.is_nfs_mountpoint():
+                    devicelist += [device]
+    else:
+        for device, descr in mountstats.items():
+            stats = DeviceData()
+            stats.parse_stats(descr)
+            if stats.is_nfs_mountpoint():
+                devicelist += [device]
+    return devicelist
+
 def iostat_command(args):
     """iostat-like command for NFS mount points
     """
     mountstats = parse_stats_file(args.infile)
-    devices = [os.path.normpath(mp) for mp in args.mountpoints]
+    origdevices = [os.path.normpath(mp) for mp in args.mountpoints]
 
     if args.since:
         old_mountstats = parse_stats_file(args.since)
@@ -983,23 +1004,7 @@ def iostat_command(args):
         old_mountstats = None
 
     # make certain devices contains only NFS mount points
-    if len(devices) > 0:
-        check = []
-        for device in devices:
-            stats = DeviceData()
-            try:
-                stats.parse_stats(mountstats[device])
-                if stats.is_nfs_mountpoint():
-                    check += [device]
-            except KeyError:
-                continue
-        devices = check
-    else:
-        for device, descr in mountstats.items():
-            stats = DeviceData()
-            stats.parse_stats(descr)
-            if stats.is_nfs_mountpoint():
-                devices += [device]
+    devices = list_nfs_mounts(origdevices, mountstats)
     if len(devices) == 0:
         print('No NFS mount points were found')
         return 1
@@ -1018,6 +1023,12 @@ def iostat_command(args):
             time.sleep(args.interval)
             sample_time = args.interval
             mountstats = parse_stats_file(args.infile)
+            # nfs mountpoints may appear or disappear, so we need to
+            # recheck the devices list each time we parse mountstats
+            devices = list_nfs_mounts(origdevices, mountstats)
+            if len(devices) == 0:
+                print('No NFS mount points were found')
+                return
             count -= 1
     else: 
         while True:
@@ -1026,6 +1037,12 @@ def iostat_command(args):
             time.sleep(args.interval)
             sample_time = args.interval
             mountstats = parse_stats_file(args.infile)
+            # nfs mountpoints may appear or disappear, so we need to
+            # recheck the devices list each time we parse mountstats
+            devices = list_nfs_mounts(origdevices, mountstats)
+            if len(devices) == 0:
+                print('No NFS mount points were found')
+                return
 
     args.infile.close()
     if args.since:
-- 
2.48.1

[nfs-utils PATCH 7/8] mountstats/nfsiostat: Move the checks for empty mountpoint list into the print function

[Frank Sorenson]

The test for empty device list and 'No NFS mount points found' message
terminate the program immediately if no nfs mounts are present during
a particular iteration.  However, if multiple iterations are specified,
it makes more sense to simply output the message and sleep for the
next iteration, since there may be nfs mounts next time through.

If we move the test and message into the print function, we still
get the message when appropriate, don't terminate on an empty list,
and also eliminate two extra copies of the same test and message.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 tools/mountstats/mountstats.py | 13 ++++---------
 tools/nfs-iostat/nfs-iostat.py | 14 ++++----------
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index e640642a..fbd57f51 100755
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -958,6 +958,10 @@ def nfsstat_command(args):
     return 0
 
 def print_iostat_summary(old, new, devices, time):
+    if len(devices) == 0:
+        print('No NFS mount points were found')
+        return
+
     for device in devices:
         stats = DeviceData()
         stats.parse_stats(new[device])
@@ -1005,9 +1009,6 @@ def iostat_command(args):
 
     # make certain devices contains only NFS mount points
     devices = list_nfs_mounts(origdevices, mountstats)
-    if len(devices) == 0:
-        print('No NFS mount points were found')
-        return 1
 
     sample_time = 0
 
@@ -1026,9 +1027,6 @@ def iostat_command(args):
             # nfs mountpoints may appear or disappear, so we need to
             # recheck the devices list each time we parse mountstats
             devices = list_nfs_mounts(origdevices, mountstats)
-            if len(devices) == 0:
-                print('No NFS mount points were found')
-                return
             count -= 1
     else: 
         while True:
@@ -1040,9 +1038,6 @@ def iostat_command(args):
             # nfs mountpoints may appear or disappear, so we need to
             # recheck the devices list each time we parse mountstats
             devices = list_nfs_mounts(origdevices, mountstats)
-            if len(devices) == 0:
-                print('No NFS mount points were found')
-                return
 
     args.infile.close()
     if args.since:
diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
index 1502b431..f69ffb0e 100755
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -478,6 +478,10 @@ def parse_stats_file(filename):
 def print_iostat_summary(old, new, devices, time, options):
     display_stats = {}
 
+    if len(devices) == 0:
+        print('No NFS mount points were found')
+        return
+
     for device in devices:
         stats = DeviceData()
         stats.parse_stats(new[device])
@@ -612,10 +616,6 @@ client are listed.
 
     # make certain devices contains only NFS mount points
     devices = list_nfs_mounts(origdevices, mountstats)
-    if len(devices) == 0:
-        print('No NFS mount points were found')
-        return
-
 
     old_mountstats = None
     sample_time = 0.0
@@ -634,9 +634,6 @@ client are listed.
             # nfs mountpoints may appear or disappear, so we need to
             # recheck the devices list each time we parse mountstats
             devices = list_nfs_mounts(origdevices,mountstats)
-            if len(devices) == 0:
-                print('No NFS mount points were found')
-                return
             count -= 1
     else: 
         while True:
@@ -648,9 +645,6 @@ client are listed.
             # nfs mountpoints may appear or disappear, so we need to
             # recheck the devices list each time we parse mountstats
             devices = list_nfs_mounts(origdevices,mountstats)
-            if len(devices) == 0:
-                print('No NFS mount points were found')
-                return
 
 #
 # Main
-- 
2.48.1

[nfs-utils PATCH 8/8] mountstats/nfsiostat: merge and rework the infinite and counted loops

[Frank Sorenson]

We always want to print at least once, so move the first print
before the interval/count tests.

The infinite loop and counted loop are nearly identical, so
we merge them, and break out of the loop if necessary.

By decrementing the count and checking at the beginning of the
loop, we also fix a bug where we sleep and parse the mountstats
file one extra time after count iterations.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 tools/mountstats/mountstats.py | 40 ++++++++++++++--------------------
 tools/nfs-iostat/nfs-iostat.py | 40 ++++++++++++++--------------------
 2 files changed, 32 insertions(+), 48 deletions(-)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index fbd57f51..d488f9e1 100755
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -1007,37 +1007,29 @@ def iostat_command(args):
     else:
         old_mountstats = None
 
+    sample_time = 0
+
     # make certain devices contains only NFS mount points
     devices = list_nfs_mounts(origdevices, mountstats)
-
-    sample_time = 0
+    print_iostat_summary(old_mountstats, mountstats, devices, sample_time)
 
     if args.interval is None:
-        print_iostat_summary(old_mountstats, mountstats, devices, sample_time)
         return
 
-    if args.count is not None:
-        count = args.count
-        while count != 0:
-            print_iostat_summary(old_mountstats, mountstats, devices, sample_time)
-            old_mountstats = mountstats
-            time.sleep(args.interval)
-            sample_time = args.interval
-            mountstats = parse_stats_file(args.infile)
-            # nfs mountpoints may appear or disappear, so we need to
-            # recheck the devices list each time we parse mountstats
-            devices = list_nfs_mounts(origdevices, mountstats)
+    count = args.count
+    while True:
+        if count is not None:
             count -= 1
-    else: 
-        while True:
-            print_iostat_summary(old_mountstats, mountstats, devices, sample_time)
-            old_mountstats = mountstats
-            time.sleep(args.interval)
-            sample_time = args.interval
-            mountstats = parse_stats_file(args.infile)
-            # nfs mountpoints may appear or disappear, so we need to
-            # recheck the devices list each time we parse mountstats
-            devices = list_nfs_mounts(origdevices, mountstats)
+            if count == 0:
+                break
+        time.sleep(args.interval)
+        old_mountstats = mountstats
+        sample_time = args.interval
+        mountstats = parse_stats_file(args.infile)
+        # nfs mountpoints may appear or disappear, so we need to
+        # recheck the devices list each time we parse mountstats
+        devices = list_nfs_mounts(origdevices, mountstats)
+        print_iostat_summary(old_mountstats, mountstats, devices, sample_time)
 
     args.infile.close()
     if args.since:
diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
index f69ffb0e..e46b1a83 100755
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -614,37 +614,29 @@ client are listed.
                 print('Illegal <count> value %s' % arg)
                 return
 
-    # make certain devices contains only NFS mount points
-    devices = list_nfs_mounts(origdevices, mountstats)
-
     old_mountstats = None
     sample_time = 0.0
 
+    # make certain devices contains only NFS mount points
+    devices = list_nfs_mounts(origdevices, mountstats)
+    print_iostat_summary(old_mountstats, mountstats, devices, sample_time, options)
+
     if not interval_seen:
-        print_iostat_summary(old_mountstats, mountstats, devices, sample_time, options)
         return
 
-    if count_seen:
-        while count != 0:
-            print_iostat_summary(old_mountstats, mountstats, devices, sample_time, options)
-            old_mountstats = mountstats
-            time.sleep(interval)
-            sample_time = interval
-            mountstats = parse_stats_file('/proc/self/mountstats')
-            # nfs mountpoints may appear or disappear, so we need to
-            # recheck the devices list each time we parse mountstats
-            devices = list_nfs_mounts(origdevices,mountstats)
+    while True:
+        if count_seen:
             count -= 1
-    else: 
-        while True:
-            print_iostat_summary(old_mountstats, mountstats, devices, sample_time, options)
-            old_mountstats = mountstats
-            time.sleep(interval)
-            sample_time = interval
-            mountstats = parse_stats_file('/proc/self/mountstats')
-            # nfs mountpoints may appear or disappear, so we need to
-            # recheck the devices list each time we parse mountstats
-            devices = list_nfs_mounts(origdevices,mountstats)
+            if count == 0:
+                break
+        time.sleep(interval)
+        old_mountstats = mountstats
+        sample_time = interval
+        mountstats = parse_stats_file('/proc/self/mountstats')
+        # nfs mountpoints may appear or disappear, so we need to
+        # recheck the devices list each time we parse mountstats
+        devices = list_nfs_mounts(origdevices, mountstats)
+        print_iostat_summary(old_mountstats, mountstats, devices, sample_time, options)
 
 #
 # Main
-- 
2.48.1

Re: [nfs-utils PATCH 0/8] mountstats/nfsiostat: bugfixes for iostat

[Chuck Lever]

On 1/30/25 9:19 AM, Frank Sorenson wrote:
> This patchset is intended to fix several bugs in nfsiostat and
> 'mountstats iostat', in particular when an <interval> is specified.
> 
> Specifically, the patches address the following issues when printing
> multiple times:
> 
> * both nfsiostat and 'mountstats iostat':
>    - if <count> is also specified, the scripts sleep an additional
>      <interval> after printing <count> times, and parse mountstats
>      unnecessarily before checking the <count>
> 
>    - if no nfs mounts are present when printing, the scripts output
>      a message that there are no nfs mounts, and exit immediately.
> 
>      However, if multiple intervals are indicated, it makes more sense
>      to output the message and sleep until the next iteration; there
>      may be nfs mounts the next time through.
> 
>    - if mountpoints are specified on the command-line, but are not
>      present during an iteration, the scripts crash.
> 
> 
>  * 'mountstats iostat':
>    - if an nfs filesystem is unmounted between iterations, the
>      script will crash attempting to reference the non-existent
>      mountpoint in the new mountstats file (or if another filesystem
>      type is mounted at that location, will instead crash while
>      comparing old and new stats)
> 
>    - new nfs mounts are not detected
> 
> 
>  * nfsiostat:
>    - if a new nfs filesystem is mounted between iterations, the
>      script will crash attempting to reference the non-existent
>      mountpoint in the old mountstats file
> 
>    - if a mountpoint is specified on the command-line, but topmost
>      mount there is not nfs, the script crashes while trying to
>      compare old and new stats
> 
> 
> To address these issues, the patches do the following:
>  * when printing diff stats from previous iteration:
>    - verify that a device exists in the old mountstats before referencing it
>    - verify that both old and new fstypes are the same
> 
>  * when filtering the current mountstats file:
>    - verify the device exists in the mountstats file before referencing it
>    - filter the list of nfs mountpoints each iostat iteration
> 
>  * check for empty device list and output the 'No NFS mount points found'
>     message, but don't immediately exit the script
> 
>  * merge the infinite loop and counted loop, and (for the counted loop)
>     decrement and check the count before sleeping and parsing the mountstats
>     file
> 
> 
> Frank Sorenson (8):
>   mountstats/nfsiostat: add a function to return the fstype
>   mountstats: when printing iostats, verify that old and new types are
>     the same
>   nfsiostat: mirror how mountstats iostat prints the stats
>   nfsiostat: fix crash when filtering mountstats after unmount
>   nfsiostat: make comment explain mount/unmount more broadly
>   mountstats: filter for nfs mounts in a function, each iostat iteration
>   mountstats/nfsiostat: Move the checks for empty mountpoint list into
>     the print function
>   mountstats/nfsiostat: merge and rework the infinite and counted loops
> 
>  tools/mountstats/mountstats.py | 102 ++++++++++++++++--------------
>  tools/nfs-iostat/nfs-iostat.py | 110 +++++++++++++--------------------
>  2 files changed, 100 insertions(+), 112 deletions(-)
> 

Hi Frank, I browsed this series briefly. I'm not sure that counts as a
full review, but it all seems sensible to me. Thanks for the clear
explanations!


-- 
Chuck Lever

Re: [nfs-utils PATCH 0/8] mountstats/nfsiostat: bugfixes for iostat

[Steve Dickson]

On 1/30/25 9:19 AM, Frank Sorenson wrote:
> This patchset is intended to fix several bugs in nfsiostat and
> 'mountstats iostat', in particular when an <interval> is specified.
> 
> Specifically, the patches address the following issues when printing
> multiple times:
> 
> * both nfsiostat and 'mountstats iostat':
>     - if <count> is also specified, the scripts sleep an additional
>       <interval> after printing <count> times, and parse mountstats
>       unnecessarily before checking the <count>
> 
>     - if no nfs mounts are present when printing, the scripts output
>       a message that there are no nfs mounts, and exit immediately.
> 
>       However, if multiple intervals are indicated, it makes more sense
>       to output the message and sleep until the next iteration; there
>       may be nfs mounts the next time through.
> 
>     - if mountpoints are specified on the command-line, but are not
>       present during an iteration, the scripts crash.
> 
> 
>   * 'mountstats iostat':
>     - if an nfs filesystem is unmounted between iterations, the
>       script will crash attempting to reference the non-existent
>       mountpoint in the new mountstats file (or if another filesystem
>       type is mounted at that location, will instead crash while
>       comparing old and new stats)
> 
>     - new nfs mounts are not detected
> 
> 
>   * nfsiostat:
>     - if a new nfs filesystem is mounted between iterations, the
>       script will crash attempting to reference the non-existent
>       mountpoint in the old mountstats file
> 
>     - if a mountpoint is specified on the command-line, but topmost
>       mount there is not nfs, the script crashes while trying to
>       compare old and new stats
> 
> 
> To address these issues, the patches do the following:
>   * when printing diff stats from previous iteration:
>     - verify that a device exists in the old mountstats before referencing it
>     - verify that both old and new fstypes are the same
> 
>   * when filtering the current mountstats file:
>     - verify the device exists in the mountstats file before referencing it
>     - filter the list of nfs mountpoints each iostat iteration
> 
>   * check for empty device list and output the 'No NFS mount points found'
>      message, but don't immediately exit the script
> 
>   * merge the infinite loop and counted loop, and (for the counted loop)
>      decrement and check the count before sleeping and parsing the mountstats
>      file
> 
> 
> Frank Sorenson (8):
>    mountstats/nfsiostat: add a function to return the fstype
>    mountstats: when printing iostats, verify that old and new types are
>      the same
>    nfsiostat: mirror how mountstats iostat prints the stats
>    nfsiostat: fix crash when filtering mountstats after unmount
>    nfsiostat: make comment explain mount/unmount more broadly
>    mountstats: filter for nfs mounts in a function, each iostat iteration
>    mountstats/nfsiostat: Move the checks for empty mountpoint list into
>      the print function
>    mountstats/nfsiostat: merge and rework the infinite and counted loops
> 
>   tools/mountstats/mountstats.py | 102 ++++++++++++++++--------------
>   tools/nfs-iostat/nfs-iostat.py | 110 +++++++++++++--------------------
>   2 files changed, 100 insertions(+), 112 deletions(-)
> 
Frank... nice work... but I hate to break the news... with all
these changes... You now own these commands! :-) Just ask mrchuck ;-)

Committed... (tag nfs-utils-2-8-3-rc5)

steved.