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 <[email protected]>
---
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 <[email protected]>
> ---
> 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
--
Jeff Layton <[email protected]>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > ---
> > 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
> >
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> 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 <[email protected]>
> > ---
> > 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
> >
> On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi <[email protected]> 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
On Jul 10, Chuck Lever III wrote:
>
>
> > On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi <[email protected]> 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
>
>
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 <[email protected]>
> > > ---
> > > 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
> > >
On Mon, 2023-07-10 at 15:55 +0000, Chuck Lever III wrote:
>
> > On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi <[email protected]> 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.
--
Jeff Layton <[email protected]>
> On Jul 10, 2023, at 12:36 PM, Jeff Layton <[email protected]> wrote:
>
> On Mon, 2023-07-10 at 15:55 +0000, Chuck Lever III wrote:
>>
>>> On Jul 10, 2023, at 11:33 AM, Lorenzo Bianconi <[email protected]> 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