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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
