[PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Lorenzo Bianconi]

Introduce version field to nfsd_rpc_status handler in order to help
the user to maintain backward compatibility.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 fs/nfsd/nfssvc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 33ad91dd3a2d..6d5feeeb09a7 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -1117,6 +1117,9 @@ int nfsd_stats_release(struct inode *inode, struct file *file)
 	return ret;
 }
 
+/* Increment NFSD_RPC_STATUS_VERSION adding new info to the handler */
+#define NFSD_RPC_STATUS_VERSION		1
+
 static int nfsd_rpc_status_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = file_inode(m->file);
@@ -1125,6 +1128,8 @@ static int nfsd_rpc_status_show(struct seq_file *m, void *v)
 
 	rcu_read_lock();
 
+	seq_printf(m, "# version %u\n", NFSD_RPC_STATUS_VERSION);
+
 	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
 		struct svc_rqst *rqstp;
 
-- 
2.41.0

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[NeilBrown]

On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> Introduce version field to nfsd_rpc_status handler in order to help
> the user to maintain backward compatibility.

I wonder if this really helps.  What do I do if I see a version that I
don't understand?  Ignore the whole file?  That doesn't make for a good
user experience.

I would suggest that the first step to promoting compatibility is to
document the format, including how you expect to extend it.  Jeff's
suggestion of a header line with field names makes a lot of sense for a
file with space-separated fields like this.  You should probably promise
not to remove fields, but to deprecate fields by replacing them with "X"
or whatever.

A tool really needs to be able to extract anything it can understand,
and know how to avoid what it doesn't understand.  A version number
doesn't help with that.

And if you really wanted to change the format so much that old tools
cannot use any of the content, it would likely make most sense to change
the name of the file...  or have two files - legacy file with old name
and new-improved file with new name.

So I'm not keen on a version number.

Thanks,
NeilBrown


> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  fs/nfsd/nfssvc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 33ad91dd3a2d..6d5feeeb09a7 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -1117,6 +1117,9 @@ int nfsd_stats_release(struct inode *inode, struct file *file)
>  	return ret;
>  }
>  
> +/* Increment NFSD_RPC_STATUS_VERSION adding new info to the handler */
> +#define NFSD_RPC_STATUS_VERSION		1
> +
>  static int nfsd_rpc_status_show(struct seq_file *m, void *v)
>  {
>  	struct inode *inode = file_inode(m->file);
> @@ -1125,6 +1128,8 @@ static int nfsd_rpc_status_show(struct seq_file *m, void *v)
>  
>  	rcu_read_lock();
>  
> +	seq_printf(m, "# version %u\n", NFSD_RPC_STATUS_VERSION);
> +
>  	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
>  		struct svc_rqst *rqstp;
>  
> -- 
> 2.41.0
> 
> 

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Jeff Layton]

On Tue, 2023-08-08 at 10:21 +0200, Lorenzo Bianconi wrote:
> Introduce version field to nfsd_rpc_status handler in order to help
> the user to maintain backward compatibility.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  fs/nfsd/nfssvc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 33ad91dd3a2d..6d5feeeb09a7 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -1117,6 +1117,9 @@ int nfsd_stats_release(struct inode *inode, struct file *file)
>  	return ret;
>  }
>  
> +/* Increment NFSD_RPC_STATUS_VERSION adding new info to the handler */
> +#define NFSD_RPC_STATUS_VERSION		1
> +
>  static int nfsd_rpc_status_show(struct seq_file *m, void *v)
>  {
>  	struct inode *inode = file_inode(m->file);
> @@ -1125,6 +1128,8 @@ static int nfsd_rpc_status_show(struct seq_file *m, void *v)
>  
>  	rcu_read_lock();
>  
> +	seq_printf(m, "# version %u\n", NFSD_RPC_STATUS_VERSION);
> +
>  	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
>  		struct svc_rqst *rqstp;
>  

Should we also add a line that starts with '#' to show what the fields
indicate? Either way, this is fine:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Chuck Lever]

On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > Introduce version field to nfsd_rpc_status handler in order to help
> > the user to maintain backward compatibility.
> 
> I wonder if this really helps.  What do I do if I see a version that I
> don't understand?  Ignore the whole file?  That doesn't make for a good
> user experience.

There is no UX consideration here. A user browsing the file directly
will not care about the version.

This file is intended to be parsable by scripts and they have to
keep up with the occasional changes in format. Scripts can handle an
unrecogized version however they like.

This is what we typically get with a made-up format that isn't .ini
or JSON or XML. The file format isn't self-documenting. The final
field on each row is a variable number of tokens, so it will be
nearly impossible to simply add another field without breaking
something.


> I would suggest that the first step to promoting compatibility is to
> document the format, including how you expect to extend it.

I'd be OK with seeing that documentation added as a kdoc comment for
nfsd_rpc_status_show(), sure.


