Re: [PATCH v14 15/25] nfs_common: introduce nfs_localio_ctx struct and interfaces

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Aug 29, 2024 at 12:40:27PM -0400, Jeff Layton wrote:
> On Wed, 2024-08-28 at 21:04 -0400, Mike Snitzer wrote:
> > Introduce struct nfs_localio_ctx and the interfaces
> > nfs_localio_ctx_alloc() and nfs_localio_ctx_free().  The next commit
> > will introduce nfsd_open_local_fh() which returns a nfs_localio_ctx
> > structure.
> > 
> > Also, expose localio's required NFSD symbols to NFS client:
> > - Cache nfsd_open_local_fh symbol and other required NFSD symbols in a
> >   globally accessible 'nfs_to' nfs_to_nfsd_t struct.  Add interfaces
> >   get_nfs_to_nfsd_symbols() and put_nfs_to_nfsd_symbols() to allow
> >   each NFS client to take a reference on NFSD symbols.
> > 
> > - Apologies for the DEFINE_NFS_TO_NFSD_SYMBOL macro that makes
> >   defining get_##NFSD_SYMBOL() and put_##NFSD_SYMBOL() functions far
> >   simpler (and avoids cut-n-paste bugs, which is what motivated the
> >   development and use of a macro for this). But as C macros go it is a
> >   very simple one and there are many like it all over the kernel.
> > 
> > - Given the unique nature of NFS LOCALIO being an optional feature
> >   that when used requires NFS share access to NFSD memory: a unique
> >   bridging of NFSD resources to NFS (via nfs_common) is needed.  But
> >   that bridge must be dynamic, hence the use of symbol_request() and
> >   symbol_put().  Proposed ideas to accomolish the same without using
> >   symbol_{request,put} would be far more tedious to implement and
> >   very likely no easier to review.  Anyway: sorry NeilBrown...
> > 
> > - Despite the use of indirect function calls, caching these nfsd
> >   symbols for use by the client offers a ~10% performance win
> >   (compared to always doing get+call+put) for high IOPS workloads.
> > 
> > - Introduce nfsd_file_file() wrapper that provides access to
> >   nfsd_file's backing file.  Keeps nfsd_file structure opaque to NFS
> >   client (as suggested by Jeff Layton).
> > 
> > - The addition of nfsd_file_get, nfsd_file_put and nfsd_file_file
> >   symbols prepares for the NFS client to use nfsd_file for localio.
> > 
> > Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com> # nfs_to
> > Suggested-by: Jeff Layton <jlayton@kernel.org> # nfsd_file_file
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfs_common/nfslocalio.c | 159 +++++++++++++++++++++++++++++++++++++
> >  fs/nfsd/filecache.c        |  25 ++++++
> >  fs/nfsd/filecache.h        |   1 +
> >  fs/nfsd/nfssvc.c           |   5 ++
> >  include/linux/nfslocalio.h |  38 +++++++++
> >  5 files changed, 228 insertions(+)
> > 
> > diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> > index 1a35a4a6dbe0..cc30fdb0cb46 100644
> > --- a/fs/nfs_common/nfslocalio.c
> > +++ b/fs/nfs_common/nfslocalio.c
> > @@ -72,3 +72,162 @@ bool nfs_uuid_is_local(const uuid_t *uuid, struct net *net, struct auth_domain *
> >  	return is_local;
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_uuid_is_local);
> > +
> > +/*
> > + * The nfs localio code needs to call into nfsd using various symbols (below),
> > + * but cannot be statically linked, because that will make the nfs module
> > + * depend on the nfsd module.
> > + *
> > + * Instead, do dynamic linking to the nfsd module (via nfs_common module). The
> > + * nfs_common module will only hold a reference on nfsd when localio is in use.
> > + * This allows some sanity checking, like giving up on localio if nfsd isn't loaded.
> > + */
> > +static DEFINE_SPINLOCK(nfs_to_nfsd_lock);
> > +nfs_to_nfsd_t nfs_to;
> > +EXPORT_SYMBOL_GPL(nfs_to);
> > +
> > +/* Macro to define nfs_to get and put methods, avoids copy-n-paste bugs */
> > +#define DEFINE_NFS_TO_NFSD_SYMBOL(NFSD_SYMBOL)		\
> > +static nfs_to_##NFSD_SYMBOL##_t get_##NFSD_SYMBOL(void)	\
> > +{							\
> > +	return symbol_request(NFSD_SYMBOL);		\
> > +}							\
> > +static void put_##NFSD_SYMBOL(void)			\
> > +{							\
> > +	symbol_put(NFSD_SYMBOL);			\
> > +	nfs_to.NFSD_SYMBOL = NULL;			\
> > +}
> > +
> > +/* The nfs localio code needs to call into nfsd to map filehandle -> struct nfsd_file */
> > +extern struct nfs_localio_ctx *
> > +nfsd_open_local_fh(struct net *, struct auth_domain *, struct rpc_clnt *,
> > +		   const struct cred *, const struct nfs_fh *, const fmode_t);
> > +DEFINE_NFS_TO_NFSD_SYMBOL(nfsd_open_local_fh);
> > +
> > +/* The nfs localio code needs to call into nfsd to acquire the nfsd_file */
> > +extern struct nfsd_file *nfsd_file_get(struct nfsd_file *nf);
> > +DEFINE_NFS_TO_NFSD_SYMBOL(nfsd_file_get);
> > +
> > +/* The nfs localio code needs to call into nfsd to release the nfsd_file */
> > +extern void nfsd_file_put(struct nfsd_file *nf);
> > +DEFINE_NFS_TO_NFSD_SYMBOL(nfsd_file_put);
> > +
> > +/* The nfs localio code needs to call into nfsd to access the nf->nf_file */
> > +extern struct file * nfsd_file_file(struct nfsd_file *nf);
> > +DEFINE_NFS_TO_NFSD_SYMBOL(nfsd_file_file);
> > +
> > +/* The nfs localio code needs to call into nfsd to release nn->nfsd_serv */
> > +extern void nfsd_serv_put(struct nfsd_net *nn);
> > +DEFINE_NFS_TO_NFSD_SYMBOL(nfsd_serv_put);
> > +#undef DEFINE_NFS_TO_NFSD_SYMBOL
> > +
> 
> I have the same concerns as Neil did with this patch in v13. An ops
> structure that nfsd registers with nfs_common and that has pointers to
> all of these functions would be a lot cleaner. I think it'll end up
> being less code too.
> 
> In fact, for that I'd probably break my usual guideline of not
> introducing new interfaces without callers, and just do a separate
> patch that adds the ops structure and sets up the handling of the
> pointer to it in nfs_common.

OK, as much as it pains me to set aside proven code that I put a
decent amount of time to honing: I'll humor you guys and try to make
an ops structure workable. (we can always fall back to my approach if
I/we come up short).

I'm just concerned about the optional use aspect.  There is the pain
point of how does NFS client come to _know_ NFSD loaded?  Using
symbol_request() deals with that nicely.

I really don't want all calls in NFS client (or nfs_common) to have to
first check if nfs_common's 'nfs_to' ops structure is NULL or not.

But yeah, I'll put more time to it... ;)

Mike