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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Aug 26, 2009, at 10:53 AM, Lans Carstensen wrote:

Greetings!  Comments inline.  Chuck Lever wrote:
Hi Lans-
I was thinking about this feature a little more. Would it be interesting to allow sorting by other metrics besides ops/s ? That's another feature that the "top" command has that this new option doesn't. I also don't like the bare integer options. What if, at some point, we want "-4" and "-6" for IPv6 support (not that we do, but what if)? Plus, having the bare integers doesn't give any clues about what the number means.
In which case I propose:
 --sort=ops --list=<n>
which provides more flexibility and helps make the options more self-documenting.

Again, thanks for looking this over.

I had thought about the sorting a little bit, but decided that every case we cared about devolved to sorting by ops/s. We'd use a tool like this in an operational group where they've lost control of a particular rogue workload in the mix of thousands of other workloads, and the operational group is trying to figure out what workload is generating a disproportionate amount of traffic (ops/ s). If someone wanted to extend the implementation further where leaving "--sort" unadorned sorts by ops and using "-- sort=readdirplus" or somesuch, I'd be all for it - but it didn't seem worth the work for our use. So I'd encourage that to be adopted as-is and if someone wants to put the extra effort into it I'm all for it.

Well, the problem is once we choose a command line option, it's hard to change it later. I think having two choices, like --sort=ops and -- sort=def[ault] would be fine for now, and give us room to grow later.

For the bare integer options I was thinking in terms of the existing "tail" and "head" implementations - that's what made the most sense to me and the couple of others looking over this here. Changing it to "--list=<n>" is fine if that suites you and others more. I can't quite fathom going after IPv4 or IPv6 stats with this, as /proc/self/ mountstats a) doesn't expose any relevant data that would be IPv4/ IPv6 oriented, and b) really never would as that's at an implementation layer below NFS ops. So the existing patch makes most sense to me, but if you and others want it changed I'd be fine w/ doing it.

The modern command line option on head/tail is --lines=<n>, which is also an OK name for this option.

IPv4/IPv6 was just an example... some applications, like ssh, use "-4" and "-6" already, which makes the use of -<n> ambiguous, I think.

On Aug 26, 2009, at 12:59 AM, Lans Carstensen wrote:
commit 062577877bd3f1d8da1edeb889a09d380da83364
Author: Lans Carstensen <Lans.Carstensen@xxxxxxxxxxxxxx>
Date:   Tue Aug 25 21:53:54 2009 -0700

  Adds --sort option to display mount point stats sorted by ops/s
  Adds -<n> option to only display stats for first <n> mount points
  E.g. the use of "--sort -1" should be useful in seeing stats for
  only the mountpoint with the highest ops/s.

diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs- iostat.py
index 9f3b3eb..5df9bfb 100644
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -20,7 +20,7 @@ along with this program; if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
"""

-import sys, os, time
+import sys, os, time, re

Iostats_version = '0.3'

@@ -353,6 +353,12 @@ class DeviceData:
       print '\t%7.3f' % rtt_per_op,
       print '\t%7.3f' % exe_per_op

+    def ops(self, sample_time):
+        sends = float(self.__rpc_data['rpcsends'])
+        if sample_time == 0:
+            sample_time = float(self.__nfs_data['age'])
+        return (sends / sample_time)
+
   def display_iostats(self, sample_time, which):
       """Display NFS and RPC stats in an iostat-like way
       """
@@ -421,6 +427,11 @@ def print_iostat_help(name):
print ' If one or more <mount point> names are specified, statistics for only these' print ' mount points will be displayed. Otherwise, all NFS mount points on the'
   print ' client are listed.'
+    print
+ print ' You can also specify "--sort" to sort the NFS mount points by ops/second,' + print ' and specify a number of mount points to return with - <num>, e.g. -1.' + print ' For example, use of "--sort -1" will iterate only showing you the stats'
+    print ' for the mount point with the highest ops/second.'

def parse_stats_file(filename):
   """pop the contents of a mountstats file into a dictionary,
@@ -446,7 +457,10 @@ def parse_stats_file(filename):

   return ms_dict

-def print_iostat_summary(old, new, devices, time, ac):
+def print_iostat_summary(old, new, devices, time, ac, sortbyops, entrycount):
+    diff_stats = {}
+    count = 1
+
   if old:
# Trim device list to only include intersection of old and new data,
       # this addresses umounts due to autofs mountpoints
@@ -455,15 +469,34 @@ def print_iostat_summary(old, new, devices, time, ac):
       devicelist = devices

   for device in devicelist:
+        count += 1
       stats = DeviceData()
       stats.parse_stats(new[device])
       if not old:
           stats.display_iostats(time, ac)
+            if (count>entrycount):
+                return
       else:
           old_stats = DeviceData()
           old_stats.parse_stats(old[device])
-            diff_stats = stats.compare_iostats(old_stats)
-            diff_stats.display_iostats(time, ac)
+            diff_stats[device] = stats.compare_iostats(old_stats)
+            if not sortbyops:
+                diff_stats[device].display_iostats(time, ac)
+                if (count>entrycount):
+                    return
+
+    if old and sortbyops:
+        # We now have compared data and can print a comparison
+        # ordered by mountpoint ops per second
+        count = 1
+
+ devicelist.sort(key=lambda x: diff_stats[x].ops(time), reverse=True)
+
+        for device in devicelist:
+            count += 1
+            diff_stats[device].display_iostats(time, ac)
+            if (count>entrycount):
+                return

def list_nfs_mounts(givenlist, mountstats):
   """return a list of NFS mounts given a list to validate or
@@ -497,6 +530,8 @@ def iostat_command(name):
   which = 0
   interval_seen = False
   count_seen = False
+    sortbyops = False
+    entrycount = sys.maxint

   for arg in sys.argv:
       if arg in ['-h', '--help', 'help', 'usage']:
@@ -507,6 +542,19 @@ def iostat_command(name):
           print '%s version %s' % (name, Iostats_version)
           return

+        if arg in ['-s', '--sort', 'sort']:
+            sortbyops = True
+            # sorted display infers a loop, default to 1 second
+            if not interval_seen:
+                interval = 1
+                interval_seen = True
+            continue
+
+        stop_re = re.compile('-[0-9]+')
+        if stop_re.match(arg):
+            entrycount = int(arg.lstrip('-'))
+            continue
+
       if arg in ['-a', '--attr']:
           which = 1
           continue
@@ -546,12 +594,12 @@ def iostat_command(name):
   sample_time = 0.0

   if not interval_seen:
- print_iostat_summary(old_mountstats, mountstats, devices, sample_time, which) + print_iostat_summary(old_mountstats, mountstats, devices, sample_time, which, sortbyops, entrycount)
       return

   if count_seen:
       while count != 0:
- print_iostat_summary(old_mountstats, mountstats, devices, sample_time, which) + print_iostat_summary(old_mountstats, mountstats, devices, sample_time, which, sortbyops, entrycount)
           old_mountstats = mountstats
           time.sleep(interval)
           sample_time = interval
@@ -562,7 +610,7 @@ def iostat_command(name):
           count -= 1
   else:
       while True:
- print_iostat_summary(old_mountstats, mountstats, devices, sample_time, which) + print_iostat_summary(old_mountstats, mountstats, devices, sample_time, which, sortbyops, entrycount)
           old_mountstats = mountstats
           time.sleep(interval)
           sample_time = interval



--
To unsubscribe from this list: send the line "unsubscribe linux- nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com


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



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux