Received-SPF: pass (google.com: domain of linux-nfs+bounces-3674-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Mike Snitzer" <snitzer@kernel.org>
Cc: "Jeff Layton" <jlayton@kernel.org>, linux-nfs@vger.kernel.org,
 "Chuck Lever" <chuck.lever@oracle.com>,
 "Trond Myklebust" <trondmy@hammerspace.com>, snitzer@hammerspace.com
Subject: Re: [for-6.11 PATCH 10/29] nfs/nfsd: add "local io" support
In-reply-to: <ZmkHNr5jtGHDpko_@kernel.org>
References: <>, <ZmkHNr5jtGHDpko_@kernel.org>
Date: Wed, 12 Jun 2024 13:17:05 +1000
Message-id: <171816222529.14261.9832643931623454806@noble.neil.brown.name>

On Wed, 12 Jun 2024, Mike Snitzer wrote:
> 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>
> > > >=20
> > > > Add client support for bypassing NFS for localhost reads, writes, and=
 commits.
> > > >=20
> > > > This is only useful when the client and the server are running on the=
 same
> > > > host and in the same container.
> > > >=20
> > > > This has dynamic binding with the nfsd module. Local i/o will only wo=
rk if
> > > > nfsd is already loaded.
> > > >=20
> > > > [snitm: rebase accounted for commit d8b26071e65e8 ("NFSD: simplify st=
ruct nfsfh")
> > > > =C2=A0and commit 7c98f7cb8fda ("remove call_{read,write}_iter() funct=
ions")]
> > > >=20
> > > > 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 c=
red *cred)
> > > > +{
> > > > +	struct svc_rqst *rqstp;
> > > > +	struct net *net =3D rpc_net_ns(rpc_clnt);
> > > > +	struct nfsd_net *nn =3D 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);
> > > > +	}
> > > > +
> > >=20
> > > 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?).
> >=20
> > Yes, worst case we should fallback to going over the network.
>=20
> Actual worst case is we could crash... ;)
>=20
> > > > +	rqstp =3D kzalloc(sizeof(*rqstp), GFP_KERNEL);
> > > > +	if (!rqstp)
> > > > +		return ERR_PTR(-ENOMEM);
> > > > +
> > > > +	rqstp->rq_xprt =3D kzalloc(sizeof(*rqstp->rq_xprt), GFP_KERNEL);
> > > > +	if (!rqstp->rq_xprt) {
> > > > +		status =3D -ENOMEM;
> > > > +		goto out_err;
> > > > +	}
> > > > +
> > > > +	rqstp->rq_xprt->xpt_net =3D net;
> > > > +	__set_bit(RQ_SECURE, &rqstp->rq_flags);
> > > > +	rqstp->rq_proc =3D 1;
> > > > +	rqstp->rq_vers =3D 3;
> > > > +	rqstp->rq_prot =3D IPPROTO_TCP;
> > > > +	rqstp->rq_server =3D nn->nfsd_serv;
> > > > +
> > >=20
> > > 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.
> > >=20
> > > 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.
> >=20
> > OK, I can look closer at the implications.
>=20
> SO I looked, and I'm saddened to see Neil's 6.8 commit 1e3577a4521e
> ("SUNRPC: discard sv_refcnt, and svc_get/svc_put").
>=20
> [the lack of useful refcounting with the current code kind of blew me
> away.. but nice to see it existed not too long ago.]
>=20
> 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.
>=20
> But it really does feel like I _need_ svc_{get,put} and nfsd_{get,put}

You are taking a reference, and at the right time.  But it is to the
wrong thing.
You call symbol_request(nfsd_open_local_fh) and so get a reference to
the nfsd module.  But you really want a reference to the nfsd service.

I would suggest that you use symbol_request() to get a function which
you then call and immediately symbol_put().... unless you need to use it
to discard the reference to the service later.
The function would take nfsd_mutex, check there is an nfsd_serv, sets a
flag or whatever to indicate the serv is being used for local_io, and
maybe returns the nfsd_serv.  As long as that flag is set the serv
cannot be destroy.

Do you need there to be available threads for LOCAL_IO to work?  If so
the flag would cause setting the num threads to zero to fail.
If not ....  that is weird.  It would mean that setting the number of
threads to zero would not destroy the service and I don't think we want
to do that.

So I think that when LOCAL_IO is in use, setting number of threads to
zero must return EBUSY or similar, even if you don't need the threads.

NeilBrown