On Thu, 2012-02-09 at 20:23 +0000, Adamson, Dros wrote:
> On Feb 9, 2012, at 2:58 PM, Myklebust, Trond wrote:
>
> > On Thu, 2012-02-09 at 19:48 +0000, Adamson, Dros wrote:
> >>>
> >>>> 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.
> >>
> >> Yeah, that's easy enough and i guess we could ignore locking, but we are still left with the same problem: how is this supposed to account for different mount points using the same nfs_client? nfs_client only has one rpc_client member. I doubt we want to make a hash lookup on nfs_server to get the right rpc_client (which could all use the same underlying xprt).
> >>
> >> Maybe it's time to move these stats into fs/nfs/ where they really belong? We could get rid of the hack that overloads procnum with opnum from inside the compound for v4+ and finally only show a specific mount's RPC stats.
> >
> > Actually, how about just adding a callback into struct rpc_call_ops
> > that, if it is defined, would be called instead of rpc_count_iostats().
> > If you then modify rpc_count_iostats() to take the 'stats' variable as a
> > parameter, you can simply have the new callback call rpc_count_iostats
> > with the right arguments.
>
> Yeah, that could work! On my walk home from the cafe (they always seem to help) I came up with a slightly different strategy:
>
> - remove the cl_metrics (struct rpc_iostats) member from rpc_clnt
> - add an *optional* rpc_iostats pointer to rpc_task (ignored if NULL)
> - allocate one rpc_iostats structure (really array of structs, but you know what I mean) per nfs_server structure
> - when setting up rpc_tasks in nfs-land, pass the correct rpc_iostats
>
> with this strategy we don't accumulate stats when they aren't ever used (like an rpc_client used for mount protocol). again, only NFS uses the rpc stats interface.
>
> I kind of like this better than a callback, but either way is fine with me. Which way would you prefer?
I'd prefer not to have to grow the size of the rpc_task unless we really
need to. That's why I suggested the callback instead.
Cheers
Trond
--
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�����٥
