[PATCH] nfsiostat: Add error count to end of RPC iostats version 1.1

[PATCH] nfsiostat: Add error count to end of RPC iostats version 1.1

[Dave Wysochanski]

With RPC iostats 1.1 there is a new metric which counts the RPCs
completing with errors (tk_status < 0).  Add these to the output at
the end of the line.  This increases the length of an output line
to 136 columns from 120, but keeps consistent format and spacing:

read:              ops/s            kB/s           kB/op         retrans    avg RTT (ms)    avg exe (ms)  avg queue (ms)          errors
                   0.000           0.106         512.316        0 (0.0%)          17.500          17.500           0.000        0 (0.0%)
write:             ops/s            kB/s           kB/op         retrans    avg RTT (ms)    avg exe (ms)  avg queue (ms)          errors
                   0.001           0.476         512.398        0 (0.0%)           1.667           5.778           3.889       1 (11.1%)

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/nfs-iostat/nfs-iostat.py | 15 +++++++++++++--
 tools/nfs-iostat/nfsiostat.man |  8 ++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 tools/nfs-iostat/nfs-iostat.py

diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
old mode 100644
new mode 100755
index dec0e861..1b0c4843
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -329,6 +329,8 @@ class DeviceData:
         queued_for = float(rpc_stats[5])
         rtt = float(rpc_stats[6])
         exe = float(rpc_stats[7])
+        if self.__rpc_data['statsvers'] == 1.1:
+            errs = int(rpc_stats[8])
 
         # prevent floating point exceptions
         if ops != 0:
@@ -337,6 +339,8 @@ class DeviceData:
             rtt_per_op = rtt / ops
             exe_per_op = exe / ops
             queued_for_per_op = queued_for / ops
+            if self.__rpc_data['statsvers'] == 1.1:
+                errs_percent = (errs * 100) / ops
         else:
             kb_per_op = 0.0
             retrans_percent = 0.0
@@ -352,7 +356,10 @@ class DeviceData:
         print(format('retrans', '>16s'), end='')
         print(format('avg RTT (ms)', '>16s'), end='')
         print(format('avg exe (ms)', '>16s'), end='')
-        print(format('avg queue (ms)', '>16s'))
+        print(format('avg queue (ms)', '>16s'), end='')
+        if self.__rpc_data['statsvers'] == 1.1:
+            print(format('errors', '>16s'), end='')
+        print('')
 
         print(format((ops / sample_time), '>24.3f'), end='')
         print(format((kilobytes / sample_time), '>16.3f'), end='')
@@ -361,7 +368,11 @@ class DeviceData:
         print(format(retransmits, '>16'), end='')
         print(format(rtt_per_op, '>16.3f'), end='')
         print(format(exe_per_op, '>16.3f'), end='')
-        print(format(queued_for_per_op, '>16.3f'))
+        print(format(queued_for_per_op, '>16.3f'), end='')
+        if self.__rpc_data['statsvers'] == 1.1:
+            errors = '{0:>10.0f} ({1:>3.1f}%)'.format(errs, errs_percent).strip()
+            print(format(errors, '>16'), end='')
+        print('')
 
     def ops(self, sample_time):
         sends = float(self.__rpc_data['rpcsends'])
diff --git a/tools/nfs-iostat/nfsiostat.man b/tools/nfs-iostat/nfsiostat.man
index 9ae94c5f..940c0431 100644
--- a/tools/nfs-iostat/nfsiostat.man
+++ b/tools/nfs-iostat/nfsiostat.man
@@ -97,6 +97,14 @@ This is the duration from the time the NFS client created the RPC request task t
 .RE
 .RE
 .RE
+.RS 8
+- \fBerrors\fR
+.RS
+This is the number of operations that completed with an error status (status < 0).  This count is only available on kernels with RPC iostats version 1.1 or above.
+.RS
+.RE
+.RE
+.RE
 .TP
 Note that if an interval is used as argument to \fBnfsiostat\fR, then the diffrence from previous interval will be displayed, otherwise the results will be from the time that the share was mounted.
 
-- 
2.20.1

[PATCHv2 1/3] nfsiostat: Add error counts to output when RPC iostats version >= 1.1

[Dave Wysochanski]

With RPC iostats 1.1 there is a new metric which counts the RPCs
completing with errors (tk_status < 0).  Add these to the output at
the end of the line.  This increases the length of an output line
to 136 columns from 120, but keeps consistent format and spacing:

