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

[Chuck Lever <chuck.lever@oracle.com>]

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