[PATCH 0/2] Mountstats Fixes

[PATCH 0/2] Mountstats Fixes

[Anna Schumaker]

I had some trouble using mountstats to analyze READ_PLUS operations, so I
wrote these patches to fix my issues.

I found that /proc/self/mountstats was reporting "GETDEVICELIST" as "51",
most likely because it was removed from the client but the nfs4_procedures
array still has a placeholder for it.  Mountstats crashes when it tries to
analyze "51", so the second patch fixes this issue by looking for numeric
operation names.  I'm willing to put in stubs for GETDEVICELIST back into
the client if that would be a better fix.

Thanks,
Anna


Anna Schumaker (2):
  mountstats: Resync NFSv4 Operations List
  mountstats: Only count operations that have names

 tools/mountstats/mountstats.py | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
2.3.5

[PATCH 1/2] mountstats: Resync NFSv4 Operations List

[Anna Schumaker]

We've added several operations and removed GETDEVICELIST,  Let's keep
the Nfsv4ops list up to date with the kernel.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 tools/mountstats/mountstats.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index 38943eb..8f2e26e 100644
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -195,16 +195,18 @@ Nfsv4ops = [
     'SEQUENCE',
     'GET_LEASE_TIME',
     'RECLAIM_COMPLETE',
-    'LAYOUTGET',
     'GETDEVICEINFO',
+    'LAYOUTGET',
     'LAYOUTCOMMIT',
     'LAYOUTRETURN',
     'SECINFO_NO_NAME',
     'TEST_STATEID',
     'FREE_STATEID',
-    'GETDEVICELIST',
     'BIND_CONN_TO_SESSION',
-    'DESTROY_CLIENTID'
+    'DESTROY_CLIENTID',
+    'SEEK',
+    'ALLOCATE',
+    'DEALLOCATE',
 ]
 
 class DeviceData:
-- 
2.3.5

[PATCH 2/2] mountstats: Only count operations that have names

[Anna Schumaker]

GETDEVICELIST was removed from the kernel, but due to the way the client
is coded it still shows up in /proc/self/mountstats as "51".  This
causes mountstats to crash when asked to output "raw" statistics, so
this patch teaches mountsats to ignore any operations that only have
numeric names.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 tools/mountstats/mountstats.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
index 8f2e26e..5f243ac 100644
--- a/tools/mountstats/mountstats.py
+++ b/tools/mountstats/mountstats.py
@@ -284,8 +284,9 @@ class DeviceData:
             self.__rpc_data['per-op'] = words
         else:
             op = words[0][:-1]
-            self.__rpc_data['ops'] += [op]
-            self.__rpc_data[op] = [int(word) for word in words[1:]]
+            if not op.isnumeric():
+                self.__rpc_data['ops'] += [op]
+                self.__rpc_data[op] = [int(word) for word in words[1:]]
 
     def parse_stats(self, lines):
         """Turn a list of lines from a mount stat file into a 
-- 
2.3.5

Re: [PATCH 0/2] Mountstats Fixes

[Trond Myklebust]

On Wed, Apr 8, 2015 at 12:47 PM, Anna Schumaker
<Anna.Schumaker@netapp.com> wrote:
> I had some trouble using mountstats to analyze READ_PLUS operations, so I
> wrote these patches to fix my issues.
>
> I found that /proc/self/mountstats was reporting "GETDEVICELIST" as "51",

Shouldn't we try to fix that as being a regression in the client? I
see no reason why we shouldn't be able to add in a p_name for
GETDEVICELIST.

> most likely because it was removed from the client but the nfs4_procedures
> array still has a placeholder for it.  Mountstats crashes when it tries to
> analyze "51", so the second patch fixes this issue by looking for numeric
> operation names.  I'm willing to put in stubs for GETDEVICELIST back into
> the client if that would be a better fix.
>
> Thanks,
> Anna
>
>
> Anna Schumaker (2):
>   mountstats: Resync NFSv4 Operations List
>   mountstats: Only count operations that have names
>
>  tools/mountstats/mountstats.py | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> --
> 2.3.5
>
> --
> 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

Re: [PATCH 0/2] Mountstats Fixes

[Anna Schumaker]

On 04/08/2015 01:01 PM, Trond Myklebust wrote:
> On Wed, Apr 8, 2015 at 12:47 PM, Anna Schumaker
> <Anna.Schumaker@netapp.com> wrote:
>> I had some trouble using mountstats to analyze READ_PLUS operations, so I
>> wrote these patches to fix my issues.
>>
>> I found that /proc/self/mountstats was reporting "GETDEVICELIST" as "51",
> 
> Shouldn't we try to fix that as being a regression in the client? I
> see no reason why we shouldn't be able to add in a p_name for
> GETDEVICELIST.

I'm cool with that.  I wasn't sure if it fell under "don't break userspace" if I'm the only person to have a problem in however long it's been since GETDEVICELIST was removed.

Anna

> 
>> most likely because it was removed from the client but the nfs4_procedures
>> array still has a placeholder for it.  Mountstats crashes when it tries to
>> analyze "51", so the second patch fixes this issue by looking for numeric
>> operation names.  I'm willing to put in stubs for GETDEVICELIST back into
>> the client if that would be a better fix.
>>
>> Thanks,
>> Anna
>>
>>
>> Anna Schumaker (2):
>>   mountstats: Resync NFSv4 Operations List
>>   mountstats: Only count operations that have names
>>
>>  tools/mountstats/mountstats.py | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> --
>> 2.3.5
>>
>> --
>> 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

Re: [PATCH 1/2] mountstats: Resync NFSv4 Operations List

[Christoph Hellwig]

On Wed, Apr 08, 2015 at 12:47:26PM -0400, Anna Schumaker wrote:
> We've added several operations and removed GETDEVICELIST,  Let's keep
> the Nfsv4ops list up to date with the kernel.

Please keep GETDEVICELIST, as newer mountstats should handle older
kernels as well.

Re: [PATCH 1/2] mountstats: Resync NFSv4 Operations List

[Anna Schumaker]

On 04/08/2015 01:57 PM, Christoph Hellwig wrote:
> On Wed, Apr 08, 2015 at 12:47:26PM -0400, Anna Schumaker wrote:
>> We've added several operations and removed GETDEVICELIST,  Let's keep
>> the Nfsv4ops list up to date with the kernel.
> 
> Please keep GETDEVICELIST, as newer mountstats should handle older
> kernels as well.
> 
Fair enough.  I'm also working on a patch to add GETDEVICELIST back into the client.

Anna