diff mbox series

[RFC] NFSD: add rpc_status entry in nfsd debug filesystem

Message ID bac972c22c5acfa43969bb1bf164341b16ea045c.1688032742.git.lorenzo@kernel.org (mailing list archive)
State New, archived
Headers show
Series [RFC] NFSD: add rpc_status entry in nfsd debug filesystem | expand

Commit Message

Lorenzo Bianconi June 29, 2023, 10:17 a.m. UTC
Introduce rpc_status entry in nfsd debug filesystem in order to dump
pending RPC requests debugging information.

Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=366
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 fs/nfsd/nfs4proc.c |  4 +--
 fs/nfsd/nfsctl.c   | 18 ++++++++++++++
 fs/nfsd/nfsd.h     |  2 ++
 fs/nfsd/nfssvc.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 net/sunrpc/svc.c   |  2 +-
 5 files changed, 83 insertions(+), 4 deletions(-)

Comments

Jeff Layton June 29, 2023, 11:10 a.m. UTC | #1
On Thu, 2023-06-29 at 12:17 +0200, Lorenzo Bianconi wrote:
> Introduce rpc_status entry in nfsd debug filesystem in order to dump
> pending RPC requests debugging information.
> 
> Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=366
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  fs/nfsd/nfs4proc.c |  4 +--
>  fs/nfsd/nfsctl.c   | 18 ++++++++++++++
>  fs/nfsd/nfsd.h     |  2 ++
>  fs/nfsd/nfssvc.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>  net/sunrpc/svc.c   |  2 +-
>  5 files changed, 83 insertions(+), 4 deletions(-)
> 

This looks good, Lorenzo. Nice work! Comments below.

> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 5ae670807449..a4dd1ef104c3 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2452,8 +2452,6 @@ static inline void nfsd4_increment_op_stats(u32 opnum)
>  
>  static const struct nfsd4_operation nfsd4_ops[];
>  
> -static const char *nfsd4_op_name(unsigned opnum);
> -
>  /*
>   * Enforce NFSv4.1 COMPOUND ordering rules:
>   *
> @@ -3583,7 +3581,7 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op)
>  	}
>  }
>  
> -static const char *nfsd4_op_name(unsigned opnum)
> +const char *nfsd4_op_name(unsigned opnum)
>  {
>  	if (opnum < ARRAY_SIZE(nfsd4_ops))
>  		return nfsd4_ops[opnum].op_name;
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 1489e0b703b4..9d0b0a53510b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -57,6 +57,8 @@ enum {
>  	NFSD_RecoveryDir,
>  	NFSD_V4EndGrace,
>  #endif
> +	NFSD_Rpc_Status,
> +
>  	NFSD_MaxReserved
>  };
>  
> @@ -195,6 +197,21 @@ static inline struct net *netns(struct file *file)
>  	return file_inode(file)->i_sb->s_fs_info;
>  }
>  
> +static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> +{
> +	struct nfsd_net *nn = net_generic(file_inode(m->file)->i_sb->s_fs_info,
> +					  nfsd_net_id);
> +
> +	mutex_lock(&nfsd_mutex);
> +	if (nn->nfsd_serv)
> +		nsfd_rpc_status(m, nn->nfsd_serv);
> +	mutex_unlock(&nfsd_mutex);
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(nfsd_rpc_status);
> +
>  /*
>   * write_unlock_ip - Release all locks used by a client
>   *
> @@ -1400,6 +1417,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
>  		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
>  		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
>  #endif
> +		[NFSD_Rpc_Status] = {"rpc_status", &nfsd_rpc_status_fops, S_IRUGO},
>  		/* last one */ {""}
>  	};
>  
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index d88498f8b275..a149157ec243 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -87,6 +87,7 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
>   */
>  int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
>  int		nfsd_dispatch(struct svc_rqst *rqstp);
> +void		nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv);

nit: please rename this to nfsd_rpc_status

>  
>  int		nfsd_nrthreads(struct net *);
>  int		nfsd_nrpools(struct net *);
> @@ -506,6 +507,7 @@ extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
>  
>  extern void nfsd4_init_leases_net(struct nfsd_net *nn);
>  
> +const char *nfsd4_op_name(unsigned opnum);
>  #else /* CONFIG_NFSD_V4 */
>  static inline int nfsd4_is_junction(struct dentry *dentry)
>  {
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 9c7b1ef5be40..7a881f9a3a13 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -1142,3 +1142,64 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
>  	mutex_unlock(&nfsd_mutex);
>  	return ret;
>  }
> +
> +void nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv)
> +{
> +	int i;
> +
> +	lockdep_assert_held(&nfsd_mutex);
> +
> +	rcu_read_lock();

Both sv_nrpools and the sp_all_threads list are protected by the
nfsd_mutex, so I don't think you need the rcu_read_lock here too.

It would be nice if we could do this with only the rcu_read_lock. I
think accessing all of the svc_rqst fields under the rcu_read_lock is
safe, and sv_nrpools is only set when the service is created, so I think
that would be safe.


> +
> +	for (i = 0; i < serv->sv_nrpools; i++) {
> +		struct svc_rqst *rqstp;
> +
> +		seq_puts(m, "XID        | FLAGS      | PROG       |");
> +		seq_puts(m, " VERS       | PROC\t|");
> +		seq_puts(m, " REMOTE - LOCAL IP ADDR");
> +		seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
> +		list_for_each_entry_rcu(rqstp,
> +					&serv->sv_pools[i].sp_all_threads,
> +					rq_all) {
> +			if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
> +				continue;
> +
> +			seq_printf(m,
> +				   "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
> +				   be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
> +				   rqstp->rq_prog, rqstp->rq_vers,
> +				   svc_proc_name(rqstp));
> +
> +			if (rqstp->rq_addr.ss_family == AF_INET) {
> +				seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
> +					   &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
> +					   &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
> +			} else if (rqstp->rq_addr.ss_family == AF_INET6) {
> +				seq_printf(m, " %pI6 - %pI6 |",
> +					   &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
> +					   &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
> +			} else {
> +				seq_printf(m, " Unknown address family: %hu\n",
> +					   rqstp->rq_addr.ss_family);
> +				continue;
> +			}
> +#ifdef CONFIG_NFSD_V4
> +			if (rqstp->rq_vers == NFS4_VERSION &&
> +			    rqstp->rq_proc == NFSPROC4_COMPOUND) {
> +				/* NFSv4 compund */
> +				struct nfsd4_compoundargs *args = rqstp->rq_argp;
> +				struct nfsd4_compoundres *resp = rqstp->rq_resp;
> +
> +				while (resp->opcnt < args->opcnt) {
> +					struct nfsd4_op *op = &args->ops[resp->opcnt++];
> +
> +					seq_printf(m, " %s", nfsd4_op_name(op->opnum));
> +				}
> +			}
> +#endif /* CONFIG_NFSD_V4 */
> +			seq_puts(m, "\n");
> +		}
> +	}
> +
> +	rcu_read_unlock();
> +}
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index e6d4cec61e47..f99cad92e9f4 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1625,7 +1625,7 @@ const char *svc_proc_name(const struct svc_rqst *rqstp)
>  		return rqstp->rq_procinfo->pc_name;
>  	return "unknown";
>  }
> -
> +EXPORT_SYMBOL_GPL(svc_proc_name);
>  
>  /**
>   * svc_encode_result_payload - mark a range of bytes as a result payload
Chuck Lever June 29, 2023, 1:55 p.m. UTC | #2
On Thu, Jun 29, 2023 at 12:17:33PM +0200, Lorenzo Bianconi wrote:
> Introduce rpc_status entry in nfsd debug filesystem in order to dump
> pending RPC requests debugging information.
> 
> Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=366

Nice to see progress on this.

Do you have a user space tool to monitor this file?


> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  fs/nfsd/nfs4proc.c |  4 +--
>  fs/nfsd/nfsctl.c   | 18 ++++++++++++++
>  fs/nfsd/nfsd.h     |  2 ++
>  fs/nfsd/nfssvc.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>  net/sunrpc/svc.c   |  2 +-
>  5 files changed, 83 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 5ae670807449..a4dd1ef104c3 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2452,8 +2452,6 @@ static inline void nfsd4_increment_op_stats(u32 opnum)
>  
>  static const struct nfsd4_operation nfsd4_ops[];
>  
> -static const char *nfsd4_op_name(unsigned opnum);
> -
>  /*
>   * Enforce NFSv4.1 COMPOUND ordering rules:
>   *
> @@ -3583,7 +3581,7 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op)
>  	}
>  }
>  
> -static const char *nfsd4_op_name(unsigned opnum)
> +const char *nfsd4_op_name(unsigned opnum)
>  {
>  	if (opnum < ARRAY_SIZE(nfsd4_ops))
>  		return nfsd4_ops[opnum].op_name;
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 1489e0b703b4..9d0b0a53510b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -57,6 +57,8 @@ enum {
>  	NFSD_RecoveryDir,
>  	NFSD_V4EndGrace,
>  #endif
> +	NFSD_Rpc_Status,
> +
>  	NFSD_MaxReserved
>  };
>  
> @@ -195,6 +197,21 @@ static inline struct net *netns(struct file *file)
>  	return file_inode(file)->i_sb->s_fs_info;
>  }
>  
> +static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> +{
> +	struct nfsd_net *nn = net_generic(file_inode(m->file)->i_sb->s_fs_info,
> +					  nfsd_net_id);
> +
> +	mutex_lock(&nfsd_mutex);
> +	if (nn->nfsd_serv)
> +		nsfd_rpc_status(m, nn->nfsd_serv);
> +	mutex_unlock(&nfsd_mutex);
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(nfsd_rpc_status);
> +
>  /*
>   * write_unlock_ip - Release all locks used by a client
>   *
> @@ -1400,6 +1417,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
>  		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
>  		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
>  #endif
> +		[NFSD_Rpc_Status] = {"rpc_status", &nfsd_rpc_status_fops, S_IRUGO},
>  		/* last one */ {""}
>  	};
>  
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index d88498f8b275..a149157ec243 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -87,6 +87,7 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
>   */
>  int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
>  int		nfsd_dispatch(struct svc_rqst *rqstp);
> +void		nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv);
>  
>  int		nfsd_nrthreads(struct net *);
>  int		nfsd_nrpools(struct net *);
> @@ -506,6 +507,7 @@ extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
>  
>  extern void nfsd4_init_leases_net(struct nfsd_net *nn);
>  
> +const char *nfsd4_op_name(unsigned opnum);
>  #else /* CONFIG_NFSD_V4 */
>  static inline int nfsd4_is_junction(struct dentry *dentry)
>  {
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 9c7b1ef5be40..7a881f9a3a13 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -1142,3 +1142,64 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
>  	mutex_unlock(&nfsd_mutex);
>  	return ret;
>  }
> +

