Return-Path: <linux-nfs-owner@vger.kernel.org>
Received: from rcsinet11.oracle.com ([148.87.113.123]:31987 "EHLO
	rgminet11.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751134AbZHZPr7 (ORCPT
	<rfc822;linux-nfs@vger.kernel.org>); Wed, 26 Aug 2009 11:47:59 -0400
Cc: NFS list <linux-nfs@vger.kernel.org>
Message-Id: <08E0F2AC-2747-4F98-9686-CF665490B0F1@oracle.com>
From: Chuck Lever <chuck.lever@oracle.com>
To: Lans Carstensen <Lans.Carstensen@dreamworks.com>
In-Reply-To: <4A954C4D.3060701@dreamworks.com>
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes
Subject: Re: [PATCH 4/4] nfs-utils: nfs-iostat.py autofs cleanup and option to sort by ops/s
Date: Wed, 26 Aug 2009 11:47:18 -0400
References: <4A94C13E.6070403@dreamworks.com> <10C29C3A-4BF4-4C45-AEEF-CE22F77911BD@oracle.com> <4A954C4D.3060701@dreamworks.com>
Sender: linux-nfs-owner@vger.kernel.org
List-ID: <linux-nfs.vger.kernel.org>
MIME-Version: 1.0


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@dreamworks.com>
>>> 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@vger.kernel.org
>>> 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