Message-ID: <20240913161000.33ogsvxbe3njghhw@pali>
Date: Fri, 13 Sep 2024 18:10:00 +0200
From: Pali Rohár <pali@...nel.org>
To: Chuck Lever <chuck.lever@...cle.com>
Cc: Jeff Layton <jlayton@...nel.org>, Neil Brown <neilb@...e.de>,
	Olga Kornievskaia <okorniev@...hat.com>,
	Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nfsd: Fill NFSv4.1 server implementation fields in
 OP_EXCHANGE_ID response

On Friday 13 September 2024 12:07:36 Chuck Lever wrote:
> On Fri, Sep 13, 2024 at 05:36:31PM +0200, Pali Rohár wrote:
> > On Friday 13 September 2024 11:19:25 Chuck Lever wrote:
> > > On Fri, Sep 13, 2024 at 12:09:19AM +0200, Pali Rohár wrote:
> > > > NFSv4.1 OP_EXCHANGE_ID response from server may contain server
> > > > implementation details (domain, name and build time) in optional
> > > > nfs_impl_id4 field. Currently nfsd does not fill this field.
> > > > 
> > > > NFSv4.1 OP_EXCHANGE_ID call request from client may contain client
> > > > implementation details and Linux NFSv4.1 client is already filling these
> > > > information based on runtime module param "nfs.send_implementation_id" and
> > > > build time Kconfig option "NFS_V4_1_IMPLEMENTATION_ID_DOMAIN". Module param
> > > > send_implementation_id specify whether to fill implementation fields and
> > > > Kconfig option "NFS_V4_1_IMPLEMENTATION_ID_DOMAIN" specify the domain
> > > > string.
> > > > 
> > > > Do same in nfsd, introduce new runtime param "nfsd.send_implementation_id"
> > > > and build time Kconfig option "NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN" and
> > > > based on them fill NFSv4.1 server implementation details in OP_EXCHANGE_ID
> > > > response. Logic in nfsd is exactly same as in nfs.
> > > > 
> > > > This aligns Linux NFSv4.1 server logic with Linux NFSv4.1 client logic.
> > > > 
> > > > NFSv4.1 client and server implementation fields are useful for statistic
> > > > purposes or for identifying type of clients and servers.
> > > 
> > > NFSD has gotten along for more than a decade without returning this
> > > information. The patch description should explain the use case in a
> > > little more detail, IMO.
> > > 
> > > As a general comment, I recognize that you copied the client code
> > > for EXCHANGE_ID to construct this patch. The client and server code
> > > bases are somewhat different and have different coding conventions.
> > > Most of the comments below have to do with those differences.
> > 
> > Ok, this can be adjusted/aligned.
> > 
> > > 
> > > > Signed-off-by: Pali Rohár <pali@...nel.org>
> > > > ---
> > > >  fs/nfsd/Kconfig   | 12 +++++++++++
> > > >  fs/nfsd/nfs4xdr.c | 55 +++++++++++++++++++++++++++++++++++++++++++++--
> > > >  2 files changed, 65 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> > > > index ec2ab6429e00..70067c29316e 100644
> > > > --- a/fs/nfsd/Kconfig
> > > > +++ b/fs/nfsd/Kconfig
> > > > @@ -136,6 +136,18 @@ config NFSD_FLEXFILELAYOUT
> > > >  
> > > >  	  If unsure, say N.
> > > >  
> > > > +config NFSD_V4_1_IMPLEMENTATION_ID_DOMAIN
> > > > +	string "NFSv4.1 Implementation ID Domain"
> > > > +	depends on NFSD_V4
> > > > +	default "kernel.org"
> > > > +	help
> > > > +	  This option defines the domain portion of the implementation ID that
> > > > +	  may be sent in the NFS exchange_id operation.  The value must be in
> > > 
> > > Nit: "that the server returns in its NFSv4 EXCHANGE_ID response."
> > > 
> > > 
> > > > +	  the format of a DNS domain name and should be set to the DNS domain
> > > > +	  name of the distribution.
> > > 
> > > Perhaps add: "See the description of the nii_domain field in Section
> > > 3.3.21 of RFC 8881 for details."
> > 
> > Ok.
> > 
> > > But honestly, I'm not sure why nii_domain is parametrized at all, on
> > > the client. Why not /always/ return "kernel.org" ?
> > 
> > I do not know. I just followed logic of client. In my opinion, it does
> > not make sense to have different logic in client and server. If it is
> > not needed, maybe remove it from client too?
> 
> > > What checking should be done to ensure that the value of this
> > > setting is a valid DNS label?
> > 
> > Checking for valid DNS label is not easy. Client does not do it, so is
> > it needed?
> 
> Input checking is always a good thing to do. But I haven't found a
> compliance mandate in RFC 8881 for the content of nii_domain, so
> maybe it doesn't matter.
> 
> One possibility would be to not add the parametrization of this
> string on the server unless it is found to be needed. So, this
> patch could simply always set "kernel.org", and then a Kconfig
> option can be added by a subsequent patch if/when a use case ever
> turns up.

No problem, I can drop it.

> Or... NFSD could simply re-use the client's setting. I can't think
> of a reason why the NFS client and NFS server in the same kernel
> should report different nii_domain strings.
> 
> 
> > > > +	  If the NFS server is unchanged from the upstream kernel, this
> > > > +	  option should be set to the default "kernel.org".
> > > > +
> > > >  config NFSD_V4_2_INTER_SSC
> > > >  	bool "NFSv4.2 inter server to server COPY"
> > > >  	depends on NFSD_V4 && NFS_V4_2
> > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > > index b45ea5757652..5e89f999d4c7 100644
> > > > --- a/fs/nfsd/nfs4xdr.c
> > > > +++ b/fs/nfsd/nfs4xdr.c
> > > > @@ -62,6 +62,9 @@
> > > >  #include <linux/security.h>
> > > >  #endif
> > > >  
> > > > +static bool send_implementation_id = true;
> > > > +module_param(send_implementation_id, bool, 0644);
> > > > +MODULE_PARM_DESC(send_implementation_id, "Send implementation ID with NFSv4.1 exchange_id");
> > > 
> > > I'd rather not add a module parameter if we don't have to. Can you
> > > explain why this new parameter is necessary? For instance, is there
> > > a reason why an administrator who runs NFSD on a stock distro kernel
> > > would want to change this setting to "false" ?
> > 
> > I really do not know. Client has this parameter, so I though it is a
> > good idea to have it.
> > 
> > > If it turns out that the parameter is valuable, is there admin
> > > documentation to go with it?
> > 
> > I'm not sure if client have documentation for it.
> 
> Again, if we don't have a clear use case in front of us, it is
> sensible to postpone the addition of this parameter.
> 
> 
> [ ... snip ... ]
> 
> > > Regarding the content of these fields: I don't mind filling in
> > > nii_date, duplicating what might appear in the nii_name field, if
> > > that is not a bother.
> > 
> > I looked at this, and getting timestamp in numeric form is not possible.
> > Kernel utsname() and UTS functions provides date only in `date` format
> > which is unsuitable for parsing in kernel and converting into seconds
> > since epoch. Moreover uts structures are exported to userspace, so
> > changing and providing numeric value would be harder.
> 
> Not a big deal. And, it's something that can be changed later if
> someone finds a clean way to extract a numeric build time.

Ok.

Powered by blists - more mailing lists