Please add a kdoc comment here that describes the purpose of this
function, its API contract, and any non-obvious assumptions.


> +void nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv)
> +{
> +	int i;
> +
> +	lockdep_assert_held(&nfsd_mutex);

The nfsd_mutex is held, I assume, to serialize with service
shutdown. IMO you should add a comment to that effect.

> +
> +	rcu_read_lock();

The RCU read lock protects the sp_all_threads list. OK.

> +
> +	for (i = 0; i < serv->sv_nrpools; i++) {
> +		struct svc_rqst *rqstp;
> +
> +		seq_puts(m, "XID        | FLAGS      | PROG       |");
> +		seq_puts(m, " VERS       | PROC\t|");
> +		seq_puts(m, " REMOTE - LOCAL IP ADDR");
> +		seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
> +		list_for_each_entry_rcu(rqstp,
> +					&serv->sv_pools[i].sp_all_threads,
> +					rq_all) {
> +			if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
> +				continue;
> +
> +			seq_printf(m,
> +				   "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
> +				   be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
> +				   rqstp->rq_prog, rqstp->rq_vers,
> +				   svc_proc_name(rqstp));
> +
> +			if (rqstp->rq_addr.ss_family == AF_INET) {
> +				seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
> +					   &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
> +					   &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
> +			} else if (rqstp->rq_addr.ss_family == AF_INET6) {
> +				seq_printf(m, " %pI6 - %pI6 |",
> +					   &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
> +					   &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
> +			} else {
> +				seq_printf(m, " Unknown address family: %hu\n",
> +					   rqstp->rq_addr.ss_family);
> +				continue;
> +			}
> +#ifdef CONFIG_NFSD_V4
> +			if (rqstp->rq_vers == NFS4_VERSION &&
> +			    rqstp->rq_proc == NFSPROC4_COMPOUND) {
> +				/* NFSv4 compund */
> +				struct nfsd4_compoundargs *args = rqstp->rq_argp;
> +				struct nfsd4_compoundres *resp = rqstp->rq_resp;
> +
> +				while (resp->opcnt < args->opcnt) {
> +					struct nfsd4_op *op = &args->ops[resp->opcnt++];
> +
> +					seq_printf(m, " %s", nfsd4_op_name(op->opnum));
> +				}
> +			}
> +#endif /* CONFIG_NFSD_V4 */
> +			seq_puts(m, "\n");

My only quibble here is that the file format needs to be parsable
by scripts as well as readable by humans. I'm not sure I have a
specific comment, but it's something that needs some attention and
verification (with, say, a sample user space tool, hint hint).


> +		}
> +	}
> +
> +	rcu_read_unlock();
> +}
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index e6d4cec61e47..f99cad92e9f4 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1625,7 +1625,7 @@ const char *svc_proc_name(const struct svc_rqst *rqstp)
>  		return rqstp->rq_procinfo->pc_name;
>  	return "unknown";
>  }
> -
> +EXPORT_SYMBOL_GPL(svc_proc_name);
>  
>  /**
>   * svc_encode_result_payload - mark a range of bytes as a result payload
> -- 
> 2.41.0
>
Chuck Lever June 29, 2023, 9:52 p.m. UTC | #3
On Thu, Jun 29, 2023 at 09:55:28AM -0400, Chuck Lever wrote:
> On Thu, Jun 29, 2023 at 12:17:33PM +0200, Lorenzo Bianconi wrote:
> > Introduce rpc_status entry in nfsd debug filesystem in order to dump
> > pending RPC requests debugging information.
> > 
> > Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=366
> 
> Nice to see progress on this.
> 
> Do you have a user space tool to monitor this file?
> 
> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  fs/nfsd/nfs4proc.c |  4 +--
> >  fs/nfsd/nfsctl.c   | 18 ++++++++++++++
> >  fs/nfsd/nfsd.h     |  2 ++
> >  fs/nfsd/nfssvc.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
> >  net/sunrpc/svc.c   |  2 +-
> >  5 files changed, 83 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 5ae670807449..a4dd1ef104c3 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -2452,8 +2452,6 @@ static inline void nfsd4_increment_op_stats(u32 opnum)
> >  
> >  static const struct nfsd4_operation nfsd4_ops[];
> >  
> > -static const char *nfsd4_op_name(unsigned opnum);
> > -
> >  /*
> >   * Enforce NFSv4.1 COMPOUND ordering rules:
> >   *
> > @@ -3583,7 +3581,7 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op)
> >  	}
> >  }
> >  
> > -static const char *nfsd4_op_name(unsigned opnum)
> > +const char *nfsd4_op_name(unsigned opnum)
> >  {
> >  	if (opnum < ARRAY_SIZE(nfsd4_ops))
> >  		return nfsd4_ops[opnum].op_name;
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 1489e0b703b4..9d0b0a53510b 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -57,6 +57,8 @@ enum {
> >  	NFSD_RecoveryDir,
> >  	NFSD_V4EndGrace,
> >  #endif
> > +	NFSD_Rpc_Status,
> > +
> >  	NFSD_MaxReserved
> >  };
> >  
> > @@ -195,6 +197,21 @@ static inline struct net *netns(struct file *file)
> >  	return file_inode(file)->i_sb->s_fs_info;
> >  }
> >  
> > +static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> > +{
> > +	struct nfsd_net *nn = net_generic(file_inode(m->file)->i_sb->s_fs_info,
> > +					  nfsd_net_id);
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +	if (nn->nfsd_serv)
> > +		nsfd_rpc_status(m, nn->nfsd_serv);
> > +	mutex_unlock(&nfsd_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(nfsd_rpc_status);
> > +
> >  /*
> >   * write_unlock_ip - Release all locks used by a client
> >   *
> > @@ -1400,6 +1417,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
> >  		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
> >  		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
> >  #endif
> > +		[NFSD_Rpc_Status] = {"rpc_status", &nfsd_rpc_status_fops, S_IRUGO},
> >  		/* last one */ {""}
> >  	};
> >  
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index d88498f8b275..a149157ec243 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -87,6 +87,7 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
> >   */
> >  int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
> >  int		nfsd_dispatch(struct svc_rqst *rqstp);
> > +void		nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv);
> >  
> >  int		nfsd_nrthreads(struct net *);
> >  int		nfsd_nrpools(struct net *);
> > @@ -506,6 +507,7 @@ extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
> >  
> >  extern void nfsd4_init_leases_net(struct nfsd_net *nn);
> >  
> > +const char *nfsd4_op_name(unsigned opnum);
> >  #else /* CONFIG_NFSD_V4 */
> >  static inline int nfsd4_is_junction(struct dentry *dentry)
> >  {
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 9c7b1ef5be40..7a881f9a3a13 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -1142,3 +1142,64 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
> >  	mutex_unlock(&nfsd_mutex);
> >  	return ret;
> >  }
> > +
> 
> Please add a kdoc comment here that describes the purpose of this
> function, its API contract, and any non-obvious assumptions.
> 
> 
> > +void nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv)
> > +{
> > +	int i;
> > +
> > +	lockdep_assert_held(&nfsd_mutex);
> 
> The nfsd_mutex is held, I assume, to serialize with service
> shutdown. IMO you should add a comment to that effect.
> 
> > +
> > +	rcu_read_lock();
> 
> The RCU read lock protects the sp_all_threads list. OK.
> 
> > +
> > +	for (i = 0; i < serv->sv_nrpools; i++) {
> > +		struct svc_rqst *rqstp;
> > +
> > +		seq_puts(m, "XID        | FLAGS      | PROG       |");
> > +		seq_puts(m, " VERS       | PROC\t|");
> > +		seq_puts(m, " REMOTE - LOCAL IP ADDR");
> > +		seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
> > +		list_for_each_entry_rcu(rqstp,
> > +					&serv->sv_pools[i].sp_all_threads,
> > +					rq_all) {
> > +			if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
> > +				continue;
> > +
> > +			seq_printf(m,
> > +				   "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
> > +				   be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
> > +				   rqstp->rq_prog, rqstp->rq_vers,
> > +				   svc_proc_name(rqstp));

Had another thought on this. svc_rqst::rq_stime should be included here
because if both the XID and stime don't change between peeks into this
file, that's a sure sign that the transaction is not making progress.


> > +			if (rqstp->rq_addr.ss_family == AF_INET) {
> > +				seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
> > +					   &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
> > +					   &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
> > +			} else if (rqstp->rq_addr.ss_family == AF_INET6) {
> > +				seq_printf(m, " %pI6 - %pI6 |",
> > +					   &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
> > +					   &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
> > +			} else {
> > +				seq_printf(m, " Unknown address family: %hu\n",
> > +					   rqstp->rq_addr.ss_family);
> > +				continue;
> > +			}
> > +#ifdef CONFIG_NFSD_V4
> > +			if (rqstp->rq_vers == NFS4_VERSION &&
> > +			    rqstp->rq_proc == NFSPROC4_COMPOUND) {
> > +				/* NFSv4 compund */
> > +				struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > +				struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > +
> > +				while (resp->opcnt < args->opcnt) {
> > +					struct nfsd4_op *op = &args->ops[resp->opcnt++];
> > +
> > +					seq_printf(m, " %s", nfsd4_op_name(op->opnum));
> > +				}
> > +			}
> > +#endif /* CONFIG_NFSD_V4 */
> > +			seq_puts(m, "\n");
> 
> My only quibble here is that the file format needs to be parsable
> by scripts as well as readable by humans. I'm not sure I have a
> specific comment, but it's something that needs some attention and
> verification (with, say, a sample user space tool, hint hint).
> 
> 
> > +		}
> > +	}
> > +
> > +	rcu_read_unlock();
> > +}
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index e6d4cec61e47..f99cad92e9f4 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -1625,7 +1625,7 @@ const char *svc_proc_name(const struct svc_rqst *rqstp)
> >  		return rqstp->rq_procinfo->pc_name;
> >  	return "unknown";
> >  }
> > -
> > +EXPORT_SYMBOL_GPL(svc_proc_name);
> >  
> >  /**
> >   * svc_encode_result_payload - mark a range of bytes as a result payload
> > -- 
> > 2.41.0
> >
Lorenzo Bianconi July 10, 2023, 3:14 p.m. UTC | #4
> On Thu, 2023-06-29 at 12:17 +0200, Lorenzo Bianconi wrote:
> > Introduce rpc_status entry in nfsd debug filesystem in order to dump
> > pending RPC requests debugging information.
> > 
> > Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=366
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  fs/nfsd/nfs4proc.c |  4 +--
> >  fs/nfsd/nfsctl.c   | 18 ++++++++++++++
> >  fs/nfsd/nfsd.h     |  2 ++
> >  fs/nfsd/nfssvc.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
> >  net/sunrpc/svc.c   |  2 +-
> >  5 files changed, 83 insertions(+), 4 deletions(-)
> > 
> 
> This looks good, Lorenzo. Nice work! Comments below.

Hi Jeff,

thx for the review.

> 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 5ae670807449..a4dd1ef104c3 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -2452,8 +2452,6 @@ static inline void nfsd4_increment_op_stats(u32 opnum)
> >  
> >  static const struct nfsd4_operation nfsd4_ops[];
> >  
> > -static const char *nfsd4_op_name(unsigned opnum);
> > -
> >  /*
> >   * Enforce NFSv4.1 COMPOUND ordering rules:
> >   *
> > @@ -3583,7 +3581,7 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op)
> >  	}
> >  }
> >  
> > -static const char *nfsd4_op_name(unsigned opnum)
> > +const char *nfsd4_op_name(unsigned opnum)
> >  {
> >  	if (opnum < ARRAY_SIZE(nfsd4_ops))
> >  		return nfsd4_ops[opnum].op_name;
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 1489e0b703b4..9d0b0a53510b 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -57,6 +57,8 @@ enum {
> >  	NFSD_RecoveryDir,
> >  	NFSD_V4EndGrace,
> >  #endif
> > +	NFSD_Rpc_Status,
> > +
> >  	NFSD_MaxReserved
> >  };
> >  
> > @@ -195,6 +197,21 @@ static inline struct net *netns(struct file *file)
> >  	return file_inode(file)->i_sb->s_fs_info;
> >  }
> >  
> > +static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> > +{
> > +	struct nfsd_net *nn = net_generic(file_inode(m->file)->i_sb->s_fs_info,
> > +					  nfsd_net_id);
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +	if (nn->nfsd_serv)
> > +		nsfd_rpc_status(m, nn->nfsd_serv);
> > +	mutex_unlock(&nfsd_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(nfsd_rpc_status);
> > +
> >  /*
> >   * write_unlock_ip - Release all locks used by a client
> >   *
> > @@ -1400,6 +1417,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
> >  		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
> >  		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
> >  #endif
> > +		[NFSD_Rpc_Status] = {"rpc_status", &nfsd_rpc_status_fops, S_IRUGO},
> >  		/* last one */ {""}
> >  	};
> >  
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index d88498f8b275..a149157ec243 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -87,6 +87,7 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
> >   */
> >  int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
> >  int		nfsd_dispatch(struct svc_rqst *rqstp);
> > +void		nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv);
> 
> nit: please rename this to nfsd_rpc_status

