On Thu, 2012-02-09 at 19:13 +0000, Adamson, Dros wrote:
> On Feb 9, 2012, at 1:16 PM, Myklebust, Trond wrote:
>
> > On Thu, 2012-02-09 at 11:41 -0500, Weston Andros Adamson wrote:
> >> Include RPC statistics from all data servers in /proc/self/mountstats for pNFS
> >> filelayout mounts.
> >>
> >> The additional info printed on the "per-op statistics" line does not break
> >> current mountstats(8) from nfs-utils.
> >>
> >> Signed-off-by: Weston Andros Adamson <dros@xxxxxxxxxx>
> >> ---
> >>
> >> This is clearly a better approach than I was taking trying to keep DS rpc stats
> >> separate in /proc/self/mountstats.
> >>
> >> FYI, there are no other callers of rpc_print_iostats().
> >>
> >> The new output of /proc/self/mountstats works fine with current mountstats
> >> userland tool. The additional data on the per-op statistics line doesn't make
> >> it into the output, but I left it in /proc/self/mountstats because it doesn't
> >> break anything and can be included in newer versions of mountstats.
> >>
> >> I plan on adding the per-DS stats (rpc and others) somewhere in /proc/fs/nfsfs/
> >> soon.
> >>
> >> fs/nfs/nfs4filelayout.c | 30 ++++++++++++++++++++++++++
> >> fs/nfs/pnfs.h | 23 ++++++++++++++++++++
> >> fs/nfs/pnfs_dev.c | 45 ++++++++++++++++++++++++++++++++++++++++
> >> fs/nfs/super.c | 3 +-
> >> include/linux/sunrpc/metrics.h | 13 +++++++++-
> >> net/sunrpc/stats.c | 38 ++++++++++++++++++++++++++++++++-
> >> 6 files changed, 147 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> >> index 79be7ac..51c71e5 100644
> >> --- a/fs/nfs/nfs4filelayout.c
> >> +++ b/fs/nfs/nfs4filelayout.c
> >> @@ -33,6 +33,8 @@
> >> #include <linux/nfs_page.h>
> >> #include <linux/module.h>
> >>
> >> +#include <linux/sunrpc/metrics.h>
> >> +
> >> #include "internal.h"
> >> #include "nfs4filelayout.h"
> >>
> >> @@ -918,6 +920,33 @@ filelayout_free_deveiceid_node(struct nfs4_deviceid_node *d)
> >> nfs4_fl_free_deviceid(container_of(d, struct nfs4_file_layout_dsaddr, id_node));
> >> }
> >>
> >> +static unsigned int
> >> +filelayout_rpc_add_iostats(struct rpc_iostats *total,
> >> + unsigned int maxproc,
> >> + struct nfs4_deviceid_node *d)
> >> +{
> >> + struct nfs4_file_layout_dsaddr *dsaddr;
> >> + struct nfs4_pnfs_ds *ds;
> >> + unsigned int num_ds = 0;
> >> + u32 i;
> >> +
> >> + dsaddr = container_of(d, struct nfs4_file_layout_dsaddr, id_node);
> >> +
> >> + for (i = 0; i < dsaddr->ds_num; i++) {
> >> + ds = dsaddr->ds_list[i];
> >> +
> >> + /* only add iostats if nfs_client is different from MDS */
> >> + if (ds && ds->ds_clp && d->nfs_client != ds->ds_clp) {
> >> + struct rpc_clnt *clnt = ds->ds_clp->cl_rpcclient;
> >
> > Hmmm.... Shouldn't this rather be the NFS_CLIENT(inode)? Also, why not
> > do this in the read/write completion code? Why walk the deviceid list at
> > all?
>
> I'm not really following you. Note that these are rpc_iostats not nfs_iostats.
> nfs_iostats have been properly counting operations to DSs for some time now.
> rpc_iostats count operations, rtt, exectime, etc per rpc procedure (or in the case of NFSv4.X there is a hack to use operations within compounds).
>
> We have to walk the device list to get access to the underlying DS's nfs_client->rpc_client -- I can't just put this in the read/write completion because, unlike nfs_iostats, rpc_iostats count all operations and not just read/write events/bytes (not to mention that the accumulation is done at the rpc layer).
In doing this, you are assuming that DSes are not shared among multiple
filesystems or MDSes (i.e. that each DS is used by a single struct
nfs_server). For something like a NetApp filer in c-mode, that is
clearly not a valid assumption.
> I do have an implementation that doesn't need to walk the deviceid list by allowing a shared rpc_iostats struct between multiple rpc_clients (in addition to the current rpc_iostats structure), but that required adding locking and reference counting -- all for printing stats (obviously not what we want).
This might be more in line with what we want. Note that it should be
easy to clone an rpc_client and then replace its rpc_iostats struct. I
don't think that needs any extra locking. We're already ignoring locking
here between different rpc_tasks, so throwing in different rpc_clients
to the mix will make no difference.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