> Jeff's
> suggestion of a header line with field names makes a lot of sense for a
> file with space-separated fields like this.  You should probably promise
> not to remove fields, but to deprecate fields by replacing them with "X"
> or whatever.
> 
> A tool really needs to be able to extract anything it can understand,
> and know how to avoid what it doesn't understand.  A version number
> doesn't help with that.

It's how mountstats format changes are managed. We have bumped that
version number over the years, so there is precedent for it.


> And if you really wanted to change the format so much that old tools
> cannot use any of the content, it would likely make most sense to change
> the name of the file...  or have two files - legacy file with old name
> and new-improved file with new name.
> 
> So I'm not keen on a version number.

I'm a little surprised to get push-back on "# version" but OK, we
can drop that idea in favor of a comment line in rpc_status that
acts as a header row, just like in /proc/fs/nfsd/pool_stats.
Scripts can treat that header as format version information.


> Thanks,
> NeilBrown
> 
> 
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  fs/nfsd/nfssvc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 33ad91dd3a2d..6d5feeeb09a7 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -1117,6 +1117,9 @@ int nfsd_stats_release(struct inode *inode, struct file *file)
> >  	return ret;
> >  }
> >  
> > +/* Increment NFSD_RPC_STATUS_VERSION adding new info to the handler */
> > +#define NFSD_RPC_STATUS_VERSION		1
> > +
> >  static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> >  {
> >  	struct inode *inode = file_inode(m->file);
> > @@ -1125,6 +1128,8 @@ static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> >  
> >  	rcu_read_lock();
> >  
> > +	seq_printf(m, "# version %u\n", NFSD_RPC_STATUS_VERSION);
> > +
> >  	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
> >  		struct svc_rqst *rqstp;
> >  
> > -- 
> > 2.41.0
> > 
> > 
> 

-- 
Chuck Lever

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Jeff Layton]

On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > Introduce version field to nfsd_rpc_status handler in order to help
> > > the user to maintain backward compatibility.
> > 
> > I wonder if this really helps.  What do I do if I see a version that I
> > don't understand?  Ignore the whole file?  That doesn't make for a good
> > user experience.
> 
> There is no UX consideration here. A user browsing the file directly
> will not care about the version.
> 
> This file is intended to be parsable by scripts and they have to
> keep up with the occasional changes in format. Scripts can handle an
> unrecogized version however they like.
> 
> This is what we typically get with a made-up format that isn't .ini
> or JSON or XML. The file format isn't self-documenting. The final
> field on each row is a variable number of tokens, so it will be
> nearly impossible to simply add another field without breaking
> something.
> 

It shouldn't be a variable number of tokens per line. If there is, then
that's a bug, IMO. We do want it to be simple to just add a new field,
published version info notwithstanding.

> 
> > I would suggest that the first step to promoting compatibility is to
> > document the format, including how you expect to extend it.
> 
> I'd be OK with seeing that documentation added as a kdoc comment for
> nfsd_rpc_status_show(), sure.
> 
> 
> > Jeff's
> > suggestion of a header line with field names makes a lot of sense for a
> > file with space-separated fields like this.  You should probably promise
> > not to remove fields, but to deprecate fields by replacing them with "X"
> > or whatever.
> > 
> > A tool really needs to be able to extract anything it can understand,
> > and know how to avoid what it doesn't understand.  A version number
> > doesn't help with that.
> 
> It's how mountstats format changes are managed. We have bumped that
> version number over the years, so there is precedent for it.
> 
> 
> > And if you really wanted to change the format so much that old tools
> > cannot use any of the content, it would likely make most sense to change
> > the name of the file...  or have two files - legacy file with old name
> > and new-improved file with new name.
> > 
> > So I'm not keen on a version number.
> 
> I'm a little surprised to get push-back on "# version" but OK, we
> can drop that idea in favor of a comment line in rpc_status that
> acts as a header row, just like in /proc/fs/nfsd/pool_stats.
> Scripts can treat that header as format version information.
> 
> 
> > Thanks,
> > NeilBrown
> > 
> > 
> > > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  fs/nfsd/nfssvc.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > > index 33ad91dd3a2d..6d5feeeb09a7 100644
> > > --- a/fs/nfsd/nfssvc.c
> > > +++ b/fs/nfsd/nfssvc.c
> > > @@ -1117,6 +1117,9 @@ int nfsd_stats_release(struct inode *inode, struct file *file)
> > >  	return ret;
> > >  }
> > >  
> > > +/* Increment NFSD_RPC_STATUS_VERSION adding new info to the handler */
> > > +#define NFSD_RPC_STATUS_VERSION		1
> > > +
> > >  static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> > >  {
> > >  	struct inode *inode = file_inode(m->file);
> > > @@ -1125,6 +1128,8 @@ static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> > >  
> > >  	rcu_read_lock();
> > >  
> > > +	seq_printf(m, "# version %u\n", NFSD_RPC_STATUS_VERSION);
> > > +
> > >  	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
> > >  		struct svc_rqst *rqstp;
> > >  
> > > -- 
> > > 2.41.0
> > > 
> > > 
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Chuck Lever]

