Message-id: <169982054606.2582.2797096885323571291@noble.neil.brown.name>
Date: Mon, 13 Nov 2023 07:22:26 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Jeff Layton" <jlayton@...nel.org>
Cc: "Lorenzo Bianconi" <lorenzo@...nel.org>, linux-nfs@...r.kernel.org,
 lorenzo.bianconi@...hat.com, 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, 12 Nov 2023, Jeff Layton wrote:
> 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.

If you set up sockets but *do* write something to the "threads" file,
then those sockets will *still* sit around in perpetuity.
i.e. until you shut down the NFS server (rpc.nfsd 0).

I don't really see the problem.

It is true that you can use use the interface to ask for meaningless
things.  The maxim that applies is "If you make it fool-proof, only a
fool will use it". :-)

I'm not against exploring changes to the interface style in conjunction
with moving from nfsd-fs to netlink, but I would want a bit more
justification for any change.

Thanks,
NeilBrown

> 
> > > 
> > > 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.

Restarting the server is not zero-cost.  It restarts the grace period.
So I would rather not require it for minor changes.

NeilBrown


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