On Tue, 2024-06-18 at 16:19 -0400, Mike Snitzer wrote:
> Get nfsd_open_local_fh and store it in rpc_client during client
> creation, put the symbol during nfs_local_disable -- which is also
> called during client destruction.
>
> Eliminates the need for nfs_local_open_ctx and extra locking and
> refcounting work in fs/nfs/localio.c
>
> Also makes it so the reference to the nfsd_open_local_fh symbol is
> managed by the nfs_common module instead of the nfs client modules.
>
> Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> ---
> fs/nfs/client.c | 1 +
> fs/nfs/inode.c | 1 -
> fs/nfs/internal.h | 18 +++++---
> fs/nfs/localio.c | 86 +++-----------------------------------
> fs/nfs_common/nfslocalio.c | 26 ++++++++++++
> include/linux/nfs.h | 4 --
> include/linux/nfs_fs_sb.h | 2 +
> include/linux/nfslocalio.h | 8 ++++
> 8 files changed, 54 insertions(+), 92 deletions(-)
>
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 7044b8b3b332..cbabcdf3d785 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -171,6 +171,7 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
>
> INIT_LIST_HEAD(&clp->cl_superblocks);
> clp->cl_rpcclient = clp->cl_rpcclient_localio = ERR_PTR(-EINVAL);
> + clp->nfsd_open_local_fh = NULL;
>
> clp->cl_flags = cl_init->init_flags;
> clp->cl_proto = cl_init->proto;
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 4f88b860494f..f9923cbf6058 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -2499,7 +2499,6 @@ static int __init init_nfs_fs(void)
> if (err)
> goto out1;
>
> - nfs_local_init();
> err = register_nfs_fs();
> if (err)
> goto out0;
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index fb2fb59e7ed0..d30a2e63063c 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -464,15 +464,22 @@ nfs_init_localioclient(struct nfs_client *clp,
> goto out;
> clp->cl_rpcclient_localio = rpc_bind_new_program(clp->cl_rpcclient,
> program, vers);
> + if (IS_ERR(clp->cl_rpcclient_localio))
> + goto out;
> + /* No errors! Assume that localio is supported */
> + clp->nfsd_open_local_fh = get_nfsd_open_local_fh();
> + if (!clp->nfsd_open_local_fh) {
> + rpc_shutdown_client(clp->cl_rpcclient_localio);
> + clp->cl_rpcclient_localio = ERR_PTR(-EINVAL);
> + }
> out:
> - dfprintk_rcu(CLIENT, "%s: server (%s) %s NFSv%u LOCALIO\n", __func__,
> - rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR),
> - (IS_ERR(clp->cl_rpcclient_localio) ?
> - "does not support" : "supports"), vers);
> + dfprintk_rcu(CLIENT, "%s: server (%s) %s NFSv%u LOCALIO, nfsd_open_local_fh is %s.\n",
> + __func__, rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR),
> + (IS_ERR(clp->cl_rpcclient_localio) ? "does not support" : "supports"), vers,
> + (clp->nfsd_open_local_fh ? "set" : "not set"));
> }
>
> /* localio.c */
> -extern void nfs_local_init(void);
> extern void nfs_local_disable(struct nfs_client *);
> extern void nfs_local_probe(struct nfs_client *);
> extern struct file *nfs_local_open_fh(struct nfs_client *, const struct cred *,
> @@ -489,7 +496,6 @@ extern int nfs_local_commit(struct file *, struct nfs_commit_data *,
> extern bool nfs_server_is_local(const struct nfs_client *clp);
>
> #else
> -static inline void nfs_local_init(void) {}
> static inline void nfs_local_disable(struct nfs_client *clp) {}
> static inline void nfs_local_probe(struct nfs_client *clp) {}
> static inline struct file *nfs_local_open_fh(struct nfs_client *clp,
> diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> index 54c41933173c..ddd17549812e 100644
> --- a/fs/nfs/localio.c
> +++ b/fs/nfs/localio.c
> @@ -29,26 +29,6 @@
>
> #define NFSDBG_FACILITY NFSDBG_VFS
>
> -extern int nfsd_open_local_fh(struct rpc_clnt *rpc_clnt,
> - const struct cred *cred,
> - const struct nfs_fh *nfs_fh, const fmode_t fmode,
> - struct file **pfilp);
> -/*
> - * The localio code needs to call into nfsd to do the filehandle -> struct path
> - * mapping, 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. This way the nfs module
> - * will only hold a reference on nfsd when it's actually in use. This also
> - * allows some sanity checking, like giving up on localio if nfsd isn't loaded.
> - */
> -
> -struct nfs_local_open_ctx {
> - spinlock_t lock;
> - nfs_to_nfsd_open_t open_f;
> - atomic_t refcount;
> -};
> -
> struct nfs_local_kiocb {
> struct kiocb kiocb;
> struct bio_vec *bvec;
> @@ -135,8 +115,6 @@ nfs4errno(int errno)
> return NFS4ERR_SERVERFAULT;
> }
>
> -static struct nfs_local_open_ctx __local_open_ctx __read_mostly;
> -
> static bool localio_enabled __read_mostly = true;
> module_param(localio_enabled, bool, 0644);
>
> @@ -151,65 +129,12 @@ bool nfs_server_is_local(const struct nfs_client *clp)
> }
> EXPORT_SYMBOL_GPL(nfs_server_is_local);
>
> -void
> -nfs_local_init(void)
> -{
> - struct nfs_local_open_ctx *ctx = &__local_open_ctx;
> -
> - ctx->open_f = NULL;
> - spin_lock_init(&ctx->lock);
> - atomic_set(&ctx->refcount, 0);
> -}
> -
> -static bool
> -nfs_local_get_lookup_ctx(void)
> -{
> - struct nfs_local_open_ctx *ctx = &__local_open_ctx;
> - nfs_to_nfsd_open_t fn = NULL;
> -
> - spin_lock(&ctx->lock);
> - if (ctx->open_f == NULL) {
> - spin_unlock(&ctx->lock);
> -
> - fn = symbol_request(nfsd_open_local_fh);
> - if (!fn)
> - return false;
> -
> - spin_lock(&ctx->lock);
> - /* catch race */
> - if (ctx->open_f == NULL) {
> - ctx->open_f = fn;
> - fn = NULL;
> - }
> - }
> - atomic_inc(&ctx->refcount);
> - spin_unlock(&ctx->lock);
> - if (fn)
> - symbol_put(nfsd_open_local_fh);
> - return true;
> -}
> -
> -static void
> -nfs_local_put_lookup_ctx(void)
> -{
> - struct nfs_local_open_ctx *ctx = &__local_open_ctx;
> - nfs_to_nfsd_open_t fn;
> -
> - if (atomic_dec_and_lock(&ctx->refcount, &ctx->lock)) {
> - fn = ctx->open_f;
> - ctx->open_f = NULL;
> - spin_unlock(&ctx->lock);
> - if (fn)
> - symbol_put(nfsd_open_local_fh);
> - }
> -}
> -
It seems like the new nfs_common infrastructure should be added earlier
in the series so you don't need to rip out the code above.
> /*
> * nfs_local_enable - attempt to enable local i/o for an nfs_client
> */
> static void nfs_local_enable(struct nfs_client *clp)
> {
> - if (nfs_local_get_lookup_ctx()) {
> + if (READ_ONCE(clp->nfsd_open_local_fh)) {
> set_bit(NFS_CS_LOCAL_IO, &clp->cl_flags);
> trace_nfs_local_enable(clp);
> }
> @@ -218,12 +143,12 @@ static void nfs_local_enable(struct nfs_client *clp)
> /*
> * nfs_local_disable - disable local i/o for an nfs_client
> */
> -void
> -nfs_local_disable(struct nfs_client *clp)
> +void nfs_local_disable(struct nfs_client *clp)
> {
> if (test_and_clear_bit(NFS_CS_LOCAL_IO, &clp->cl_flags)) {
> trace_nfs_local_disable(clp);
> - nfs_local_put_lookup_ctx();
> + put_nfsd_open_local_fh();
> + clp->nfsd_open_local_fh = NULL;
> if (!IS_ERR(clp->cl_rpcclient_localio)) {
> rpc_shutdown_client(clp->cl_rpcclient_localio);
> clp->cl_rpcclient_localio = ERR_PTR(-EINVAL);
> @@ -312,14 +237,13 @@ struct file *
> nfs_local_open_fh(struct nfs_client *clp, const struct cred *cred,
> struct nfs_fh *fh, const fmode_t mode)
> {
> - struct nfs_local_open_ctx *ctx = &__local_open_ctx;
> struct file *filp;
> int status;
>
> if (mode & ~(FMODE_READ | FMODE_WRITE))
> return ERR_PTR(-EINVAL);
>
> - status = ctx->open_f(clp->cl_rpcclient, cred, fh, mode, &filp);
> + status = clp->nfsd_open_local_fh(clp->cl_rpcclient, cred, fh, mode, &filp);
> if (status < 0) {
> dprintk("%s: open local file failed error=%d\n",
> __func__, status);
> diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> index f214cc6754a1..c454c4100976 100644
> --- a/fs/nfs_common/nfslocalio.c
> +++ b/fs/nfs_common/nfslocalio.c
> @@ -40,3 +40,29 @@ bool nfsd_uuid_is_local(const uuid_t *uuid)
> return !uuid_is_null(nfsd_uuid);
> }
> EXPORT_SYMBOL_GPL(nfsd_uuid_is_local);
> +
> +/*
> + * The nfs localio code needs to call into nfsd to do the filehandle -> struct path
> + * mapping, 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.
> + */
> +
> +extern int nfsd_open_local_fh(struct rpc_clnt *rpc_clnt,
> + const struct cred *cred, const struct nfs_fh *nfs_fh,
> + const fmode_t fmode, struct file **pfilp);
> +
> +nfs_to_nfsd_open_t get_nfsd_open_local_fh(void)
> +{
> + return symbol_request(nfsd_open_local_fh);
> +}
> +EXPORT_SYMBOL_GPL(get_nfsd_open_local_fh);
> +
> +void put_nfsd_open_local_fh(void)
> +{
> + symbol_put(nfsd_open_local_fh);
> +}
> +EXPORT_SYMBOL_GPL(put_nfsd_open_local_fh);
> diff --git a/include/linux/nfs.h b/include/linux/nfs.h
> index 2dacfe9742c6..64ed672a0b34 100644
> --- a/include/linux/nfs.h
> +++ b/include/linux/nfs.h
> @@ -48,10 +48,6 @@ enum nfs3_stable_how {
> NFS_INVALID_STABLE_HOW = -1
> };
>
> -typedef int (*nfs_to_nfsd_open_t)(struct rpc_clnt *, const struct cred *,
> - const struct nfs_fh *, const fmode_t,
> - struct file **);
> -
> #ifdef CONFIG_CRC32
> /**
> * nfs_fhandle_hash - calculate the crc32 hash for the filehandle
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index efcdb4d8e9de..f5760b05ec87 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -8,6 +8,7 @@
> #include <linux/wait.h>
> #include <linux/nfs_xdr.h>
> #include <linux/sunrpc/xprt.h>
> +#include <linux/nfslocalio.h>
>
> #include <linux/atomic.h>
> #include <linux/refcount.h>
> @@ -131,6 +132,7 @@ struct nfs_client {
> struct timespec64 cl_nfssvc_boot;
> seqlock_t cl_boot_lock;
> struct rpc_clnt * cl_rpcclient_localio; /* localio RPC client handle */
> + nfs_to_nfsd_open_t nfsd_open_local_fh;
> };
>
> /*
> diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
> index d0bbacd0adcf..b8df1b9f248d 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/nfs.h>
>
> /*
> * Global list of nfsd_uuid_t instances, add/remove
> @@ -26,4 +27,11 @@ typedef struct {
>
> bool nfsd_uuid_is_local(const uuid_t *uuid);
>
> +typedef int (*nfs_to_nfsd_open_t)(struct rpc_clnt *, const struct cred *,
> + const struct nfs_fh *, const fmode_t,
> + struct file **);
> +
> +nfs_to_nfsd_open_t get_nfsd_open_local_fh(void);
> +void put_nfsd_open_local_fh(void);
> +
> #endif /* __LINUX_NFSLOCALIO_H */
--
Jeff Layton <jlayton@xxxxxxxxxx>
