nfs-utils: nfs-iostat.py autofs cleanup and option to sort by ops/s

PATCH In review Lans Carstensen 2 participants 5 messages Aug 26, 2009 – Aug 27, 2009

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Lans Carstensen

author

2009-08-26 04:59 UTC #0 lore raw

PATCH 3/4 nfs-utils: nfs-iostat.py autofs cleanup and option to sort by ops/s

commit ef558b7b978418ac3da35e36dcf02a223e4ebe2d

Author: Lans Carstensen <[email protected]>
Date:   Tue Aug 25 21:52:49 2009 -0700

Update list of mount points to parse stats for and drop mount points from stats collection when they are umounted. This ensures proper stats collection for autofs mountpoints.

tools/nfs-iostat/nfs-iostat.py +40 ~~-19~~

index 6ce31fc..9f3b3eb 100644
@@ -447,7 +447,14 @@ def parse_stats_file(filename):
     return ms_dict
 
 def print_iostat_summary(old, new, devices, time, ac):
-    for device in devices:
+    if old:
+        # Trim device list to only include intersection of old and new data,
+        # this addresses umounts due to autofs mountpoints
+        devicelist = filter(lambda x:x in devices,old)
+    else:
+        devicelist = devices
+
+    for device in devicelist:
         stats = DeviceData()
         stats.parse_stats(new[device])
         if not old:

@@ -458,11 +465,35 @@ def print_iostat_summary(old, new, devices, time, ac):
             diff_stats = stats.compare_iostats(old_stats)
             diff_stats.display_iostats(time, ac)
 
+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
+    """
+    list = []
+    if len(givenlist) > 0:
+        for device in givenlist:
+            stats = DeviceData()
+            stats.parse_stats(mountstats[device])
+            if stats.is_nfs_mountpoint():
+                list += [device]
+    else:
+        for device, descr in mountstats.iteritems():
+            stats = DeviceData()
+            stats.parse_stats(descr)
+            if stats.is_nfs_mountpoint():
+                list += [device]
+    if len(list) == 0:
+        print 'No NFS mount points were found'
+        return
+
+    return list
+
 def iostat_command(name):
     """iostat-like command for NFS mount points
     """
     mountstats = parse_stats_file('/proc/self/mountstats')
     devices = []
+    origdevices = []
     which = 0
     interval_seen = False
     count_seen = False

@@ -492,7 +523,7 @@ def iostat_command(name):
             continue
 
         if arg in mountstats:
-            devices += [arg]
+            origdevices += [arg]
         elif not interval_seen:
             interval = int(arg)
             if interval > 0:

@@ -509,23 +540,7 @@ def iostat_command(name):
                 return
 
     # make certain devices contains only NFS mount points
-    if len(devices) > 0:
-        check = []
-        for device in devices:
-            stats = DeviceData()
-            stats.parse_stats(mountstats[device])
-            if stats.is_nfs_mountpoint():
-                check += [device]
-        devices = check
-    else:
-        for device, descr in mountstats.iteritems():
-            stats = DeviceData()
-            stats.parse_stats(descr)
-            if stats.is_nfs_mountpoint():
-                devices += [device]
-    if len(devices) == 0:
-        print 'No NFS mount points were found'
-        return
+    devices = list_nfs_mounts(origdevices, mountstats)
 
     old_mountstats = None
     sample_time = 0.0

@@ -541,6 +556,9 @@ def iostat_command(name):
             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
+            devices = list_nfs_mounts(origdevices,mountstats)
             count -= 1
     else:
         while True:

@@ -549,6 +567,9 @@ def iostat_command(name):
             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
+            devices = list_nfs_mounts(origdevices,mountstats)
 
 #
 # Main

Chuck Lever

2009-08-26 14:31 UTC #1 lore raw

I'd like to try this one out so I'm sure I understand what's going on.

Quoted 120 lines · Lans Carstensen

commit ef558b7b978418ac3da35e36dcf02a223e4ebe2d

Author: Lans Carstensen <[email protected]>
Date:   Tue Aug 25 21:52:49 2009 -0700

Update list of mount points to parse stats for and drop mount points from stats collection when they are umounted. This ensures proper stats collection for autofs mountpoints.

tools/nfs-iostat/nfs-iostat.py +0 -0

index 6ce31fc..9f3b3eb 100644
@@ -447,7 +447,14 @@ def parse_stats_file(filename):
    return ms_dict

def print_iostat_summary(old, new, devices, time, ac):

tools/nfs-iostat/nfs-iostat.py +2 -1

-    for device in devices:
+    if old:
+        # Trim device list to only include intersection of old and

new data,

tools/nfs-iostat/nfs-iostat.py +6 -0

+        # this addresses umounts due to autofs mountpoints
+        devicelist = filter(lambda x:x in devices,old)
+    else:
+        devicelist = devices
+
+    for device in devicelist:
        stats = DeviceData()
        stats.parse_stats(new[device])
        if not old:

@@ -458,11 +465,35 @@ def print_iostat_summary(old, new, devices,

time, ac):
diff_stats = stats.compare_iostats(old_stats) diff_stats.display_iostats(time, ac)

tools/nfs-iostat/nfs-iostat.py +23 -0

+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
+    """
+    list = []
+    if len(givenlist) > 0:
+        for device in givenlist:
+            stats = DeviceData()
+            stats.parse_stats(mountstats[device])
+            if stats.is_nfs_mountpoint():
+                list += [device]
+    else:
+        for device, descr in mountstats.iteritems():
+            stats = DeviceData()
+            stats.parse_stats(descr)
+            if stats.is_nfs_mountpoint():
+                list += [device]
+    if len(list) == 0:
+        print 'No NFS mount points were found'
+        return
+
+    return list
+