ack, I will fix it.

> 
> >  
> >  int		nfsd_nrthreads(struct net *);
> >  int		nfsd_nrpools(struct net *);
> > @@ -506,6 +507,7 @@ extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
> >  
> >  extern void nfsd4_init_leases_net(struct nfsd_net *nn);
> >  
> > +const char *nfsd4_op_name(unsigned opnum);
> >  #else /* CONFIG_NFSD_V4 */
> >  static inline int nfsd4_is_junction(struct dentry *dentry)
> >  {
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 9c7b1ef5be40..7a881f9a3a13 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -1142,3 +1142,64 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
> >  	mutex_unlock(&nfsd_mutex);
> >  	return ret;
> >  }
> > +
> > +void nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv)
> > +{
> > +	int i;
> > +
> > +	lockdep_assert_held(&nfsd_mutex);
> > +
> > +	rcu_read_lock();
> 
> Both sv_nrpools and the sp_all_threads list are protected by the
> nfsd_mutex, so I don't think you need the rcu_read_lock here too.
> 
> It would be nice if we could do this with only the rcu_read_lock. I
> think accessing all of the svc_rqst fields under the rcu_read_lock is
> safe, and sv_nrpools is only set when the service is created, so I think
> that would be safe.

ack, I will fix it.

Regards,
Lorenzo

> 
> 
> > +
> > +	for (i = 0; i < serv->sv_nrpools; i++) {
> > +		struct svc_rqst *rqstp;
> > +
> > +		seq_puts(m, "XID        | FLAGS      | PROG       |");
> > +		seq_puts(m, " VERS       | PROC\t|");
> > +		seq_puts(m, " REMOTE - LOCAL IP ADDR");
> > +		seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
> > +		list_for_each_entry_rcu(rqstp,
> > +					&serv->sv_pools[i].sp_all_threads,
> > +					rq_all) {
> > +			if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
> > +				continue;
> > +
> > +			seq_printf(m,
> > +				   "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
> > +				   be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
> > +				   rqstp->rq_prog, rqstp->rq_vers,
> > +				   svc_proc_name(rqstp));
> > +
> > +			if (rqstp->rq_addr.ss_family == AF_INET) {
> > +				seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
> > +					   &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
> > +					   &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
> > +			} else if (rqstp->rq_addr.ss_family == AF_INET6) {
> > +				seq_printf(m, " %pI6 - %pI6 |",
> > +					   &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
> > +					   &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
> > +			} else {
> > +				seq_printf(m, " Unknown address family: %hu\n",
> > +					   rqstp->rq_addr.ss_family);
> > +				continue;
> > +			}
> > +#ifdef CONFIG_NFSD_V4
> > +			if (rqstp->rq_vers == NFS4_VERSION &&
> > +			    rqstp->rq_proc == NFSPROC4_COMPOUND) {
> > +				/* NFSv4 compund */
> > +				struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > +				struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > +
> > +				while (resp->opcnt < args->opcnt) {
> > +					struct nfsd4_op *op = &args->ops[resp->opcnt++];
> > +
> > +					seq_printf(m, " %s", nfsd4_op_name(op->opnum));
> > +				}
> > +			}
> > +#endif /* CONFIG_NFSD_V4 */
> > +			seq_puts(m, "\n");
> > +		}
> > +	}
> > +
> > +	rcu_read_unlock();
> > +}
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index e6d4cec61e47..f99cad92e9f4 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -1625,7 +1625,7 @@ const char *svc_proc_name(const struct svc_rqst *rqstp)
> >  		return rqstp->rq_procinfo->pc_name;
> >  	return "unknown";
> >  }
> > -
> > +EXPORT_SYMBOL_GPL(svc_proc_name);
> >  
> >  /**
> >   * svc_encode_result_payload - mark a range of bytes as a result payload
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
Lorenzo Bianconi July 10, 2023, 3:33 p.m. UTC | #5
> On Thu, Jun 29, 2023 at 12:17:33PM +0200, Lorenzo Bianconi wrote:
> > Introduce rpc_status entry in nfsd debug filesystem in order to dump
> > pending RPC requests debugging information.
> > 
> > Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=366
> 
> Nice to see progress on this.

Hi Chuck,

thx for the review.

> 
> Do you have a user space tool to monitor this file?

not so far.

> 
> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  fs/nfsd/nfs4proc.c |  4 +--
> >  fs/nfsd/nfsctl.c   | 18 ++++++++++++++
> >  fs/nfsd/nfsd.h     |  2 ++
> >  fs/nfsd/nfssvc.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
> >  net/sunrpc/svc.c   |  2 +-
> >  5 files changed, 83 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 5ae670807449..a4dd1ef104c3 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -2452,8 +2452,6 @@ static inline void nfsd4_increment_op_stats(u32 opnum)
> >  
> >  static const struct nfsd4_operation nfsd4_ops[];
> >  
> > -static const char *nfsd4_op_name(unsigned opnum);
> > -
> >  /*
> >   * Enforce NFSv4.1 COMPOUND ordering rules:
> >   *
> > @@ -3583,7 +3581,7 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op)
> >  	}
> >  }
> >  
> > -static const char *nfsd4_op_name(unsigned opnum)
> > +const char *nfsd4_op_name(unsigned opnum)
> >  {
> >  	if (opnum < ARRAY_SIZE(nfsd4_ops))
> >  		return nfsd4_ops[opnum].op_name;
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 1489e0b703b4..9d0b0a53510b 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -57,6 +57,8 @@ enum {
> >  	NFSD_RecoveryDir,
> >  	NFSD_V4EndGrace,
> >  #endif
> > +	NFSD_Rpc_Status,
> > +
> >  	NFSD_MaxReserved
> >  };
> >  
> > @@ -195,6 +197,21 @@ static inline struct net *netns(struct file *file)
> >  	return file_inode(file)->i_sb->s_fs_info;
> >  }
> >  
> > +static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> > +{
> > +	struct nfsd_net *nn = net_generic(file_inode(m->file)->i_sb->s_fs_info,
> > +					  nfsd_net_id);
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +	if (nn->nfsd_serv)
> > +		nsfd_rpc_status(m, nn->nfsd_serv);
> > +	mutex_unlock(&nfsd_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(nfsd_rpc_status);
> > +
> >  /*
> >   * write_unlock_ip - Release all locks used by a client
> >   *
> > @@ -1400,6 +1417,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
> >  		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
> >  		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
> >  #endif
> > +		[NFSD_Rpc_Status] = {"rpc_status", &nfsd_rpc_status_fops, S_IRUGO},
> >  		/* last one */ {""}
> >  	};
> >  
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index d88498f8b275..a149157ec243 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -87,6 +87,7 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
> >   */
> >  int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
> >  int		nfsd_dispatch(struct svc_rqst *rqstp);
> > +void		nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv);
> >  
> >  int		nfsd_nrthreads(struct net *);
> >  int		nfsd_nrpools(struct net *);
> > @@ -506,6 +507,7 @@ extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
> >  
> >  extern void nfsd4_init_leases_net(struct nfsd_net *nn);
> >  
> > +const char *nfsd4_op_name(unsigned opnum);
> >  #else /* CONFIG_NFSD_V4 */
> >  static inline int nfsd4_is_junction(struct dentry *dentry)
> >  {
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 9c7b1ef5be40..7a881f9a3a13 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -1142,3 +1142,64 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
> >  	mutex_unlock(&nfsd_mutex);
> >  	return ret;
> >  }
> > +
> 
> Please add a kdoc comment here that describes the purpose of this
> function, its API contract, and any non-obvious assumptions.

