| Message ID | cover.1699095665.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | convert write_threads, write_version and write_ports to netlink commands | expand |
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@kernel.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... 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. 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? That does presuppose we can send down a variable-length frame though, but I assume that is possible with netlink.
> 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@kernel.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? > > 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? > > 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. Regards, Lorenzo > -- > Jeff Layton <jlayton@kernel.org>
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@kernel.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.
> On Nov 12, 2023, at 6:09 AM, Jeff Layton <jlayton@kernel.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@kernel.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@kernel.org> -- Chuck Lever
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@kernel.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@kernel.org> >
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@kernel.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. > > 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@kernel.org>
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@kernel.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(-)