Message-ID: <3f035d3bc494ec03b83ae237e407c42f2ddc4c53.camel@kernel.org>
Date: Tue, 23 Jan 2024 09:28:36 -0500
From: Jeff Layton <jlayton@...nel.org>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: Chuck Lever <chuck.lever@...cle.com>, NeilBrown <neilb@...e.de>, 
	linux-nfs@...r.kernel.org, lorenzo.bianconi@...hat.com, kuba@...nel.org, 
	horms@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v6 3/3] NFSD: add write_ports to netlink command

On Tue, 2024-01-23 at 14:21 +0100, Lorenzo Bianconi wrote:
> > On Tue, 2024-01-23 at 10:59 +0100, Lorenzo Bianconi wrote:
> > > > On Tue, Jan 23, 2024 at 08:35:07AM +1100, NeilBrown wrote:
> > > > > On Tue, 23 Jan 2024, Jeff Layton wrote:
> > > > > > On Sat, 2024-01-20 at 18:33 +0100, Lorenzo Bianconi wrote:
> > > > > > > Introduce write_ports netlink command. For listener-set, userspace is
> > > > > > > expected to provide a NFS listeners list it wants to enable (all the
> > > > > > > other ports will be closed).
> > > > > > > 
> > > > > > 
> > > > > > Ditto here. This is a change to a declarative interface, which I think
> > > > > > is a better way to handle this, but we should be aware of the change.
> > > > > 
> > > > > I agree it is better, and thanks for highlighting the change.
> > > > > 
> > > > > > > +	/* 2- remove stale listeners */
> > > > > > 
> > > > > > 
> > > > > > The old portlist interface was weird, in that it was only additive. You
> > > > > > couldn't use it to close a listening socket (AFAICT). We may be able to
> > > > > > support that now with this interface, but we'll need to test that case
> > > > > > carefully.
> > > > > 
> > > > > Do we ever want/need to remove listening sockets?
> > > > 
> > > > I think that might be an interesting use case. Disabling RDMA, for
> > > > example, should kill the RDMA listening endpoints but leave
> > > > listening sockets in place.
> > > > 
> > > > But for now, our socket listeners are "any". Wondering how net
> > > > namespaces play into this.
> > > > 
> > > > 
> > > > > Normal practice when making any changes is to stop and restart where
> > > > > "stop" removes all sockets, unexports all filesystems, disables all
> > > > > versions.
> > > > > I don't exactly object to supporting fine-grained changes, but I suspect
> > > > > anything that is not used by normal service start will hardly ever be
> > > > > used in practice, so will not be tested.
> > > > 
> > > > Well, there is that. I guess until we have test coverage for NFSD
> > > > administrative interfaces, we should leave well enough alone.
> > > 
> > > So to summarize it:
> > > - we will allow to remove enabled versions (as it is in patch v6 2/3)
> > > - we will allow to add new listening sockets but we will not allow to remove
> > >   them (the user/admin will need to stop/start the server).
> > > 
> > > Agree? If so I will work on it and post v7.
> > > 
> > > 
> > 
> > That sounds about right to me. We could eventually relax the restriction
> > about removing sockets later, but for now it's probably best to prohibit
> > it (like Neil suggests).
> 
> Do we want to add even the capability to specify the socket file descriptor
> (similar to what we do in __write_ports_addfd())?
> 

That's a great question. We do need to properly support the -H option to
rpc.nfsd. What we do today is look up the hostname or address using
getaddrinfo, and then open a listening socket for that address and then
pass that fd down to the kernel, which I think then takes the socket and
sticks it on sv_permsocks.

All of that seems a bit klunky. Ideally, I'd say the best thing would be
to allow userland to pass the sockaddr we look up directly via netlink,
and then let the kernel open the socket. That will probably mean
refactoring some of the svc_xprt_create machinery to take a sockaddr,
but I don't think it looks too hard to do.

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists