On Mon, Jun 24, 2024 at 02:26:42PM -0400, Chuck Lever wrote:
> On Mon, Jun 24, 2024 at 12:27:27PM -0400, Mike Snitzer wrote:
> > From: Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
> >
> > Add client support for bypassing NFS for localhost reads, writes, and
> > commits. This is only useful when the client and the server are
> > running on the same host.
> >
> > nfs_local_probe() is stubbed out, later commits will enable client and
> > server handshake via a Linux-only LOCALIO auxiliary RPC protocol.
> >
> > This has dynamic binding with the nfsd module (via nfs_localio module
> > which is part of nfs_common). Localio will only work if nfsd is
> > already loaded.
> >
> > The "localio_enabled" nfs kernel module parameter can be used to
> > disable and enable the ability to use localio support.
> >
> > Tracepoints were added for nfs_local_open_fh, nfs_local_enable and
> > nfs_local_disable.
> >
> > Also, pass the stored cl_nfssvc_net from the client to the server as
> > first argument to nfsd_open_local_fh() to ensure the proper network
> > namespace is used for localio.
> >
> > Signed-off-by: Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
> > Signed-off-by: Peng Tao <tao.peng@xxxxxxxxxxxxxxx>
> > Signed-off-by: Lance Shelton <lance.shelton@xxxxxxxxxxxxxxx>
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > ---
> > fs/nfs/Makefile | 1 +
> > fs/nfs/client.c | 3 +
> > fs/nfs/inode.c | 4 +
> > fs/nfs/internal.h | 51 +++
> > fs/nfs/localio.c | 654 ++++++++++++++++++++++++++++++++++++++
> > fs/nfs/nfstrace.h | 61 ++++
> > fs/nfs/pagelist.c | 3 +
> > fs/nfs/write.c | 3 +
>
> Hi Mike -
>
> I'd prefer to see this patch split into two patches, one for the
> NFS client, and one for the NFS server. The other patches in this
> series seem to have a clear line between server and client
> changes.
Will do.
> > diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> > new file mode 100644
> > index 000000000000..e9aa0997f898
> > --- /dev/null
> > +++ b/fs/nfsd/localio.c
> > @@ -0,0 +1,244 @@
> > +// 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>
> > + */
> > +
> > +#include <linux/exportfs.h>
> > +#include <linux/sunrpc/svcauth_gss.h>
> > +#include <linux/sunrpc/clnt.h>
> > +#include <linux/nfs.h>
> > +#include <linux/string.h>
> > +
> > +#include "nfsd.h"
> > +#include "vfs.h"
> > +#include "netns.h"
> > +#include "filecache.h"
> > +
> > +#define NFSDDBG_FACILITY NFSDDBG_FH
>
> I think I'd rather prefer to see trace points in here rather than
> new dprintk call sites. In any event, perhaps NFSDDBG_FH is not
> especially appropriate?
I think NFSDDBG_FH is most appropriate given this localio is most
focused on opening a file handle. (The getuuid not so much)
I'm not loving how overdone the trace points interface is. in my
experience, trace points are also a serious source of conflicts when
backporting changes to older kernels.
But if you were inclined to switch this code over to trace points once
it is merged, who am I to stop you! ;)
> > +
> > +/**
> > + * nfs_stat_to_errno - convert an NFS status code to a local errno
> > + * @status: NFS status code to convert
> > + *
> > + * Returns a local errno value, or -EIO if the NFS status code is
> > + * not recognized. This function is used jointly by NFSv2 and NFSv3.
> > + */
> > +static int nfs_stat_to_errno(enum nfs_stat status)
> > +{
> > + int i;
> > +
> > + for (i = 0; nfs_common_errtbl[i].stat != -1; i++) {
> > + if (nfs_common_errtbl[i].stat == (int)status)
> > + return nfs_common_errtbl[i].errno;
> > + }
> > + return nfs_common_errtbl[i].errno;
> > +}
>
> I get it: The existing nfserrno() function on the server is the
> reverse mapping from this one, and you need to undo the nfsstat
> returned from nfsd_file_acquire().
>
> The documenting comments here confusing in the context of why this
> function is necessary on the server. For example, "This function is
> used jointly by NFSv2 and NFSv3"... Maybe instead, explain that
> nfsd_file_acquire() returns an nfsstat that needs to be translated
> to an errno before being returned to a local client application.
Thanks, fixed.
> > +static void
> > +nfsd_local_fakerqst_destroy(struct svc_rqst *rqstp)
> > +{
> > + if (rqstp->rq_client)
> > + auth_domain_put(rqstp->rq_client);
> > + if (rqstp->rq_cred.cr_group_info)
> > + put_group_info(rqstp->rq_cred.cr_group_info);
> > + /* rpcauth_map_to_svc_cred_local() clears cr_principal */
> > + WARN_ON_ONCE(rqstp->rq_cred.cr_principal != NULL);
> > + kfree(rqstp->rq_xprt);
> > + kfree(rqstp);
> > +}
> > +
> > +static struct svc_rqst *
> > +nfsd_local_fakerqst_create(struct net *net, struct rpc_clnt *rpc_clnt,
> > + const struct cred *cred)
> > +{
> > + struct svc_rqst *rqstp;
> > + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > + int status;
> > +
> > + /* FIXME: not running in nfsd context, must get reference on nfsd_serv */
>
> What's your plan for addressing this FIXME?
The SRCU (and nfsd_serv_get, nfsd_serv_put and nfsd_serv_sync) in
later patches addresses it.
FYI: And now that I looked closer at Neil's earlier suggestion (to
have a single SRCU for all of nfsd_net -- rather than having one
embedded in each): I'll try to include that change for v8.
> > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> > index 57cd70062048..af07bb146e81 100644
> > --- a/fs/nfsd/vfs.h
> > +++ b/fs/nfsd/vfs.h
> > @@ -36,6 +36,8 @@
> > #define NFSD_MAY_CREATE (NFSD_MAY_EXEC|NFSD_MAY_WRITE)
> > #define NFSD_MAY_REMOVE (NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
> >
> > +#define NFSD_MAY_LOCALIO 0x800000
> > +
>
> Nit: I'd prefer to see NFSD_MAY_LOCALIO inserted just after
> NFSD_MAY_64BIT_COOKIE. Is there a reason the value of the new flag
> should not be 0x2000 ?
Fixed.