def iostat_command(name):
"""iostat-like command for NFS mount points """
mountstats = parse_stats_file('/proc/self/mountstats') devices = []

tools/nfs-iostat/nfs-iostat.py +4 ~~-18~~

+    origdevices = []
    which = 0
    interval_seen = False
    count_seen = False

@@ -492,7 +523,7 @@ def iostat_command(name):
            continue
 
        if arg in mountstats:
-            devices += [arg]
+            origdevices += [arg]
        elif not interval_seen:
            interval = int(arg)
            if interval > 0:

@@ -509,23 +540,7 @@ def iostat_command(name):
                return
 
    # make certain devices contains only NFS mount points
-    if len(devices) > 0:
-        check = []
-        for device in devices:
-            stats = DeviceData()
-            stats.parse_stats(mountstats[device])
-            if stats.is_nfs_mountpoint():
-                check += [device]
-        devices = check
-    else:
-        for device, descr in mountstats.iteritems():
-            stats = DeviceData()
-            stats.parse_stats(descr)
-            if stats.is_nfs_mountpoint():
-                devices += [device]
-    if len(devices) == 0:
-        print 'No NFS mount points were found'
-        return
+    devices = list_nfs_mounts(origdevices, mountstats)
 
    old_mountstats = None
    sample_time = 0.0

@@ -541,6 +556,9 @@ def iostat_command(name):
            time.sleep(interval)
            sample_time = interval
            mountstats = parse_stats_file('/proc/self/mountstats')
+            # automount mountpoints add and drop, if automount is

involved

tools/nfs-iostat/nfs-iostat.py +3 -0

+            # we need to recheck the devices list when reparsing
+            devices = list_nfs_mounts(origdevices,mountstats)
            count -= 1
    else:
        while True:

@@ -549,6 +567,9 @@ def iostat_command(name):
            time.sleep(interval)
            sample_time = interval
            mountstats = parse_stats_file('/proc/self/mountstats')
+            # automount mountpoints add and drop, if automount is

involved

tools/nfs-iostat/nfs-iostat.py +2 -0

+            # we need to recheck the devices list when reparsing
+            devices = list_nfs_mounts(origdevices,mountstats)

Quoted 8 lines