On Tue, Aug 08, 2023 at 09:48:42AM -0400, Jeff Layton wrote:
> On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> > On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > > Introduce version field to nfsd_rpc_status handler in order to help
> > > > the user to maintain backward compatibility.
> > > 
> > > I wonder if this really helps.  What do I do if I see a version that I
> > > don't understand?  Ignore the whole file?  That doesn't make for a good
> > > user experience.
> > 
> > There is no UX consideration here. A user browsing the file directly
> > will not care about the version.
> > 
> > This file is intended to be parsable by scripts and they have to
> > keep up with the occasional changes in format. Scripts can handle an
> > unrecogized version however they like.
> > 
> > This is what we typically get with a made-up format that isn't .ini
> > or JSON or XML. The file format isn't self-documenting. The final
> > field on each row is a variable number of tokens, so it will be
> > nearly impossible to simply add another field without breaking
> > something.
> > 
> 
> It shouldn't be a variable number of tokens per line.

That's how NFSv4 COMPOUND operations are displayed. For example:

0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ

The list of operations in the displayed compound are currently
blank-separated tokens at the end of each row.


> If there is, then that's a bug, IMO. We do want it to be simple to
> just add a new field, published version info notwithstanding.

They could be wrapped in curly braces, or separated by commas, to
make them all one token.

I haven't looked at NFSv3 output yet, but I expect those extra
tokens won't even be there in that case.

JSON, yaml, or xml would all address the extensibility problem, just
as an alternative thought.


-- 
Chuck Lever

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Jeff Layton]

On Tue, 2023-08-08 at 10:03 -0400, Chuck Lever wrote:
> On Tue, Aug 08, 2023 at 09:48:42AM -0400, Jeff Layton wrote:
> > On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> > > On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > > > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > > > Introduce version field to nfsd_rpc_status handler in order to help
> > > > > the user to maintain backward compatibility.
> > > > 
> > > > I wonder if this really helps.  What do I do if I see a version that I
> > > > don't understand?  Ignore the whole file?  That doesn't make for a good
> > > > user experience.
> > > 
> > > There is no UX consideration here. A user browsing the file directly
> > > will not care about the version.
> > > 
> > > This file is intended to be parsable by scripts and they have to
> > > keep up with the occasional changes in format. Scripts can handle an
> > > unrecogized version however they like.
> > > 
> > > This is what we typically get with a made-up format that isn't .ini
> > > or JSON or XML. The file format isn't self-documenting. The final
> > > field on each row is a variable number of tokens, so it will be
> > > nearly impossible to simply add another field without breaking
> > > something.
> > > 
> > 
> > It shouldn't be a variable number of tokens per line.
> 
> That's how NFSv4 COMPOUND operations are displayed. For example:
> 
> 0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ
> 
> The list of operations in the displayed compound are currently
> blank-separated tokens at the end of each row.
> 

Oh! That's a bug in missed in my latest review then. The operations
field was delimited by ':' chars at one point. Lorenzo, did you mean to
change that?

IMO, the list of operations should be one field, separated by a distinct
delimiter (like ':').

> 
> > If there is, then that's a bug, IMO. We do want it to be simple to
> > just add a new field, published version info notwithstanding.
> 
> They could be wrapped in curly braces, or separated by commas, to
> make them all one token.
> 
> I haven't looked at NFSv3 output yet, but I expect those extra
> tokens won't even be there in that case.
> 

That's probably another bug. Anything not a v4 COMPOUND should have
something as a placeholder. It could just be a single '-' character.

> JSON, yaml, or xml would all address the extensibility problem, just
> as an alternative thought.
> 

It would probably be fairly simple to output well-formed yaml instead.
JSON and XML are a bit more of a pain.

For now, we can change the output. We do need to have this settled
before this goes to Linus' tree though.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Chuck Lever]

