Received-SPF: pass (google.com: domain of linux-nfs+bounces-3770-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 13 Jun 2024 11:57:46 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
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
Subject: Re: [PATCH v2 2/5] nfsd: make nfsd_svc call nfsd_set_nrthreads
Message-ID: <ZmsW+kmUdtebrUcd@tissot.1015granger.net>
References: <20240613-nfsd-next-v2-0-20bf690d65fb@kernel.org>
 <20240613-nfsd-next-v2-2-20bf690d65fb@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240613-nfsd-next-v2-2-20bf690d65fb@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?04MUAUOT1saly0wEIx9KbbYNdlAKlBoSIldLlv9eM2m3lEbq/sLBBlGK67xZ?=
 =?us-ascii?Q?FsWS2r6bADo9BH/mcTa6IrmzaqGVcTkrVjGQXIIu7414xgdE4inRRvpHfk1Y?=
 =?us-ascii?Q?HSpcx9CwUJsUJXd4jKK5ChXOKG0Jc6m8p8WKwdTLdQ4B2BjexCKYyckdomse?=
 =?us-ascii?Q?v/CH7f8BRdNeiT/Nqcg/ceWYUDhtuQ8HuWMQSqCmdMYmCKsDyRi0K+CtSLKl?=
 =?us-ascii?Q?InCZrCawTnD0AifpDjnq5+pPr01/r5p5eW9AHqYA266N8Ccp9wJljUcuijmq?=
 =?us-ascii?Q?bHULMSD2iZNEkQwY3CTzvJ1DkDyArdCzOVfTrMUqHHPj46hdvwi6p8jLT6RZ?=
 =?us-ascii?Q?dIwWDKNNspag8jF0sn8xqJK3mKIJSobkGUlFEMR0lnOscoVo8nZoP9gQYpUE?=
 =?us-ascii?Q?W9JhrJaOWLl79jm1w5Qid7WtNPYpFdkWp0IrAcbx0CuzIURUlp8EBikJ0V99?=
 =?us-ascii?Q?QO3jitBlv37evUfcFStWjJEz3GyEV0vv3g7QPncxNXI0wK3VXBeSn9gy7ZCD?=
 =?us-ascii?Q?dLHxbGMfU+okr225fjILBJddYi6jCIhKLbCzQ/m+oVOh9/CCuBspqehKCk+h?=
 =?us-ascii?Q?etrvcFe1VVwj6uz4CuOw7VlP9bh+s4D1HpYSPrAbWKEg6KO6XGG7MsasdLFG?=
 =?us-ascii?Q?PC/n4HMDMPJO+DlqMt7pPkCrYOzvEJjkvzbr0IIEE9Dt2/iWthE97ibAWemf?=
 =?us-ascii?Q?8DGXdpt1jl3yu096+NevmYadVnbXyKt1UeMC3LzBNS3Lguqsa/leiFB42v5Y?=
 =?us-ascii?Q?3m83LY5V0i7ldnyPhs/Vb3sRtJpEbuMKMLoDyMJl2ZE2E97MAZskDhXzZ2P9?=
 =?us-ascii?Q?zzoyNPlx8RCVCessIMzjEDFzt1tXWx1cxU6Bv/68ZigETMxteKhixHt5cJpR?=
 =?us-ascii?Q?VSJc9CDjzdoD3orjTVh9t4zOi7zv6Vk3gA99Z22pxwqsB7pJ4gPYLbqu5us5?=
 =?us-ascii?Q?lWVePJp9EoNrtX9o+7SbBPEdmlZn4R9Wb2bkIyYJlrCEpNd18pcgmhWXxTgG?=
 =?us-ascii?Q?0Ex082U1B5mUDmUyrlBCa06JwMpdyNmvqPzoCOmdHlAH7E7x6u1lJCTuWz/A?=
 =?us-ascii?Q?/uvhwnFBT0Njay/cFmGoi9DNsdt6M+EH+uWMCMsCeF1XmPXiGfR+5rhoba6b?=
 =?us-ascii?Q?jJae8HYDdJK76R7w/NqteRVwWh9hZLYnczkccsr8HZ5OaWcMph5+KW1MZE6P?=
 =?us-ascii?Q?AeqLo6+p8f4lKpxXwgTc0gUw41sllaeSVmHXJmQEjPI7lBJQHnMAOv4EqWS0?=
 =?us-ascii?Q?9sfzZBUy4uw0BVgNalUIfx5Jkg6qA2k83Qlm9tOUnnlS2EK3m3U10mKWFkKF?=
 =?us-ascii?Q?POkcsEtxqAZ87fl/xsUHl6BxKflHQ2gverNkJN+PEoi7T3ba/OzXs5Rghv7v?=
 =?us-ascii?Q?6J8pczVd4ex1GkGtE8lOome3YjjI4kZU5kNVNnFw5x1B1evdaOzFtp2pBWsX?=
 =?us-ascii?Q?jkc/hMtGoHSnj4sWjDwP3lrNUlmPpZuc+V2RhJ+OFMUKbExpYnedwesdsyoV?=
 =?us-ascii?Q?E69euVFCZUF22Fsy22dHhwNmlcavmqqmvCJvZ7HhnQjDzEPi67/xFSUVRg2o?=
 =?us-ascii?Q?2cTSjZmVdO+JT0aCWPjaF3K3xpIv1m9FGAuT/qB9+D+oa1bNRoDdk61SesuT?=
 =?us-ascii?Q?cQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	MZN3luBFhbSlm+FY+X+W6K6YlcpL8GMSG/xSFdZ2+2pJ9XLMTQXyaqxYM4JpdIa/MXEdDRRNkVNUn/BNTHmVPuDtGMAjXj/ruZIUfVjk2wpDp4o/Tb0qkruiYRQYOSECl6AzMwuJAb9zFN/VWVaa84vVac9x6dT6Ao92oAOoyiO8nGPH3JY9KEf+ClUHUjms5n/pzhF/tV2ssvuqjP63zMHa4/+O/jt4Z89QBIdaiu5V+Z9rT9DaMcHDA9+D0ySrqgxgkKeaduvajeWvQa6k8CZcz4WqqWmFKVfKOtfYjCjJqKu7n3unRNQwHkw/ZmtkbXqajCe9i9IHOvifqdJZjq0Usnm2WEZREEHtmok05Rbm5PiaqPprWkxJ4363AXNojd6zhpe58eH8LFx48kLwQI91lHTRi9Ow7FJGPITFBm9VNab7dgmUKywA/4sXKhHTMOiP2orOJSXcNCykR1pDyedV4T0sfAZwCa/YK0K/G/IXDRze5Zh0ooTU6sGd0JWv5OY97LA2WGrM6lFbAMr/jUpaX8uay5inlkrEGFD1blNyC7xiL4txuh+OlQmiAMdaWF4EDY3NSOWECvQTkX5Vrk1bASAu+ax6TvF42O8pDLE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e7103d24-e02a-41a1-b75f-08dc8bc1933a
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2024 15:57:49.6222
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: as4pOUNfT5CDtmn93cEwMw2E58gJ2K8wlswSkf6RbsrFn5ZW2uEc4I48wzS6aXM9mp+IBORMI2s0f87IaOMRUg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6673
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-13_09,2024-06-13_02,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0
 malwarescore=0 spamscore=0 mlxscore=0 phishscore=0 mlxlogscore=999
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2406130114
X-Proofpoint-ORIG-GUID: 5kRKsijjts2_CotOFTP1JxRmWfdBntVh
X-Proofpoint-GUID: 5kRKsijjts2_CotOFTP1JxRmWfdBntVh

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.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfsctl.c | 14 ++++++++++++--
>  fs/nfsd/nfsd.h   |  3 ++-
>  fs/nfsd/nfssvc.c | 18 ++----------------
>  3 files changed, 16 insertions(+), 19 deletions(-)
> 
> 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)
>  			return -EINVAL;
>  		trace_nfsd_ctl_threads(net, newthreads);
>  		mutex_lock(&nfsd_mutex);
> -		rv = nfsd_svc(newthreads, net, file->f_cred, NULL);
> +		rv = nfsd_svc(1, &newthreads, net, file->f_cred, NULL);
>  		mutex_unlock(&nfsd_mutex);
>  		if (rv < 0)
>  			return rv;
> @@ -481,6 +481,16 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
>  				goto out_free;
>  			trace_nfsd_ctl_pool_threads(net, i, nthreads[i]);
>  		}
> +
> +		/*
> +		 * 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?

Hi, how do you plan to decide this question?


> +		 */
> +		if (nthreads[0] == 0)
> +			nthreads[0] = 1;
> +
>  		rv = nfsd_set_nrthreads(i, nthreads, net);
>  		if (rv)
>  			goto out_free;
> @@ -1722,7 +1732,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
>  			scope = nla_data(attr);
>  	}
>  
> -	ret = nfsd_svc(nthreads, net, get_current_cred(), scope);
> +	ret = nfsd_svc(1, &nthreads, net, get_current_cred(), scope);
>  
>  out_unlock:
>  	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,
>  /*
>   * Function prototypes.
>   */
> -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);
>  int		nfsd_dispatch(struct svc_rqst *rqstp);
>  
>  int		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)

Since you are slightly changing the API contract for this publicly
visible function, now would be a good time to add a kdoc comment.


>  		}
>  	}
>  
> -	/*
> -	 * There must always be a thread in pool 0; the admin
> -	 * can't shut down NFS completely using pool_threads.
> -	 */
> -	if (nthreads[0] == 0)
> -		nthreads[0] = 1;
> -
>  	/* apply the new numbers */
>  	for (i = 0; i < n; i++) {
>  		err = svc_set_num_threads(nn->nfsd_serv,
> @@ -768,7 +761,7 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
>   * this is the first time nrservs is nonzero.
>   */
>  int
> -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)

Ditto: the patch changes the synopsis of nfsd_svc(), so I'd like a
kdoc comment to go with it.

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).

A kdoc comment from sunrpc_get_pool_mode() should also be added
in 4/5.


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

-- 
Chuck Lever