On Sat, 07 Sep 2024, Mike Snitzer wrote: > > But I'd just like to point out that something like the below patch > > wouldn't be needed if we kept my "heavy" approach (nfs reference on > > nfsd modules via nfs_common using symbol_request): > > https://marc.info/?l=linux-nfs&m=172499445027800&w=2 > > (that patch has stuff I since cleaned up, e.g. removed typedefs and > > EXPORT_SYMBOL_GPLs..) > > > > I knew we were going to pay for being too cute with how nfs took its > > reference on nfsd. > > > > So here we are, needing fiddly incremental fixes like this to close a > > really-small-yet-will-be-deadly race: > > <snip required delicate rcu re-locking requirements patch> > > I prefer this incremental re-implementation of my symbol_request patch > that eliminates all concerns about the validity of 'nfs_to' calls: 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. .... BTW you probably wanted to use symbol_get(), not symbol_request(). The latter tries to load the module if it isn't already loaded. Using symbol_get() does have the benefit that you don't need any locking dance to prevent the module unloading while we get the ref. So if we really want to go for less tricky locking that might be a justification - but you don't need much locking for try_module_get()... Thanks, NeilBrown