On Tue, Aug 08, 2023 at 10:20:44AM -0400, Jeff Layton wrote:
> On Tue, 2023-08-08 at 10:03 -0400, Chuck Lever wrote:
> > On Tue, Aug 08, 2023 at 09:48:42AM -0400, Jeff Layton wrote:
> > > On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> > > > On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > > > > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > > > > Introduce version field to nfsd_rpc_status handler in order to help
> > > > > > the user to maintain backward compatibility.
> > > > > 
> > > > > I wonder if this really helps.  What do I do if I see a version that I
> > > > > don't understand?  Ignore the whole file?  That doesn't make for a good
> > > > > user experience.
> > > > 
> > > > There is no UX consideration here. A user browsing the file directly
> > > > will not care about the version.
> > > > 
> > > > This file is intended to be parsable by scripts and they have to
> > > > keep up with the occasional changes in format. Scripts can handle an
> > > > unrecogized version however they like.
> > > > 
> > > > This is what we typically get with a made-up format that isn't .ini
> > > > or JSON or XML. The file format isn't self-documenting. The final
> > > > field on each row is a variable number of tokens, so it will be
> > > > nearly impossible to simply add another field without breaking
> > > > something.
> > > > 
> > > 
> > > It shouldn't be a variable number of tokens per line.
> > 
> > That's how NFSv4 COMPOUND operations are displayed. For example:
> > 
> > 0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ
> > 
> > The list of operations in the displayed compound are currently
> > blank-separated tokens at the end of each row.
> > 
> 
> Oh! That's a bug in missed in my latest review then. The operations
> field was delimited by ':' chars at one point. Lorenzo, did you mean to
> change that?
> 
> IMO, the list of operations should be one field, separated by a distinct
> delimiter (like ':').
> 
> > 
> > > If there is, then that's a bug, IMO. We do want it to be simple to
> > > just add a new field, published version info notwithstanding.
> > 
> > They could be wrapped in curly braces, or separated by commas, to
> > make them all one token.
> > 
> > I haven't looked at NFSv3 output yet, but I expect those extra
> > tokens won't even be there in that case.
> > 
> 
> That's probably another bug. Anything not a v4 COMPOUND should have
> something as a placeholder. It could just be a single '-' character.
> 
> > JSON, yaml, or xml would all address the extensibility problem, just
> > as an alternative thought.
> > 
> 
> It would probably be fairly simple to output well-formed yaml instead.
> JSON and XML are a bit more of a pain.

If folks don't mind, I would like more structured output like one of
these self-documenting formats. (I know I said I didn't care before,
but I'm beginning to care now ;-)

I'm also wondering if we really ought not add another file under
/proc, which is essentially obsolete. Would /sys/fs/nfsd/yada be
better for this facility?

I hesitate to even mention network namespaces...


> For now, we can change the output. We do need to have this settled
> before this goes to Linus' tree though.

Agreed. As fair warning, I might drop this from v6.6 if we need more
time to get it right. That doesn't mean I'm not excited about having
this facility available for all our users.


-- 
Chuck Lever

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Chuck Lever]

On Tue, Aug 08, 2023 at 10:20:44AM -0400, Jeff Layton wrote:
> On Tue, 2023-08-08 at 10:03 -0400, Chuck Lever wrote:
> > On Tue, Aug 08, 2023 at 09:48:42AM -0400, Jeff Layton wrote:
> > > On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> > > > On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > > > > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > > > > Introduce version field to nfsd_rpc_status handler in order to help
> > > > > > the user to maintain backward compatibility.
> > > > > 
> > > > > I wonder if this really helps.  What do I do if I see a version that I
> > > > > don't understand?  Ignore the whole file?  That doesn't make for a good
> > > > > user experience.
> > > > 
> > > > There is no UX consideration here. A user browsing the file directly
> > > > will not care about the version.
> > > > 
> > > > This file is intended to be parsable by scripts and they have to
> > > > keep up with the occasional changes in format. Scripts can handle an
> > > > unrecogized version however they like.
> > > > 
> > > > This is what we typically get with a made-up format that isn't .ini
> > > > or JSON or XML. The file format isn't self-documenting. The final
> > > > field on each row is a variable number of tokens, so it will be
> > > > nearly impossible to simply add another field without breaking
> > > > something.
> > > > 
> > > 
> > > It shouldn't be a variable number of tokens per line.
> > 
> > That's how NFSv4 COMPOUND operations are displayed. For example:
> > 
> > 0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ
> > 
> > The list of operations in the displayed compound are currently
> > blank-separated tokens at the end of each row.
> > 
> 
> Oh! That's a bug in missed in my latest review then. The operations
> field was delimited by ':' chars at one point. Lorenzo, did you mean to
> change that?
> 
> IMO, the list of operations should be one field, separated by a distinct
> delimiter (like ':').
> 
> > 
> > > If there is, then that's a bug, IMO. We do want it to be simple to
> > > just add a new field, published version info notwithstanding.
> > 
> > They could be wrapped in curly braces, or separated by commas, to
> > make them all one token.
> > 
> > I haven't looked at NFSv3 output yet, but I expect those extra
> > tokens won't even be there in that case.
> > 
> 
> That's probably another bug. Anything not a v4 COMPOUND should have
> something as a placeholder. It could just be a single '-' character.

Confirmed, rows reporting NFSv3 procedures have nothing on the end.

I'll also note that rq_prog and the "NFSv" string are problematic.
Is it the case that all RPCs handled in this thread pool are going
to be NFS requests?

If we expect non-NFS requests to be handled in this thread pool
(like svc_wake_up or NFSACL) then the loop should simply skip
threads whose rq_prog != NFS_PROGRAM.

And, if the rpc_status file is supposed to display only NFS
requests (and I believe the answer to that is yes), then let's drop
the rq_prog field, since it will always show the same value.