ack, will do.

> 
> 
> > +void nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv)
> > +{
> > +	int i;
> > +
> > +	lockdep_assert_held(&nfsd_mutex);
> 
> The nfsd_mutex is held, I assume, to serialize with service
> shutdown. IMO you should add a comment to that effect.

I will rework it addressing Jeff's comments.

> 
> > +
> > +	rcu_read_lock();
> 
> The RCU read lock protects the sp_all_threads list. OK.
> 
> > +
> > +	for (i = 0; i < serv->sv_nrpools; i++) {
> > +		struct svc_rqst *rqstp;
> > +
> > +		seq_puts(m, "XID        | FLAGS      | PROG       |");
> > +		seq_puts(m, " VERS       | PROC\t|");
> > +		seq_puts(m, " REMOTE - LOCAL IP ADDR");
> > +		seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
> > +		list_for_each_entry_rcu(rqstp,
> > +					&serv->sv_pools[i].sp_all_threads,
> > +					rq_all) {
> > +			if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
> > +				continue;
> > +
> > +			seq_printf(m,
> > +				   "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
> > +				   be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
> > +				   rqstp->rq_prog, rqstp->rq_vers,
> > +				   svc_proc_name(rqstp));
> > +
> > +			if (rqstp->rq_addr.ss_family == AF_INET) {
> > +				seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
> > +					   &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
> > +					   &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
> > +			} else if (rqstp->rq_addr.ss_family == AF_INET6) {
> > +				seq_printf(m, " %pI6 - %pI6 |",
> > +					   &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
> > +					   &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
> > +			} else {
> > +				seq_printf(m, " Unknown address family: %hu\n",
> > +					   rqstp->rq_addr.ss_family);
> > +				continue;
> > +			}
> > +#ifdef CONFIG_NFSD_V4
> > +			if (rqstp->rq_vers == NFS4_VERSION &&
> > +			    rqstp->rq_proc == NFSPROC4_COMPOUND) {
> > +				/* NFSv4 compund */
> > +				struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > +				struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > +
> > +				while (resp->opcnt < args->opcnt) {
> > +					struct nfsd4_op *op = &args->ops[resp->opcnt++];
> > +
> > +					seq_printf(m, " %s", nfsd4_op_name(op->opnum));
> > +				}
> > +			}
> > +#endif /* CONFIG_NFSD_V4 */
> > +			seq_puts(m, "\n");
> 
> My only quibble here is that the file format needs to be parsable
> by scripts as well as readable by humans. I'm not sure I have a
> specific comment, but it's something that needs some attention and
> verification (with, say, a sample user space tool, hint hint).

maybe we can add a csv hanlder, what do you think? not sure.

Regards,
Lorenzo

> 
> 
> > +		}
> > +	}
> > +
> > +	rcu_read_unlock();
> > +}
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index e6d4cec61e47..f99cad92e9f4 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -1625,7 +1625,7 @@ const char *svc_proc_name(const struct svc_rqst *rqstp)
> >  		return rqstp->rq_procinfo->pc_name;
> >  	return "unknown";
> >  }
> > -
> > +EXPORT_SYMBOL_GPL(svc_proc_name);
> >  
> >  /**
> >   * svc_encode_result_payload - mark a range of bytes as a result payload
> > -- 
> > 2.41.0
> >
Chuck Lever July 10, 2023, 3:55 p.m. UTC | #6
> On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
>>> + for (i = 0; i < serv->sv_nrpools; i++) {
>>> + struct svc_rqst *rqstp;
>>> +
>>> + seq_puts(m, "XID        | FLAGS      | PROG       |");
>>> + seq_puts(m, " VERS       | PROC\t|");
>>> + seq_puts(m, " REMOTE - LOCAL IP ADDR");
>>> + seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
>>> + list_for_each_entry_rcu(rqstp,
>>> + &serv->sv_pools[i].sp_all_threads,
>>> + rq_all) {
>>> + if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
>>> + continue;
>>> +
>>> + seq_printf(m,
>>> +    "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
>>> +    be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
>>> +    rqstp->rq_prog, rqstp->rq_vers,
>>> +    svc_proc_name(rqstp));
>>> +
>>> + if (rqstp->rq_addr.ss_family == AF_INET) {
>>> + seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
>>> +    &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
>>> +    &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
>>> + } else if (rqstp->rq_addr.ss_family == AF_INET6) {
>>> + seq_printf(m, " %pI6 - %pI6 |",
>>> +    &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
>>> +    &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
>>> + } else {
>>> + seq_printf(m, " Unknown address family: %hu\n",
>>> +    rqstp->rq_addr.ss_family);
>>> + continue;
>>> + }
>>> +#ifdef CONFIG_NFSD_V4
>>> + if (rqstp->rq_vers == NFS4_VERSION &&
>>> +     rqstp->rq_proc == NFSPROC4_COMPOUND) {
>>> + /* NFSv4 compund */
>>> + struct nfsd4_compoundargs *args = rqstp->rq_argp;
>>> + struct nfsd4_compoundres *resp = rqstp->rq_resp;
>>> +
>>> + while (resp->opcnt < args->opcnt) {
>>> + struct nfsd4_op *op = &args->ops[resp->opcnt++];
>>> +
>>> + seq_printf(m, " %s", nfsd4_op_name(op->opnum));
>>> + }
>>> + }
>>> +#endif /* CONFIG_NFSD_V4 */
>>> + seq_puts(m, "\n");
>> 
>> My only quibble here is that the file format needs to be parsable
>> by scripts as well as readable by humans. I'm not sure I have a
>> specific comment, but it's something that needs some attention and
>> verification (with, say, a sample user space tool, hint hint).
> 
> maybe we can add a csv hanlder, what do you think? not sure.

