netdev - Re: [PATCH v4 0/3] convert write_threads, write_version and write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BB9DF012-0E69-46CA-B7FF-6B963AA22C02@oracle.com>
Date: Sun, 12 Nov 2023 15:33:23 +0000
From: Chuck Lever III <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>
CC: Lorenzo Bianconi <lorenzo@...nel.org>,
        Linux NFS Mailing List
	<linux-nfs@...r.kernel.org>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Neil Brown <neilb@...e.de>,
        "open list:NETWORKING [GENERAL]"
	<netdev@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: Re: [PATCH v4 0/3] convert write_threads, write_version and
 write_ports to netlink commands



> On Nov 12, 2023, at 6:09 AM, Jeff Layton <jlayton@...nel.org> 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.
> 
>>> 
>>> 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.

I don't have a problem creating a single "set" netlink command
that takes a number of optional arguments to adjust nfsd's
configuration in a single operation.

And a matching "get" command to query all of the server settings
at one time.


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


--
Chuck Lever