> > JSON, yaml, or xml would all address the extensibility problem, just
> > as an alternative thought.
> > 
> 
> It would probably be fairly simple to output well-formed yaml instead.
> JSON and XML are a bit more of a pain.
> 
> For now, we can change the output. We do need to have this settled
> before this goes to Linus' tree though.

Lorenzo, I'll drop the v5 of this series from nfsd-next. When you're
ready, please send another version with the discussed changes
squashed in.


-- 
Chuck Lever

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[NeilBrown]

On Tue, 08 Aug 2023, Chuck Lever wrote:
> On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > Introduce version field to nfsd_rpc_status handler in order to help
> > > the user to maintain backward compatibility.
> > 
> > I wonder if this really helps.  What do I do if I see a version that I
> > don't understand?  Ignore the whole file?  That doesn't make for a good
> > user experience.
> 
> There is no UX consideration here. A user browsing the file directly
> will not care about the version.

Surely a user would care about whether the script works...

> 
> This file is intended to be parsable by scripts and they have to
> keep up with the occasional changes in format. Scripts can handle an
> unrecogized version however they like.

I think it is naive to introduce versioning and not describe what it
means.  Scripts need to handle version numbers the way the author of the
version intends. That is how protocols work.

version numbers are great when the request can ask for a version or
version range and the responder can provide any of a number of versions.
This is how NFS versions work.  So embedding the version number in the
file name would be fine - it would allow negotiation.

> 
> This is what we typically get with a made-up format that isn't .ini
> or JSON or XML. The file format isn't self-documenting. The final
> field on each row is a variable number of tokens, so it will be
> nearly impossible to simply add another field without breaking
> something.

There is an existing pattern in line/field files to terminate a variable
length array with something like "-".  /proc/self/mountinfo does this.

I'm not against yaml though

> 
> 
> > I would suggest that the first step to promoting compatibility is to
> > document the format, including how you expect to extend it.
> 
> I'd be OK with seeing that documentation added as a kdoc comment for
> nfsd_rpc_status_show(), sure.
> 
> 
> > Jeff's
> > suggestion of a header line with field names makes a lot of sense for a
> > file with space-separated fields like this.  You should probably promise
> > not to remove fields, but to deprecate fields by replacing them with "X"
> > or whatever.
> > 
> > A tool really needs to be able to extract anything it can understand,
> > and know how to avoid what it doesn't understand.  A version number
> > doesn't help with that.
> 
> It's how mountstats format changes are managed. We have bumped that
> version number over the years, so there is precedent for it.
> 

NFS_IOSTATS_VERS has changed once, from 1.0 to 1.1.
This was in 2012 for a patch which says:
  Add the new fields at the end of the mountstats xprt stanza so that
  mountstats outputs the previous correct values and ignores the new
  fields.

so the new format was deliberately backward compatible and the version
change wasn't really needed.

Thanks,
NeilBrown


> 
> > And if you really wanted to change the format so much that old tools
> > cannot use any of the content, it would likely make most sense to change
> > the name of the file...  or have two files - legacy file with old name
> > and new-improved file with new name.
> > 
> > So I'm not keen on a version number.
> 
> I'm a little surprised to get push-back on "# version" but OK, we
> can drop that idea in favor of a comment line in rpc_status that
> acts as a header row, just like in /proc/fs/nfsd/pool_stats.
> Scripts can treat that header as format version information.
> 
> 
> > Thanks,
> > NeilBrown
> > 
> > 
> > > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  fs/nfsd/nfssvc.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > > index 33ad91dd3a2d..6d5feeeb09a7 100644
> > > --- a/fs/nfsd/nfssvc.c
> > > +++ b/fs/nfsd/nfssvc.c
> > > @@ -1117,6 +1117,9 @@ int nfsd_stats_release(struct inode *inode, struct file *file)
> > >  	return ret;
> > >  }
> > >  
> > > +/* Increment NFSD_RPC_STATUS_VERSION adding new info to the handler */
> > > +#define NFSD_RPC_STATUS_VERSION		1
> > > +
> > >  static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> > >  {
> > >  	struct inode *inode = file_inode(m->file);
> > > @@ -1125,6 +1128,8 @@ static int nfsd_rpc_status_show(struct seq_file *m, void *v)
> > >  
> > >  	rcu_read_lock();
> > >  
> > > +	seq_printf(m, "# version %u\n", NFSD_RPC_STATUS_VERSION);
> > > +
> > >  	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
> > >  		struct svc_rqst *rqstp;
> > >  
> > > -- 
> > > 2.41.0
> > > 
> > > 
> > 
> 
> -- 
> Chuck Lever
> 

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[NeilBrown]

On Wed, 09 Aug 2023, Chuck Lever wrote:
> On Tue, Aug 08, 2023 at 10:20:44AM -0400, Jeff Layton wrote:
> > 
> > It would probably be fairly simple to output well-formed yaml instead.
> > JSON and XML are a bit more of a pain.
> 
> If folks don't mind, I would like more structured output like one of
> these self-documenting formats. (I know I said I didn't care before,
> but I'm beginning to care now ;-)

Lustre, which I am somewhat involved with, uses YAML for various things.
If someone else introduced yaml-producing sysfs files to the kernel
first, that might make the path for lustre smoother :-)

Another option is netlink which lustre is stating to use for
configuration and stats.  It is a self-describing format.  The code
looks verbose, but it is widely used in the kernel and so well supported.

> 
> I'm also wondering if we really ought not add another file under
> /proc, which is essentially obsolete. Would /sys/fs/nfsd/yada be
> better for this facility?

It is only under /proc because that is where it is mounted by default :-)
I think it might be sensible to create a node under /sys where all the
content of the nfsd filesystem also appears.
I'm not keen on /sys/fs/nfsd because nfsd isn't a filesystem, it is a
service.
But the /sys/fs seems to be largely unstructured (/proc v2??) so maybe
putting nfsd there is OK.  We could claim that /proc/fs/nfsd is a
filesystem :-)

