Re: [PATCH v7 06/20] nfs/nfsd: add "localio" support

[Mike Snitzer <snitzer@kernel.org>]

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@primarydata.com>
> > 
> > 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@primarydata.com>
> > Signed-off-by: Peng Tao <tao.peng@primarydata.com>
> > Signed-off-by: Lance Shelton <lance.shelton@hammerspace.com>
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  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@primarydata.com>
> > + * Copyright (C) 2019 Trond Myklebust <trond.myklebust@hammerspace.com>
> > + * Copyright (C) 2024 Mike Snitzer <snitzer@hammerspace.com>
> > + */
> > +
> > +#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.