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

[Jeff Layton <jlayton@kernel.org>]

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>