> 
> I hesitate to even mention network namespaces...

Please do mention them - I find them too easy to forget about.
/proc/fs/nfsd/ inherits the network namespace from whoever mounts it.
So this can work perfectly.
If we created a mirror in /sys/ we would presumably use the namespace of
the process that opens the file.

Thanks,
NeilBrown

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Chuck Lever]

On Wed, Aug 09, 2023 at 07:45:06AM +1000, NeilBrown wrote:
> On Wed, 09 Aug 2023, Chuck Lever wrote:
> > On Tue, Aug 08, 2023 at 10:20:44AM -0400, Jeff Layton wrote:
> > > 
> > > It would probably be fairly simple to output well-formed yaml instead.
> > > JSON and XML are a bit more of a pain.
> > 
> > If folks don't mind, I would like more structured output like one of
> > these self-documenting formats. (I know I said I didn't care before,
> > but I'm beginning to care now ;-)
> 
> Lustre, which I am somewhat involved with, uses YAML for various things.
> If someone else introduced yaml-producing sysfs files to the kernel
> first, that might make the path for lustre smoother :-)

It worries me that there isn't yet kernel infrastructure for
formating yaml in sysfs files. That broadens the scope of this
work significantly.


> Another option is netlink which lustre is stating to use for
> configuration and stats.  It is a self-describing format.  The code
> looks verbose, but it is widely used in the kernel and so well supported.

I just spent the last 6 months building a netlink upcall to handle
TLS handshake requests for in-kernel TLS consumers. It is built on
the recently-added yaml netlink specs and code generator. The yaml
netlink specs are kept under:

  Documentation/netlink/specs/

Using netlink would give us a lot of infrastructure for this
facility, but I'm not sure it's worth the extra complexity. And it
would /require/ the use of user space tooling (ie, not 'cat') to get
to the information exported from the kernel. <shrug>


> > I'm also wondering if we really ought not add another file under
> > /proc, which is essentially obsolete. Would /sys/fs/nfsd/yada be
> > better for this facility?
> 
> It is only under /proc because that is where it is mounted by default :-)
> I think it might be sensible to create a node under /sys where all the
> content of the nfsd filesystem also appears.

There are things in the nfsd filesystem that really belong under
/proc/net/rpc or elsewhere, so IMO such migration needs to be
handled on a case-by-case basis -- different project for another
time.


> I'm not keen on /sys/fs/nfsd because nfsd isn't a filesystem, it is a
> service.

How about /sys/module/nfsd ?


> > I hesitate to even mention network namespaces...
> 
> Please do mention them - I find them too easy to forget about.
> /proc/fs/nfsd/ inherits the network namespace from whoever mounts it.
> So this can work perfectly.
> If we created a mirror in /sys/ we would presumably use the namespace of
> the process that opens the file.

I agree: the network namespace of the process that opens the
rpc_status file is just what we want to limit access to in-flight
requests. The current network namespace of each thread is available
via SVC_NET(rqst), so it should be quite simple to display only
in-flight requests that match the opener's namespace.


-- 
Chuck Lever

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[NeilBrown]