read:              ops/s            kB/s           kB/op         retrans    avg RTT (ms)    avg exe (ms)  avg queue (ms)          errors
                   0.000           0.106         512.316        0 (0.0%)          17.500          17.500           0.000        0 (0.0%)
write:             ops/s            kB/s           kB/op         retrans    avg RTT (ms)    avg exe (ms)  avg queue (ms)          errors
                   0.001           0.476         512.398        0 (0.0%)           1.667           5.778           3.889       1 (11.1%)

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/nfs-iostat/nfs-iostat.py | 15 +++++++++++++--
 tools/nfs-iostat/nfsiostat.man |  8 ++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 tools/nfs-iostat/nfs-iostat.py

diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
old mode 100644
new mode 100755
index dec0e861..ef840efe
--- a/tools/nfs-iostat/nfs-iostat.py
+++ b/tools/nfs-iostat/nfs-iostat.py
@@ -329,6 +329,8 @@ class DeviceData:
         queued_for = float(rpc_stats[5])
         rtt = float(rpc_stats[6])
         exe = float(rpc_stats[7])
+        if self.__rpc_data['statsvers'] >= 1.1:
+            errs = float(rpc_stats[8])
 
         # prevent floating point exceptions
         if ops != 0:
@@ -337,6 +339,8 @@ class DeviceData:
             rtt_per_op = rtt / ops
             exe_per_op = exe / ops
             queued_for_per_op = queued_for / ops
+            if self.__rpc_data['statsvers'] >= 1.1:
+                errs_percent = (errs * 100) / ops
         else:
             kb_per_op = 0.0
             retrans_percent = 0.0
@@ -352,7 +356,10 @@ class DeviceData:
         print(format('retrans', '>16s'), end='')
         print(format('avg RTT (ms)', '>16s'), end='')
         print(format('avg exe (ms)', '>16s'), end='')
-        print(format('avg queue (ms)', '>16s'))
+        print(format('avg queue (ms)', '>16s'), end='')
+        if self.__rpc_data['statsvers'] >= 1.1:
+            print(format('errors', '>16s'), end='')
+        print()
 
         print(format((ops / sample_time), '>24.3f'), end='')
         print(format((kilobytes / sample_time), '>16.3f'), end='')
@@ -361,7 +368,11 @@ class DeviceData:
         print(format(retransmits, '>16'), end='')
         print(format(rtt_per_op, '>16.3f'), end='')
         print(format(exe_per_op, '>16.3f'), end='')
-        print(format(queued_for_per_op, '>16.3f'))
+        print(format(queued_for_per_op, '>16.3f'), end='')
+        if self.__rpc_data['statsvers'] >= 1.1:
+            errors = '{0:>10.0f} ({1:>3.1f}%)'.format(errs, errs_percent).strip()
+            print(format(errors, '>16'), end='')
+        print()
 
     def ops(self, sample_time):
         sends = float(self.__rpc_data['rpcsends'])
diff --git a/tools/nfs-iostat/nfsiostat.man b/tools/nfs-iostat/nfsiostat.man
index 9ae94c5f..940c0431 100644
--- a/tools/nfs-iostat/nfsiostat.man
+++ b/tools/nfs-iostat/nfsiostat.man
@@ -97,6 +97,14 @@ This is the duration from the time the NFS client created the RPC request task t
 .RE
 .RE
 .RE
+.RS 8
+- \fBerrors\fR
+.RS
+This is the number of operations that completed with an error status (status < 0).  This count is only available on kernels with RPC iostats version 1.1 or above.
+.RS
+.RE
+.RE
+.RE
 .TP
 Note that if an interval is used as argument to \fBnfsiostat\fR, then the diffrence from previous interval will be displayed, otherwise the results will be from the time that the share was mounted.
 
-- 
2.20.1

[PATCH 2/3] mountstats: Add per-op error counts to iostat command when RPC iostats version >= 1.1

[Dave Wysochanski]

This replicates the functionality in the nfsiostat command.
---
 tools/mountstats/mountstats.py | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index 2f525f4b..5f13bf8e 100755
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -607,6 +607,8 @@ class DeviceData:
         queued_for = float(rpc_stats[5])
         rtt = float(rpc_stats[6])
         exe = float(rpc_stats[7])
