On Sun, 01 Sep 2024, Mike Snitzer wrote:
> From: Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
>
> Add server support for bypassing NFS for localhost reads, writes, and
> commits. This is only useful when both the client and server are
> running on the same host.
>
> If nfsd_open_local_fh() fails then the NFS client will both retry and
> fallback to normal network-based read, write and commit operations if
> localio is no longer supported.
>
> Care is taken to ensure the same NFS security mechanisms are used
> (authentication, etc) regardless of whether localio or regular NFS
> access is used. The auth_domain established as part of the traditional
> NFS client access to the NFS server is also used for localio. Store
> auth_domain for localio in nfsd_uuid_t and transfer it to the client
> if it is local to the server.
>
> Relative to containers, localio gives the client access to the network
> namespace the server has. This is required to allow the client to
> access the server's per-namespace nfsd_net struct.
>
> This commit also introduces the use of NFSD's percpu_ref to interlock
> nfsd_destroy_serv and nfsd_open_local_fh, to ensure nn->nfsd_serv is
> not destroyed while in use by nfsd_open_local_fh and other LOCALIO
> client code.
>
> CONFIG_NFS_LOCALIO enables NFS server support for LOCALIO.
>
> Signed-off-by: Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Co-developed-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> Co-developed-by: NeilBrown <neilb@xxxxxxx>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
>
> Not-Acked-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> Not-Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/nfsd/Makefile | 1 +
> fs/nfsd/filecache.c | 2 +-
> fs/nfsd/localio.c | 112 +++++++++++++++++++++++++++++++++++++
> fs/nfsd/netns.h | 4 ++
> fs/nfsd/nfsctl.c | 25 ++++++++-
> fs/nfsd/trace.h | 3 +-
> fs/nfsd/vfs.h | 2 +
> include/linux/nfslocalio.h | 8 +++
> 8 files changed, 154 insertions(+), 3 deletions(-)
> create mode 100644 fs/nfsd/localio.c
>
> diff --git a/fs/nfsd/Makefile b/fs/nfsd/Makefile
> index b8736a82e57c..18cbd3fa7691 100644
> --- a/fs/nfsd/Makefile
> +++ b/fs/nfsd/Makefile
> @@ -23,3 +23,4 @@ nfsd-$(CONFIG_NFSD_PNFS) += nfs4layouts.o
> nfsd-$(CONFIG_NFSD_BLOCKLAYOUT) += blocklayout.o blocklayoutxdr.o
> nfsd-$(CONFIG_NFSD_SCSILAYOUT) += blocklayout.o blocklayoutxdr.o
> nfsd-$(CONFIG_NFSD_FLEXFILELAYOUT) += flexfilelayout.o flexfilelayoutxdr.o
> +nfsd-$(CONFIG_NFS_LOCALIO) += localio.o
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 89ff380ec31e..348c1b97092e 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -52,7 +52,7 @@
> #define NFSD_FILE_CACHE_UP (0)
>
> /* We only care about NFSD_MAY_READ/WRITE for this cache */
> -#define NFSD_FILE_MAY_MASK (NFSD_MAY_READ|NFSD_MAY_WRITE)
> +#define NFSD_FILE_MAY_MASK (NFSD_MAY_READ|NFSD_MAY_WRITE|NFSD_MAY_LOCALIO)
>
> static DEFINE_PER_CPU(unsigned long, nfsd_file_cache_hits);
> static DEFINE_PER_CPU(unsigned long, nfsd_file_acquisitions);
> diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> new file mode 100644
> index 000000000000..75df709c6903
> --- /dev/null
> +++ b/fs/nfsd/localio.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NFS server support for local clients to bypass network stack
> + *
> + * Copyright (C) 2014 Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
> + * Copyright (C) 2019 Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> + * Copyright (C) 2024 Mike Snitzer <snitzer@xxxxxxxxxxxxxxx>
> + * Copyright (C) 2024 NeilBrown <neilb@xxxxxxx>
> + */
> +
> +#include <linux/exportfs.h>
> +#include <linux/sunrpc/svcauth.h>
> +#include <linux/sunrpc/clnt.h>
> +#include <linux/nfs.h>
> +#include <linux/nfs_common.h>
> +#include <linux/nfslocalio.h>
> +#include <linux/string.h>
> +
> +#include "nfsd.h"
> +#include "vfs.h"
> +#include "netns.h"
> +#include "filecache.h"
> +
> +static const struct nfsd_localio_operations nfsd_localio_ops = {
> + .nfsd_open_local_fh = nfsd_open_local_fh,
> + .nfsd_file_put_local = nfsd_file_put_local,
> + .nfsd_file_file = nfsd_file_file,
> +};
> +
> +void nfsd_localio_ops_init(void)
> +{
> + memcpy(&nfs_to, &nfsd_localio_ops, sizeof(nfsd_localio_ops));
> +}
Why isn't this
nfs_to = &nfsd_loclaio_ops;
??
Why do we copy all the pointers in the struct instead of just the
pointer to the struct?
Is this to avoid an extra dereference? If so we need an in-code comment
explaining this optimisation - and why we need it while most used of
_operations structures don't.
>
> +#if IS_ENABLED(CONFIG_NFS_LOCALIO)
> +/**
> + * nfsd_net_pre_exit - Disconnect localio clients from net namespace
> + * @net: a network namespace that is about to be destroyed
> + *
> + * This invalidated ->net pointers held by localio clients
> + * while they can still safely access nn->counter.
> + */
> +static __net_exit void nfsd_net_pre_exit(struct net *net)
> +{
> + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +
> + nfs_uuid_invalidate_clients(&nn->local_clients);
> +}
> +#endif
> +
> /**
> * nfsd_net_exit - Release the nfsd_net portion of a net namespace
> * @net: a network namespace that is about to be destroyed
> @@ -2285,6 +2304,9 @@ static __net_exit void nfsd_net_exit(struct net *net)
>
> static struct pernet_operations nfsd_net_ops = {
> .init = nfsd_net_init,
> +#if IS_ENABLED(CONFIG_NFS_LOCALIO)
> + .pre_exit = nfsd_net_pre_exit,
> +#endif
I would rather that these #ifs were not here, but that in the
NFS_LOCALIO disabled case, nfs_uuid_invalidate_clients() were an empty
static inline function.
I think that code of .pre_exit sometime being an empty function is not
significant.
Section 21 of codiing-style. Or maybe section 20, depending on release
(there is a new section on "Using bool").
https://www.kernel.org/doc/html/v4.14/process/coding-style.html#conditional-compilation
NeilBrown
