On Thu, 2024-01-25 at 01:54 +0000, Chuck Lever III wrote:
>
> > On Jan 24, 2024, at 7:06 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > On Wed, 2024-01-24 at 18:47 -0500, Chuck Lever wrote:
> > > On Wed, Jan 24, 2024 at 06:41:27PM -0500, Jeff Layton wrote:
> > > > On Wed, 2024-01-24 at 18:18 -0500, Josef Bacik wrote:
> > > > > On Wed, Jan 24, 2024 at 05:57:06PM -0500, Jeff Layton wrote:
> > > > > > On Wed, 2024-01-24 at 17:12 -0500, Josef Bacik wrote:
> > > > > > > On Wed, Jan 24, 2024 at 03:32:06PM -0500, Chuck Lever wrote:
> > > > > > > > On Wed, Jan 24, 2024 at 02:37:00PM -0500, Josef Bacik wrote:
> > > > > > > > > We are running nfsd servers inside of containers with their own network
> > > > > > > > > namespace, and we want to monitor these services using the stats found
> > > > > > > > > in /proc. However these are not exposed in the proc inside of the
> > > > > > > > > container, so we have to bind mount the host /proc into our containers
> > > > > > > > > to get at this information.
> > > > > > > > >
> > > > > > > > > Separate out the stat counters init and the proc registration, and move
> > > > > > > > > the proc registration into the pernet operations entry and exit points
> > > > > > > > > so that these stats can be exposed inside of network namespaces.
> > > > > > > >
> > > > > > > > Maybe I missed something, but this looks like it exposes the global
> > > > > > > > stat counters to all net namespaces...? Is that an information leak?
> > > > > > > > As an administrator I might be surprised by that behavior.
> > > > > > > >
> > > > > > > > Seems like this patch needs to make nfsdstats and nfsd_svcstats into
> > > > > > > > per-namespace objects as well.
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > I've got the patches written for this, but I've got a question. There's a
> > > > > > >
> > > > > > > svc_seq_show(seq, &nfsd_svcstats);
> > > > > > >
> > > > > > > in nfsd/stats.c. This appears to be an empty struct, there's nothing that
> > > > > > > utilizes it, so this is always going to print 0 right? There's a svc_info in
> > > > > > > the nfsd_net, and that stats block appears to get updated properly. Should I
> > > > > > > print this out here? I don't see anywhere we get the rpc stats out of nfsd, am
> > > > > > > I missing something? I don't want to rip out stuff that I don't quite
> > > > > > > understand. Thanks,
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > nfsd_svcstats ends up being the sv_stats for the nfsd service. The RPC
> > > > > > code has some counters in there for counting different sorts of net and
> > > > > > rpc events (see svc_process_common, and some of the recv and accept
> > > > > > handlers). I think nfsstat(8) may fetch that info via the above
> > > > > > seqfile, so it's definitely not unused (and it should be printing more
> > > > > > than just a '0').
> > > > >
> > > > > Ahhh, I missed this bit
> > > > >
> > > > > struct svc_program nfsd_program = {
> > > > > #if defined(CONFIG_NFSD_V2_ACL) || defined(CONFIG_NFSD_V3_ACL)
> > > > > .pg_next = &nfsd_acl_program,
> > > > > #endif
> > > > > .pg_prog = NFS_PROGRAM, /* program number */
> > > > > .pg_nvers = NFSD_NRVERS, /* nr of entries in
> > > > > nfsd_version */
> > > > > .pg_vers = nfsd_version, /* version table */
> > > > > .pg_name = "nfsd", /* program name */
> > > > > .pg_class = "nfsd", /* authentication class
> > > > > */
> > > > > .pg_stats = &nfsd_svcstats, /* version table */
> > > > > .pg_authenticate = &svc_set_client, /* export authentication
> > > > > */
> > > > > .pg_init_request = nfsd_init_request,
> > > > > .pg_rpcbind_set = nfsd_rpcbind_set,
> > > > > };
> > > > >
> > > > > and so nfsd_svcstats definitely is getting used.
> > > > >
> > > > > >
> > > > > > svc_info is a completely different thing: it's a container for the
> > > > > > svc_serv...so I'm not sure I understand your question?
> > > > >
> > > > > I was just confused, and still am a little bit.
> > > > >
> > > > > The counters are easy, I put those into the nfsd_net struct and make everything
> > > > > mess with those counters and report those from proc.
> > > > >
> > > > > However the nfsd_svcstats are in this svc_program thing, which appears to need
> > > > > to be global? Or do I need to make it per net as well? Or do I need to do
> > > > > something completely different to track the rpc stats per network namespace?
> > > >
> > > > Making the svc_program per-net is unnecessary for this (and probably not
> > > > desirable). That structure sort of describes the nfsd rpc "program" and
> > > > that is pretty much the same between containers.
> > >
> > > Maybe we want per-namespace svc_programs. Some RPC programs will
> > > be registered in some namespaces, some in others? That might be
> > > the simplest approach.
> > >
> >
> > That seems like a much heavier lift, and I'm not sure I see the benefit.
> > Here's nfsd_program today:
> >
> > struct svc_program nfsd_program = {
> > #if defined(CONFIG_NFSD_V2_ACL) || defined(CONFIG_NFSD_V3_ACL)
> > .pg_next = &nfsd_acl_program,
> > #endif
> > .pg_prog = NFS_PROGRAM, /* program number */
> > .pg_nvers = NFSD_NRVERS, /* nr of entries in nfsd_version */
> > .pg_vers = nfsd_version, /* version table */
> > .pg_name = "nfsd", /* program name */
> > .pg_class = "nfsd", /* authentication class */
> > .pg_stats = &nfsd_svcstats, /* version table */
> > .pg_authenticate = &svc_set_client, /* export authentication */
> > .pg_init_request = nfsd_init_request,
> > .pg_rpcbind_set = nfsd_rpcbind_set,
> > };
> >
> > All of that seems fairly constant across containers. The main exception
> > is the svc_stats, which does need to be per-program and per-net, at
> > least for nfsd.
>
> This would be the benefit right here: the stats need to be matrixed
> per program and per net. The stats and the program (set of RPC
> procedures) are pretty tightly interconnected.
>
> Some namespaces might want NFSv2 or NFSv3, some might want NFSv4 only.
>
I don't think you'd want to do that by changing the svc_program though.
We already have ways to enable and disable versions on a per svc_serv
basis.
> But I don't have a strong opinion about it at this point. You could
> be right that this would be the more obtuse approach. There is one
> fixed definition for each RPC program, so having one svc_program
> per RPC program, and having each live in global module memory, is
> sensible.
>
> The way stats work now is from a long-ago era.
>
>
Now that I look more closely, I think Josef will need to make two
different structs into per-net objects:
The nfsd_svcstats is the generic RPC stats counters for the nfsd
service, whereas nfsdstats has counters for specific nfsd operations and
events. You'll need to make both into per-net objects, I think.
> > FWIW, looking at the other services that set pg_stats, none of them have
> > a way to actually report them! They are write-only. We should probably
> > make the others just set pg_stats to NULL so we don't bother
> > incrementing on them.
> >
> > That should simplify reworking how this works for nfsd too...
>
> True, but I've been told that having NLM RPC stats available would be
> helpful for distro support teams. It would be cool to delete some lines
> of code, but we should ask around before tossing out this unused
> infrastructure.
>
> In fact, Josef: what do you think? Would having NLM stats for your
> NFSv3 servers be helpful?
>
How are they useful if there is no way to report them? Are they poking
around in vmcore dumps or BPF to get at it?
In any case, if we want to keep them, then we should probably make a new
procfile that reports the stats for them (and write a nlmstat program or
something to make them pretty).
I'd still suggest just removing that stuff for now though until and
unless we can prove that they are useful.
My suggestion for the svc_serv stuff:
1/ drop all of the svc_stats structs except for nfsd's
2/ eliminate ->pg_stats and make __svc_create and svc_create_pooled take
a svc_stats pointer arg. Most callers can pass in NULL there, but
nfsd_create_serv will want to allocate one per net namespace.
3/ Bonus points: convert svc_stats to use percpu vars
Making nfsdstats per-net are sort of a separate project, but you'll need
to do that in order for this to work correctly.
--
Jeff Layton <jlayton@xxxxxxxxxx>
