Re: [PATCH v13 09/19] nfs_common: add NFS LOCALIO auxiliary protocol enablement

[Mike Snitzer <snitzer@kernel.org>]

On Mon, Aug 26, 2024 at 10:32:30AM +1000, NeilBrown wrote:
> On Sat, 24 Aug 2024, Mike Snitzer wrote:
> > 
> > Also, expose localio's required nfsd symbols to NFS client:
> > - Cache nfsd_open_local_fh symbol (defined in next commit) and other
> >   required nfsd symbols in a globally accessible 'nfs_to'
> >   nfs_to_nfsd_t struct.
> 
> I'm not thrilled with the mechanism for getting these symbols.
> 
> I'd rather nfsd passed the symbols to nfs_uuid_is_local(), and it stored
> them somewhere that nfs can see them.  No need for reference counting
> etc.  If nfs/localio holds an auth_domain, then it implicitly holds a
> reference to the nfsd module and the functions cannot disappear.
>
> I would created an 'nfs_localio_operations' structure which is defined
> in nfsd as a constant.
> The address of this is passed to nfs_uud_is_local() and that address
> is stored in nfs_to if it doesn't already have the correct value.
> 
> So no need for symbol_request() or symbol_put().

I'm not seeing why we'd want to actively engineer some even more
bespoke way to access nfsd symbols.  The symbol refcounting is only
done when the client first connects as part of the LOCALIO handshake
(or when client is destroyed and LOCALIO enabled), so it isn't getting
in the way.

Happy to revisit this but I'd really prefer to use standard convention
(symbol_request and symbol_put) to establish nfs's dependency on
nfsd's symbols.

> > +
> > +DEFINE_MUTEX(nfs_uuid_mutex);
> 
> This doesn't need to be a mutex - a spinlock is sufficient.

Fixed, thanks.

> > +
> > +bool nfs_uuid_is_local(const uuid_t *uuid, struct net *net, struct auth_domain *dom)
> > +{
> > +	bool is_local = false;
> > +	nfs_uuid_t *nfs_uuid;
> > +
> > +	rcu_read_lock();
> > +	nfs_uuid = nfs_uuid_lookup(uuid);
> > +	if (nfs_uuid) {
> > +		is_local = true;
> > +		nfs_uuid->net = net;
> 
> It looks odd that you don't take a reference to the net.
> It is probably correct but a comment explaining why would help.
> Is it that the dom implies a reference to the net?

No, I just made the code tolerate the net having been torn down
(see: fs/nfsd/localio.c:nfsd_open_local_fh), but it'd be safer to take
a proper reference here.

I'll work through it once more and update accordingly.

Thanks!