+        if self.__rpc_data['statsvers'] >= 1.1:
+            errs = int(rpc_stats[8])
 
         # prevent floating point exceptions
         if ops != 0:
@@ -615,6 +617,8 @@ class DeviceData:
             rtt_per_op = rtt / ops
             exe_per_op = exe / ops
             queued_for_per_op = queued_for / ops
+            if self.__rpc_data['statsvers'] >= 1.1:
+                errs_percent = (errs * 100) / ops
         else:
             kb_per_op = 0.0
             retrans_percent = 0.0
@@ -630,7 +634,10 @@ class DeviceData:
         print(format('retrans', '>16s'), end='')
         print(format('avg RTT (ms)', '>16s'), end='')
         print(format('avg exe (ms)', '>16s'), end='')
-        print(format('avg queue (ms)', '>16s'))
+        print(format('avg queue (ms)', '>16s'), end='')
+        if self.__rpc_data['statsvers'] >= 1.1:
+            print(format('errors', '>16s'), end='')
+        print()
 
         print(format((ops / sample_time), '>24.3f'), end='')
         print(format((kilobytes / sample_time), '>16.3f'), end='')
@@ -639,7 +646,11 @@ class DeviceData:
         print(format(retransmits, '>16'), end='')
         print(format(rtt_per_op, '>16.3f'), end='')
         print(format(exe_per_op, '>16.3f'), end='')
