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

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

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

-- 
2.41.0

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.

nit: please rename this to nfsd_rpc_status

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.

-- 
Jeff Layton [off-list ref]

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?

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

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.

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

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:

quoted

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?

quoted

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.

quoted

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

quoted

+
+	rcu_read_lock();

The RCU read lock protects the sp_all_threads list. OK.

quoted

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

quoted

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

quoted

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

On Thu, 2023-06-29 at 12:17 +0200, Lorenzo Bianconi wrote:

quoted

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.

quoted

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.

quoted

 
 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

quoted

+
+	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 [off-list ref]

On Thu, Jun 29, 2023 at 12:17:33PM +0200, Lorenzo Bianconi wrote:

quoted

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.

quoted

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.

quoted

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

quoted

+
+	rcu_read_lock();

The RCU read lock protects the sp_all_threads list. OK.

quoted

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

quoted

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

On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi [off-list ref] wrote:

quoted

quoted

+ 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

On Jun 29, Chuck Lever wrote:

On Thu, Jun 29, 2023 at 09:55:28AM -0400, Chuck Lever wrote:

quoted

On Thu, Jun 29, 2023 at 12:17:33PM +0200, Lorenzo Bianconi wrote:

quoted

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?

quoted

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.

quoted

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

quoted

+
+	rcu_read_lock();

The RCU read lock protects the sp_all_threads list. OK.

quoted

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

quoted

quoted

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

quoted

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

On Jul 10, Chuck Lever III wrote:

quoted

On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi [off-list ref] wrote:

quoted

quoted

+ 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

On Mon, 2023-07-10 at 15:55 +0000, Chuck Lever III wrote:

quoted

On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi [off-list ref] wrote:

quoted

quoted

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

-- 
Jeff Layton [off-list ref]

On Jul 10, 2023, at 12:36 PM, Jeff Layton [off-list ref] wrote:

On Mon, 2023-07-10 at 15:55 +0000, Chuck Lever III wrote:

quoted

quoted

On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi [off-list ref] wrote:

quoted

quoted

+ 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