#
# Main
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Chuck Lever

2009-08-26 21:12 UTC #2 lore raw

In reply to Lans Carstensen

Quoted 44 lines · Lans Carstensen

commit ef558b7b978418ac3da35e36dcf02a223e4ebe2d
Author: Lans Carstensen <[email protected]>
Date:   Tue Aug 25 21:52:49 2009 -0700
Update list of mount points to parse stats for and drop mount points from stats collection when they are umounted. This ensures proper stats collection for autofs mountpoints.
tools/nfs-iostat/nfs-iostat.py +0 -0
index 6ce31fc..9f3b3eb 100644
@@ -447,7 +447,14 @@ def parse_stats_file(filename):
    return ms_dict
 
def print_iostat_summary(old, new, devices, time, ac):
tools/nfs-iostat/nfs-iostat.py +2 -1
-    for device in devices:
+    if old:
+        # Trim device list to only include intersection of old and  
new data,
tools/nfs-iostat/nfs-iostat.py +6 -0
+        # this addresses umounts due to autofs mountpoints
+        devicelist = filter(lambda x:x in devices,old)
+    else:
+        devicelist = devices
+
+    for device in devicelist:
        stats = DeviceData()
        stats.parse_stats(new[device])
        if not old:
@@ -458,11 +465,35 @@ def print_iostat_summary(old, new, devices,  
time, ac):
diff_stats = stats.compare_iostats(old_stats) diff_stats.display_iostats(time, ac)
tools/nfs-iostat/nfs-iostat.py +10 -0
+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
+    """
+    list = []
+    if len(givenlist) > 0:
+        for device in givenlist:
+            stats = DeviceData()
+            stats.parse_stats(mountstats[device])
+            if stats.is_nfs_mountpoint():

tools/nfs-iostat/nfs-iostat.py def print_iostat_summary(old, new, devices, time, ac): · line 487

Quoted 10 lines

tools/nfs-iostat/nfs-iostat.py +10 -0

+                list += [device]
+    else:
+        for device, descr in mountstats.iteritems():
+            stats = DeviceData()
+            stats.parse_stats(descr)
+            if stats.is_nfs_mountpoint():
+                list += [device]
+    if len(list) == 0:
+        print 'No NFS mount points were found'
+        return

You refactored this logic from the main routine. I think this last "return" was OK in the main routine, but in a subroutine it may not do exactly what you want. A NoneType object is passed to print_iostat_summary in this case.

[cel@matisse tmp]$ ./nfs-iostat.py
No NFS mount points were found
Traceback (most recent call last):
File "./nfs-iostat.py", line 580, in <module>
iostat_command(prog)
File "./nfs-iostat.py", line 549, in iostat_command
print_iostat_summary(old_mountstats, mountstats, devices,
sample_time, which)
File "./nfs-iostat.py", line 457, in print_iostat_summary
for device in devicelist:
TypeError: 'NoneType' object is not iterable

Quoted 66 lines

tools/nfs-iostat/nfs-iostat.py +3 -0

+
+    return list
+

def iostat_command(name):
"""iostat-like command for NFS mount points """
mountstats = parse_stats_file('/proc/self/mountstats') devices = []

tools/nfs-iostat/nfs-iostat.py +4 ~~-18~~

+    origdevices = []
    which = 0
    interval_seen = False
    count_seen = False

@@ -492,7 +523,7 @@ def iostat_command(name):
            continue
 
        if arg in mountstats:
-            devices += [arg]
+            origdevices += [arg]
        elif not interval_seen:
            interval = int(arg)
            if interval > 0:

@@ -509,23 +540,7 @@ def iostat_command(name):
                return
 
    # make certain devices contains only NFS mount points
-    if len(devices) > 0:
-        check = []
-        for device in devices:
-            stats = DeviceData()
-            stats.parse_stats(mountstats[device])
-            if stats.is_nfs_mountpoint():
-                check += [device]
-        devices = check
-    else:
-        for device, descr in mountstats.iteritems():
-            stats = DeviceData()
-            stats.parse_stats(descr)
-            if stats.is_nfs_mountpoint():
-                devices += [device]
-    if len(devices) == 0:
-        print 'No NFS mount points were found'
-        return
+    devices = list_nfs_mounts(origdevices, mountstats)
 
    old_mountstats = None
    sample_time = 0.0

@@ -541,6 +556,9 @@ def iostat_command(name):
            time.sleep(interval)
            sample_time = interval
            mountstats = parse_stats_file('/proc/self/mountstats')
+            # automount mountpoints add and drop, if automount is

involved

tools/nfs-iostat/nfs-iostat.py +3 -0

+            # we need to recheck the devices list when reparsing
+            devices = list_nfs_mounts(origdevices,mountstats)
            count -= 1
    else:
        while True:

@@ -549,6 +567,9 @@ def iostat_command(name):
            time.sleep(interval)
            sample_time = interval
            mountstats = parse_stats_file('/proc/self/mountstats')
+            # automount mountpoints add and drop, if automount is

involved

tools/nfs-iostat/nfs-iostat.py +2 -0

+            # we need to recheck the devices list when reparsing
+            devices = list_nfs_mounts(origdevices,mountstats)

Quoted 8 lines

#
# Main
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Lans Carstensen

author

2009-08-26 23:56 UTC #3 lore raw

Quoted 60 lines · Chuck Lever

Quoted 44 lines · Lans Carstensen
commit ef558b7b978418ac3da35e36dcf02a223e4ebe2d
Author: Lans Carstensen <[email protected]>
Date:   Tue Aug 25 21:52:49 2009 -0700
Update list of mount points to parse stats for and drop mount points from stats collection when they are umounted. This ensures proper stats collection for autofs mountpoints.
tools/nfs-iostat/nfs-iostat.py +0 -0
index 6ce31fc..9f3b3eb 100644
@@ -447,7 +447,14 @@ def parse_stats_file(filename):
    return ms_dict
 
def print_iostat_summary(old, new, devices, time, ac):
tools/nfs-iostat/nfs-iostat.py +2 -1
-    for device in devices:
+    if old:
+        # Trim device list to only include intersection of old and 
new data,
tools/nfs-iostat/nfs-iostat.py +6 -0
+        # this addresses umounts due to autofs mountpoints
+        devicelist = filter(lambda x:x in devices,old)
+    else:
+        devicelist = devices
+
+    for device in devicelist:
        stats = DeviceData()
        stats.parse_stats(new[device])
        if not old:
@@ -458,11 +465,35 @@ def print_iostat_summary(old, new, devices, 
time, ac):
diff_stats = stats.compare_iostats(old_stats) diff_stats.display_iostats(time, ac)
tools/nfs-iostat/nfs-iostat.py +10 -0
+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
+    """
+    list = []
+    if len(givenlist) > 0:
+        for device in givenlist:
+            stats = DeviceData()
+            stats.parse_stats(mountstats[device])
+            if stats.is_nfs_mountpoint():
tools/nfs-iostat/nfs-iostat.py def print_iostat_summary(old, new, devices, time, ac): · line 487
Quoted 10 lines
tools/nfs-iostat/nfs-iostat.py +10 -0
+                list += [device]
+    else:
+        for device, descr in mountstats.iteritems():
+            stats = DeviceData()
+            stats.parse_stats(descr)
+            if stats.is_nfs_mountpoint():
+                list += [device]
+    if len(list) == 0:
+        print 'No NFS mount points were found'
+        return
You refactored this logic from the main routine. I think this last "return" was OK in the main routine, but in a subroutine it may not do exactly what you want. A NoneType object is passed to print_iostat_summary in this case

