Re: [for-6.11 PATCH 10/29] nfs/nfsd: add "local io" support

[Mike Snitzer <snitzer@kernel.org>]

On Mon, Jun 10, 2024 at 12:42:54PM -0400, Mike Snitzer wrote:
> On Mon, Jun 10, 2024 at 08:43:34AM -0400, Jeff Layton wrote:
> > On Fri, 2024-06-07 at 10:26 -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 and in the same container.
> > > 
> > > This has dynamic binding with the nfsd module. Local i/o will only work if
> > > nfsd is already loaded.
> > > 
> > > [snitm: rebase accounted for commit d8b26071e65e8 ("NFSD: simplify struct nfsfh")
> > >  and commit 7c98f7cb8fda ("remove call_{read,write}_iter() functions")]
> > > 
> > > Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
> > > Signed-off-by: Jeff Layton <jeff.layton@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>
...
> > > diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> > > new file mode 100644
> > > index 000000000000..ff68454a4017
> > > --- /dev/null
> > > +++ b/fs/nfsd/localio.c
> > > @@ -0,0 +1,179 @@
> > > +/*
> > > + * NFS server support for local clients to bypass network stack
> > > + *
> > > + * Copyright (C) 2014 Weston Andros Adamson <dros@primarydata.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
> > > +
> > > +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);
> > > +	kfree(rqstp->rq_cred.cr_principal);
> > > +	kfree(rqstp->rq_xprt);
> > > +	kfree(rqstp);
> > > +}
> > > +
> > > +static struct svc_rqst *
> > > +nfsd_local_fakerqst_create(struct rpc_clnt *rpc_clnt, const struct cred *cred)
> > > +{
> > > +	struct svc_rqst *rqstp;
> > > +	struct net *net = rpc_net_ns(rpc_clnt);
> > > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > > +	int status;
> > > +
> > > +	if (!nn->nfsd_serv) {
> > > +		dprintk("%s: localio denied. Server not running\n", __func__);
> > > +		return ERR_PTR(-ENXIO);
> > > +	}
> > > +
> > 
> > Note that the above check is racy. The nfsd_serv can go away at any
> > time since you're not holding the (global) nfsd_mutex (I assume?).
> 
> Yes, worst case we should fallback to going over the network.

Actual worst case is we could crash... ;)

> > > +	rqstp = kzalloc(sizeof(*rqstp), GFP_KERNEL);
> > > +	if (!rqstp)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	rqstp->rq_xprt = kzalloc(sizeof(*rqstp->rq_xprt), GFP_KERNEL);
> > > +	if (!rqstp->rq_xprt) {
> > > +		status = -ENOMEM;
> > > +		goto out_err;
> > > +	}
> > > +
> > > +	rqstp->rq_xprt->xpt_net = net;
> > > +	__set_bit(RQ_SECURE, &rqstp->rq_flags);
> > > +	rqstp->rq_proc = 1;
> > > +	rqstp->rq_vers = 3;
> > > +	rqstp->rq_prot = IPPROTO_TCP;
> > > +	rqstp->rq_server = nn->nfsd_serv;
> > > +
> > 
> > I suspect you need to carry a reference of some sort so that the
> > nfsd_serv doesn't go away out from under you while this is running,
> > since this is not operating in nfsd thread context.
> > 
> > Typically, every nfsd thread holds a reference to the serv (in serv-
> > >sv_nrthreads), so that when you shut down all of the threads, it goes
> > away. The catch is that that refcount is currently under the protection
> > of the global nfsd_mutex and I doubt you want to take that in this
> > codepath.
> 
> OK, I can look closer at the implications.

SO I looked, and I'm saddened to see Neil's 6.8 commit 1e3577a4521e
("SUNRPC: discard sv_refcnt, and svc_get/svc_put").

[the lack of useful refcounting with the current code kind of blew me
away.. but nice to see it existed not too long ago.]

Rather than immediately invest the effort to revert commit
1e3577a4521e for my apparent needs... I'll send out v2 to allow for
further review and discussion.

But it really does feel like I _need_ svc_{get,put} and nfsd_{get,put}

Mike