On Sun, 01 Sep 2024, Mike Snitzer wrote:
> The next commit will introduce nfsd_open_local_fh() which returns an
> nfsd_file structure. This commit exposes LOCALIO's required NFSD
> symbols to the 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 (global access suggested by Trond, nfsd_localio_operations
> suggested by NeilBrown). The next commit will also introduce
> nfsd_localio_ops_init() that init_nfsd() will call to initialize
> 'nfs_to'.
>
> - Introduce nfsd_file_file() that provides access to nfsd_file's
> backing file. Keeps nfsd_file structure opaque to NFS client (as
> suggested by Jeff Layton).
>
> - Introduce nfsd_file_put_local() that will put the reference to the
> nfsd_file's associated nn->nfsd_serv and then put the reference to
> the nfsd_file (as suggested by NeilBrown).
>
> Suggested-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> # nfs_to
> Suggested-by: NeilBrown <neilb@xxxxxxx> # nfsd_localio_operations
> Suggested-by: Jeff Layton <jlayton@xxxxxxxxxx> # nfsd_file_file
> Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> ---
> fs/nfs_common/nfslocalio.c | 23 +++++++++++++++++++++++
> fs/nfsd/filecache.c | 30 ++++++++++++++++++++++++++++++
> fs/nfsd/filecache.h | 2 ++
> fs/nfsd/nfssvc.c | 2 ++
> include/linux/nfslocalio.h | 30 ++++++++++++++++++++++++++++++
> 5 files changed, 87 insertions(+)
>
> diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> index 22b0ddf225ca..64f75a3a370a 100644
> --- a/fs/nfs_common/nfslocalio.c
> +++ b/fs/nfs_common/nfslocalio.c
> @@ -114,3 +114,26 @@ void nfs_uuid_invalidate_one_client(nfs_uuid_t *nfs_uuid)
> }
> }
> EXPORT_SYMBOL_GPL(nfs_uuid_invalidate_one_client);
> +
> +/*
> + * 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 on the NFSD module (above),
> + * 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);
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 2dc72de31f61..89ff380ec31e 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -390,6 +390,36 @@ nfsd_file_put(struct nfsd_file *nf)
> nfsd_file_free(nf);
> }
>
> +/**
> + * nfsd_file_put_local - put the reference to nfsd_file and local nfsd_serv
> + * @nf: nfsd_file of which to put the references
> + *
> + * First put the reference of the nfsd_file's associated nn->nfsd_serv and
> + * then put the reference to the nfsd_file.
> + */
> +void
> +nfsd_file_put_local(struct nfsd_file *nf)
> +{
> + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> +
> + nfsd_serv_put(nn);
> + nfsd_file_put(nf);
> +}
> +EXPORT_SYMBOL_GPL(nfsd_file_put_local);
This and the others doesn't need to be exported. The name is only used
inside this module.
> +
> +/**
> + * 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..cadf3c2689c4 100644
> --- a/fs/nfsd/filecache.h
> +++ b/fs/nfsd/filecache.h
> @@ -55,7 +55,9 @@ void nfsd_file_cache_shutdown(void);
> 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);
> +void nfsd_file_put_local(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 4165ff8390c1..62419c4bc8f1 100644
> --- a/include/linux/nfslocalio.h
> +++ b/include/linux/nfslocalio.h
> @@ -9,6 +9,7 @@
> #include <linux/module.h>
> #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>
> @@ -33,4 +34,33 @@ void nfs_uuid_is_local(const uuid_t *, struct list_head *,
> void nfs_uuid_invalidate_clients(struct list_head *list);
> void nfs_uuid_invalidate_one_client(nfs_uuid_t *nfs_uuid);
>
> +struct nfsd_file;
> +
> +/* localio needs to map filehandle -> struct nfsd_file */
> +typedef struct nfsd_file *
> +(*nfs_to_nfsd_open_local_fh_t)(nfs_uuid_t *, struct rpc_clnt *,
> + const struct cred *, const struct nfs_fh *,
> + const fmode_t);
> +
> +extern struct nfsd_file *
> +nfsd_open_local_fh(nfs_uuid_t *, struct rpc_clnt *,
> + const struct cred *, const struct nfs_fh *,
> + const fmode_t) __must_hold(rcu);
> +
> +/* 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 and its associated nn->nfsd_serv */
> +typedef void (*nfs_to_nfsd_file_put_local_t)(struct nfsd_file *);
> +/* localio needs to access the nf->nf_file */
> +typedef struct file * (*nfs_to_nfsd_file_file_t)(struct nfsd_file *);
> +
> +struct nfsd_localio_operations {
> + nfs_to_nfsd_open_local_fh_t nfsd_open_local_fh;
> + nfs_to_nfsd_file_put_local_t nfsd_file_put_local;
> + nfs_to_nfsd_file_file_t nfsd_file_file;
> +} ____cacheline_aligned;
What benefits do you see in these typedef?
The standard practice for operations structures is:
struct nfsd_localio_operations {
struct nfsd_file *(*nfsd_open_local_fh)(nfs_uuid_t *, struct rpc_clnt *,
const struct cred *, const struct nfs_fh *,
const fmode_t);
void (*nfsd_file_put_local)(struct nfsd_file *);
struct file *(*nfsd_file_file)(struct nfsd_file *);
};
which I find to be much readable.
(I'd prefer "struct nfsd_uuid" to "nfs_uuid_t" too but I've decided to
not push that for now).
This can easily be fixed up after the series lands so it doesn't need to
block landing for v15 if there are no show-stoppers, but I needed to say
something... But I'm so happy to see an operations structure that I
don't want to sound too negative.
See section 5 of Documentation/process/coding-style.rst
https://www.kernel.org/doc/html/v4.14/process/coding-style.html#typedefs
NeilBrown
