| Message ID | 20190613120314.1864-3-dwysocha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsiostat: Add error count to end of RPC iostats version 1.1 | expand |
> 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
> 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
> 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
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 > > >
> 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
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 > > >
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('')
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(-)