On Wed, 09 Aug 2023, Chuck Lever wrote:
> On Wed, Aug 09, 2023 at 07:45:06AM +1000, NeilBrown wrote:
> > On Wed, 09 Aug 2023, Chuck Lever wrote:
> > > On Tue, Aug 08, 2023 at 10:20:44AM -0400, Jeff Layton wrote:
> > > > 
> > > > It would probably be fairly simple to output well-formed yaml instead.
> > > > JSON and XML are a bit more of a pain.
> > > 
> > > If folks don't mind, I would like more structured output like one of
> > > these self-documenting formats. (I know I said I didn't care before,
> > > but I'm beginning to care now ;-)
> > 
> > Lustre, which I am somewhat involved with, uses YAML for various things.
> > If someone else introduced yaml-producing sysfs files to the kernel
> > first, that might make the path for lustre smoother :-)
> 
> It worries me that there isn't yet kernel infrastructure for
> formating yaml in sysfs files. That broadens the scope of this
> work significantly.
> 
> 
> > Another option is netlink which lustre is stating to use for
> > configuration and stats.  It is a self-describing format.  The code
> > looks verbose, but it is widely used in the kernel and so well supported.
> 
> I just spent the last 6 months building a netlink upcall to handle
> TLS handshake requests for in-kernel TLS consumers. It is built on
> the recently-added yaml netlink specs and code generator. The yaml
> netlink specs are kept under:
> 
>   Documentation/netlink/specs/
> 
> Using netlink would give us a lot of infrastructure for this
> facility, but I'm not sure it's worth the extra complexity. And it
> would /require/ the use of user space tooling (ie, not 'cat') to get
> to the information exported from the kernel. <shrug>
> 

I do like the "cat" approach.  Unfortunately it doesn't scale and you
never really know when it needs to scale.
The nfsd/rpc cache.c auth cache is still a sore point for me.  It works
nicely expect that it breaks for gss because the keys get too big.  So
we've had a couple of attempts to "fix" that.  The fixes work, but they
are *different*.

The other well known pain point is /proc/mounts.  It is really cool that
you can "cat" that, but with thousands of mounts it can take tools like
systemd a long time to find changes.

does any of that matter for collecting stats?  Will we hit a wall?  I
really don't know.  I'd like to think that we won't....

> 
> > > I'm also wondering if we really ought not add another file under
> > > /proc, which is essentially obsolete. Would /sys/fs/nfsd/yada be
> > > better for this facility?
> > 
> > It is only under /proc because that is where it is mounted by default :-)
> > I think it might be sensible to create a node under /sys where all the
> > content of the nfsd filesystem also appears.
> 
> There are things in the nfsd filesystem that really belong under
> /proc/net/rpc or elsewhere, so IMO such migration needs to be
> handled on a case-by-case basis -- different project for another
> time.

abolutely.

> 
> 
> > I'm not keen on /sys/fs/nfsd because nfsd isn't a filesystem, it is a
> > service.
> 
> How about /sys/module/nfsd ?

Not worse than /sys/fs/nfsd.  Not really better though.
Everything in /sys/module/foo is about the module as a chunk of code -
except "parameters".  I guess we could add "state".

Maybe configfs is the thing ...  but I never liked configfs.  It seems
like a solution in search of a problem.

I complained that /sys/fs is like the provfs-v2.  It is more that
everything other than devices (and block,bus,class) is procfs-v2.
There are little bits of regularity, but no big-picture.

I would probably argue for /sys/services/sunrpc/{nfsd,lockd,nfsv4.1-cb}

Alternately, we could go for /sys/devices/virtual/sunrpc.  The virtual
directory contains "workqueue" which is a service in some sense, and
contains subdirectories for specific work-queues.
I'm not sure that all of the nfsd stuff would fit under there...

but maybe I'm over-thinking things.

Thanks,
NeilBrown


> 
> 
> > > I hesitate to even mention network namespaces...
> > 
> > Please do mention them - I find them too easy to forget about.
> > /proc/fs/nfsd/ inherits the network namespace from whoever mounts it.
> > So this can work perfectly.
> > If we created a mirror in /sys/ we would presumably use the namespace of
> > the process that opens the file.
> 
> I agree: the network namespace of the process that opens the
> rpc_status file is just what we want to limit access to in-flight
> requests. The current network namespace of each thread is available
> via SVC_NET(rqst), so it should be quite simple to display only
> in-flight requests that match the opener's namespace.
> 
> 
> -- 
> Chuck Lever
> 

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4556 bytes --]

