Message-ID: <ZuRjSIyHguz3ult4@tissot.1015granger.net>
Date: Fri, 13 Sep 2024 12:07:36 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Pali Rohár <pali@...nel.org>
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 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.

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.


-- 
Chuck Lever

Powered by blists - more mailing lists