New kstats for NFS server and xdrmblk_getpos fix
Review Request #60 — Created June 16, 2015 and submitted
Information | |
---|---|
marcel | |
illumos-gate | |
5907, 6090 | |
Reviewers | |
general | |
webrev: http://cr.illumos.org/~webrev/marcel/il-nfs-kstat/
This is implementation of the detailed IOPS, bandwidth, and latency kstats for NFS server. The kstats are two level: one "global" for the NFS server and broken down per share.
Together with the kstats implementation there is a fix in the xdrmblk implementation which is a prerequisite for the kstat work.
I ran the debug bits on the NFS server for several hours with various tests to
make sure everything works as exepcted. I checked all of the kstats (for all
NFS and NFS_ACL protocol versions) are properly updated. I also make sure that
the re-share works as expected and the kstats are properly persisted.As a sanity check I also ran the debug bits on NFS cleint just to make sure
there is no regression.After every test run I generated the crash dump file and checked it for leaks
using the ::findleaks mdb command. This testing helped me during the
development to realize that the xdrmblk users usually does not call the
XDR_DESTROY() for the used XDR stream. I checkeck (and fixed) all of the
xdrmblk users to make sure the XDR stream is properly destroyed/freed.Using the ::findleaks mdb command I confirmed that there are no new memory
leaks introduced by the change.
Status: Re-opened
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Bugs: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Groups: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 1 (+1141 -230) |
-
-
usr/src/uts/common/fs/nfs/nfs4_dispatch.c (Diff revision 1) Would a
size_t
or an explicitly sized type (uint32_t
or whatever), be better? -
usr/src/uts/common/fs/nfs/nfs4_srv.c (Diff revision 1) "we cannot associate it with any exportinfo", I think
-
usr/src/uts/common/fs/nfs/nfs4_srv.c (Diff revision 1) "associate" again, I think? And in all similar comments.
-
usr/src/uts/common/fs/nfs/nfs_export.c (Diff revision 1) is there a reason to spell "compare" without the 'e'? Prevailing style in NFS maybe?
-
usr/src/uts/common/fs/nfs/nfs_server.c (Diff revision 1) size_t
or integer type with specified size? -
usr/src/uts/common/nfs/nfs4_kprot.h (Diff revision 1) Again, why not
size_t
. I'm suspecting the more I run into these that you have a reason :) -
-
Thanks Marcel!
General comments, not really suitable for inline review:I'd like to see a class other than "misc"
My first reaction is to use "disk" which will get automatically picked up by iostat. However, this generates way too much noise for iostat's traditional use case.
I propose a new class "nfsops"(?) to allow someone to build an extensible iostat-like tool.Overloading state into the name is painful. Rather than having:
nfs:2:share:path / (pseudo)it is better to add another name indicating (pseudo) so that consumers don't have to parse the stat looking for the " (pseudo)" token. I'm inclined towards a the name "filesystem", rather than a boolean, to be more consistent with rfc 3530 section 7.3:
nfs:2:share:filesystem pseudo|real
Change Summary:
Implemented feedback from Rich Lowe.
Change Summary:
Implemented the filesystem pseudo|real kstat as suggested by Richard Elling.
Diff: |
Revision 3 (+1187 -230)
|
---|
Change Summary:
Changed class to be more valuable (based on suggestion by Richard Elling).
Diff: |
Revision 4 (+1188 -230)
|
---|
Change Summary:
Added a comment to xdr_mblk.c as requested by Dan McDonald.
Diff: |
Revision 5 (+1202 -230)
|
---|
Change Summary:
Improved the newly added comment in xdr_mblk.c (as requested by Dan McDonald).
Diff: |
Revision 6 (+1205 -230)
|
---|
Status: Closed (submitted)
Change Summary:
commit 22146ea93e24c7deb02c49c33b2ab98605ce78b4 Author: Marcel Telka <marcel.telka@nexenta.com> Date: Wed Sep 2 08:18:08 2015 +0200 6090 IOPS, bandwidth, and latency kstats for NFS server Reviewed by: Josef 'Jeff' Sipek <josef.sipek@nexenta.com> Reviewed by: Richard Elling <Richard.Elling@RichardElling.com> Approved by: Dan McDonald <danmcd@omniti.com> commit cf98b944cdc2063fc14f3fd525e284de3ed29fd0 Author: Marcel Telka <marcel.telka@nexenta.com> Date: Thu Sep 3 19:47:20 2015 +0200 5907 xdrmblk_getpos() is unreliable Reviewed by: Josef 'Jeff' Sipek <josef.sipek@nexenta.com> Reviewed by: Yuri Pankov <yuri.pankov@nexenta.com> Reviewed by: Richard Elling <Richard.Elling@RichardElling.com> Approved by: Dan McDonald <danmcd@omniti.com>