On Wed, 2019-06-19 at 15:46 -0400, Chuck Lever wrote: > > On Jun 19, 2019, at 3:42 PM, Dave Wysochanski <dwysocha@xxxxxxxxxx> > > 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@xxxxxxxxxx> > > > > wrote: > > > > > > > > > > > > > > > > > On Jun 19, 2019, at 1:22 PM, David Wysochanski < > > > > > dwysocha@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > On Wed, Jun 19, 2019 at 12:35 PM Chuck Lever < > > > > > chuck.lever@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > On Jun 13, 2019, at 8:03 AM, Dave Wysochanski < > > > > > > dwysocha@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > > > --- > > > > > > 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 > > >
