From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mta1.dreamworks.com ([208.71.56.12]:60553 "EHLO michael.anim.dreamworks.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755616AbZHYS4R (ORCPT ); Tue, 25 Aug 2009 14:56:17 -0400 Message-ID: <4A942F4A.9040902@dreamworks.com> Date: Tue, 25 Aug 2009 11:36:58 -0700 From: Lans Carstensen To: Chuck Lever CC: Steve Dickson , NFS list Subject: Re: [NFS] [PATCH] nfs-utils: nfs-iostat.py option to sort by ops/s References: <4A93214A.5000404@dreamworks.com> <009376AC-B08B-4E90-A46B-E5BC069A5A78@oracle.com> <4A942089.1010102@RedHat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 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 -- flag to only display stats for the first 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 >>> " would be a better name, and --top and -- 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