On Feb 16, 2012, at 4:01 PM, Myklebust, Trond wrote: > On Thu, 2012-02-16 at 20:44 +0000, Adamson, Dros wrote: >> On Feb 16, 2012, at 2:48 PM, Myklebust, Trond wrote: >>> 2. Shouldn't we be calling _either_ rpc_count_iostats(), _or_ >>> task->tk_ops->rpc_count_stats()? As far as I can see, the >>> nfsstat output will now be double-counting and pNFS reads, >>> writes and commits that are sent to the DS. >> >> >> No, this doesn't double count with nfsstats. nfsstats uses rpc_clnt::cl_stats (and not rpc_clnt::cl_metrics). >> >> I probably should have done an if/else for this patch -- with the current code, the DSs' rpc_clnt:cl_metrics will never be used. >> I left it accumulating here because we want to have per-DS stats eventually and my plan was to print out stats in /proc/fs/nfsfs per *client* (so not separated by mountpoint). >> >> I can repost with if/else, but looking at this some more made me realize that we are *still* doing this wrong :) >> >> The callback method in this patch fails to accumulate stats for operations to the DS other than read/write/commit -- that seems right, but what about null, exchange_id, session heartbeats, etc? >> >> In order to properly accumulate those we are back to two obvious choices: >> 1) add a count_iostats callback to the ~25 other rpc_call_ops in nfs-land (yuck) >> 2) add an 'additional stats' pointer to the rpc_task structure (trond already said you don't want to add to task struct) >> >> Or do we just not care about displaying those operations? For my purposes (nfsometer perf testing), it'd be nice to have *all* of the operations. > > As far as I'm concerned, the administrative traffic to the DS should not > be accounted for in the mountstats: that would be wrong since DSes can > be shared not only by different filesystems but even by different MDSes. > > So the performance overhead of lease and session setup to the DSes needs > to be accounted for by some other mechanism. Fair enough. The per-client stats should help me with that. I'll repost with if/else and change it back with the next stats patch. -dros
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
