On Mon, 2023-11-13 at 07:22 +1100, NeilBrown wrote: > 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@xxxxxxxxxx> > > > > > 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. > > Mostly my justification is that that occasionally we do add new settings for nfsd, and having a single extensible command may make that simpler to deal with. > > > > > > > > > > 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. > > That is a good point. That said, we wouldn't necessarily need to require restarting the server on a reconfigure. Some settings could be changed without a server restart. In any case, I'll withdraw my objection and we can do this with multiple commands for now. We can always add a single command later, and just fix up rpc.nfsd to hide all of the details of the different interfaces at that time if we think it's helpful. Cheers, -- Jeff Layton <jlayton@xxxxxxxxxx>
