Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Lorenzo Bianconi" <lorenzo@kernel.org>
Cc:     linux-nfs@vger.kernel.org, lorenzo.bianconi@redhat.com,
        chuck.lever@oracle.com, jlayton@kernel.org
Subject: Re: [PATCH] NFSD: add version field to nfsd_rpc_status_show handler
In-reply-to: <6431d0ea2295a1e128f83cd76a419dee161e4c44.1691482815.git.lorenzo@kernel.org>
References: <6431d0ea2295a1e128f83cd76a419dee161e4c44.1691482815.git.lorenzo@kernel.org>
Date:   Tue, 08 Aug 2023 21:33:23 +1000
Message-id: <169149440399.32308.1010201101079709026@noble.neil.brown.name>
Precedence: bulk

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


>=20
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  fs/nfsd/nfssvc.c | 5 +++++
>  1 file changed, 5 insertions(+)
>=20
> 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 fi=
le *file)
>  	return ret;
>  }
> =20
> +/* 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 =3D file_inode(m->file);
> @@ -1125,6 +1128,8 @@ static int nfsd_rpc_status_show(struct seq_file *m, v=
oid *v)
> =20
>  	rcu_read_lock();
> =20
> +	seq_printf(m, "# version %u\n", NFSD_RPC_STATUS_VERSION);
> +
>  	for (i =3D 0; i < nn->nfsd_serv->sv_nrpools; i++) {
>  		struct svc_rqst *rqstp;
> =20
> --=20
> 2.41.0
>=20
>=20