Re: [PATCH v15 16/26] nfsd: add LOCALIO support

[Mike Snitzer <snitzer@kernel.org>]

On Sat, Sep 07, 2024 at 09:14:57AM +1000, NeilBrown wrote:
> On Sat, 07 Sep 2024, Chuck Lever III wrote:
> > 
> > 
> > > On Sep 6, 2024, at 5:56 PM, NeilBrown <neilb@suse.de> wrote:
> > > 
> > > We could achieve the same effect without using symbol_request() (which
> > > hardly anyone uses) if we did a __module_get (or try_module_get) at the
> > > same place you are calling symbol_request(), and module_put() where you
> > > do symbol_put().
> > > 
> > > This would mean that once NFS LOCALIO had detected a path to the local
> > > server, it would hold the nfsd module until the nfs server were shutdown
> > > and the nfs client noticed.  So you wouldn't be able to unmount the nfsd
> > > module immediately after stopping all nfsd servers.
> > > 
> > > Maybe that doesn't matter.  I think it is important to be able to
> > > completely shut down the NFS server at any time.  I think it is
> > > important to be able to completely shutdown a network namespace at any
> > > time.  I am less concerned about being able to rmmod the nfsd module
> > > after all obvious users have been disabled.
> > > 
> > > So if others think that the improvements in code maintainability are
> > > worth the loss of being able to rmmod nfsd without (potentially) having
> > > to unmount all NFS filesystems, then I won't argue against it.  But I
> > > really would want it to be get/put of the module, not of some symbol.
> > 
> > The client and server are potentially in separate containers,
> > administered independently. An NFS mount should not pin either
> > the NFS server's running status, its ability to unexport a
> > shared file system, the ability for the NFS server's
> > administrator to rmmod nfsd.ko, the ability for the
> > administrator to rmmod a network device that is in use by the
> > NFS server, or the ability to destroy the NFS server's
> > namespace once NFSD has shut down.
> 
> While I mostly agree, I should point out that nfsd.ko is a global
> resource across all containers.  So if the client and server are
> administer separately, there is no certainty that the server
> administrator is at all related to the global moderator who controls
> when nfsd.ko might be unloaded.  So preventing the unload of nfsd.ko is
> quite a different class of problem to preventing the shutdown of the
> nfsd service or of the container that it runs in.

Right, and in practice LOCALIO doesn't prevent any expected NFS client
or server shutdown within containers.

Proper refcounting of required resources from host is needed, new
requirement for LOCALIO is that nfsd be properly refcounted until
consuming nfs client code exits.  But if LOCALIO client is connected
to a server, if/when that server shuts down it isn't blocked from
doing so simply because a LOCALIO client is active.

Rather than have general concern for LOCALIO doing something wrong,
we'd do well to make sure there is proper test coverage for required
shutdown sequences (completely indepent of LOCALIO, maybe that already
exists?).  I'm 99.99% sure LOCALIO will pass all of that testing
(informed by manual testing I have done with container shutdown, etc).

Mike