[PATCH 0/2] Make nfs and nfsd stats visible in network ns

[PATCH 0/2] Make nfs and nfsd stats visible in network ns

[Josef Bacik]

Hello,

We're currently deploying NFS internally and have run into some oddities with
our usage of containers.  All of the services that mount and export NFS volumes
run inside of containers, specifically all the namespaces including network
namespaces.  Our monitoring is done on a per-container basis, so we need access
to the nfs and nfsd stats that are under /proc/net/sunrpc.  However these are
only tied to the init_net, which makes them invisible to containers in a
different network namespace.

Fix this so that these files are tied to the network namespace.  This allows us
to avoid the hack of bind mounting the hosts /proc into the container in order
to do proper monitoring.  Thanks,

Josef

Josef Bacik (2):
  nfs: expose /proc/net/sunrpc/nfs in net namespaces
  nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

 fs/nfs/inode.c   |  6 ++----
 fs/nfsd/nfsctl.c |  8 +++++---
 fs/nfsd/stats.c  | 21 ++++++---------------
 fs/nfsd/stats.h  |  6 ++++--
 4 files changed, 17 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH 1/2] nfs: expose /proc/net/sunrpc/nfs in net namespaces

[Josef Bacik]

We're using nfs mounts inside of containers in production and noticed
that the nfs stats are not exposed in /proc.  This is a problem for us
as we use these stats for monitoring, and have to do this awkward bind
mount from the main host into the container in order to get to these
states.

Add the rpc_proc_register call to the pernet operations entry and exit
points so these stats can be exposed inside of network namespaces.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/nfs/inode.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index ebb8d60e1152..e11e9c34aa56 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -2427,11 +2427,13 @@ EXPORT_SYMBOL_GPL(nfs_net_id);
 static int nfs_net_init(struct net *net)
 {
 	nfs_clients_init(net);
+	rpc_proc_register(net, &nfs_rpcstat);
 	return nfs_fs_proc_net_init(net);
 }
 
 static void nfs_net_exit(struct net *net)
 {
+	rpc_proc_unregister(net, "nfs");
 	nfs_fs_proc_net_exit(net);
 	nfs_clients_exit(net);
 }
@@ -2486,15 +2488,12 @@ static int __init init_nfs_fs(void)
 	if (err)
 		goto out1;
 
-	rpc_proc_register(&init_net, &nfs_rpcstat);
-
 	err = register_nfs_fs();
 	if (err)
 		goto out0;
 
 	return 0;
 out0:
-	rpc_proc_unregister(&init_net, "nfs");
 	nfs_destroy_directcache();
 out1:
 	nfs_destroy_writepagecache();
@@ -2524,7 +2523,6 @@ static void __exit exit_nfs_fs(void)
 	nfs_destroy_inodecache();
 	nfs_destroy_nfspagecache();
 	unregister_pernet_subsys(&nfs_net_ops);
-	rpc_proc_unregister(&init_net, "nfs");
 	unregister_nfs_fs();
 	nfs_fs_proc_exit();
 	nfsiod_stop();
-- 
2.43.0

[PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Josef Bacik]

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(-)

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

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Chuck Lever]

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
> 
> 

-- 
Chuck Lever

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Josef Bacik]

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

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Josef Bacik]

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

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Jeff Layton]

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?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Josef Bacik]

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

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Jeff Layton]

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@kernel.org>

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Chuck Lever]

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.


-- 
Chuck Lever

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Jeff Layton]

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.
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Chuck Lever III]

> 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

Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@kernel.org>