On Tue, 06 Feb 2024, Chuck Lever III wrote: > > > > On Feb 5, 2024, at 2:44 PM, NeilBrown <neilb@xxxxxxx> wrote: > > > > On Tue, 06 Feb 2024, Lorenzo Bianconi wrote: > >>> On Fri, 2024-02-02 at 18:08 +0100, Lorenzo Bianconi wrote: > >>>>> The existing rpc.nfsd program was designed during a different time, when > >>>>> we just didn't require that much control over how it behaved. It's > >>>>> klunky to work with. > >>>>> > >>>>> In a response to Chuck's recent RFC patch to add knob to disable > >>>>> READ_PLUS calls, I mentioned that it might be a good time to make a > >>>>> clean break from the past and start a new program for controlling nfsd. > >>>>> > >>>>> Here's what I'm thinking: > >>>>> > >>>>> Let's build a swiss-army-knife kind of interface like git or virsh: > >>>>> > >>>>> # nfsdctl stats <--- fetch the new stats that got merged > >>>>> # nfsdctl add_listener <--- add a new listen socket, by address or hostname > >>>>> # nfsdctl set v3 on <--- enable NFSv3 > >>>>> # nfsdctl set splice_read off <--- disable splice reads (per Chuck's recent patch) > >>>>> # nfsdctl set threads 128 <--- spin up the threads > >>>>> > >>>>> We could start with just the bare minimum for now (the stats interface), > >>>>> and then expand on it. Once we're at feature parity with rpc.nfsd, we'd > >>>>> want to have systemd preferentially use nfsdctl instead of rpc.nfsd to > >>>>> start and stop the server. systemd will also need to fall back to using > >>>>> rpc.nfsd if nfsdctl or the netlink program isn't present. > >>>>> > >>>>> Note that I think this program will have to be a compiled binary vs. a > >>>>> python script or the like, given that it'll be involved in system > >>>>> startup. > >>>>> > >>>>> It turns out that Lorenzo already has a C program that has a lot of the > >>>>> plumbing we'd need: > >>>>> > >>>>> https://github.com/LorenzoBianconi/nfsd-netlink > >>>> > >>>> This is something I developed just for testing the new interface but I agree we > >>>> could start from it. > >>>> > >>>> Regarding the kernel part I addressed the comments I received upstream on v6 and > >>>> pushed the code here [0]. > >>>> How do you guys prefer to proceed? Is the better to post v7 upstream and continue > >>>> the discussion in order to have something usable to develop the user-space part or > >>>> do you prefer to have something for the user-space first? > >>>> I do not have a strong opinion on it. > >>>> > >>>> Regards, > >>>> Lorenzo > >>>> > >>>> [0] https://github.com/LorenzoBianconi/nfsd-next/tree/nfsd-next-netlink-new-cmds-public-v7 > >>>> > >>>> > >>> > >>> My advice? > >>> > >>> Step back and spend some time working on the userland bits before > >>> posting another revision. Experience has shown that you never realize > >>> what sort of warts an interface like this has until you have to work > >>> with it. > >>> > >>> You may find that you want to tweak it some once you do, and it's much > >>> easier to do that before we merge anything. This will be part of the > >>> kernel ABI, so once it's in a shipping kernel, we're sort of stuck with > >>> it. > >>> > >>> Having a userland program ready to go will allow us to do things like > >>> set up the systemd service for this too, which is primarily how this new > >>> program will be called. > >> > >> I agree on it. In order to proceed I guess we should define a list of > >> requirements/expected behaviour on this new user-space tool used to > >> configure nfsd. I am not so familiar with the user-space requirements > >> for nfsd so I am just copying what you suggested, something like: > >> > >> $ nfsdctl stats <--- fetch the new stats that got merged > >> $ nfsdctl xprt add proto <udp|tcp> host <host> [port <port>] <--- add a new listen socket, by address or hostname > >> $ nfsdctl proto v3.0 v4.0 v4.1 <--- enable NFSv3 and v4.1 > >> $ nfsdctl set threads 128 <--- spin up the threads > > > > My preference would be: > > > > nfsdctl start > > and > > nfsdctl stop > > > > where nfsdctl reads a config file to discover what setting are required. > > I cannot see any credible use case for 'xprt' or 'proto' or 'threads' > > commands. > > > > Possibly nfsctl would accept config on the command line: > > nfsdctl start proto=3,4.1 threads=42 proto=tcp:localhost:2049 > > or similar. > > You've got proto= listed twice here. Yep - the second was meant to be xprt= > > I'm more in favor of having more subcommands, each of which do > something simple. Easier to understand, easier to test. Fewer subcommands are easier to test than more. OK - forget the "config on command line" idea. Just read config from /etc/nfs.conf and action that. > > > > It would also be helpful to have "nfsdinfo" or similar which has "stats" > > and "status" commands. Maybe that could be combined with "nfsdctl", but > > extracting info is not a form of "control". Or we could find a more > > generic verb. For soft-raid I wrote "mdadm" "adm" for "administer" > > which seemed less specific than "control" (mdctl). Though probably the > > main reason was that I like palindromes and "mdadm" was a bit easier to > > say. nfsdadm ?? (see also udevadm, drdbadm, fsadm ....) But maybe I'm > > just too fuss. > > > > In my experience working with our customers and support team, they are > > not at all interested in fine control. > > This is an interface to be used by systemctl. I don't think customers > or support teams would ever need to make use of it directly. If it's to be used by systemctl, then there is zero justification for anything other than start/reload/stop. We already have /etc/nfs.conf.d/* for configuring nfsd. Requiring tools to edit systemctl unit files as well is a retrograde step. NeilBrown > > > > "systemctl restart nfs-server" is > > their second preference when "reboot" isn't appropriate for some reason. > > > > You might be able to convince me that "nfdctl reload" was useful - it > > could be called from "systemctl reload nfs-server". That would then > > justify kernel interfaces to remove xprts. But having all the > > fine-control just increases the required testing needed with little > > practical gain. > > > > NeilBrown > > > -- > Chuck Lever > > >
