Re: [NFS] [PATCH] nfs-utils: nfs-iostat.py option to sort by ops/s

[Lans Carstensen <Lans.Carstensen@dreamworks.com>]

Chuck Lever wrote:
> On Aug 25, 2009, at 1:34 PM, Steve Dickson wrote:
>> On 08/25/2009 12:29 PM, Chuck Lever wrote:
>>> [ Cc: changed to correct mailing list.  sf.net list is deprecated. ]
>>>
>>> On Aug 24, 2009, at 7:24 PM, Lans Carstensen wrote:
>>>> Hi,
>>>>
>>>> I've recently made tools/nfs-iostat/nfs-iostat.py more useful in our
>>>> autofs environment with a variety of cleanups and am offering this
>>>> patch up for discussion and/or inclusion in nfs-utils.  It does the
>>>> following:
>>>>
>>>> * Adds a --top flag to sort the display of mountpoint entries by ops/s.
>>>> * Adds a --<n> flag to only display stats for the first <n> mountpoints
>>>> * Re-reads the mountpoint list on intervals since it's dynamic in an
>>>> autofs environment.
>>>> * Conforms the Python path to the LSB 3.2+ standard of /usr/bin/python
>>>> http://refspecs.freestandards.org/LSB_3.2.0/LSB-Languages/LSB-Languages/pylocation.html 
>>>>
>>>>
>>>
>>> A couple of overall comments.
>>>
>>> 1.  These seem to be logically independent changes, so we would prefer
>>> them in separate patches.  Each logical change can be refined and voted
>>> up or down separately.
>> I guess you could break this up into more patches... but overall its
>> by no means unruly....
> 
> Didn't mean to imply "unruly" but we do _prefer_ having logical changes 
> separated.
> 
>>> 2.  Sorting by ops is OK, but not sure about "--top".  Since the script
>>> doesn't generate a curses like display like "top" does, maybe "--sort
>>> <n>" would be a better name, and --top and --<n> could be combined.
>> Hmm.. I kinda like like the --top flag... its pretty descriptive to
>> what it does..
> 
> Some people aren't so imaginative, might expect "--top" to produce a 
> full curses-like display, then be pissed when they get a simple list.  
> And it seems to me that the two new options are related and could be 
> combined to good effect.
> 
> I think "--top" should be reserved for an actual top-like implementation 
> (which might be pretty cool).
> 
>>> 3.  The distributors should weigh in on the Python path change.
>> Using '#!/usr/bin/python' is better than '#!/usr/bin/env python'
>> since you know exactly which python binary you will be using.
>> It was also pointed out the env convention will confuse
>> rpm's dependency generator
> 
> Good enough for me.  Lans, it would be nice to document this last bit 
> (about rpm) in the patch description.

Thanks for looking at this, guys.

* I don't live in git, so it'll take me a little bit to break out the 
patches - but I certainly don't mind if that helps this along.

* To aid your testing on rescanning, there is a rescan but currently no 
handling of differences in the "new" and "old" mount table and there is 
no re-evaluation of the list of monitored mountpoints based on changes 
in the mount table.  The existing implementation will throw a Python 
exception when a mountpoint (say a user homedirectory) is unmounted from 
a system during a trace.  The displayed data is also misleading if the 
list of monitored mountpoints isn't grown to reflect new automounts that 
might be your current top talkers.

* "--sort" doesn't offend me and I can switch to that.  The semantic of 
"--top -1" was the most intuitive for a couple of us here but "--sort 
-1" isn't that far off.

* I agree a real "top" implementation would be cool.

-- Lans