I suggested JSON to Jeff as another option, but I don't think we want
to swing that far in the other direction.

There are plenty of examples of /sys files that are both parsable and
human-friendly. I'll leave it to you to find one or two formats that
seem capable of the task at hand, and let's pick from one of those.


--
Chuck Lever
Lorenzo Bianconi July 10, 2023, 4:18 p.m. UTC | #7
On Jun 29, Chuck Lever wrote:
> On Thu, Jun 29, 2023 at 09:55:28AM -0400, Chuck Lever wrote:
> > On Thu, Jun 29, 2023 at 12:17:33PM +0200, Lorenzo Bianconi wrote:
> > > Introduce rpc_status entry in nfsd debug filesystem in order to dump
> > > pending RPC requests debugging information.
> > > 
> > > Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=366
> > 
> > Nice to see progress on this.
> > 
> > Do you have a user space tool to monitor this file?
> > 
> > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  fs/nfsd/nfs4proc.c |  4 +--
> > >  fs/nfsd/nfsctl.c   | 18 ++++++++++++++
> > >  fs/nfsd/nfsd.h     |  2 ++
> > >  fs/nfsd/nfssvc.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  net/sunrpc/svc.c   |  2 +-
> > >  5 files changed, 83 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 5ae670807449..a4dd1ef104c3 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -2452,8 +2452,6 @@ static inline void nfsd4_increment_op_stats(u32 opnum)
> > >  
> > >  static const struct nfsd4_operation nfsd4_ops[];
> > >  
> > > -static const char *nfsd4_op_name(unsigned opnum);
> > > -
> > >  /*
> > >   * Enforce NFSv4.1 COMPOUND ordering rules:
> > >   *
> > > @@ -3583,7 +3581,7 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op)
> > >  	}
> > >  }
> > >  
> > > -static const char *nfsd4_op_name(unsigned opnum)
> > > +const char *nfsd4_op_name(unsigned opnum)
> > >  {
> > >  	if (opnum < ARRAY_SIZE(nfsd4_ops))
> > >  		return nfsd4_ops[opnum].op_name;
> > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > > index 1489e0b703b4..9d0b0a53510b 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -57,6 +57,8 @@ enum {
> > >  	NFSD_RecoveryDir,
> > >  	NFSD_V4EndGrace,
> > >  #endif
> > > +	NFSD_Rpc_Status,
> > > +
> > >  	NFSD_MaxReserved
> > >  };
> > >  
> > > @@ -195,6 +197,21 @@ static inline struct net *netns(struct file *file)
> > >  	return file_inode(file)->i_sb->s_fs_info;
> > >  }
> > >  
> > > +static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> > > +{
> > > +	struct nfsd_net *nn = net_generic(file_inode(m->file)->i_sb->s_fs_info,
> > > +					  nfsd_net_id);
> > > +
> > > +	mutex_lock(&nfsd_mutex);
> > > +	if (nn->nfsd_serv)
> > > +		nsfd_rpc_status(m, nn->nfsd_serv);
> > > +	mutex_unlock(&nfsd_mutex);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +DEFINE_SHOW_ATTRIBUTE(nfsd_rpc_status);
> > > +
> > >  /*
> > >   * write_unlock_ip - Release all locks used by a client
> > >   *
> > > @@ -1400,6 +1417,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
> > >  		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
> > >  		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
> > >  #endif
> > > +		[NFSD_Rpc_Status] = {"rpc_status", &nfsd_rpc_status_fops, S_IRUGO},
> > >  		/* last one */ {""}
> > >  	};
> > >  
> > > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > > index d88498f8b275..a149157ec243 100644
> > > --- a/fs/nfsd/nfsd.h
> > > +++ b/fs/nfsd/nfsd.h
> > > @@ -87,6 +87,7 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
> > >   */
> > >  int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
> > >  int		nfsd_dispatch(struct svc_rqst *rqstp);
> > > +void		nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv);
> > >  
> > >  int		nfsd_nrthreads(struct net *);
> > >  int		nfsd_nrpools(struct net *);
> > > @@ -506,6 +507,7 @@ extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
> > >  
> > >  extern void nfsd4_init_leases_net(struct nfsd_net *nn);
> > >  
> > > +const char *nfsd4_op_name(unsigned opnum);
> > >  #else /* CONFIG_NFSD_V4 */
> > >  static inline int nfsd4_is_junction(struct dentry *dentry)
> > >  {
> > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > > index 9c7b1ef5be40..7a881f9a3a13 100644
> > > --- a/fs/nfsd/nfssvc.c
> > > +++ b/fs/nfsd/nfssvc.c
> > > @@ -1142,3 +1142,64 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
> > >  	mutex_unlock(&nfsd_mutex);
> > >  	return ret;
> > >  }
> > > +
> > 
> > Please add a kdoc comment here that describes the purpose of this
> > function, its API contract, and any non-obvious assumptions.
> > 
> > 
> > > +void nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv)
> > > +{
> > > +	int i;
> > > +
> > > +	lockdep_assert_held(&nfsd_mutex);
> > 
> > The nfsd_mutex is held, I assume, to serialize with service
> > shutdown. IMO you should add a comment to that effect.
> > 
> > > +
> > > +	rcu_read_lock();
> > 
> > The RCU read lock protects the sp_all_threads list. OK.
> > 
> > > +
> > > +	for (i = 0; i < serv->sv_nrpools; i++) {
> > > +		struct svc_rqst *rqstp;
> > > +
> > > +		seq_puts(m, "XID        | FLAGS      | PROG       |");
> > > +		seq_puts(m, " VERS       | PROC\t|");
> > > +		seq_puts(m, " REMOTE - LOCAL IP ADDR");
> > > +		seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
> > > +		list_for_each_entry_rcu(rqstp,
> > > +					&serv->sv_pools[i].sp_all_threads,
> > > +					rq_all) {
> > > +			if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
> > > +				continue;
> > > +
> > > +			seq_printf(m,
> > > +				   "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
> > > +				   be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
> > > +				   rqstp->rq_prog, rqstp->rq_vers,
> > > +				   svc_proc_name(rqstp));
> 
> Had another thought on this. svc_rqst::rq_stime should be included here
> because if both the XID and stime don't change between peeks into this
> file, that's a sure sign that the transaction is not making progress.

ack, I will add it.

Regards,
Lorenzo

> 
> 
> > > +			if (rqstp->rq_addr.ss_family == AF_INET) {
> > > +				seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
> > > +					   &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
> > > +					   &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
> > > +			} else if (rqstp->rq_addr.ss_family == AF_INET6) {
> > > +				seq_printf(m, " %pI6 - %pI6 |",
> > > +					   &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
> > > +					   &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
> > > +			} else {
> > > +				seq_printf(m, " Unknown address family: %hu\n",
> > > +					   rqstp->rq_addr.ss_family);
> > > +				continue;
> > > +			}
> > > +#ifdef CONFIG_NFSD_V4
> > > +			if (rqstp->rq_vers == NFS4_VERSION &&
> > > +			    rqstp->rq_proc == NFSPROC4_COMPOUND) {
> > > +				/* NFSv4 compund */
> > > +				struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > > +				struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > > +
> > > +				while (resp->opcnt < args->opcnt) {
> > > +					struct nfsd4_op *op = &args->ops[resp->opcnt++];
> > > +
> > > +					seq_printf(m, " %s", nfsd4_op_name(op->opnum));
> > > +				}
> > > +			}
> > > +#endif /* CONFIG_NFSD_V4 */
> > > +			seq_puts(m, "\n");
> > 
> > My only quibble here is that the file format needs to be parsable
> > by scripts as well as readable by humans. I'm not sure I have a
> > specific comment, but it's something that needs some attention and
> > verification (with, say, a sample user space tool, hint hint).
> > 
> > 
> > > +		}
> > > +	}
> > > +
> > > +	rcu_read_unlock();
> > > +}
> > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > > index e6d4cec61e47..f99cad92e9f4 100644
> > > --- a/net/sunrpc/svc.c
> > > +++ b/net/sunrpc/svc.c
> > > @@ -1625,7 +1625,7 @@ const char *svc_proc_name(const struct svc_rqst *rqstp)
> > >  		return rqstp->rq_procinfo->pc_name;
> > >  	return "unknown";
> > >  }
> > > -
> > > +EXPORT_SYMBOL_GPL(svc_proc_name);
> > >  
> > >  /**
> > >   * svc_encode_result_payload - mark a range of bytes as a result payload
> > > -- 
> > > 2.41.0
> > >
Lorenzo Bianconi July 10, 2023, 4:19 p.m. UTC | #8
On Jul 10, Chuck Lever III wrote:
> 
> 
> > On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > 
> >>> + for (i = 0; i < serv->sv_nrpools; i++) {
> >>> + struct svc_rqst *rqstp;
> >>> +
> >>> + seq_puts(m, "XID        | FLAGS      | PROG       |");
> >>> + seq_puts(m, " VERS       | PROC\t|");
> >>> + seq_puts(m, " REMOTE - LOCAL IP ADDR");
> >>> + seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
> >>> + list_for_each_entry_rcu(rqstp,
> >>> + &serv->sv_pools[i].sp_all_threads,
> >>> + rq_all) {
> >>> + if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
> >>> + continue;
> >>> +
> >>> + seq_printf(m,
> >>> +    "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
> >>> +    be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
> >>> +    rqstp->rq_prog, rqstp->rq_vers,
> >>> +    svc_proc_name(rqstp));
> >>> +
> >>> + if (rqstp->rq_addr.ss_family == AF_INET) {
> >>> + seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
> >>> +    &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
> >>> +    &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
> >>> + } else if (rqstp->rq_addr.ss_family == AF_INET6) {
> >>> + seq_printf(m, " %pI6 - %pI6 |",
> >>> +    &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
> >>> +    &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
> >>> + } else {
> >>> + seq_printf(m, " Unknown address family: %hu\n",
> >>> +    rqstp->rq_addr.ss_family);
> >>> + continue;
> >>> + }
> >>> +#ifdef CONFIG_NFSD_V4
> >>> + if (rqstp->rq_vers == NFS4_VERSION &&
> >>> +     rqstp->rq_proc == NFSPROC4_COMPOUND) {
> >>> + /* NFSv4 compund */
> >>> + struct nfsd4_compoundargs *args = rqstp->rq_argp;
> >>> + struct nfsd4_compoundres *resp = rqstp->rq_resp;
> >>> +
> >>> + while (resp->opcnt < args->opcnt) {
> >>> + struct nfsd4_op *op = &args->ops[resp->opcnt++];
> >>> +
> >>> + seq_printf(m, " %s", nfsd4_op_name(op->opnum));
> >>> + }
> >>> + }
> >>> +#endif /* CONFIG_NFSD_V4 */
> >>> + seq_puts(m, "\n");
> >> 
> >> My only quibble here is that the file format needs to be parsable
> >> by scripts as well as readable by humans. I'm not sure I have a
> >> specific comment, but it's something that needs some attention and
> >> verification (with, say, a sample user space tool, hint hint).
> > 
> > maybe we can add a csv hanlder, what do you think? not sure.
> 
> I suggested JSON to Jeff as another option, but I don't think we want
> to swing that far in the other direction.
> 
> There are plenty of examples of /sys files that are both parsable and
> human-friendly. I'll leave it to you to find one or two formats that
> seem capable of the task at hand, and let's pick from one of those.

