| Message ID | 71058c29683d44644aba8ab295fa028ee41365a8.1706124811.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make nfs and nfsd stats visible in network ns | expand |
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. > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/nfsd/nfsctl.c | 8 +++++--- > fs/nfsd/stats.c | 21 ++++++--------------- > fs/nfsd/stats.h | 6 ++++-- > 3 files changed, 15 insertions(+), 20 deletions(-) > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index f206ca32e7f5..b57480b50e35 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -1679,6 +1679,7 @@ static __net_init int nfsd_net_init(struct net *net) > nfsd4_init_leases_net(nn); > get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key)); > seqlock_init(&nn->writeverf_lock); > + nfsd_proc_stat_init(net); > > return 0; > > @@ -1699,6 +1700,7 @@ static __net_exit void nfsd_net_exit(struct net *net) > { > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > > + nfsd_proc_stat_shutdown(net); > nfsd_net_reply_cache_destroy(nn); > nfsd_idmap_shutdown(net); > nfsd_export_shutdown(net); > @@ -1722,7 +1724,7 @@ static int __init init_nfsd(void) > retval = nfsd4_init_pnfs(); > if (retval) > goto out_free_slabs; > - retval = nfsd_stat_init(); /* Statistics */ > + retval = nfsd_stat_counters_init(); /* Statistics */ > if (retval) > goto out_free_pnfs; > retval = nfsd_drc_slab_create(); > @@ -1762,7 +1764,7 @@ static int __init init_nfsd(void) > nfsd_lockd_shutdown(); > nfsd_drc_slab_free(); > out_free_stat: > - nfsd_stat_shutdown(); > + nfsd_stat_counters_destroy(); > out_free_pnfs: > nfsd4_exit_pnfs(); > out_free_slabs: > @@ -1780,7 +1782,7 @@ static void __exit exit_nfsd(void) > nfsd_drc_slab_free(); > remove_proc_entry("fs/nfs/exports", NULL); > remove_proc_entry("fs/nfs", NULL); > - nfsd_stat_shutdown(); > + nfsd_stat_counters_destroy(); > nfsd_lockd_shutdown(); > nfsd4_free_slabs(); > nfsd4_exit_pnfs(); > diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c > index 12d79f5d4eb1..394a65a33942 100644 > --- a/fs/nfsd/stats.c > +++ b/fs/nfsd/stats.c > @@ -108,31 +108,22 @@ void nfsd_percpu_counters_destroy(struct percpu_counter counters[], int num) > percpu_counter_destroy(&counters[i]); > } > > -static int nfsd_stat_counters_init(void) > +int nfsd_stat_counters_init(void) > { > return nfsd_percpu_counters_init(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM); > } > > -static void nfsd_stat_counters_destroy(void) > +void nfsd_stat_counters_destroy(void) > { > nfsd_percpu_counters_destroy(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM); > } > > -int nfsd_stat_init(void) > +void nfsd_proc_stat_init(struct net *net) > { > - int err; > - > - err = nfsd_stat_counters_init(); > - if (err) > - return err; > - > - svc_proc_register(&init_net, &nfsd_svcstats, &nfsd_proc_ops); > - > - return 0; > + svc_proc_register(net, &nfsd_svcstats, &nfsd_proc_ops); > } > > -void nfsd_stat_shutdown(void) > +void nfsd_proc_stat_shutdown(struct net *net) > { > - nfsd_stat_counters_destroy(); > - svc_proc_unregister(&init_net, "nfsd"); > + svc_proc_unregister(net, "nfsd"); > } > diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h > index 14f50c660b61..5cd6517b52a9 100644 > --- a/fs/nfsd/stats.h > +++ b/fs/nfsd/stats.h > @@ -40,8 +40,10 @@ extern struct svc_stat nfsd_svcstats; > int nfsd_percpu_counters_init(struct percpu_counter *counters, int num); > void nfsd_percpu_counters_reset(struct percpu_counter *counters, int num); > void nfsd_percpu_counters_destroy(struct percpu_counter *counters, int num); > -int nfsd_stat_init(void); > -void nfsd_stat_shutdown(void); > +int nfsd_stat_counters_init(void); > +void nfsd_stat_counters_destroy(void); > +void nfsd_proc_stat_init(struct net *net); > +void nfsd_proc_stat_shutdown(struct net *net); > > static inline void nfsd_stats_rc_hits_inc(void) > { > -- > 2.43.0 > >
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. > Yeah I was worried you might say that. I misread the sunrpc code and thought it was handling the magic for me, but it's literally just making the proc entry under its own thing. I'll brb with proper patches. Thanks, Josef
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, Josef
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'). svc_info is a completely different thing: it's a container for the svc_serv...so I'm not sure I understand your question?
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? Thanks, Josef
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?
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. > 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? Yes, it needs to be made into a vector of per-cpu counters. I've had this on my to-do list for some time, but every time I look at it, I try to find something else to do.
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. 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... > > > 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? > > Yes, it needs to be made into a vector of per-cpu counters. I've > had this on my to-do list for some time, but every time I look at > it, I try to find something else to do. >
> On Jan 24, 2024, at 7:06 PM, Jeff Layton <jlayton@kernel.org> 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. 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. > 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? -- Chuck Lever
On Thu, 2024-01-25 at 01:54 +0000, Chuck Lever III wrote: > > > On Jan 24, 2024, at 7:06 PM, Jeff Layton <jlayton@kernel.org> 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.
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index f206ca32e7f5..b57480b50e35 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -1679,6 +1679,7 @@ static __net_init int nfsd_net_init(struct net *net) nfsd4_init_leases_net(nn); get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key)); seqlock_init(&nn->writeverf_lock); + nfsd_proc_stat_init(net); return 0; @@ -1699,6 +1700,7 @@ static __net_exit void nfsd_net_exit(struct net *net) { struct nfsd_net *nn = net_generic(net, nfsd_net_id); + nfsd_proc_stat_shutdown(net); nfsd_net_reply_cache_destroy(nn); nfsd_idmap_shutdown(net); nfsd_export_shutdown(net); @@ -1722,7 +1724,7 @@ static int __init init_nfsd(void) retval = nfsd4_init_pnfs(); if (retval) goto out_free_slabs; - retval = nfsd_stat_init(); /* Statistics */ + retval = nfsd_stat_counters_init(); /* Statistics */ if (retval) goto out_free_pnfs; retval = nfsd_drc_slab_create(); @@ -1762,7 +1764,7 @@ static int __init init_nfsd(void) nfsd_lockd_shutdown(); nfsd_drc_slab_free(); out_free_stat: - nfsd_stat_shutdown(); + nfsd_stat_counters_destroy(); out_free_pnfs: nfsd4_exit_pnfs(); out_free_slabs: @@ -1780,7 +1782,7 @@ static void __exit exit_nfsd(void) nfsd_drc_slab_free(); remove_proc_entry("fs/nfs/exports", NULL); remove_proc_entry("fs/nfs", NULL); - nfsd_stat_shutdown(); + nfsd_stat_counters_destroy(); nfsd_lockd_shutdown(); nfsd4_free_slabs(); nfsd4_exit_pnfs(); diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c index 12d79f5d4eb1..394a65a33942 100644 --- a/fs/nfsd/stats.c +++ b/fs/nfsd/stats.c @@ -108,31 +108,22 @@ void nfsd_percpu_counters_destroy(struct percpu_counter counters[], int num) percpu_counter_destroy(&counters[i]); } -static int nfsd_stat_counters_init(void) +int nfsd_stat_counters_init(void) { return nfsd_percpu_counters_init(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM); } -static void nfsd_stat_counters_destroy(void) +void nfsd_stat_counters_destroy(void) { nfsd_percpu_counters_destroy(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM); } -int nfsd_stat_init(void) +void nfsd_proc_stat_init(struct net *net) { - int err; - - err = nfsd_stat_counters_init(); - if (err) - return err; - - svc_proc_register(&init_net, &nfsd_svcstats, &nfsd_proc_ops); - - return 0; + svc_proc_register(net, &nfsd_svcstats, &nfsd_proc_ops); } -void nfsd_stat_shutdown(void) +void nfsd_proc_stat_shutdown(struct net *net) { - nfsd_stat_counters_destroy(); - svc_proc_unregister(&init_net, "nfsd"); + svc_proc_unregister(net, "nfsd"); } diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h index 14f50c660b61..5cd6517b52a9 100644 --- a/fs/nfsd/stats.h +++ b/fs/nfsd/stats.h @@ -40,8 +40,10 @@ extern struct svc_stat nfsd_svcstats; int nfsd_percpu_counters_init(struct percpu_counter *counters, int num); void nfsd_percpu_counters_reset(struct percpu_counter *counters, int num); void nfsd_percpu_counters_destroy(struct percpu_counter *counters, int num); -int nfsd_stat_init(void); -void nfsd_stat_shutdown(void); +int nfsd_stat_counters_init(void); +void nfsd_stat_counters_destroy(void); +void nfsd_proc_stat_init(struct net *net); +void nfsd_proc_stat_shutdown(struct net *net); static inline void nfsd_stats_rc_hits_inc(void) {
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. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/nfsd/nfsctl.c | 8 +++++--- fs/nfsd/stats.c | 21 ++++++--------------- fs/nfsd/stats.h | 6 ++++-- 3 files changed, 15 insertions(+), 20 deletions(-)