Received-SPF: pass (google.com: domain of linux-nfs+bounces-3777-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <5064ae6b3156a1601eb7a7f4f890fb125933aefb.camel@kernel.org>
Subject: Re: [PATCH v2 2/5] nfsd: make nfsd_svc call nfsd_set_nrthreads
From: Jeff Layton <jlayton@kernel.org>
To: Chuck Lever <chuck.lever@oracle.com>
Cc: Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>, Dai
 Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>, "David S. Miller"
 <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
 <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Lorenzo Bianconi
 <lorenzo@kernel.org>, Trond Myklebust <trond.myklebust@hammerspace.com>, 
 Anna Schumaker <anna@kernel.org>, linux-nfs@vger.kernel.org,
 linux-kernel@vger.kernel.org,  netdev@vger.kernel.org
Date: Thu, 13 Jun 2024 12:58:15 -0400
In-Reply-To: <ZmsW+kmUdtebrUcd@tissot.1015granger.net>
References: <20240613-nfsd-next-v2-0-20bf690d65fb@kernel.org>
	 <20240613-nfsd-next-v2-2-20bf690d65fb@kernel.org>
	 <ZmsW+kmUdtebrUcd@tissot.1015granger.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Thu, 2024-06-13 at 11:57 -0400, Chuck Lever wrote:
> On Thu, Jun 13, 2024 at 08:16:39AM -0400, Jeff Layton wrote:
> > Now that the refcounting is fixed, rework nfsd_svc to use the same
> > thread setup as the pool_threads interface. Since the new netlink
> > interface doesn't have the same restriction as pool_threads, move
> > the
> > guard against shutting down all threads to write_pool_threads.
> >=20
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > =C2=A0fs/nfsd/nfsctl.c | 14 ++++++++++++--
> > =C2=A0fs/nfsd/nfsd.h=C2=A0=C2=A0 |=C2=A0 3 ++-
> > =C2=A0fs/nfsd/nfssvc.c | 18 ++----------------
> > =C2=A03 files changed, 16 insertions(+), 19 deletions(-)
> >=20
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 202140df8f82..121b866125d4 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -406,7 +406,7 @@ static ssize_t write_threads(struct file *file,
> > char *buf, size_t size)
> > =C2=A0			return -EINVAL;
> > =C2=A0		trace_nfsd_ctl_threads(net, newthreads);
> > =C2=A0		mutex_lock(&nfsd_mutex);
> > -		rv =3D nfsd_svc(newthreads, net, file->f_cred,
> > NULL);
> > +		rv =3D nfsd_svc(1, &newthreads, net, file->f_cred,
> > NULL);
> > =C2=A0		mutex_unlock(&nfsd_mutex);
> > =C2=A0		if (rv < 0)
> > =C2=A0			return rv;
> > @@ -481,6 +481,16 @@ static ssize_t write_pool_threads(struct file
> > *file, char *buf, size_t size)
> > =C2=A0				goto out_free;
> > =C2=A0			trace_nfsd_ctl_pool_threads(net, i,
> > nthreads[i]);
> > =C2=A0		}
> > +
> > +		/*
> > +		 * There must always be a thread in pool 0; the
> > admin
> > +		 * can't shut down NFS completely using
> > pool_threads.
> > +		 *
> > +		 * FIXME: do we really need this?
>=20
> Hi, how do you plan to decide this question?
>=20

Probably by ignoring it and letting the restriction (eventually) die
with the old pool_threads interface. I'm amenable to dropping this
restriction altogether though.

>=20
> > +		 */
> > +		if (nthreads[0] =3D=3D 0)
> > +			nthreads[0] =3D 1;
> > +
> > =C2=A0		rv =3D nfsd_set_nrthreads(i, nthreads, net);
> > =C2=A0		if (rv)
> > =C2=A0			goto out_free;
> > @@ -1722,7 +1732,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff
> > *skb, struct genl_info *info)
> > =C2=A0			scope =3D nla_data(attr);
> > =C2=A0	}
> > =C2=A0
> > -	ret =3D nfsd_svc(nthreads, net, get_current_cred(), scope);
> > +	ret =3D nfsd_svc(1, &nthreads, net, get_current_cred(),
> > scope);
> > =C2=A0
> > =C2=A0out_unlock:
> > =C2=A0	mutex_unlock(&nfsd_mutex);
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index 8f4f239d9f8a..cec8697b1cd6 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -103,7 +103,8 @@
> > bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
> > =C2=A0/*
> > =C2=A0 * Function prototypes.
> > =C2=A0 */
> > -int		nfsd_svc(int nrservs, struct net *net, const
> > struct cred *cred, const char *scope);
> > +int		nfsd_svc(int n, int *nservers, struct net *net,
> > +			 const struct cred *cred, const char
> > *scope);
> > =C2=A0int		nfsd_dispatch(struct svc_rqst *rqstp);
> > =C2=A0
> > =C2=A0int		nfsd_nrthreads(struct net *);
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index cd9a6a1a9fc8..076f35dc17e4 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -744,13 +744,6 @@ int nfsd_set_nrthreads(int n, int *nthreads,
> > struct net *net)
>=20
> Since you are slightly changing the API contract for this publicly
> visible function, now would be a good time to add a kdoc comment.
>=20