ok, I will take a look. In the meantime I posted a new revision to continue the
discussion.

Regartds,
Lorenzo

> 
> 
> --
> Chuck Lever
> 
>
Jeff Layton July 10, 2023, 4:36 p.m. UTC | #9
On Mon, 2023-07-10 at 15:55 +0000, Chuck Lever III wrote:
> 
> > On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > 
> > > > + for (i = 0; i < serv->sv_nrpools; i++) {
> > > > + struct svc_rqst *rqstp;
> > > > +
> > > > + seq_puts(m, "XID        | FLAGS      | PROG       |");
> > > > + seq_puts(m, " VERS       | PROC\t|");
> > > > + seq_puts(m, " REMOTE - LOCAL IP ADDR");
> > > > + seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
> > > > + list_for_each_entry_rcu(rqstp,
> > > > + &serv->sv_pools[i].sp_all_threads,
> > > > + rq_all) {
> > > > + if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
> > > > + continue;
> > > > +
> > > > + seq_printf(m,
> > > > +    "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
> > > > +    be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
> > > > +    rqstp->rq_prog, rqstp->rq_vers,
> > > > +    svc_proc_name(rqstp));
> > > > +
> > > > + if (rqstp->rq_addr.ss_family == AF_INET) {
> > > > + seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
> > > > +    &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
> > > > +    &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
> > > > + } else if (rqstp->rq_addr.ss_family == AF_INET6) {
> > > > + seq_printf(m, " %pI6 - %pI6 |",
> > > > +    &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
> > > > +    &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
> > > > + } else {
> > > > + seq_printf(m, " Unknown address family: %hu\n",
> > > > +    rqstp->rq_addr.ss_family);
> > > > + continue;
> > > > + }
> > > > +#ifdef CONFIG_NFSD_V4
> > > > + if (rqstp->rq_vers == NFS4_VERSION &&
> > > > +     rqstp->rq_proc == NFSPROC4_COMPOUND) {
> > > > + /* NFSv4 compund */
> > > > + struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > > > + struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > > > +
> > > > + while (resp->opcnt < args->opcnt) {
> > > > + struct nfsd4_op *op = &args->ops[resp->opcnt++];
> > > > +
> > > > + seq_printf(m, " %s", nfsd4_op_name(op->opnum));
> > > > + }
> > > > + }
> > > > +#endif /* CONFIG_NFSD_V4 */
> > > > + seq_puts(m, "\n");
> > > 
> > > My only quibble here is that the file format needs to be parsable
> > > by scripts as well as readable by humans. I'm not sure I have a
> > > specific comment, but it's something that needs some attention and
> > > verification (with, say, a sample user space tool, hint hint).
> > 
> > maybe we can add a csv hanlder, what do you think? not sure.
> 
> I suggested JSON to Jeff as another option, but I don't think we want
> to swing that far in the other direction.
> 
> There are plenty of examples of /sys files that are both parsable and
> human-friendly. I'll leave it to you to find one or two formats that
> seem capable of the task at hand, and let's pick from one of those.
> 

Are there already kernel libraries for this, or any examples of kernel
interfaces that emit JSON? Most of the kernel interfaces I have
experience with just use well-known fields delimited by spaces.
Chuck Lever July 10, 2023, 4:39 p.m. UTC | #10
> On Jul 10, 2023, at 12:36 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2023-07-10 at 15:55 +0000, Chuck Lever III wrote:
>> 
>>> On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>>> 
>>>>> + for (i = 0; i < serv->sv_nrpools; i++) {
>>>>> + struct svc_rqst *rqstp;
>>>>> +
>>>>> + seq_puts(m, "XID        | FLAGS      | PROG       |");
>>>>> + seq_puts(m, " VERS       | PROC\t|");
>>>>> + seq_puts(m, " REMOTE - LOCAL IP ADDR");
>>>>> + seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
>>>>> + list_for_each_entry_rcu(rqstp,
>>>>> + &serv->sv_pools[i].sp_all_threads,
>>>>> + rq_all) {
>>>>> + if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
>>>>> + continue;
>>>>> +
>>>>> + seq_printf(m,
>>>>> +    "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
>>>>> +    be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
>>>>> +    rqstp->rq_prog, rqstp->rq_vers,
>>>>> +    svc_proc_name(rqstp));
>>>>> +
>>>>> + if (rqstp->rq_addr.ss_family == AF_INET) {
>>>>> + seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
>>>>> +    &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
>>>>> +    &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
>>>>> + } else if (rqstp->rq_addr.ss_family == AF_INET6) {
>>>>> + seq_printf(m, " %pI6 - %pI6 |",
>>>>> +    &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
>>>>> +    &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
>>>>> + } else {
>>>>> + seq_printf(m, " Unknown address family: %hu\n",
>>>>> +    rqstp->rq_addr.ss_family);
>>>>> + continue;
>>>>> + }
>>>>> +#ifdef CONFIG_NFSD_V4
>>>>> + if (rqstp->rq_vers == NFS4_VERSION &&
>>>>> +     rqstp->rq_proc == NFSPROC4_COMPOUND) {
>>>>> + /* NFSv4 compund */
>>>>> + struct nfsd4_compoundargs *args = rqstp->rq_argp;
>>>>> + struct nfsd4_compoundres *resp = rqstp->rq_resp;
>>>>> +
>>>>> + while (resp->opcnt < args->opcnt) {
>>>>> + struct nfsd4_op *op = &args->ops[resp->opcnt++];
>>>>> +
>>>>> + seq_printf(m, " %s", nfsd4_op_name(op->opnum));
>>>>> + }
>>>>> + }
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> + seq_puts(m, "\n");
>>>> 
>>>> My only quibble here is that the file format needs to be parsable
>>>> by scripts as well as readable by humans. I'm not sure I have a
>>>> specific comment, but it's something that needs some attention and
>>>> verification (with, say, a sample user space tool, hint hint).
>>> 
>>> maybe we can add a csv hanlder, what do you think? not sure.
>> 
>> I suggested JSON to Jeff as another option, but I don't think we want
>> to swing that far in the other direction.
>> 
>> There are plenty of examples of /sys files that are both parsable and
>> human-friendly. I'll leave it to you to find one or two formats that
>> seem capable of the task at hand, and let's pick from one of those.
>> 
> 
> Are there already kernel libraries for this, or any examples of kernel
> interfaces that emit JSON? Most of the kernel interfaces I have
> experience with just use well-known fields delimited by spaces.

As I said, I don't think we need to go with a heavily structured
format. JSON was just "yet another crazy idea."

Other /sys files seem to strike a sensible balance, so let's research
what's already in use first.


--
Chuck Lever
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 5ae670807449..a4dd1ef104c3 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2452,8 +2452,6 @@  static inline void nfsd4_increment_op_stats(u32 opnum)
 
 static const struct nfsd4_operation nfsd4_ops[];
 
-static const char *nfsd4_op_name(unsigned opnum);
-
 /*
  * Enforce NFSv4.1 COMPOUND ordering rules:
  *
@@ -3583,7 +3581,7 @@  void warn_on_nonidempotent_op(struct nfsd4_op *op)
 	}
 }
 
-static const char *nfsd4_op_name(unsigned opnum)
+const char *nfsd4_op_name(unsigned opnum)
 {
 	if (opnum < ARRAY_SIZE(nfsd4_ops))
 		return nfsd4_ops[opnum].op_name;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 1489e0b703b4..9d0b0a53510b 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -57,6 +57,8 @@  enum {
 	NFSD_RecoveryDir,
 	NFSD_V4EndGrace,
 #endif
+	NFSD_Rpc_Status,
+
 	NFSD_MaxReserved
 };
 
@@ -195,6 +197,21 @@  static inline struct net *netns(struct file *file)
 	return file_inode(file)->i_sb->s_fs_info;
 }
 
+static int nfsd_rpc_status_show(struct seq_file *m, void *v)
+{
+	struct nfsd_net *nn = net_generic(file_inode(m->file)->i_sb->s_fs_info,
+					  nfsd_net_id);
+
+	mutex_lock(&nfsd_mutex);
+	if (nn->nfsd_serv)
+		nsfd_rpc_status(m, nn->nfsd_serv);
+	mutex_unlock(&nfsd_mutex);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(nfsd_rpc_status);
+
 /*
  * write_unlock_ip - Release all locks used by a client
  *
@@ -1400,6 +1417,7 @@  static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
 		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
 #endif
+		[NFSD_Rpc_Status] = {"rpc_status", &nfsd_rpc_status_fops, S_IRUGO},
 		/* last one */ {""}
 	};
 
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index d88498f8b275..a149157ec243 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -87,6 +87,7 @@  bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
  */
 int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
 int		nfsd_dispatch(struct svc_rqst *rqstp);
+void		nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv);
 
 int		nfsd_nrthreads(struct net *);
 int		nfsd_nrpools(struct net *);
@@ -506,6 +507,7 @@  extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
 
 extern void nfsd4_init_leases_net(struct nfsd_net *nn);
 
+const char *nfsd4_op_name(unsigned opnum);
 #else /* CONFIG_NFSD_V4 */
 static inline int nfsd4_is_junction(struct dentry *dentry)
 {
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 9c7b1ef5be40..7a881f9a3a13 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -1142,3 +1142,64 @@  int nfsd_pool_stats_release(struct inode *inode, struct file *file)
 	mutex_unlock(&nfsd_mutex);
 	return ret;
 }
+
+void nsfd_rpc_status(struct seq_file *m, struct svc_serv *serv)
+{
+	int i;
+
+	lockdep_assert_held(&nfsd_mutex);
+
+	rcu_read_lock();
+
+	for (i = 0; i < serv->sv_nrpools; i++) {
+		struct svc_rqst *rqstp;
+
+		seq_puts(m, "XID        | FLAGS      | PROG       |");
+		seq_puts(m, " VERS       | PROC\t|");
+		seq_puts(m, " REMOTE - LOCAL IP ADDR");
+		seq_puts(m, "\t\t\t\t\t\t\t\t   | NFS4 COMPOUND OPS\n");
+		list_for_each_entry_rcu(rqstp,
+					&serv->sv_pools[i].sp_all_threads,
+					rq_all) {
+			if (!test_bit(RQ_BUSY, &rqstp->rq_flags))
+				continue;
+
+			seq_printf(m,
+				   "0x%08x | 0x%08lx | 0x%08x | NFSv%d      | %s\t|",
+				   be32_to_cpu(rqstp->rq_xid), rqstp->rq_flags,
+				   rqstp->rq_prog, rqstp->rq_vers,
+				   svc_proc_name(rqstp));
+
+			if (rqstp->rq_addr.ss_family == AF_INET) {
+				seq_printf(m, " %pI4 - %pI4\t\t\t\t\t\t\t   |",
+					   &((struct sockaddr_in *)&rqstp->rq_addr)->sin_addr,
+					   &((struct sockaddr_in *)&rqstp->rq_daddr)->sin_addr);
+			} else if (rqstp->rq_addr.ss_family == AF_INET6) {
+				seq_printf(m, " %pI6 - %pI6 |",
+					   &((struct sockaddr_in6 *)&rqstp->rq_addr)->sin6_addr,
+					   &((struct sockaddr_in6 *)&rqstp->rq_daddr)->sin6_addr);
+			} else {
+				seq_printf(m, " Unknown address family: %hu\n",
+					   rqstp->rq_addr.ss_family);
+				continue;
+			}
+#ifdef CONFIG_NFSD_V4
+			if (rqstp->rq_vers == NFS4_VERSION &&
+			    rqstp->rq_proc == NFSPROC4_COMPOUND) {
+				/* NFSv4 compund */
+				struct nfsd4_compoundargs *args = rqstp->rq_argp;
+				struct nfsd4_compoundres *resp = rqstp->rq_resp;
+
+				while (resp->opcnt < args->opcnt) {
+					struct nfsd4_op *op = &args->ops[resp->opcnt++];
+
+					seq_printf(m, " %s", nfsd4_op_name(op->opnum));
+				}
+			}
+#endif /* CONFIG_NFSD_V4 */
+			seq_puts(m, "\n");
+		}
+	}
+
+	rcu_read_unlock();
+}
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index e6d4cec61e47..f99cad92e9f4 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1625,7 +1625,7 @@  const char *svc_proc_name(const struct svc_rqst *rqstp)
 		return rqstp->rq_procinfo->pc_name;
 	return "unknown";
 }
-
+EXPORT_SYMBOL_GPL(svc_proc_name);
 
 /**
  * svc_encode_result_payload - mark a range of bytes as a result payload