| Message ID | 20240829010424.83693-16-snitzer@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | nfs/nfsd: add support for LOCALIO | expand |
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. > +static struct kmem_cache *nfs_localio_ctx_cache; > + > +struct nfs_localio_ctx *nfs_localio_ctx_alloc(void) > +{ > + return kmem_cache_alloc(nfs_localio_ctx_cache, > + GFP_KERNEL | __GFP_ZERO); > +} > +EXPORT_SYMBOL_GPL(nfs_localio_ctx_alloc); > + > +void nfs_localio_ctx_free(struct nfs_localio_ctx *localio) > +{ > + if (localio->nf) > + nfs_to.nfsd_file_put(localio->nf); > + if (localio->nn) > + nfs_to.nfsd_serv_put(localio->nn); > + kmem_cache_free(nfs_localio_ctx_cache, localio); > +} > +EXPORT_SYMBOL_GPL(nfs_localio_ctx_free); > + > +bool get_nfs_to_nfsd_symbols(void) > +{ > + spin_lock(&nfs_to_nfsd_lock); > + > + /* Only get symbols on first reference */ > + if (refcount_read(&nfs_to.ref) == 0) > + refcount_set(&nfs_to.ref, 1); > + else { > + refcount_inc(&nfs_to.ref); > + spin_unlock(&nfs_to_nfsd_lock); > + return true; > + } > + > + nfs_to.nfsd_open_local_fh = get_nfsd_open_local_fh(); > + if (!nfs_to.nfsd_open_local_fh) > + goto out_nfsd_open_local_fh; > + > + nfs_to.nfsd_file_get = get_nfsd_file_get(); > + if (!nfs_to.nfsd_file_get) > + goto out_nfsd_file_get; > + > + nfs_to.nfsd_file_put = get_nfsd_file_put(); > + if (!nfs_to.nfsd_file_put) > + goto out_nfsd_file_put; > + > + nfs_to.nfsd_file_file = get_nfsd_file_file(); > + if (!nfs_to.nfsd_file_file) > + goto out_nfsd_file_file; > + > + nfs_to.nfsd_serv_put = get_nfsd_serv_put(); > + if (!nfs_to.nfsd_serv_put) > + goto out_nfsd_serv_put; > + > + spin_unlock(&nfs_to_nfsd_lock); > + return true; > + > +out_nfsd_serv_put: > + put_nfsd_file_file(); > +out_nfsd_file_file: > + put_nfsd_file_put(); > +out_nfsd_file_put: > + put_nfsd_file_get(); > +out_nfsd_file_get: > + put_nfsd_open_local_fh(); > +out_nfsd_open_local_fh: > + spin_unlock(&nfs_to_nfsd_lock); > + return false; > +} > +EXPORT_SYMBOL_GPL(get_nfs_to_nfsd_symbols); > + > +void put_nfs_to_nfsd_symbols(void) > +{ > + spin_lock(&nfs_to_nfsd_lock); > + > + if (!refcount_dec_and_test(&nfs_to.ref)) > + goto out; > + > + put_nfsd_open_local_fh(); > + put_nfsd_file_get(); > + put_nfsd_file_put(); > + put_nfsd_file_file(); > + put_nfsd_serv_put(); > +out: > + spin_unlock(&nfs_to_nfsd_lock); > +} > +EXPORT_SYMBOL_GPL(put_nfs_to_nfsd_symbols); > + > +static int __init nfslocalio_init(void) > +{ > + refcount_set(&nfs_to.ref, 0); > + > + nfs_to.nfsd_open_local_fh = NULL; > + nfs_to.nfsd_file_get = NULL; > + nfs_to.nfsd_file_put = NULL; > + nfs_to.nfsd_file_file = NULL; > + nfs_to.nfsd_serv_put = NULL; > + > + nfs_localio_ctx_cache = KMEM_CACHE(nfs_localio_ctx, 0); > + if (!nfs_localio_ctx_cache) > + return -ENOMEM; > + > + return 0; > +} > + > +static void __exit nfslocalio_exit(void) > +{ > + kmem_cache_destroy(nfs_localio_ctx_cache); > +} > + > +module_init(nfslocalio_init); > +module_exit(nfslocalio_exit); > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index 2dc72de31f61..a83d469bca6b 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -39,6 +39,7 @@ > #include <linux/fsnotify.h> > #include <linux/seq_file.h> > #include <linux/rhashtable.h> > +#include <linux/nfslocalio.h> > > #include "vfs.h" > #include "nfsd.h" > @@ -345,6 +346,10 @@ nfsd_file_get(struct nfsd_file *nf) > return nf; > return NULL; > } > +EXPORT_SYMBOL_GPL(nfsd_file_get); > + > +/* Compile time type checking, not used by anything */ > +static nfs_to_nfsd_file_get_t __maybe_unused nfsd_file_get_typecheck = nfsd_file_get; > > /** > * nfsd_file_put - put the reference to a nfsd_file > @@ -389,6 +394,26 @@ nfsd_file_put(struct nfsd_file *nf) > if (refcount_dec_and_test(&nf->nf_ref)) > nfsd_file_free(nf); > } > +EXPORT_SYMBOL_GPL(nfsd_file_put); > + > +/* Compile time type checking, not used by anything */ > +static nfs_to_nfsd_file_put_t __maybe_unused nfsd_file_put_typecheck = nfsd_file_put; > + > +/** > + * nfsd_file_file - get the backing file of an nfsd_file > + * @nf: nfsd_file of which to access the backing file. > + * > + * Return backing file for @nf. > + */ > +struct file * > +nfsd_file_file(struct nfsd_file *nf) > +{ > + return nf->nf_file; > +} > +EXPORT_SYMBOL_GPL(nfsd_file_file); > + > +/* Compile time type checking, not used by anything */ > +static nfs_to_nfsd_file_file_t __maybe_unused nfsd_file_file_typecheck = nfsd_file_file; > > static void > nfsd_file_dispose_list(struct list_head *dispose) > diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h > index 26ada78b8c1e..6fbbb2e32e95 100644 > --- a/fs/nfsd/filecache.h > +++ b/fs/nfsd/filecache.h > @@ -56,6 +56,7 @@ int nfsd_file_cache_start_net(struct net *net); > void nfsd_file_cache_shutdown_net(struct net *net); > void nfsd_file_put(struct nfsd_file *nf); > struct nfsd_file *nfsd_file_get(struct nfsd_file *nf); > +struct file *nfsd_file_file(struct nfsd_file *nf); > void nfsd_file_close_inode_sync(struct inode *inode); > void nfsd_file_net_dispose(struct nfsd_net *nn); > bool nfsd_file_is_cached(struct inode *inode); > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > index c639fbe4d8c2..13c69aa40d1c 100644 > --- a/fs/nfsd/nfssvc.c > +++ b/fs/nfsd/nfssvc.c > @@ -19,6 +19,7 @@ > #include <linux/sunrpc/svc_xprt.h> > #include <linux/lockd/bind.h> > #include <linux/nfsacl.h> > +#include <linux/nfslocalio.h> > #include <linux/seq_file.h> > #include <linux/inetdevice.h> > #include <net/addrconf.h> > @@ -201,6 +202,10 @@ void nfsd_serv_put(struct nfsd_net *nn) > { > percpu_ref_put(&nn->nfsd_serv_ref); > } > +EXPORT_SYMBOL_GPL(nfsd_serv_put); > + > +/* Compile time type checking, not used by anything */ > +static nfs_to_nfsd_serv_put_t __maybe_unused nfsd_serv_put_typecheck = nfsd_serv_put; > > static void nfsd_serv_done(struct percpu_ref *ref) > { > diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h > index 9735ae8d3e5e..68f5b39f1940 100644 > --- a/include/linux/nfslocalio.h > +++ b/include/linux/nfslocalio.h > @@ -7,6 +7,8 @@ > > #include <linux/list.h> > #include <linux/uuid.h> > +#include <linux/refcount.h> > +#include <linux/sunrpc/clnt.h> > #include <linux/sunrpc/svcauth.h> > #include <linux/nfs.h> > #include <net/net_namespace.h> > @@ -28,4 +30,40 @@ void nfs_uuid_begin(nfs_uuid_t *); > void nfs_uuid_end(nfs_uuid_t *); > bool nfs_uuid_is_local(const uuid_t *, struct net *, struct auth_domain *); > > +struct nfsd_file; > +struct nfsd_net; > + > +struct nfs_localio_ctx { > + struct nfsd_file *nf; > + struct nfsd_net *nn; > +}; > + > +typedef struct nfs_localio_ctx * > +(*nfs_to_nfsd_open_local_fh_t)(struct net *, struct auth_domain *, > + struct rpc_clnt *, const struct cred *, > + const struct nfs_fh *, const fmode_t); > +typedef struct nfsd_file * (*nfs_to_nfsd_file_get_t)(struct nfsd_file *); > +typedef void (*nfs_to_nfsd_file_put_t)(struct nfsd_file *); > +typedef struct file * (*nfs_to_nfsd_file_file_t)(struct nfsd_file *); > +typedef unsigned int (*nfs_to_nfsd_net_id_value_t)(void); > +typedef void (*nfs_to_nfsd_serv_put_t)(struct nfsd_net *); > + > +typedef struct { > + refcount_t ref; > + nfs_to_nfsd_open_local_fh_t nfsd_open_local_fh; > + nfs_to_nfsd_file_get_t nfsd_file_get; > + nfs_to_nfsd_file_put_t nfsd_file_put; > + nfs_to_nfsd_file_file_t nfsd_file_file; > + nfs_to_nfsd_net_id_value_t nfsd_net_id_value; > + nfs_to_nfsd_serv_put_t nfsd_serv_put; > +} nfs_to_nfsd_t; > + > +extern nfs_to_nfsd_t nfs_to; > + > +bool get_nfs_to_nfsd_symbols(void); > +void put_nfs_to_nfsd_symbols(void); > + > +struct nfs_localio_ctx *nfs_localio_ctx_alloc(void); > +void nfs_localio_ctx_free(struct nfs_localio_ctx *); > + > #endif /* __LINUX_NFSLOCALIO_H */
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
On Thu, 2024-08-29 at 12:52 -0400, Mike Snitzer wrote: > 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. > Have a pointer to a struct nfsd_localio_ops or something in the nfs_common module. That's initially set to NULL. Then, have a static structure of that type in nfsd.ko, and have its __init routine set the pointer in nfs_common to point to the right structure. The __exit routine will later set it to NULL. > 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. Neil seems to think that's not necessary: "If nfs/localio holds an auth_domain, then it implicitly holds a reference to the nfsd module and the functions cannot disappear." That'll need to be clearly documented though as it's a subtle thing to rely on for this.
On Fri, 30 Aug 2024, Jeff Layton wrote: > On Thu, 2024-08-29 at 12:52 -0400, Mike Snitzer wrote: > > 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. > > > > Have a pointer to a struct nfsd_localio_ops or something in the > nfs_common module. That's initially set to NULL. Then, have a static > structure of that type in nfsd.ko, and have its __init routine set the > pointer in nfs_common to point to the right structure. The __exit > routine will later set it to NULL. > > > 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. > > Neil seems to think that's not necessary: > > "If nfs/localio holds an auth_domain, then it implicitly holds a > reference to the nfsd module and the functions cannot disappear." On reflection that isn't quite right, but it is the sort of approach that I think we need to take. There are several things that the NFS client needs to hold one to. 1/ It needs a reference to the nfsd module (or symbols in the module). I think this can be held long term but we need a clear mechanism for it to be dropped. 2/ It needs a reference to the nfsd_serv which it gets through the 'struct net' pointer. I've posted patches to handle that better. 3/ It needs a reference to an auth_domain. This can safely be a long term reference. It can already be invalidated and the code to free it is in sunrpc which nfs already pins. Any delay in freeing it only wastes memory (not much), it doesn't impact anything else. 4/ It needs a reference to the nfsd_file and/or file. This is currently done only while the ref to the nfsd_serv is held, so I think there is no problem there. So possibly we could take a reference to the nfsd module whenever we store a net in nfs_uuid. and drop the ref whenever we clear that. That means we cannot call nfsd_open_local_fh() without first getting a ref on the nfsd_serv which my latest code doesn't do. That is easily fixed. I'll send a patch for consideration... Thanks, NeilBrown
On Fri, Aug 30, 2024 at 02:36:13PM +1000, NeilBrown wrote: > On Fri, 30 Aug 2024, Jeff Layton wrote: > > On Thu, 2024-08-29 at 12:52 -0400, Mike Snitzer wrote: > > > 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. > > > > > > > Have a pointer to a struct nfsd_localio_ops or something in the > > nfs_common module. That's initially set to NULL. Then, have a static > > structure of that type in nfsd.ko, and have its __init routine set the > > pointer in nfs_common to point to the right structure. The __exit > > routine will later set it to NULL. > > > > > 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. > > > > Neil seems to think that's not necessary: > > > > "If nfs/localio holds an auth_domain, then it implicitly holds a > > reference to the nfsd module and the functions cannot disappear." > > On reflection that isn't quite right, but it is the sort of approach > that I think we need to take. > There are several things that the NFS client needs to hold one to. > > 1/ It needs a reference to the nfsd module (or symbols in the module). > I think this can be held long term but we need a clear mechanism for > it to be dropped. > 2/ It needs a reference to the nfsd_serv which it gets through the > 'struct net' pointer. I've posted patches to handle that better. > 3/ It needs a reference to an auth_domain. This can safely be a long > term reference. It can already be invalidated and the code to free > it is in sunrpc which nfs already pins. Any delay in freeing it only > wastes memory (not much), it doesn't impact anything else. > 4/ It needs a reference to the nfsd_file and/or file. This is currently > done only while the ref to the nfsd_serv is held, so I think there is > no problem there. > > So possibly we could take a reference to the nfsd module whenever we > store a net in nfs_uuid. and drop the ref whenever we clear that. > > That means we cannot call nfsd_open_local_fh() without first getting a > ref on the nfsd_serv which my latest code doesn't do. That is easily > fixed. I'll send a patch for consideration... I already implemented 2 different versions today, meant for v15. First is a relaxed version of the v14 code (less code, only using symbol_request on nfsd_open_local_fh. Second is much more relaxed, because it leverages your original assumption that the auth_domain ref sufficient. I'll reply twice to this mail with each each respective patch. Maybe I save you some time...
On Fri, Aug 30, 2024 at 01:01:55AM -0400, Mike Snitzer wrote: > On Fri, Aug 30, 2024 at 02:36:13PM +1000, NeilBrown wrote: > > On Fri, 30 Aug 2024, Jeff Layton wrote: > > > > > > Have a pointer to a struct nfsd_localio_ops or something in the > > > nfs_common module. That's initially set to NULL. Then, have a static > > > structure of that type in nfsd.ko, and have its __init routine set the > > > pointer in nfs_common to point to the right structure. The __exit > > > routine will later set it to NULL. > > > > > > > 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. > > > > > > Neil seems to think that's not necessary: > > > > > > "If nfs/localio holds an auth_domain, then it implicitly holds a > > > reference to the nfsd module and the functions cannot disappear." > > > > On reflection that isn't quite right, but it is the sort of approach > > that I think we need to take. > > There are several things that the NFS client needs to hold one to. > > > > 1/ It needs a reference to the nfsd module (or symbols in the module). > > I think this can be held long term but we need a clear mechanism for > > it to be dropped. > > 2/ It needs a reference to the nfsd_serv which it gets through the > > 'struct net' pointer. I've posted patches to handle that better. > > 3/ It needs a reference to an auth_domain. This can safely be a long > > term reference. It can already be invalidated and the code to free > > it is in sunrpc which nfs already pins. Any delay in freeing it only > > wastes memory (not much), it doesn't impact anything else. > > 4/ It needs a reference to the nfsd_file and/or file. This is currently > > done only while the ref to the nfsd_serv is held, so I think there is > > no problem there. > > > > So possibly we could take a reference to the nfsd module whenever we > > store a net in nfs_uuid. and drop the ref whenever we clear that. > > > > That means we cannot call nfsd_open_local_fh() without first getting a > > ref on the nfsd_serv which my latest code doesn't do. That is easily > > fixed. I'll send a patch for consideration... > > I already implemented 2 different versions today, meant for v15. > > First is a relaxed version of the v14 code (less code, only using > symbol_request on nfsd_open_local_fh. This is the corresponding code that is needed in fs/nfsd/localio.c +static const struct nfsd_localio_operations nfsd_localio_ops = { + .nfsd_open_local_fh = nfsd_open_local_fh, + .nfsd_file_get = nfsd_file_get, + .nfsd_file_put = nfsd_file_put, + .nfsd_file_file = nfsd_file_file, + .nfsd_serv_put = nfsd_serv_put, +}; + +void init_nfs_to_nfsd_localio_ops(void) +{ + memcpy(&nfs_to, &nfsd_localio_ops, sizeof(nfsd_localio_ops)); +} +EXPORT_SYMBOL_GPL(init_nfs_to_nfsd_localio_ops); From: Mike Snitzer <snitzer@kernel.org> Date: Wed, 28 Aug 2024 17:04:44 -0500 Subject: [PATCH v15.option1] nfs_common: introduce nfs_localio_ctx struct and interfaces Introduce struct nfs_localio_ctx (which has nfsd_file and nfsd_net members) 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: - Make nfsd_open_local_fh() symbol and other required NFSD symbols available to NFS in a global 'nfs_to' nfsd_localio_operations struct. Add interfaces get_nfs_to_nfsd_localio_ops() and put_nfs_to_nfsd_localio_ops() to allow each NFS client to take a reference on NFSD (indirectly through nfs_common). - 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() of the one init_nfs_to_nfsd_localio_ops() symbol which nfs_common's LOCALIO code uses as a bellwether for both if NFSD is available and supports LOCALIO. - Use of a refcount_t (managed by {get,put}_nfs_to_nfsd_localio_ops) is required to ensure NFS doesn't become disjoint from NFSD's availability in the face of LOCALIO support being toggled on/off coupled with the NFSD module (possibly) being unloaded. - 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: NeilBrown <neilb@suse.de> # nfsd_localio_operations Suggested-by: Jeff Layton <jlayton@kernel.org> # nfsd_file_file Signed-off-by: Mike Snitzer <snitzer@kernel.org> --- fs/nfs_common/nfslocalio.c | 107 +++++++++++++++++++++++++++++++++++++ fs/nfsd/filecache.c | 16 ++++++ fs/nfsd/filecache.h | 1 + fs/nfsd/nfssvc.c | 2 + include/linux/nfslocalio.h | 47 ++++++++++++++++ 5 files changed, 173 insertions(+) diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c index 0b2e17c2068f..5f12610a877c 100644 --- a/fs/nfs_common/nfslocalio.c +++ b/fs/nfs_common/nfslocalio.c @@ -5,6 +5,7 @@ #include <linux/module.h> #include <linux/rculist.h> +#include <linux/refcount.h> #include <linux/nfslocalio.h> #include <net/netns/generic.h> @@ -72,3 +73,109 @@ 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); +static refcount_t nfs_to_ref; +struct nfsd_localio_operations nfs_to; +EXPORT_SYMBOL_GPL(nfs_to); + +bool get_nfs_to_nfsd_localio_ops(void) +{ + bool ret = false; + init_nfs_to_nfsd_localio_ops_t init_nfs_to; + + spin_lock(&nfs_to_nfsd_lock); + + /* Only get nfsd_localio_operations on first reference */ + if (refcount_read(&nfs_to_ref) == 0) + refcount_set(&nfs_to_ref, 1); + else { + refcount_inc(&nfs_to_ref); + ret = true; + goto out; + } + + /* + * If NFSD isn't available LOCALIO isn't possible. + * Use init_nfs_to_nfsd_ops symbol as the bellwether, + * if available then nfs_common has NFSD module reference + * on NFS's behalf and can initialize global 'nfs_to'. + */ + init_nfs_to = symbol_request(init_nfs_to_nfsd_localio_ops); + if (init_nfs_to) { + init_nfs_to(); + if (WARN_ON_ONCE(!nfs_to.nfsd_open_local_fh)) { + symbol_put(init_nfs_to_nfsd_localio_ops); + goto out; + } + ret = true; + } +out: + spin_unlock(&nfs_to_nfsd_lock); + return ret; +} +EXPORT_SYMBOL_GPL(get_nfs_to_nfsd_localio_ops); + +void put_nfs_to_nfsd_localio_ops(void) +{ + spin_lock(&nfs_to_nfsd_lock); + + if (!refcount_dec_and_test(&nfs_to_ref)) + goto out; + + symbol_put(init_nfs_to_nfsd_localio_ops); + memset(&nfs_to, 0, sizeof(nfs_to)); +out: + spin_unlock(&nfs_to_nfsd_lock); +} +EXPORT_SYMBOL_GPL(put_nfs_to_nfsd_localio_ops); + +/* + * nfs_localio_ctx cache and alloc/free interfaces. + */ +static struct kmem_cache *nfs_localio_ctx_cache; + +struct nfs_localio_ctx *nfs_localio_ctx_alloc(void) +{ + return kmem_cache_alloc(nfs_localio_ctx_cache, + GFP_KERNEL | __GFP_ZERO); +} +EXPORT_SYMBOL_GPL(nfs_localio_ctx_alloc); + +void nfs_localio_ctx_free(struct nfs_localio_ctx *localio) +{ + if (localio->nf) + nfs_to.nfsd_file_put(localio->nf); + if (localio->nn) + nfs_to.nfsd_serv_put(localio->nn); + kmem_cache_free(nfs_localio_ctx_cache, localio); +} +EXPORT_SYMBOL_GPL(nfs_localio_ctx_free); + +static int __init nfslocalio_init(void) +{ + refcount_set(&nfs_to_ref, 0); + + nfs_localio_ctx_cache = KMEM_CACHE(nfs_localio_ctx, 0); + if (!nfs_localio_ctx_cache) + return -ENOMEM; + + return 0; +} + +static void __exit nfslocalio_exit(void) +{ + kmem_cache_destroy(nfs_localio_ctx_cache); +} + +module_init(nfslocalio_init); +module_exit(nfslocalio_exit); diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index 2dc72de31f61..1a26f5812578 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -39,6 +39,7 @@ #include <linux/fsnotify.h> #include <linux/seq_file.h> #include <linux/rhashtable.h> +#include <linux/nfslocalio.h> #include "vfs.h" #include "nfsd.h" @@ -345,6 +346,7 @@ nfsd_file_get(struct nfsd_file *nf) return nf; return NULL; } +EXPORT_SYMBOL_GPL(nfsd_file_get); /** * nfsd_file_put - put the reference to a nfsd_file @@ -389,6 +391,20 @@ nfsd_file_put(struct nfsd_file *nf) if (refcount_dec_and_test(&nf->nf_ref)) nfsd_file_free(nf); } +EXPORT_SYMBOL_GPL(nfsd_file_put); + +/** + * nfsd_file_file - get the backing file of an nfsd_file + * @nf: nfsd_file of which to access the backing file. + * + * Return backing file for @nf. + */ +struct file * +nfsd_file_file(struct nfsd_file *nf) +{ + return nf->nf_file; +} +EXPORT_SYMBOL_GPL(nfsd_file_file); static void nfsd_file_dispose_list(struct list_head *dispose) diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h index 26ada78b8c1e..6fbbb2e32e95 100644 --- a/fs/nfsd/filecache.h +++ b/fs/nfsd/filecache.h @@ -56,6 +56,7 @@ int nfsd_file_cache_start_net(struct net *net); void nfsd_file_cache_shutdown_net(struct net *net); void nfsd_file_put(struct nfsd_file *nf); struct nfsd_file *nfsd_file_get(struct nfsd_file *nf); +struct file *nfsd_file_file(struct nfsd_file *nf); void nfsd_file_close_inode_sync(struct inode *inode); void nfsd_file_net_dispose(struct nfsd_net *nn); bool nfsd_file_is_cached(struct inode *inode); diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index c639fbe4d8c2..7b9119b8dd1b 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -19,6 +19,7 @@ #include <linux/sunrpc/svc_xprt.h> #include <linux/lockd/bind.h> #include <linux/nfsacl.h> +#include <linux/nfslocalio.h> #include <linux/seq_file.h> #include <linux/inetdevice.h> #include <net/addrconf.h> @@ -201,6 +202,7 @@ void nfsd_serv_put(struct nfsd_net *nn) { percpu_ref_put(&nn->nfsd_serv_ref); } +EXPORT_SYMBOL_GPL(nfsd_serv_put); static void nfsd_serv_done(struct percpu_ref *ref) { diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h index 9735ae8d3e5e..6cc870122e2a 100644 --- a/include/linux/nfslocalio.h +++ b/include/linux/nfslocalio.h @@ -7,6 +7,7 @@ #include <linux/list.h> #include <linux/uuid.h> +#include <linux/sunrpc/clnt.h> #include <linux/sunrpc/svcauth.h> #include <linux/nfs.h> #include <net/net_namespace.h> @@ -28,4 +29,50 @@ void nfs_uuid_begin(nfs_uuid_t *); void nfs_uuid_end(nfs_uuid_t *); bool nfs_uuid_is_local(const uuid_t *, struct net *, struct auth_domain *); +struct nfsd_file; +struct nfsd_net; + +struct nfs_localio_ctx { + struct nfsd_file *nf; + struct nfsd_net *nn; +}; + +/* localio needs to map filehandle -> struct nfsd_file */ +typedef struct nfs_localio_ctx * +(*nfs_to_nfsd_open_local_fh_t)(struct net *, struct auth_domain *, + struct rpc_clnt *, const struct cred *, + const struct nfs_fh *, const fmode_t); + +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); + +/* localio needs to acquire an nfsd_file */ +typedef struct nfsd_file * (*nfs_to_nfsd_file_get_t)(struct nfsd_file *); +/* localio needs to release an nfsd_file */ +typedef void (*nfs_to_nfsd_file_put_t)(struct nfsd_file *); +/* localio needs to access the nf->nf_file */ +typedef struct file * (*nfs_to_nfsd_file_file_t)(struct nfsd_file *); +/* localio needs to release nn->nfsd_serv */ +typedef void (*nfs_to_nfsd_serv_put_t)(struct nfsd_net *); + +struct nfsd_localio_operations { + nfs_to_nfsd_open_local_fh_t nfsd_open_local_fh; + nfs_to_nfsd_file_get_t nfsd_file_get; + nfs_to_nfsd_file_put_t nfsd_file_put; + nfs_to_nfsd_file_file_t nfsd_file_file; + nfs_to_nfsd_serv_put_t nfsd_serv_put; +} ____cacheline_aligned; + +extern struct nfsd_localio_operations nfs_to; + +typedef void (*init_nfs_to_nfsd_localio_ops_t)(void); +extern void init_nfs_to_nfsd_localio_ops(void); +bool get_nfs_to_nfsd_localio_ops(void); +void put_nfs_to_nfsd_localio_ops(void); + +struct nfs_localio_ctx *nfs_localio_ctx_alloc(void); +void nfs_localio_ctx_free(struct nfs_localio_ctx *); + #endif /* __LINUX_NFSLOCALIO_H */
On Fri, Aug 30, 2024 at 01:01:55AM -0400, Mike Snitzer wrote: > On Fri, Aug 30, 2024 at 02:36:13PM +1000, NeilBrown wrote: > > On Fri, 30 Aug 2024, Jeff Layton wrote: > > > > > > Have a pointer to a struct nfsd_localio_ops or something in the > > > nfs_common module. That's initially set to NULL. Then, have a static > > > structure of that type in nfsd.ko, and have its __init routine set the > > > pointer in nfs_common to point to the right structure. The __exit > > > routine will later set it to NULL. > > > > > > > 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. > > > > > > Neil seems to think that's not necessary: > > > > > > "If nfs/localio holds an auth_domain, then it implicitly holds a > > > reference to the nfsd module and the functions cannot disappear." > > > > On reflection that isn't quite right, but it is the sort of approach > > that I think we need to take. > > There are several things that the NFS client needs to hold one to. > > > > 1/ It needs a reference to the nfsd module (or symbols in the module). > > I think this can be held long term but we need a clear mechanism for > > it to be dropped. > > 2/ It needs a reference to the nfsd_serv which it gets through the > > 'struct net' pointer. I've posted patches to handle that better. > > 3/ It needs a reference to an auth_domain. This can safely be a long > > term reference. It can already be invalidated and the code to free > > it is in sunrpc which nfs already pins. Any delay in freeing it only > > wastes memory (not much), it doesn't impact anything else. > > 4/ It needs a reference to the nfsd_file and/or file. This is currently > > done only while the ref to the nfsd_serv is held, so I think there is > > no problem there. > > > > So possibly we could take a reference to the nfsd module whenever we > > store a net in nfs_uuid. and drop the ref whenever we clear that. > > > > That means we cannot call nfsd_open_local_fh() without first getting a > > ref on the nfsd_serv which my latest code doesn't do. That is easily > > fixed. I'll send a patch for consideration... > > I already implemented 2 different versions today, meant for v15. > > First is a relaxed version of the v14 code (less code, only using > symbol_request on nfsd_open_local_fh. > > Second is much more relaxed, because it leverages your original > assumption that the auth_domain ref sufficient. Corresponding code needed in fs/nfsd/localio.c: static const struct nfsd_localio_operations nfsd_localio_ops = { .nfsd_open_local_fh = nfsd_open_local_fh, .nfsd_file_get = nfsd_file_get, .nfsd_file_put = nfsd_file_put, .nfsd_file_file = nfsd_file_file, .nfsd_serv_put = nfsd_serv_put, }; void nfsd_localio_ops_init(void) { memcpy(&nfs_to, &nfsd_localio_ops, sizeof(nfsd_localio_ops)); } From: Mike Snitzer <snitzer@kernel.org> Date: Wed, 28 Aug 2024 17:04:44 -0500 Subject: [PATCH v15.option2] nfs_common: introduce nfs_localio_ctx struct and interfaces Introduce struct nfs_localio_ctx (which has nfsd_file and nfsd_net members) 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: - Make nfsd_open_local_fh() symbol and other required NFSD symbols available to NFS in a global 'nfs_to' nfsd_localio_operations struct. The next commit will also introduce nfsd_localio_ops_init() that init_nfsd() will call to initialize 'nfs_to'. - 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: NeilBrown <neilb@suse.de> # nfsd_localio_operations Suggested-by: Jeff Layton <jlayton@kernel.org> # nfsd_file_file Signed-off-by: Mike Snitzer <snitzer@kernel.org> --- fs/nfs_common/nfslocalio.c | 62 ++++++++++++++++++++++++++++++++++++++ fs/nfsd/filecache.c | 16 ++++++++++ fs/nfsd/filecache.h | 1 + fs/nfsd/nfssvc.c | 2 ++ include/linux/nfslocalio.h | 43 ++++++++++++++++++++++++++ 5 files changed, 124 insertions(+) diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c index 0b2e17c2068f..175064e37a75 100644 --- a/fs/nfs_common/nfslocalio.c +++ b/fs/nfs_common/nfslocalio.c @@ -72,3 +72,65 @@ 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, + * but cannot be statically linked, because that will make the NFS + * module always depend on the NFSD module. + * + * 'nfs_to' provides NFS access to NFSD functions needed for LOCALIO, + * its lifetime is tightly coupled to the NFSD module and will always + * be available to NFS LOCALIO because any successful client<->server + * LOCALIO handshake results in a reference taken on an auth_domain, + * so NFS implicitly holds a reference to the NFSD module and its + * functions in the 'nfs_to' nfsd_localio_operations cannot disappear. + * + * If the last NFS client using LOCALIO disconnects (and its reference + * on NFSD dropped) then NFSD could be unloaded, resulting in 'nfs_to' + * functions being invalid pointers. But if NFSD isn't loaded then NFS + * will not be able to handshake with NFSD and will have no cause to + * try to call 'nfs_to' function pointers. If/when NFSD is reloaded it + * will reinitialize the 'nfs_to' function pointers and make LOCALIO + * possible. + */ +struct nfsd_localio_operations nfs_to; +EXPORT_SYMBOL_GPL(nfs_to); + +/* + * nfs_localio_ctx cache and alloc/free interfaces. + */ +static struct kmem_cache *nfs_localio_ctx_cache; + +struct nfs_localio_ctx *nfs_localio_ctx_alloc(void) +{ + return kmem_cache_alloc(nfs_localio_ctx_cache, + GFP_KERNEL | __GFP_ZERO); +} +EXPORT_SYMBOL_GPL(nfs_localio_ctx_alloc); + +void nfs_localio_ctx_free(struct nfs_localio_ctx *localio) +{ + if (localio->nf) + nfs_to.nfsd_file_put(localio->nf); + if (localio->nn) + nfs_to.nfsd_serv_put(localio->nn); + kmem_cache_free(nfs_localio_ctx_cache, localio); +} +EXPORT_SYMBOL_GPL(nfs_localio_ctx_free); + +static int __init nfslocalio_init(void) +{ + nfs_localio_ctx_cache = KMEM_CACHE(nfs_localio_ctx, 0); + if (!nfs_localio_ctx_cache) + return -ENOMEM; + + return 0; +} + +static void __exit nfslocalio_exit(void) +{ + kmem_cache_destroy(nfs_localio_ctx_cache); +} + +module_init(nfslocalio_init); +module_exit(nfslocalio_exit); diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index 2dc72de31f61..1a26f5812578 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -39,6 +39,7 @@ #include <linux/fsnotify.h> #include <linux/seq_file.h> #include <linux/rhashtable.h> +#include <linux/nfslocalio.h> #include "vfs.h" #include "nfsd.h" @@ -345,6 +346,7 @@ nfsd_file_get(struct nfsd_file *nf) return nf; return NULL; } +EXPORT_SYMBOL_GPL(nfsd_file_get); /** * nfsd_file_put - put the reference to a nfsd_file @@ -389,6 +391,20 @@ nfsd_file_put(struct nfsd_file *nf) if (refcount_dec_and_test(&nf->nf_ref)) nfsd_file_free(nf); } +EXPORT_SYMBOL_GPL(nfsd_file_put); + +/** + * nfsd_file_file - get the backing file of an nfsd_file + * @nf: nfsd_file of which to access the backing file. + * + * Return backing file for @nf. + */ +struct file * +nfsd_file_file(struct nfsd_file *nf) +{ + return nf->nf_file; +} +EXPORT_SYMBOL_GPL(nfsd_file_file); static void nfsd_file_dispose_list(struct list_head *dispose) diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h index 26ada78b8c1e..6fbbb2e32e95 100644 --- a/fs/nfsd/filecache.h +++ b/fs/nfsd/filecache.h @@ -56,6 +56,7 @@ int nfsd_file_cache_start_net(struct net *net); void nfsd_file_cache_shutdown_net(struct net *net); void nfsd_file_put(struct nfsd_file *nf); struct nfsd_file *nfsd_file_get(struct nfsd_file *nf); +struct file *nfsd_file_file(struct nfsd_file *nf); void nfsd_file_close_inode_sync(struct inode *inode); void nfsd_file_net_dispose(struct nfsd_net *nn); bool nfsd_file_is_cached(struct inode *inode); diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index c639fbe4d8c2..7b9119b8dd1b 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -19,6 +19,7 @@ #include <linux/sunrpc/svc_xprt.h> #include <linux/lockd/bind.h> #include <linux/nfsacl.h> +#include <linux/nfslocalio.h> #include <linux/seq_file.h> #include <linux/inetdevice.h> #include <net/addrconf.h> @@ -201,6 +202,7 @@ void nfsd_serv_put(struct nfsd_net *nn) { percpu_ref_put(&nn->nfsd_serv_ref); } +EXPORT_SYMBOL_GPL(nfsd_serv_put); static void nfsd_serv_done(struct percpu_ref *ref) { diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h index 9735ae8d3e5e..fdb1f278afb6 100644 --- a/include/linux/nfslocalio.h +++ b/include/linux/nfslocalio.h @@ -7,6 +7,7 @@ #include <linux/list.h> #include <linux/uuid.h> +#include <linux/sunrpc/clnt.h> #include <linux/sunrpc/svcauth.h> #include <linux/nfs.h> #include <net/net_namespace.h> @@ -28,4 +29,46 @@ void nfs_uuid_begin(nfs_uuid_t *); void nfs_uuid_end(nfs_uuid_t *); bool nfs_uuid_is_local(const uuid_t *, struct net *, struct auth_domain *); +struct nfsd_file; +struct nfsd_net; + +struct nfs_localio_ctx { + struct nfsd_file *nf; + struct nfsd_net *nn; +}; + +/* localio needs to map filehandle -> struct nfsd_file */ +typedef struct nfs_localio_ctx * +(*nfs_to_nfsd_open_local_fh_t)(struct net *, struct auth_domain *, + struct rpc_clnt *, const struct cred *, + const struct nfs_fh *, const fmode_t); + +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); + +/* localio needs to acquire an nfsd_file */ +typedef struct nfsd_file * (*nfs_to_nfsd_file_get_t)(struct nfsd_file *); +/* localio needs to release an nfsd_file */ +typedef void (*nfs_to_nfsd_file_put_t)(struct nfsd_file *); +/* localio needs to access the nf->nf_file */ +typedef struct file * (*nfs_to_nfsd_file_file_t)(struct nfsd_file *); +/* localio needs to release nn->nfsd_serv */ +typedef void (*nfs_to_nfsd_serv_put_t)(struct nfsd_net *); + +struct nfsd_localio_operations { + nfs_to_nfsd_open_local_fh_t nfsd_open_local_fh; + nfs_to_nfsd_file_get_t nfsd_file_get; + nfs_to_nfsd_file_put_t nfsd_file_put; + nfs_to_nfsd_file_file_t nfsd_file_file; + nfs_to_nfsd_serv_put_t nfsd_serv_put; +} ____cacheline_aligned; + +extern void nfsd_localio_ops_init(void); +extern struct nfsd_localio_operations nfs_to; + +struct nfs_localio_ctx *nfs_localio_ctx_alloc(void); +void nfs_localio_ctx_free(struct nfs_localio_ctx *); + #endif /* __LINUX_NFSLOCALIO_H */
On Fri, 30 Aug 2024, Mike Snitzer wrote: > On Fri, Aug 30, 2024 at 02:36:13PM +1000, NeilBrown wrote: > > On Fri, 30 Aug 2024, Jeff Layton wrote: > > > Have a pointer to a struct nfsd_localio_ops or something in the > > > nfs_common module. That's initially set to NULL. Then, have a static > > > structure of that type in nfsd.ko, and have its __init routine set the > > > pointer in nfs_common to point to the right structure. The __exit > > > routine will later set it to NULL. > > > > > > > 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. > > > > > > Neil seems to think that's not necessary: > > > > > > "If nfs/localio holds an auth_domain, then it implicitly holds a > > > reference to the nfsd module and the functions cannot disappear." > > > > On reflection that isn't quite right, but it is the sort of approach > > that I think we need to take. > > There are several things that the NFS client needs to hold one to. > > > > 1/ It needs a reference to the nfsd module (or symbols in the module). > > I think this can be held long term but we need a clear mechanism for > > it to be dropped. > > 2/ It needs a reference to the nfsd_serv which it gets through the > > 'struct net' pointer. I've posted patches to handle that better. > > 3/ It needs a reference to an auth_domain. This can safely be a long > > term reference. It can already be invalidated and the code to free > > it is in sunrpc which nfs already pins. Any delay in freeing it only > > wastes memory (not much), it doesn't impact anything else. > > 4/ It needs a reference to the nfsd_file and/or file. This is currently > > done only while the ref to the nfsd_serv is held, so I think there is > > no problem there. > > > > So possibly we could take a reference to the nfsd module whenever we > > store a net in nfs_uuid. and drop the ref whenever we clear that. > > > > That means we cannot call nfsd_open_local_fh() without first getting a > > ref on the nfsd_serv which my latest code doesn't do. That is easily > > fixed. I'll send a patch for consideration... > > I already implemented 2 different versions today, meant for v15. > > First is a relaxed version of the v14 code (less code, only using > symbol_request on nfsd_open_local_fh. > > Second is much more relaxed, because it leverages your original > assumption that the auth_domain ref sufficient. > > I'll reply twice to this mail with each each respective patch. Thanks... Unfortunately auth_domain isn't sufficient. This is my version. It should folded back into one or more earlier patches. I think it is simpler. It is against your v15 but with my 6 nfs_uuid patches replaces your equivalents. Thanks, NeilBrown diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c index 55622084d5c2..18b7554ec516 100644 --- a/fs/nfs/localio.c +++ b/fs/nfs/localio.c @@ -235,8 +235,8 @@ nfs_local_open_fh(struct nfs_client *clp, const struct cred *cred, if (mode & ~(FMODE_READ | FMODE_WRITE)) return NULL; - localio = nfs_to.nfsd_open_local_fh(&clp->cl_uuid, - clp->cl_rpcclient, cred, fh, mode); + localio = nfs_open_local_fh(&clp->cl_uuid, + clp->cl_rpcclient, cred, fh, mode); if (IS_ERR(localio)) { status = PTR_ERR(localio); trace_nfs_local_open_fh(fh, mode, status); diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c index 8545ee75f756..cd9733eb3e4f 100644 --- a/fs/nfs_common/nfslocalio.c +++ b/fs/nfs_common/nfslocalio.c @@ -54,8 +54,11 @@ static nfs_uuid_t * nfs_uuid_lookup(const uuid_t *uuid) return NULL; } +struct module *nfsd_mod; + void nfs_uuid_is_local(const uuid_t *uuid, struct list_head *list, - struct net *net, struct auth_domain *dom) + struct net *net, struct auth_domain *dom, + struct module *mod) { nfs_uuid_t *nfs_uuid; @@ -70,6 +73,9 @@ void nfs_uuid_is_local(const uuid_t *uuid, struct list_head *list, */ list_move(&nfs_uuid->list, list); nfs_uuid->net = net; + + __module_get(mod); + nfsd_mod = mod; } spin_unlock(&nfs_uuid_lock); } @@ -77,8 +83,10 @@ EXPORT_SYMBOL_GPL(nfs_uuid_is_local); static void nfs_uuid_put_locked(nfs_uuid_t *nfs_uuid) { - if (nfs_uuid->net) + if (nfs_uuid->net) { put_net(nfs_uuid->net); + module_put(nfsd_mod); + } nfs_uuid->net = NULL; if (nfs_uuid->dom) auth_domain_put(nfs_uuid->dom); @@ -107,6 +115,26 @@ void nfs_uuid_invalidate_one_client(nfs_uuid_t *nfs_uuid) } EXPORT_SYMBOL_GPL(nfs_uuid_invalidate_one_client); +struct nfs_localio_ctx *nfs_open_local_fh(nfs_uuid_t *uuid, + struct rpc_clnt *rpc_clnt, const struct cred *cred, + const struct nfs_fh *nfs_fh, const fmode_t fmode) +{ + struct nfs_localio_ctx *localio; + + rcu_read_lock(); + if (!READ_ONCE(uuid->net)) { + rcu_read_unlock(); + return ERR_PTR(-ENXIO); + } + localio = nfs_to.nfsd_open_local_fh(uuid, rpc_clnt, cred, + nfs_fh, fmode); + rcu_read_unlock(); + if (IS_ERR(localio)) + nfs_to.nfsd_serv_put(localio->nn); + return localio; +} +EXPORT_SYMBOL_GPL(nfs_open_local_fh); + /* * 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 @@ -135,7 +163,8 @@ static void put_##NFSD_SYMBOL(void) \ /* The nfs localio code needs to call into nfsd to map filehandle -> struct nfsd_file */ extern struct nfs_localio_ctx * nfsd_open_local_fh(nfs_uuid_t *, struct rpc_clnt *, - const struct cred *, const struct nfs_fh *, const fmode_t); + const struct cred *, const struct nfs_fh *, const fmode_t) + __must_hold(rcu); DEFINE_NFS_TO_NFSD_SYMBOL(nfsd_open_local_fh); /* The nfs localio code needs to call into nfsd to acquire the nfsd_file */ diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c index 491bf5017d34..d50e54406914 100644 --- a/fs/nfsd/localio.c +++ b/fs/nfsd/localio.c @@ -45,6 +45,7 @@ struct nfs_localio_ctx * nfsd_open_local_fh(nfs_uuid_t *uuid, struct rpc_clnt *rpc_clnt, const struct cred *cred, const struct nfs_fh *nfs_fh, const fmode_t fmode) + __must_hold(rcu) { int mayflags = NFSD_MAY_LOCALIO; int status = 0; @@ -58,10 +59,6 @@ nfsd_open_local_fh(nfs_uuid_t *uuid, if (nfs_fh->size > NFS4_FHSIZE) return ERR_PTR(-EINVAL); - localio = nfs_localio_ctx_alloc(); - if (!localio) - return ERR_PTR(-ENOMEM); - /* * Not running in nfsd context, so must safely get reference on nfsd_serv. * But the server may already be shutting down, if so disallow new localio. @@ -69,17 +66,22 @@ nfsd_open_local_fh(nfs_uuid_t *uuid, * uuid->net is not NULL, then nfsd_serv_try_get() is safe and if that succeeds * we will have an implied reference to the net. */ - rcu_read_lock(); net = READ_ONCE(uuid->net); if (net) nn = net_generic(net, nfsd_net_id); - if (unlikely(!nn || !nfsd_serv_try_get(nn))) { - rcu_read_unlock(); - status = -ENXIO; - goto out_nfsd_serv; - } + if (unlikely(!nn || !nfsd_serv_try_get(nn))) + return -ENXIO; + + /* Drop the rcu lock for alloc and nfsd_file_acquire_local() */ rcu_read_unlock(); + localio = nfs_localio_ctx_alloc(); + if (!localio) { + localio = ERR_PTR(-ENOMEM); + nfsd_serv_put(nn); + goto out_localio; + } + /* nfs_fh -> svc_fh */ fh_init(&fh, NFS4_FHSIZE); fh.fh_handle.fh_size = nfs_fh->size; @@ -104,11 +106,13 @@ nfsd_open_local_fh(nfs_uuid_t *uuid, fh_put(&fh); if (rq_cred.cr_group_info) put_group_info(rq_cred.cr_group_info); -out_nfsd_serv: + if (status) { nfs_localio_ctx_free(localio); - return ERR_PTR(status); + localio = ERR_PTR(status); } +out_localio: + rcu_read_lock(); return localio; } EXPORT_SYMBOL_GPL(nfsd_open_local_fh); @@ -136,7 +140,7 @@ static __be32 localio_proc_uuid_is_local(struct svc_rqst *rqstp) struct nfsd_net *nn = net_generic(net, nfsd_net_id); nfs_uuid_is_local(&argp->uuid, &nn->local_clients, - net, rqstp->rq_client); + net, rqstp->rq_client, THIS_MODULE); return rpc_success; } diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h index 2ecceb8b9d3d..c73633120997 100644 --- a/fs/nfsd/vfs.h +++ b/fs/nfsd/vfs.h @@ -164,7 +164,8 @@ void nfsd_filp_close(struct file *fp); struct nfs_localio_ctx * nfsd_open_local_fh(nfs_uuid_t *, struct rpc_clnt *, const struct cred *, - const struct nfs_fh *, const fmode_t); + const struct nfs_fh *, const fmode_t) + __must_hold(rcu); static inline int fh_want_write(struct svc_fh *fh) { diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h index e196f716a2f5..303e82e75b9e 100644 --- a/include/linux/nfslocalio.h +++ b/include/linux/nfslocalio.h @@ -29,7 +29,7 @@ typedef struct { void nfs_uuid_begin(nfs_uuid_t *); void nfs_uuid_end(nfs_uuid_t *); void nfs_uuid_is_local(const uuid_t *, struct list_head *, - struct net *, struct auth_domain *); + struct net *, struct auth_domain *, struct module *); void nfs_uuid_invalidate_clients(struct list_head *list); void nfs_uuid_invalidate_one_client(nfs_uuid_t *nfs_uuid); @@ -69,4 +69,8 @@ void put_nfs_to_nfsd_symbols(void); struct nfs_localio_ctx *nfs_localio_ctx_alloc(void); void nfs_localio_ctx_free(struct nfs_localio_ctx *); +struct nfs_localio_ctx *nfs_open_local_fh(nfs_uuid_t *uuid, + struct rpc_clnt *rpc_clnt, const struct cred *cred, + const struct nfs_fh *nfs_fh, const fmode_t fmode); + #endif /* __LINUX_NFSLOCALIO_H */
On Thu, 29 Aug 2024, Mike Snitzer wrote: > + > +struct nfs_localio_ctx { > + struct nfsd_file *nf; > + struct nfsd_net *nn; > +}; struct nfsd_file contains "struct net *nf_net" which is initialised early. So this structure is redundant. Instead of exporting nfsd_file_put() to nfs-localio, export nfsd_file_local_put() (or whatever) which both does the nfs_serv_put() and the nfsd_file_put(). NeilBrown
On Fri, Aug 30, 2024 at 03:46:31PM +1000, NeilBrown wrote: > On Thu, 29 Aug 2024, Mike Snitzer wrote: > > + > > +struct nfs_localio_ctx { > > + struct nfsd_file *nf; > > + struct nfsd_net *nn; > > +}; > > struct nfsd_file contains "struct net *nf_net" which is initialised > early. > So this structure is redundant. Oof, unwinding returning nfs_localio_ctx and going back to nfsd_file it is. > Instead of exporting nfsd_file_put() to nfs-localio, export > nfsd_file_local_put() (or whatever) which both does the nfs_serv_put() > and the nfsd_file_put(). OK, no more.. I have to catch up! ;)
On Fri, Aug 30, 2024 at 03:34:02PM +1000, NeilBrown wrote: > On Fri, 30 Aug 2024, Mike Snitzer wrote: > > On Fri, Aug 30, 2024 at 02:36:13PM +1000, NeilBrown wrote: > > > On Fri, 30 Aug 2024, Jeff Layton wrote: > > > > > Have a pointer to a struct nfsd_localio_ops or something in the > > > > nfs_common module. That's initially set to NULL. Then, have a static > > > > structure of that type in nfsd.ko, and have its __init routine set the > > > > pointer in nfs_common to point to the right structure. The __exit > > > > routine will later set it to NULL. > > > > > > > > > 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. > > > > > > > > Neil seems to think that's not necessary: > > > > > > > > "If nfs/localio holds an auth_domain, then it implicitly holds a > > > > reference to the nfsd module and the functions cannot disappear." > > > > > > On reflection that isn't quite right, but it is the sort of approach > > > that I think we need to take. > > > There are several things that the NFS client needs to hold one to. > > > > > > 1/ It needs a reference to the nfsd module (or symbols in the module). > > > I think this can be held long term but we need a clear mechanism for > > > it to be dropped. > > > 2/ It needs a reference to the nfsd_serv which it gets through the > > > 'struct net' pointer. I've posted patches to handle that better. > > > 3/ It needs a reference to an auth_domain. This can safely be a long > > > term reference. It can already be invalidated and the code to free > > > it is in sunrpc which nfs already pins. Any delay in freeing it only > > > wastes memory (not much), it doesn't impact anything else. > > > 4/ It needs a reference to the nfsd_file and/or file. This is currently > > > done only while the ref to the nfsd_serv is held, so I think there is > > > no problem there. > > > > > > So possibly we could take a reference to the nfsd module whenever we > > > store a net in nfs_uuid. and drop the ref whenever we clear that. > > > > > > That means we cannot call nfsd_open_local_fh() without first getting a > > > ref on the nfsd_serv which my latest code doesn't do. That is easily > > > fixed. I'll send a patch for consideration... > > > > I already implemented 2 different versions today, meant for v15. > > > > First is a relaxed version of the v14 code (less code, only using > > symbol_request on nfsd_open_local_fh. > > > > Second is much more relaxed, because it leverages your original > > assumption that the auth_domain ref sufficient. > > > > I'll reply twice to this mail with each each respective patch. > > Thanks... Unfortunately auth_domain isn't sufficient. > > This is my version. It should folded back into one or more earlier > patches. I think it is simpler. > > It is against your v15 but with my 6 nfs_uuid patches replaces your > equivalents. > > Thanks, > NeilBrown Looks good! But I noticed you are still using the v14 DEFINE_NFS_TO_NFSD_SYMBOL (just implies that nfs_to is getting setup using symbol_request) so your refcounting via __module_get is redundant. But I see your intent, and I can combine what you provided below with the v15.option2 that I emailed earlier (lean on your __module_get rather than the insufficnet auth_domain ref). Thanks. > > diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c > index 55622084d5c2..18b7554ec516 100644 > --- a/fs/nfs/localio.c > +++ b/fs/nfs/localio.c > @@ -235,8 +235,8 @@ nfs_local_open_fh(struct nfs_client *clp, const struct cred *cred, > if (mode & ~(FMODE_READ | FMODE_WRITE)) > return NULL; > > - localio = nfs_to.nfsd_open_local_fh(&clp->cl_uuid, > - clp->cl_rpcclient, cred, fh, mode); > + localio = nfs_open_local_fh(&clp->cl_uuid, > + clp->cl_rpcclient, cred, fh, mode); > if (IS_ERR(localio)) { > status = PTR_ERR(localio); > trace_nfs_local_open_fh(fh, mode, status); > diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c > index 8545ee75f756..cd9733eb3e4f 100644 > --- a/fs/nfs_common/nfslocalio.c > +++ b/fs/nfs_common/nfslocalio.c > @@ -54,8 +54,11 @@ static nfs_uuid_t * nfs_uuid_lookup(const uuid_t *uuid) > return NULL; > } > > +struct module *nfsd_mod; > + > void nfs_uuid_is_local(const uuid_t *uuid, struct list_head *list, > - struct net *net, struct auth_domain *dom) > + struct net *net, struct auth_domain *dom, > + struct module *mod) > { > nfs_uuid_t *nfs_uuid; > > @@ -70,6 +73,9 @@ void nfs_uuid_is_local(const uuid_t *uuid, struct list_head *list, > */ > list_move(&nfs_uuid->list, list); > nfs_uuid->net = net; > + > + __module_get(mod); > + nfsd_mod = mod; > } > spin_unlock(&nfs_uuid_lock); > } > @@ -77,8 +83,10 @@ EXPORT_SYMBOL_GPL(nfs_uuid_is_local); > > static void nfs_uuid_put_locked(nfs_uuid_t *nfs_uuid) > { > - if (nfs_uuid->net) > + if (nfs_uuid->net) { > put_net(nfs_uuid->net); > + module_put(nfsd_mod); > + } > nfs_uuid->net = NULL; > if (nfs_uuid->dom) > auth_domain_put(nfs_uuid->dom); > @@ -107,6 +115,26 @@ void nfs_uuid_invalidate_one_client(nfs_uuid_t *nfs_uuid) > } > EXPORT_SYMBOL_GPL(nfs_uuid_invalidate_one_client); > > +struct nfs_localio_ctx *nfs_open_local_fh(nfs_uuid_t *uuid, > + struct rpc_clnt *rpc_clnt, const struct cred *cred, > + const struct nfs_fh *nfs_fh, const fmode_t fmode) > +{ > + struct nfs_localio_ctx *localio; > + > + rcu_read_lock(); > + if (!READ_ONCE(uuid->net)) { > + rcu_read_unlock(); > + return ERR_PTR(-ENXIO); > + } > + localio = nfs_to.nfsd_open_local_fh(uuid, rpc_clnt, cred, > + nfs_fh, fmode); > + rcu_read_unlock(); > + if (IS_ERR(localio)) > + nfs_to.nfsd_serv_put(localio->nn); > + return localio; > +} > +EXPORT_SYMBOL_GPL(nfs_open_local_fh); > + > /* > * 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 > @@ -135,7 +163,8 @@ static void put_##NFSD_SYMBOL(void) \ > /* The nfs localio code needs to call into nfsd to map filehandle -> struct nfsd_file */ > extern struct nfs_localio_ctx * > nfsd_open_local_fh(nfs_uuid_t *, struct rpc_clnt *, > - const struct cred *, const struct nfs_fh *, const fmode_t); > + const struct cred *, const struct nfs_fh *, const fmode_t) > + __must_hold(rcu); > DEFINE_NFS_TO_NFSD_SYMBOL(nfsd_open_local_fh); > > /* The nfs localio code needs to call into nfsd to acquire the nfsd_file */ > diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c > index 491bf5017d34..d50e54406914 100644 > --- a/fs/nfsd/localio.c > +++ b/fs/nfsd/localio.c > @@ -45,6 +45,7 @@ struct nfs_localio_ctx * > nfsd_open_local_fh(nfs_uuid_t *uuid, > struct rpc_clnt *rpc_clnt, const struct cred *cred, > const struct nfs_fh *nfs_fh, const fmode_t fmode) > + __must_hold(rcu) > { > int mayflags = NFSD_MAY_LOCALIO; > int status = 0; > @@ -58,10 +59,6 @@ nfsd_open_local_fh(nfs_uuid_t *uuid, > if (nfs_fh->size > NFS4_FHSIZE) > return ERR_PTR(-EINVAL); > > - localio = nfs_localio_ctx_alloc(); > - if (!localio) > - return ERR_PTR(-ENOMEM); > - > /* > * Not running in nfsd context, so must safely get reference on nfsd_serv. > * But the server may already be shutting down, if so disallow new localio. > @@ -69,17 +66,22 @@ nfsd_open_local_fh(nfs_uuid_t *uuid, > * uuid->net is not NULL, then nfsd_serv_try_get() is safe and if that succeeds > * we will have an implied reference to the net. > */ > - rcu_read_lock(); > net = READ_ONCE(uuid->net); > if (net) > nn = net_generic(net, nfsd_net_id); > - if (unlikely(!nn || !nfsd_serv_try_get(nn))) { > - rcu_read_unlock(); > - status = -ENXIO; > - goto out_nfsd_serv; > - } > + if (unlikely(!nn || !nfsd_serv_try_get(nn))) > + return -ENXIO; > + > + /* Drop the rcu lock for alloc and nfsd_file_acquire_local() */ > rcu_read_unlock(); > > + localio = nfs_localio_ctx_alloc(); > + if (!localio) { > + localio = ERR_PTR(-ENOMEM); > + nfsd_serv_put(nn); > + goto out_localio; > + } > + > /* nfs_fh -> svc_fh */ > fh_init(&fh, NFS4_FHSIZE); > fh.fh_handle.fh_size = nfs_fh->size; > @@ -104,11 +106,13 @@ nfsd_open_local_fh(nfs_uuid_t *uuid, > fh_put(&fh); > if (rq_cred.cr_group_info) > put_group_info(rq_cred.cr_group_info); > -out_nfsd_serv: > + > if (status) { > nfs_localio_ctx_free(localio); > - return ERR_PTR(status); > + localio = ERR_PTR(status); > } > +out_localio: > + rcu_read_lock(); > return localio; > } > EXPORT_SYMBOL_GPL(nfsd_open_local_fh); > @@ -136,7 +140,7 @@ static __be32 localio_proc_uuid_is_local(struct svc_rqst *rqstp) > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > > nfs_uuid_is_local(&argp->uuid, &nn->local_clients, > - net, rqstp->rq_client); > + net, rqstp->rq_client, THIS_MODULE); > > return rpc_success; > } > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index 2ecceb8b9d3d..c73633120997 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -164,7 +164,8 @@ void nfsd_filp_close(struct file *fp); > struct nfs_localio_ctx * > nfsd_open_local_fh(nfs_uuid_t *, > struct rpc_clnt *, const struct cred *, > - const struct nfs_fh *, const fmode_t); > + const struct nfs_fh *, const fmode_t) > + __must_hold(rcu); > > static inline int fh_want_write(struct svc_fh *fh) > { > diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h > index e196f716a2f5..303e82e75b9e 100644 > --- a/include/linux/nfslocalio.h > +++ b/include/linux/nfslocalio.h > @@ -29,7 +29,7 @@ typedef struct { > void nfs_uuid_begin(nfs_uuid_t *); > void nfs_uuid_end(nfs_uuid_t *); > void nfs_uuid_is_local(const uuid_t *, struct list_head *, > - struct net *, struct auth_domain *); > + struct net *, struct auth_domain *, struct module *); > void nfs_uuid_invalidate_clients(struct list_head *list); > void nfs_uuid_invalidate_one_client(nfs_uuid_t *nfs_uuid); > > @@ -69,4 +69,8 @@ void put_nfs_to_nfsd_symbols(void); > struct nfs_localio_ctx *nfs_localio_ctx_alloc(void); > void nfs_localio_ctx_free(struct nfs_localio_ctx *); > > +struct nfs_localio_ctx *nfs_open_local_fh(nfs_uuid_t *uuid, > + struct rpc_clnt *rpc_clnt, const struct cred *cred, > + const struct nfs_fh *nfs_fh, const fmode_t fmode); > + > #endif /* __LINUX_NFSLOCALIO_H */ >
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 + +static struct kmem_cache *nfs_localio_ctx_cache; + +struct nfs_localio_ctx *nfs_localio_ctx_alloc(void) +{ + return kmem_cache_alloc(nfs_localio_ctx_cache, + GFP_KERNEL | __GFP_ZERO); +} +EXPORT_SYMBOL_GPL(nfs_localio_ctx_alloc); + +void nfs_localio_ctx_free(struct nfs_localio_ctx *localio) +{ + if (localio->nf) + nfs_to.nfsd_file_put(localio->nf); + if (localio->nn) + nfs_to.nfsd_serv_put(localio->nn); + kmem_cache_free(nfs_localio_ctx_cache, localio); +} +EXPORT_SYMBOL_GPL(nfs_localio_ctx_free); + +bool get_nfs_to_nfsd_symbols(void) +{ + spin_lock(&nfs_to_nfsd_lock); + + /* Only get symbols on first reference */ + if (refcount_read(&nfs_to.ref) == 0) + refcount_set(&nfs_to.ref, 1); + else { + refcount_inc(&nfs_to.ref); + spin_unlock(&nfs_to_nfsd_lock); + return true; + } + + nfs_to.nfsd_open_local_fh = get_nfsd_open_local_fh(); + if (!nfs_to.nfsd_open_local_fh) + goto out_nfsd_open_local_fh; + + nfs_to.nfsd_file_get = get_nfsd_file_get(); + if (!nfs_to.nfsd_file_get) + goto out_nfsd_file_get; + + nfs_to.nfsd_file_put = get_nfsd_file_put(); + if (!nfs_to.nfsd_file_put) + goto out_nfsd_file_put; + + nfs_to.nfsd_file_file = get_nfsd_file_file(); + if (!nfs_to.nfsd_file_file) + goto out_nfsd_file_file; + + nfs_to.nfsd_serv_put = get_nfsd_serv_put(); + if (!nfs_to.nfsd_serv_put) + goto out_nfsd_serv_put; + + spin_unlock(&nfs_to_nfsd_lock); + return true; + +out_nfsd_serv_put: + put_nfsd_file_file(); +out_nfsd_file_file: + put_nfsd_file_put(); +out_nfsd_file_put: + put_nfsd_file_get(); +out_nfsd_file_get: + put_nfsd_open_local_fh(); +out_nfsd_open_local_fh: + spin_unlock(&nfs_to_nfsd_lock); + return false; +} +EXPORT_SYMBOL_GPL(get_nfs_to_nfsd_symbols); + +void put_nfs_to_nfsd_symbols(void) +{ + spin_lock(&nfs_to_nfsd_lock); + + if (!refcount_dec_and_test(&nfs_to.ref)) + goto out; + + put_nfsd_open_local_fh(); + put_nfsd_file_get(); + put_nfsd_file_put(); + put_nfsd_file_file(); + put_nfsd_serv_put(); +out: + spin_unlock(&nfs_to_nfsd_lock); +} +EXPORT_SYMBOL_GPL(put_nfs_to_nfsd_symbols); + +static int __init nfslocalio_init(void) +{ + refcount_set(&nfs_to.ref, 0); + + nfs_to.nfsd_open_local_fh = NULL; + nfs_to.nfsd_file_get = NULL; + nfs_to.nfsd_file_put = NULL; + nfs_to.nfsd_file_file = NULL; + nfs_to.nfsd_serv_put = NULL; + + nfs_localio_ctx_cache = KMEM_CACHE(nfs_localio_ctx, 0); + if (!nfs_localio_ctx_cache) + return -ENOMEM; + + return 0; +} + +static void __exit nfslocalio_exit(void) +{ + kmem_cache_destroy(nfs_localio_ctx_cache); +} + +module_init(nfslocalio_init); +module_exit(nfslocalio_exit); diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index 2dc72de31f61..a83d469bca6b 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -39,6 +39,7 @@ #include <linux/fsnotify.h> #include <linux/seq_file.h> #include <linux/rhashtable.h> +#include <linux/nfslocalio.h> #include "vfs.h" #include "nfsd.h" @@ -345,6 +346,10 @@ nfsd_file_get(struct nfsd_file *nf) return nf; return NULL; } +EXPORT_SYMBOL_GPL(nfsd_file_get); + +/* Compile time type checking, not used by anything */ +static nfs_to_nfsd_file_get_t __maybe_unused nfsd_file_get_typecheck = nfsd_file_get; /** * nfsd_file_put - put the reference to a nfsd_file @@ -389,6 +394,26 @@ nfsd_file_put(struct nfsd_file *nf) if (refcount_dec_and_test(&nf->nf_ref)) nfsd_file_free(nf); } +EXPORT_SYMBOL_GPL(nfsd_file_put); + +/* Compile time type checking, not used by anything */ +static nfs_to_nfsd_file_put_t __maybe_unused nfsd_file_put_typecheck = nfsd_file_put; + +/** + * nfsd_file_file - get the backing file of an nfsd_file + * @nf: nfsd_file of which to access the backing file. + * + * Return backing file for @nf. + */ +struct file * +nfsd_file_file(struct nfsd_file *nf) +{ + return nf->nf_file; +} +EXPORT_SYMBOL_GPL(nfsd_file_file); + +/* Compile time type checking, not used by anything */ +static nfs_to_nfsd_file_file_t __maybe_unused nfsd_file_file_typecheck = nfsd_file_file; static void nfsd_file_dispose_list(struct list_head *dispose) diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h index 26ada78b8c1e..6fbbb2e32e95 100644 --- a/fs/nfsd/filecache.h +++ b/fs/nfsd/filecache.h @@ -56,6 +56,7 @@ int nfsd_file_cache_start_net(struct net *net); void nfsd_file_cache_shutdown_net(struct net *net); void nfsd_file_put(struct nfsd_file *nf); struct nfsd_file *nfsd_file_get(struct nfsd_file *nf); +struct file *nfsd_file_file(struct nfsd_file *nf); void nfsd_file_close_inode_sync(struct inode *inode); void nfsd_file_net_dispose(struct nfsd_net *nn); bool nfsd_file_is_cached(struct inode *inode); diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index c639fbe4d8c2..13c69aa40d1c 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -19,6 +19,7 @@ #include <linux/sunrpc/svc_xprt.h> #include <linux/lockd/bind.h> #include <linux/nfsacl.h> +#include <linux/nfslocalio.h> #include <linux/seq_file.h> #include <linux/inetdevice.h> #include <net/addrconf.h> @@ -201,6 +202,10 @@ void nfsd_serv_put(struct nfsd_net *nn) { percpu_ref_put(&nn->nfsd_serv_ref); } +EXPORT_SYMBOL_GPL(nfsd_serv_put); + +/* Compile time type checking, not used by anything */ +static nfs_to_nfsd_serv_put_t __maybe_unused nfsd_serv_put_typecheck = nfsd_serv_put; static void nfsd_serv_done(struct percpu_ref *ref) { diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h index 9735ae8d3e5e..68f5b39f1940 100644 --- a/include/linux/nfslocalio.h +++ b/include/linux/nfslocalio.h @@ -7,6 +7,8 @@ #include <linux/list.h> #include <linux/uuid.h> +#include <linux/refcount.h> +#include <linux/sunrpc/clnt.h> #include <linux/sunrpc/svcauth.h> #include <linux/nfs.h> #include <net/net_namespace.h> @@ -28,4 +30,40 @@ void nfs_uuid_begin(nfs_uuid_t *); void nfs_uuid_end(nfs_uuid_t *); bool nfs_uuid_is_local(const uuid_t *, struct net *, struct auth_domain *); +struct nfsd_file; +struct nfsd_net; + +struct nfs_localio_ctx { + struct nfsd_file *nf; + struct nfsd_net *nn; +}; + +typedef struct nfs_localio_ctx * +(*nfs_to_nfsd_open_local_fh_t)(struct net *, struct auth_domain *, + struct rpc_clnt *, const struct cred *, + const struct nfs_fh *, const fmode_t); +typedef struct nfsd_file * (*nfs_to_nfsd_file_get_t)(struct nfsd_file *); +typedef void (*nfs_to_nfsd_file_put_t)(struct nfsd_file *); +typedef struct file * (*nfs_to_nfsd_file_file_t)(struct nfsd_file *); +typedef unsigned int (*nfs_to_nfsd_net_id_value_t)(void); +typedef void (*nfs_to_nfsd_serv_put_t)(struct nfsd_net *); + +typedef struct { + refcount_t ref; + nfs_to_nfsd_open_local_fh_t nfsd_open_local_fh; + nfs_to_nfsd_file_get_t nfsd_file_get; + nfs_to_nfsd_file_put_t nfsd_file_put; + nfs_to_nfsd_file_file_t nfsd_file_file; + nfs_to_nfsd_net_id_value_t nfsd_net_id_value; + nfs_to_nfsd_serv_put_t nfsd_serv_put; +} nfs_to_nfsd_t; + +extern nfs_to_nfsd_t nfs_to; + +bool get_nfs_to_nfsd_symbols(void); +void put_nfs_to_nfsd_symbols(void); + +struct nfs_localio_ctx *nfs_localio_ctx_alloc(void); +void nfs_localio_ctx_free(struct nfs_localio_ctx *); + #endif /* __LINUX_NFSLOCALIO_H */
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(+)