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. I think making having a different sv_stats per-namespace makes sense. It'll be a departure from the way it works today though. Looking at nfsstat in the init_ns will no longer show global counters. I don't think it's a bad change, but it will be a change that we'll need to take into account (and maybe document). This is all really old, crusty code, and some of it like the sv_stats code originates from the 90s. Right now, sv_stats is only assigned in svc_create and it comes from the svc_program. You'll need to do something different there. Now that I look too, it looks like we're just doing bare increments to the counters without any locking, which seems a bit racy. I wonder whether we ought to be doing something percpu there instead? -- Jeff Layton <jlayton@xxxxxxxxxx>