> On Tue, Aug 08, 2023 at 10:20:44AM -0400, Jeff Layton wrote:
> > On Tue, 2023-08-08 at 10:03 -0400, Chuck Lever wrote:
> > > On Tue, Aug 08, 2023 at 09:48:42AM -0400, Jeff Layton wrote:
> > > > On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> > > > > On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > > > > > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > > > > > Introduce version field to nfsd_rpc_status handler in order to help
> > > > > > > the user to maintain backward compatibility.
> > > > > > 
> > > > > > I wonder if this really helps.  What do I do if I see a version that I
> > > > > > don't understand?  Ignore the whole file?  That doesn't make for a good
> > > > > > user experience.
> > > > > 
> > > > > There is no UX consideration here. A user browsing the file directly
> > > > > will not care about the version.
> > > > > 
> > > > > This file is intended to be parsable by scripts and they have to
> > > > > keep up with the occasional changes in format. Scripts can handle an
> > > > > unrecogized version however they like.
> > > > > 
> > > > > This is what we typically get with a made-up format that isn't .ini
> > > > > or JSON or XML. The file format isn't self-documenting. The final
> > > > > field on each row is a variable number of tokens, so it will be
> > > > > nearly impossible to simply add another field without breaking
> > > > > something.
> > > > > 
> > > > 
> > > > It shouldn't be a variable number of tokens per line.
> > > 
> > > That's how NFSv4 COMPOUND operations are displayed. For example:
> > > 
> > > 0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ
> > > 
> > > The list of operations in the displayed compound are currently
> > > blank-separated tokens at the end of each row.
> > > 
> > 
> > Oh! That's a bug in missed in my latest review then. The operations
> > field was delimited by ':' chars at one point. Lorenzo, did you mean to
> > change that?
> > 
> > IMO, the list of operations should be one field, separated by a distinct
> > delimiter (like ':').
> > 
> > > 
> > > > If there is, then that's a bug, IMO. We do want it to be simple to
> > > > just add a new field, published version info notwithstanding.
> > > 
> > > They could be wrapped in curly braces, or separated by commas, to
> > > make them all one token.
> > > 
> > > I haven't looked at NFSv3 output yet, but I expect those extra
> > > tokens won't even be there in that case.
> > > 
> > 
> > That's probably another bug. Anything not a v4 COMPOUND should have
> > something as a placeholder. It could just be a single '-' character.
> 
> Confirmed, rows reporting NFSv3 procedures have nothing on the end.
> 
> I'll also note that rq_prog and the "NFSv" string are problematic.
> Is it the case that all RPCs handled in this thread pool are going
> to be NFS requests?
> 
> If we expect non-NFS requests to be handled in this thread pool
> (like svc_wake_up or NFSACL) then the loop should simply skip
> threads whose rq_prog != NFS_PROGRAM.
> 
> And, if the rpc_status file is supposed to display only NFS
> requests (and I believe the answer to that is yes), then let's drop
> the rq_prog field, since it will always show the same value.

ack, I will fix it.

> 
> 
> > > JSON, yaml, or xml would all address the extensibility problem, just
> > > as an alternative thought.
> > > 
> > 
> > It would probably be fairly simple to output well-formed yaml instead.
> > JSON and XML are a bit more of a pain.
> > 
> > For now, we can change the output. We do need to have this settled
> > before this goes to Linus' tree though.
> 
> Lorenzo, I'll drop the v5 of this series from nfsd-next. When you're
> ready, please send another version with the discussed changes
> squashed in.

ack, fine to me. Just a couple of questions:
- do we want to expose the output in yaml or is it enough to fix the NFSv4 COMPOUND
  parsing using ":" as sub-delimiter (and add a placeholder for non NFSv4 COMPOUND)?
  The yaml approach downside is we will need to add some specific code since afaik
  there isn't any yaml code we can rely on in the kernel, right?
- what about netlink? I would say we can have both of them (cat + netlink) so
  the user does not need to have a specific userspace tool to decode the info.

I will work on v6 as soon as we have agreed on the points above.

Regards,
Lorenzo

> 
> 
> -- 
> Chuck Lever

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler

[Chuck Lever]

On Wed, Aug 09, 2023 at 09:49:55AM +0200, Lorenzo Bianconi wrote:
> > Lorenzo, I'll drop the v5 of this series from nfsd-next. When you're
> > ready, please send another version with the discussed changes
> > squashed in.
> 
> ack, fine to me. Just a couple of questions:
> - do we want to expose the output in yaml or is it enough to fix the NFSv4 COMPOUND
>   parsing using ":" as sub-delimiter (and add a placeholder for non NFSv4 COMPOUND)?
>   The yaml approach downside is we will need to add some specific code since afaik
>   there isn't any yaml code we can rely on in the kernel, right?

Would you mind spinning a series with the simple delimiter changes
and the other things we've discussed so far? It seems we have some
items that still need sorting before tackling netlink v. sysfs.


> - what about netlink? I would say we can have both of them (cat + netlink) so
>   the user does not need to have a specific userspace tool to decode the info.

The trend in network subsystems is to use netlink and a purpose-
built tool, no "cat" support. The trouble with "cat" is we can't
seem to decide on where to put the output file.

Also, I notice that rq_flags appears for each in-flight request in
raw hexadecimal form. That's not especially user-friendly and cries
out for a tool to interpret the bits in that field. (Actually IIRC
there is now a tool that can take a yaml-defined netlink protocol
and perform each of the protocol's operations and spit out the
raw results).

I'm wondering if having support for "cat" is really just an old
habit we need to discard.


Re: netlink... folks should keep in mind that the output would not
be yaml. netlink uses yaml to define the netlink protocol, which is
quite like SunRPC. This still meets our extensibility requirements,
IMO.

Creating a netlink protocol would provide a vehicle for exporting
other information. Once there is an NFSD-specific netlink protocol,
it's straightforward to define an additional netlink procedure for
any of the information that are now available in /proc/fs/nfsd/
files.


> I will work on v6 as soon as we have agreed on the points above.

Thanks, I appreciate it!


-- 
Chuck Lever