Quoted 11 lines

[cel@matisse tmp]$ ./nfs-iostat.py
No NFS mount points were found
Traceback (most recent call last):
File "./nfs-iostat.py", line 580, in <module>
iostat_command(prog)
File "./nfs-iostat.py", line 549, in iostat_command
print_iostat_summary(old_mountstats, mountstats, devices,
sample_time, which)
File "./nfs-iostat.py", line 457, in print_iostat_summary
for device in devicelist:
TypeError: 'NoneType' object is not iterable

Sigh. Thanks. I'll test more thoroughly and resubmit. Based on the --list= argument change it'll probably be easier to refactor the script to use optparse, so I'll end up regenerating the last two patches as probably three new ones.

-- Lans Carstensen, Systems Engineering, Dreamworks Animation Because they consistently observe and listen, the humble improve.
-- Wynton Marsalis

Chuck Lever

2009-08-27 14:09 UTC #4 lore raw

Quoted 69 lines · Lans Carstensen

Quoted 59 lines · Chuck Lever
Quoted 44 lines · Lans Carstensen
commit ef558b7b978418ac3da35e36dcf02a223e4ebe2d
Author: Lans Carstensen <[email protected]>
Date:   Tue Aug 25 21:52:49 2009 -0700
Update list of mount points to parse stats for and drop mount points from stats collection when they are umounted. This ensures proper stats collection for autofs mountpoints.
tools/nfs-iostat/nfs-iostat.py +0 -0
index 6ce31fc..9f3b3eb 100644
@@ -447,7 +447,14 @@ def parse_stats_file(filename):
   return ms_dict
 