-        print(format(queued_for_per_op, '>16.3f'))
+        print(format(queued_for_per_op, '>16.3f'), end='')
+        if self.__rpc_data['statsvers'] >= 1.1:
+            errors = '{0:>10.0f} ({1:>3.1f}%)'.format(errs, errs_percent).strip()
+            print(format(errors, '>16'), end='')
+        print()
 
     def display_iostats(self, sample_time):
         """Display NFS and RPC stats in an iostat-like way
-- 
2.20.1

[PATCH 3/3] mountstats: Check for RPC iostats version >= 1.1 with error counts

[Dave Wysochanski]

Add explicit check for statsvers instead of array based check.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/mountstats/mountstats.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index 5f13bf8e..2ebbf945 100755
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -476,7 +476,7 @@ class DeviceData:
                 if retrans != 0:
                     print('\t%d retrans (%d%%)' % (retrans, ((retrans * 100) / count)), end=' ')
                     print('\t%d major timeouts' % stats[3], end='')
-                if len(stats) >= 10 and stats[9] != 0:
+                if self.__rpc_data['statsvers'] >= 1.1 and stats[9] != 0:
                     print('\t%d errors (%d%%)' % (stats[9], ((stats[9] * 100) / count)))
                 else:
                     print('')
-- 
2.20.1

Re: [PATCH 3/3] mountstats: Check for RPC iostats version >= 1.1 with error counts

[Chuck Lever]

> On Jun 13, 2019, at 8:03 AM, Dave Wysochanski <dwysocha@redhat.com> wrote:
> 
> Add explicit check for statsvers instead of array based check.

Hi Dave,

I don't understand why this change is necessary. The patch description
should explain.



> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
> tools/mountstats/mountstats.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
> index 5f13bf8e..2ebbf945 100755
> --- a/tools/mountstats/mountstats.py
> +++ b/tools/mountstats/mountstats.py
> @@ -476,7 +476,7 @@ class DeviceData:
>                 if retrans != 0:
>                     print('\t%d retrans (%d%%)' % (retrans, ((retrans * 100) / count)), end=' ')
>                     print('\t%d major timeouts' % stats[3], end='')
> -                if len(stats) >= 10 and stats[9] != 0:
> +                if self.__rpc_data['statsvers'] >= 1.1 and stats[9] != 0:
>                     print('\t%d errors (%d%%)' % (stats[9], ((stats[9] * 100) / count)))
>                 else:
>                     print('')
> -- 
> 2.20.1
> 

--
Chuck Lever

Re: [PATCH 3/3] mountstats: Check for RPC iostats version >= 1.1 with error counts

[Chuck Lever]

> On Jun 19, 2019, at 1:22 PM, David Wysochanski <dwysocha@redhat.com> wrote:
> 
> 
> 
> On Wed, Jun 19, 2019 at 12:35 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> 
> > On Jun 13, 2019, at 8:03 AM, Dave Wysochanski <dwysocha@redhat.com> wrote:
> > 
> > Add explicit check for statsvers instead of array based check.
> 
> Hi Dave,
> 
> I don't understand why this change is necessary. The patch description
> should explain.
> 
> 
> Steve had already taken commit 73491ef for mountstats which was an array based check.  This just makes this patch consistent with the others.  Is that what you mean - you want a statement about consistency and refer to the other commit?  How about:
> 
> Commit 73491ef added per-op error counts for mountstats command but used an array based check rather than checking statsver. Add explicit check for statsver instead of array based check for consistency with other tools.

This is a better patch description (explains "why" not "what"),
but I'm not clear why this change is necessary in either place.


> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > ---
> > tools/mountstats/mountstats.py | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
> > index 5f13bf8e..2ebbf945 100755
> > --- a/tools/mountstats/mountstats.py
> > +++ b/tools/mountstats/mountstats.py
> > @@ -476,7 +476,7 @@ class DeviceData:
> >                 if retrans != 0:
> >                     print('\t%d retrans (%d%%)' % (retrans, ((retrans * 100) / count)), end=' ')
> >                     print('\t%d major timeouts' % stats[3], end='')
> > -                if len(stats) >= 10 and stats[9] != 0:
> > +                if self.__rpc_data['statsvers'] >= 1.1 and stats[9] != 0:
> >                     print('\t%d errors (%d%%)' % (stats[9], ((stats[9] * 100) / count)))
> >                 else:
> >                     print('')
> > -- 
> > 2.20.1
> > 
> 
> --
> Chuck Lever
> 
> 
> 
> 
> 
> -- 
> Dave Wysochanski
> Principal Software Maintenance Engineer
> T: 919-754-4024  

--
Chuck Lever

Re: [PATCH 3/3] mountstats: Check for RPC iostats version >= 1.1 with error counts

[Chuck Lever]

> On Jun 19, 2019, at 1:42 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> 
> 
>> On Jun 19, 2019, at 1:22 PM, David Wysochanski <dwysocha@redhat.com> wrote:
>> 
>> 
>> 
>> On Wed, Jun 19, 2019 at 12:35 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 
>>> On Jun 13, 2019, at 8:03 AM, Dave Wysochanski <dwysocha@redhat.com> wrote:
>>> 
>>> Add explicit check for statsvers instead of array based check.
>> 
>> Hi Dave,
>> 
>> I don't understand why this change is necessary. The patch description
>> should explain.
>> 
>> 
>> Steve had already taken commit 73491ef for mountstats which was an array based check.  This just makes this patch consistent with the others.  Is that what you mean - you want a statement about consistency and refer to the other commit?  How about:
>> 
>> Commit 73491ef added per-op error counts for mountstats command but used an array based check rather than checking statsver. Add explicit check for statsver instead of array based check for consistency with other tools.
> 
> This is a better patch description (explains "why" not "what"),
> but I'm not clear why this change is necessary in either place.

In other words, was this change necessary to fix a bug? Or is
this a defensive change to make parsing more robust?


>>> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
>>> ---
>>> tools/mountstats/mountstats.py | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
>>> index 5f13bf8e..2ebbf945 100755
>>> --- a/tools/mountstats/mountstats.py
>>> +++ b/tools/mountstats/mountstats.py
>>> @@ -476,7 +476,7 @@ class DeviceData:
>>>                if retrans != 0:
>>>                    print('\t%d retrans (%d%%)' % (retrans, ((retrans * 100) / count)), end=' ')
>>>                    print('\t%d major timeouts' % stats[3], end='')
>>> -                if len(stats) >= 10 and stats[9] != 0:
>>> +                if self.__rpc_data['statsvers'] >= 1.1 and stats[9] != 0:
>>>                    print('\t%d errors (%d%%)' % (stats[9], ((stats[9] * 100) / count)))
>>>                else:
>>>                    print('')
>>> -- 
>>> 2.20.1
>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
>> 
>> 
>> -- 
>> Dave Wysochanski
>> Principal Software Maintenance Engineer
>> T: 919-754-4024  
> 
> --
> Chuck Lever

--
Chuck Lever

Re: [PATCH 3/3] mountstats: Check for RPC iostats version >= 1.1 with error counts

[Dave Wysochanski]

On Wed, 2019-06-19 at 13:45 -0400, Chuck Lever wrote:
> > On Jun 19, 2019, at 1:42 PM, Chuck Lever <chuck.lever@oracle.com>
> > wrote:
> > 
> > 
> > 
> > > On Jun 19, 2019, at 1:22 PM, David Wysochanski <
> > > dwysocha@redhat.com> wrote:
> > > 
> > > 
> > > 
> > > On Wed, Jun 19, 2019 at 12:35 PM Chuck Lever <
> > > chuck.lever@oracle.com> wrote:
> > > 
> > > 
> > > > On Jun 13, 2019, at 8:03 AM, Dave Wysochanski <
> > > > dwysocha@redhat.com> wrote:
> > > > 
> > > > Add explicit check for statsvers instead of array based check.
> > > 
> > > Hi Dave,
> > > 
> > > I don't understand why this change is necessary. The patch
> > > description
> > > should explain.
> > > 
> > > 
> > > Steve had already taken commit 73491ef for mountstats which was
> > > an array based check.  This just makes this patch consistent with
> > > the others.  Is that what you mean - you want a statement about
> > > consistency and refer to the other commit?  How about:
> > > 
> > > Commit 73491ef added per-op error counts for mountstats command
> > > but used an array based check rather than checking statsver. Add
> > > explicit check for statsver instead of array based check for
> > > consistency with other tools.
> > 
> > This is a better patch description (explains "why" not "what"),
> > but I'm not clear why this change is necessary in either place.
> 
> In other words, was this change necessary to fix a bug? Or is
> this a defensive change to make parsing more robust?
> 

I try to state "fix" somewhere in there when it is a bug fix - so no
this does not fix a bug.  In in some ways the original check was better
because it makes no assumption of what 'statsver' means at any time. 
I'm not sure if you're really concerned about the commit message or you
would prefer the array check?  I can see argument for array check and I
can change the other patches and resubmit if you prefer that.

Commit 73491ef added per-op error counts for mountstats command
but used an array based check rather than checking statsver.  Check
statsver >= 1.1 explicitly as this documents when this new count was
added to the kernel.


> 
> > > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > > > ---
> > > > tools/mountstats/mountstats.py | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/mountstats/mountstats.py
> > > > b/tools/mountstats/mountstats.py
> > > > index 5f13bf8e..2ebbf945 100755
> > > > --- a/tools/mountstats/mountstats.py
> > > > +++ b/tools/mountstats/mountstats.py
> > > > @@ -476,7 +476,7 @@ class DeviceData:
> > > >                if retrans != 0:
> > > >                    print('\t%d retrans (%d%%)' % (retrans,
> > > > ((retrans * 100) / count)), end=' ')
> > > >                    print('\t%d major timeouts' % stats[3],
> > > > end='')
> > > > -                if len(stats) >= 10 and stats[9] != 0:
> > > > +                if self.__rpc_data['statsvers'] >= 1.1 and
> > > > stats[9] != 0:
> > > >                    print('\t%d errors (%d%%)' % (stats[9],
> > > > ((stats[9] * 100) / count)))
> > > >                else:
> > > >                    print('')
> > > > -- 
> > > > 2.20.1
> > > > 
> > > 
> > > --
> > > Chuck Lever
> > > 
> > > 
> > > 
> > > 
> > > 
> > > -- 
> > > Dave Wysochanski
> > > Principal Software Maintenance Engineer
> > > T: 919-754-4024  
> > 
> > --
> > Chuck Lever
> 
> --
> Chuck Lever
> 
> 
> 

Re: [PATCH 3/3] mountstats: Check for RPC iostats version >= 1.1 with error counts

[Chuck Lever]

> On Jun 19, 2019, at 3:42 PM, Dave Wysochanski <dwysocha@redhat.com> wrote:
> 
> On Wed, 2019-06-19 at 13:45 -0400, Chuck Lever wrote:
>>> On Jun 19, 2019, at 1:42 PM, Chuck Lever <chuck.lever@oracle.com>
>>> wrote:
>>> 
>>> 
>>> 
>>>> On Jun 19, 2019, at 1:22 PM, David Wysochanski <
>>>> dwysocha@redhat.com> wrote:
>>>> 
>>>> 
>>>> 
>>>> On Wed, Jun 19, 2019 at 12:35 PM Chuck Lever <
>>>> chuck.lever@oracle.com> wrote:
>>>> 
>>>> 
>>>>> On Jun 13, 2019, at 8:03 AM, Dave Wysochanski <
>>>>> dwysocha@redhat.com> wrote:
>>>>> 
>>>>> Add explicit check for statsvers instead of array based check.
>>>> 
>>>> Hi Dave,
>>>> 
>>>> I don't understand why this change is necessary. The patch
>>>> description
>>>> should explain.
>>>> 
>>>> 
>>>> Steve had already taken commit 73491ef for mountstats which was
>>>> an array based check.  This just makes this patch consistent with
>>>> the others.  Is that what you mean - you want a statement about
>>>> consistency and refer to the other commit?  How about:
>>>> 
>>>> Commit 73491ef added per-op error counts for mountstats command
>>>> but used an array based check rather than checking statsver. Add
>>>> explicit check for statsver instead of array based check for
>>>> consistency with other tools.
>>> 
>>> This is a better patch description (explains "why" not "what"),
>>> but I'm not clear why this change is necessary in either place.
>> 
>> In other words, was this change necessary to fix a bug? Or is
>> this a defensive change to make parsing more robust?
>> 
> 
> I try to state "fix" somewhere in there when it is a bug fix - so no
> this does not fix a bug.  In in some ways the original check was better
> because it makes no assumption of what 'statsver' means at any time. 
> I'm not sure if you're really concerned about the commit message or you
> would prefer the array check?

Both. The array check is done for all the other variables too, IIRC.
There doesn't seem to be a reason to check the statvers. If it's not
too much trouble, please resubmit so that the new checks are consistent
with existing checks.


> I can see argument for array check and I
> can change the other patches and resubmit if you prefer that.
> 
> Commit 73491ef added per-op error counts for mountstats command
> but used an array based check rather than checking statsver.  Check
> statsver >= 1.1 explicitly as this documents when this new count was
> added to the kernel.

Not sure there's a need for the statvers bump here either. There
are some rules about when a statvers bump is necessary, but in my
old age I don't remember what they are. Anyway, if the statvers is
bumped already, no big deal.


>>>>> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
>>>>> ---
>>>>> tools/mountstats/mountstats.py | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/tools/mountstats/mountstats.py
>>>>> b/tools/mountstats/mountstats.py
>>>>> index 5f13bf8e..2ebbf945 100755
>>>>> --- a/tools/mountstats/mountstats.py
>>>>> +++ b/tools/mountstats/mountstats.py
>>>>> @@ -476,7 +476,7 @@ class DeviceData:
>>>>>               if retrans != 0:
>>>>>                   print('\t%d retrans (%d%%)' % (retrans,
>>>>> ((retrans * 100) / count)), end=' ')
>>>>>                   print('\t%d major timeouts' % stats[3],
>>>>> end='')
>>>>> -                if len(stats) >= 10 and stats[9] != 0:
>>>>> +                if self.__rpc_data['statsvers'] >= 1.1 and
>>>>> stats[9] != 0:
>>>>>                   print('\t%d errors (%d%%)' % (stats[9],
>>>>> ((stats[9] * 100) / count)))
>>>>>               else:
>>>>>                   print('')
>>>>> -- 
>>>>> 2.20.1
>>>>> 
>>>> 
>>>> --
>>>> Chuck Lever
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> -- 
>>>> Dave Wysochanski
>>>> Principal Software Maintenance Engineer
>>>> T: 919-754-4024  
>>> 
>>> --
>>> Chuck Lever
>> 
>> --
>> Chuck Lever

--
Chuck Lever

Re: [PATCH 3/3] mountstats: Check for RPC iostats version >= 1.1 with error counts

[Dave Wysochanski]

On Wed, 2019-06-19 at 15:46 -0400, Chuck Lever wrote:
> > On Jun 19, 2019, at 3:42 PM, Dave Wysochanski <dwysocha@redhat.com>
> > wrote:
> > 
> > On Wed, 2019-06-19 at 13:45 -0400, Chuck Lever wrote:
> > > > On Jun 19, 2019, at 1:42 PM, Chuck Lever <
> > > > chuck.lever@oracle.com>
> > > > wrote:
> > > > 
> > > > 
> > > > 
> > > > > On Jun 19, 2019, at 1:22 PM, David Wysochanski <
> > > > > dwysocha@redhat.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Wed, Jun 19, 2019 at 12:35 PM Chuck Lever <
> > > > > chuck.lever@oracle.com> wrote:
> > > > > 
> > > > > 
> > > > > > On Jun 13, 2019, at 8:03 AM, Dave Wysochanski <
> > > > > > dwysocha@redhat.com> wrote:
> > > > > > 
> > > > > > Add explicit check for statsvers instead of array based
> > > > > > check.
> > > > > 
> > > > > Hi Dave,
> > > > > 
> > > > > I don't understand why this change is necessary. The patch
> > > > > description
> > > > > should explain.
> > > > > 
> > > > > 
> > > > > Steve had already taken commit 73491ef for mountstats which
> > > > > was
> > > > > an array based check.  This just makes this patch consistent
> > > > > with
> > > > > the others.  Is that what you mean - you want a statement
> > > > > about
> > > > > consistency and refer to the other commit?  How about:
> > > > > 
> > > > > Commit 73491ef added per-op error counts for mountstats
> > > > > command
> > > > > but used an array based check rather than checking statsver.
> > > > > Add
> > > > > explicit check for statsver instead of array based check for
> > > > > consistency with other tools.
> > > > 
> > > > This is a better patch description (explains "why" not "what"),
> > > > but I'm not clear why this change is necessary in either place.
> > > 
> > > In other words, was this change necessary to fix a bug? Or is
> > > this a defensive change to make parsing more robust?
> > > 
> > 
> > I try to state "fix" somewhere in there when it is a bug fix - so
> > no
> > this does not fix a bug.  In in some ways the original check was
> > better
> > because it makes no assumption of what 'statsver' means at any
> > time. 
> > I'm not sure if you're really concerned about the commit message or
> > you
> > would prefer the array check?
> 
> Both. The array check is done for all the other variables too, IIRC.
> There doesn't seem to be a reason to check the statvers. If it's not
> too much trouble, please resubmit so that the new checks are
> consistent
> with existing checks.
> 

No problem at all - will do!


> 
> > I can see argument for array check and I
> > can change the other patches and resubmit if you prefer that.
> > 
> > Commit 73491ef added per-op error counts for mountstats command
> > but used an array based check rather than checking statsver.  Check
> > statsver >= 1.1 explicitly as this documents when this new count
> > was
> > added to the kernel.
> 
> Not sure there's a need for the statvers bump here either. There
> are some rules about when a statvers bump is necessary, but in my
> old age I don't remember what they are. Anyway, if the statvers is
> bumped already, no big deal.
> 

As far as I understood this version covers the rpc_iostats structure so
yes I thought it was necessary so I changed it when I added
"om_error_status"
http://git.linux-nfs.org/?p=trondmy/linux-nfs.git;a=patch;h=6e034f84c67d677e87e11e42d1faaca854023d16


> 
> > > > > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > > > > > ---
> > > > > > tools/mountstats/mountstats.py | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/tools/mountstats/mountstats.py
> > > > > > b/tools/mountstats/mountstats.py
> > > > > > index 5f13bf8e..2ebbf945 100755
> > > > > > --- a/tools/mountstats/mountstats.py
> > > > > > +++ b/tools/mountstats/mountstats.py
> > > > > > @@ -476,7 +476,7 @@ class DeviceData:
> > > > > >               if retrans != 0:
> > > > > >                   print('\t%d retrans (%d%%)' % (retrans,
> > > > > > ((retrans * 100) / count)), end=' ')
> > > > > >                   print('\t%d major timeouts' % stats[3],
> > > > > > end='')
> > > > > > -                if len(stats) >= 10 and stats[9] != 0:
> > > > > > +                if self.__rpc_data['statsvers'] >= 1.1 and
> > > > > > stats[9] != 0:
> > > > > >                   print('\t%d errors (%d%%)' % (stats[9],
> > > > > > ((stats[9] * 100) / count)))
> > > > > >               else:
> > > > > >                   print('')
> > > > > > -- 
> > > > > > 2.20.1
> > > > > > 
> > > > > 
> > > > > --
> > > > > Chuck Lever
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > Dave Wysochanski
> > > > > Principal Software Maintenance Engineer
> > > > > T: 919-754-4024  
> > > > 
> > > > --
> > > > Chuck Lever
> > > 
> > > --
> > > Chuck Lever
> 
> --
> Chuck Lever
> 
> 
>