Ok.

>=20
> > =C2=A0		}
> > =C2=A0	}
> > =C2=A0
> > -	/*
> > -	 * There must always be a thread in pool 0; the admin
> > -	 * can't shut down NFS completely using pool_threads.
> > -	 */
> > -	if (nthreads[0] =3D=3D 0)
> > -		nthreads[0] =3D 1;
> > -
> > =C2=A0	/* apply the new numbers */
> > =C2=A0	for (i =3D 0; i < n; i++) {
> > =C2=A0		err =3D svc_set_num_threads(nn->nfsd_serv,
> > @@ -768,7 +761,7 @@ int nfsd_set_nrthreads(int n, int *nthreads,
> > struct net *net)
> > =C2=A0 * this is the first time nrservs is nonzero.
> > =C2=A0 */
> > =C2=A0int
> > -nfsd_svc(int nrservs, struct net *net, const struct cred *cred,
> > const char *scope)
> > +nfsd_svc(int n, int *nthreads, struct net *net, const struct cred
> > *cred, const char *scope)
>=20
> Ditto: the patch changes the synopsis of nfsd_svc(), so I'd like a
> kdoc comment to go with it.
>=20
> And, this particular change is the reason for this patch, so the
> description should state that (especially since subsequent
> patch descriptions refer to "now that nfsd_svc takes an array
> of threads..." : I had to come back to this patch and blink twice
> to see why it said that).
>=20
> A kdoc comment from sunrpc_get_pool_mode() should also be added
> in 4/5.
>=20

I'll do that and resend soon.

>=20
> > =C2=A0{
> > =C2=A0	int	error;
> > =C2=A0	struct nfsd_net *nn =3D net_generic(net, nfsd_net_id);
> > @@ -778,13 +771,6 @@ nfsd_svc(int nrservs, struct net *net, const
> > struct cred *cred, const char *scop
> > =C2=A0
> > =C2=A0	dprintk("nfsd: creating service\n");
> > =C2=A0
> > -	nrservs =3D max(nrservs, 0);
> > -	nrservs =3D min(nrservs, NFSD_MAXSERVS);
> > -	error =3D 0;
> > -
> > -	if (nrservs =3D=3D 0 && nn->nfsd_serv =3D=3D NULL)
> > -		goto out;
> > -
> > =C2=A0	strscpy(nn->nfsd_name, scope ? scope : utsname()-
> > >nodename,
> > =C2=A0		sizeof(nn->nfsd_name));
> > =C2=A0
> > @@ -796,7 +782,7 @@ nfsd_svc(int nrservs, struct net *net, const
> > struct cred *cred, const char *scop
> > =C2=A0	error =3D nfsd_startup_net(net, cred);
> > =C2=A0	if (error)
> > =C2=A0		goto out_put;
> > -	error =3D svc_set_num_threads(serv, NULL, nrservs);
> > +	error =3D nfsd_set_nrthreads(n, nthreads, net);
> > =C2=A0	if (error)
> > =C2=A0		goto out_put;
> > =C2=A0	error =3D serv->sv_nrthreads;
> >=20
> > --=20
> > 2.45.2
> >=20
>=20

--=20
Jeff Layton <jlayton@kernel.org>