def print_iostat_summary(old, new, devices, time, ac):
tools/nfs-iostat/nfs-iostat.py +2 -1
-    for device in devices:
+    if old:
+        # Trim device list to only include intersection of old  
and new data,
tools/nfs-iostat/nfs-iostat.py +6 -0
+        # this addresses umounts due to autofs mountpoints
+        devicelist = filter(lambda x:x in devices,old)
+    else:
+        devicelist = devices
+
+    for device in devicelist:
       stats = DeviceData()
       stats.parse_stats(new[device])
       if not old:
@@ -458,11 +465,35 @@ def print_iostat_summary(old, new, devices,  
time, ac):
diff_stats = stats.compare_iostats(old_stats) diff_stats.display_iostats(time, ac)
tools/nfs-iostat/nfs-iostat.py +10 -0
+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
+    """
+    list = []
+    if len(givenlist) > 0:
+        for device in givenlist:
+            stats = DeviceData()
+            stats.parse_stats(mountstats[device])
+            if stats.is_nfs_mountpoint():
tools/nfs-iostat/nfs-iostat.py def print_iostat_summary(old, new, devices, time, ac): · line 487
Quoted 10 lines
tools/nfs-iostat/nfs-iostat.py +10 -0
+                list += [device]
+    else:
+        for device, descr in mountstats.iteritems():
+            stats = DeviceData()
+            stats.parse_stats(descr)
+            if stats.is_nfs_mountpoint():
+                list += [device]
+    if len(list) == 0:
+        print 'No NFS mount points were found'
+        return
You refactored this logic from the main routine. I think this last "return" was OK in the main routine, but in a subroutine it may not do exactly what you want. A NoneType object is passed to print_iostat_summary in this case [cel@matisse tmp]$ ./nfs-iostat.py
No NFS mount points were found
Traceback (most recent call last):
Quoted 8 lines
File "./nfs-iostat.py", line 580, in <module>
iostat_command(prog)
File "./nfs-iostat.py", line 549, in iostat_command
print_iostat_summary(old_mountstats, mountstats, devices,
sample_time, which)
File "./nfs-iostat.py", line 457, in print_iostat_summary
for device in devicelist:
TypeError: 'NoneType' object is not iterable

Quoted 1 line

Sigh. Thanks. I'll test more thoroughly and resubmit. Based on the --list= argument change it'll probably be easier to refactor the script to use optparse, so I'll end up regenerating the last two patches as probably three new ones.

Thanks, these are good changes.

When you resubmit, can you add a Signed-off-by: line at the bottom of each patch description? You can look at this page for more information:

http://kerneltrap.org/taxonomy/term/245

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com