Message-ID: <0f0467f396777722022403727824104b4f0a8d85.camel@kernel.org>
Date: Sun, 12 Nov 2023 06:09:18 -0500
From: Jeff Layton <jlayton@...nel.org>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: linux-nfs@...r.kernel.org, lorenzo.bianconi@...hat.com, neilb@...e.de, 
	chuck.lever@...cle.com, netdev@...r.kernel.org, kuba@...nel.org
Subject: Re: [PATCH v4 0/3] convert write_threads, write_version and
 write_ports to netlink commands

On Sun, 2023-11-12 at 11:02 +0100, Lorenzo Bianconi wrote:
> > On Sat, 2023-11-04 at 12:13 +0100, Lorenzo Bianconi wrote:
> > > Introduce write_threads, write_version and write_ports netlink
> > > commands similar to the ones available through the procfs.
> > > 
> > > Changes since v3:
> > > - drop write_maxconn and write_maxblksize for the moment
> > > - add write_version and write_ports commands
> > > Changes since v2:
> > > - use u32 to store nthreads in nfsd_nl_threads_set_doit
> > > - rename server-attr in control-plane in nfsd.yaml specs
> > > Changes since v1:
> > > - remove write_v4_end_grace command
> > > - add write_maxblksize and write_maxconn netlink commands
> > > 
> > > This patch can be tested with user-space tool reported below:
> > > https://github.com/LorenzoBianconi/nfsd-netlink.git
> > > This series is based on the commit below available in net-next tree
> > > 
> > > commit e0fadcffdd172d3a762cb3d0e2e185b8198532d9
> > > Author: Jakub Kicinski <kuba@...nel.org>
> > > Date:   Fri Oct 6 06:50:32 2023 -0700
> > > 
> > >     tools: ynl-gen: handle do ops with no input attrs
> > > 
> > >     The code supports dumps with no input attributes currently
> > >     thru a combination of special-casing and luck.
> > >     Clean up the handling of ops with no inputs. Create empty
> > >     Structs, and skip printing of empty types.
> > >     This makes dos with no inputs work.
> > > 
> > > Lorenzo Bianconi (3):
> > >   NFSD: convert write_threads to netlink commands
> > >   NFSD: convert write_version to netlink commands
> > >   NFSD: convert write_ports to netlink commands
> > > 
> > >  Documentation/netlink/specs/nfsd.yaml |  83 ++++++++
> > >  fs/nfsd/netlink.c                     |  54 ++++++
> > >  fs/nfsd/netlink.h                     |   8 +
> > >  fs/nfsd/nfsctl.c                      | 267 +++++++++++++++++++++++++-
> > >  include/uapi/linux/nfsd_netlink.h     |  30 +++
> > >  tools/net/ynl/generated/nfsd-user.c   | 254 ++++++++++++++++++++++++
> > >  tools/net/ynl/generated/nfsd-user.h   | 156 +++++++++++++++
> > >  7 files changed, 845 insertions(+), 7 deletions(-)
> > > 
> > 
> > Nice work, Lorenzo! Now comes the bikeshedding...
> 
> Hi Jeff,
> 
> > 
> > With the nfsdfs interface, we sort of had to split things up into
> > multiple files like this, but it has some drawbacks, in particular with
> > weird behavior when people do things out of order.
> 
> what do you mean with 'weird behavior'? Something not expected?
> 

Yeah.

For instance, if you set up sockets but never write anything to the
"threads" file, those sockets will sit around in perpetuity. Granted
most people use rpc.nfsd to start the server, so this generally doesn't
happen often, but it's always been a klunky interface regardless.

> > 
> > Would it make more sense to instead have a single netlink command that
> > sets up ports and versions, and then spawns the requisite amount of
> > threads, all in one fell swoop?
> 
> I do not have a strong opinion about it but I would say having a dedicated
> set/get for each paramater allow us to have more granularity (e.g. if you want
> to change just a parameter we do not need to send all of them to the kernel).
> What do you think?
> 

It's pretty rare to need to twiddle settings on the server while it's up
and running. Restarting the server in the face of even minor changes is
not generally a huge problem, so I don't see this as necessary.

Also, it's always been a bit hit and miss as to which settings take
immediate effect. For instance, if I (e.g.) turn off NFSv4 serving
altogether on a running server, it doesn't purge the existing NFSv4
state, but v4 RPCs would be immediately rejected. Eventually it would
time out, but it is odd.

Personally, I think this is amenable to a declarative interface:

Have userland always send down a complete description of what the server
should look like, and then the kernel can do what it needs to make that
happen (starting/stopping threads, opening/closing sockets, changing
versions served, etc.).

> > 
> > That does presuppose we can send down a variable-length frame though,
> > but I assume that is possible with netlink.
> 
> sure, we can do it.
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists