Received-SPF: pass (google.com: domain of linux-nfs+bounces-1434-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 25 Jan 2024 15:56:00 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Josef Bacik <josef@toxicpanda.com>
Cc: linux-nfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH v2 03/13] sunrpc: pass in the sv_stats struct through
 svc_create*
Message-ID: <ZbLK4BsvReiFpCUo@tissot.1015granger.net>
References: <cover.1706212207.git.josef@toxicpanda.com>
 <ff6afd3ab9a70bf5ab90872497068719f2c1ec03.1706212208.git.josef@toxicpanda.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ff6afd3ab9a70bf5ab90872497068719f2c1ec03.1706212208.git.josef@toxicpanda.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?LVR28/nzcPlLjnRUkxeL/SKlLcdF0qqKiiXUjeK3pkn5odKCGPkEIq4KRZv3?=
 =?us-ascii?Q?CfQuYL/v/NHK7/k5jsykGp/RXAlmx5wDMgUh9sio+Zy9wBxIGiBobAHybD4O?=
 =?us-ascii?Q?Vsp3BTApBK7uuqmov/yhV+VBgq3bSTdX/jCMAfF/AfMZR7DVbUYfZyW66wbD?=
 =?us-ascii?Q?q6CoFSWTeywjue9BxQs603agHbnvSX4IfT6pphNGXM1b79xG4qGlJm1BoM0g?=
 =?us-ascii?Q?tx79manGNIKBVGQazhkBCylQO1yo628vpK5Y/ZVMpJFroDtiVyD9x23eAov8?=
 =?us-ascii?Q?V2b1DVHrEnnz4wEs0ewzQpHu5Y7ZNZcPsAIXAyjqSIHpujr46x5BDDkBdZRf?=
 =?us-ascii?Q?XOVsvLqDdckROK4HkcOg8nJpJHq89LrSZRaCX5/kK4We9hw8e2dS0AKMlYbt?=
 =?us-ascii?Q?/K6aQCTX+Z8c4YJ0WMVfacAY/tOB/GO8I24V26hAB2JNdIXxzZrtiIxx+qCB?=
 =?us-ascii?Q?00IPveMsLT7oAC5KPZ1BbglcIxCUxqvDvFjGg4ckKDPAhrznV0maLCcJ5rVJ?=
 =?us-ascii?Q?zv68ZLu/cvfzusQl2fDgqVJVoLpLpbrI4WQFjt5x6ZUxmYvt0ILNcEVpovB4?=
 =?us-ascii?Q?Eacs4hLxHbS3OEAgwQUw0O5cXakRtAZ7rHSLMVI5stp5C/dQjhgB51SYRMtq?=
 =?us-ascii?Q?D22/QSs2a0OkLgglrB1spQGznbMbwN8HEkmFTYOlm/b1FtKmy4DHSR23NrPM?=
 =?us-ascii?Q?nQE94JHEmYp0idlGv+EfsCMw9ZaX4tTDpBHcg7Xmc5eP/yzQmEsBFPwxLaa1?=
 =?us-ascii?Q?wELQTQiLD6ZmBa46GY8ehbpUgpKsQR863s3YcAF2WY4MbpLQAAFOy1ewm8a6?=
 =?us-ascii?Q?cKJeAZKTeUuMEaleBEkc5sJJEAYQVlP1PKCe6osOWRoy2Cz537XKXg5gbxjS?=
 =?us-ascii?Q?6y8vdlxaCTahckDpMGueFkuVbfyc6htyFedj0u5yucqIcZCPJQAdTg25ZTsF?=
 =?us-ascii?Q?J62MNm/sClm9Y0vr6yD/HzSMNyPtg+T5Xnw/4p+zFFzUx5I6spUBq4JddsFB?=
 =?us-ascii?Q?7T0K9+6EvXSiQe2dLRixYPvt4FNgnOZ4jUcWKXPdzbf0t4ArYBi8N6AdPW4j?=
 =?us-ascii?Q?JyYhzryAZjW4M6tOl3OHqvXbIANkZjH5nNR9zeaDBA8TpDM/LOeKWstzwA0E?=
 =?us-ascii?Q?4Vd1M1RYhwRz9JFZUPtzdCxPvqifNozhlPacJXCS41hwrb3dwepkvN2FGo+4?=
 =?us-ascii?Q?7/BuA8SbvGjKZWfjBmFnd7Jt7wPcTSoBjzE7gm99rsNlp9vw9fBgnn6ZmmzU?=
 =?us-ascii?Q?TjKmYc5JpHGVvEV812lE/SWjXoDFpbYlQo2CvSXdDi6rvg9xjkf13AGQ5tBG?=
 =?us-ascii?Q?TpdJEgH31+tJ5BA5tueh0Zu/3OOUCucYYJsq/D11A0JuCDF2GEfU0UWM/new?=
 =?us-ascii?Q?aIkLNLNrkN+cwYXd2pS6/Wj2oKBeU0X/Knpl34Xk0Ni2W+bBTvX4yd0hQLIz?=
 =?us-ascii?Q?uLhPZG263UjVrWd2QWOBPNt7+7Gkx9zKEqnfqpKuNZ0qFopVgPMnKCqvFfHA?=
 =?us-ascii?Q?BLAOqPNYa0ByN3cXM9VGYI8WO/5O/ngdfwUEUHxHxcMpD0IEB60O5ET9EEpr?=
 =?us-ascii?Q?L94XWyxMPwJx9AK8F84SgGEK7+rl/auJwLd04Fvyv3dGypCAf/fqgchHC+UQ?=
 =?us-ascii?Q?rQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	0ZPsSJKRrQ7ptqxVBGHWCOZcyV5GG8J2+RjuLdqvfWVyHGtWVFaM3d/cexFu8kYbSiSRxgNh9Xova3o6L0/IDZ6beqg+wlK2/YDdweZxtmrSoBmtb66mCh1ZsI9mi12UkaeGc0FxGJqEQoDY03UxkZg8033yF0JXEB+rB8W3tXa7dl0pbxkxRGWXwEuNSv4oQiRtdM/a/9EwDyskPxLkJq4NGzTUnds/CrF2EORJ3DByFjIDcnSwXJz7PchsTcElV+G+JzJB+ypQJg3Ll5OZulSr02X/ZJFuLX7PrbP3O7nGskqXXKWjpvAP1JXHPOYJHGC04AqpLcPalfJBwJoJcbukmqtUUlscm1kWrWfMoSJqyo13KeR/XZApyY0zk86lRYdFoi4iTrk4YV8NrJ/by63PAfc6/l1pyvd6nBn31CqIaxHoul1M5HyqHlOD6aCYcvInkaxItqOBasxue2o4l2/0pKUTAVjIZMt3cRvs/4AtJv1fjOEk0Db3lLONmYs3tBITwZOoy+u3tGPVF93CEjeMI+fwHqVdmzZUZpX66ohk5IUH4jps2JvOWxWXNJOwL9jwmTiQPgbL/5VSj4nPu98YZX57TbSmBw5yNfacjSE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 61d55571-86f1-4c3f-8ed7-08dc1de80ab0
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jan 2024 20:56:03.0125
 (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: goisURuiysMAKMZE7VrjR0RxA8lERgXNqQLW8ruN9Zio8BYHg2W+wmv7JU0oA6DbfZoaiLHCVe4Oekd//ZzFUw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB4298
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-25_13,2024-01-25_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999
 malwarescore=0 mlxscore=0 phishscore=0 bulkscore=0 suspectscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401250151
X-Proofpoint-GUID: dx7Eg0xv8DDSUc23gfYbkLk7PnifyOh9
X-Proofpoint-ORIG-GUID: dx7Eg0xv8DDSUc23gfYbkLk7PnifyOh9

On Thu, Jan 25, 2024 at 02:53:13PM -0500, Josef Bacik wrote:
> Since only one service actually reports the rpc stats there's not much
> of a reason to have a pointer to it in the svc_program struct.  Adjust
> the svc_create* functions to take the sv_stats as an argument and pass
> the struct through there as desired instead of getting it from the
> svc_program->pg_stats.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/lockd/svc.c             |  2 +-
>  fs/nfs/callback.c          |  2 +-
>  fs/nfsd/nfssvc.c           |  3 ++-
>  include/linux/sunrpc/svc.h |  8 ++++----
>  net/sunrpc/svc.c           | 17 ++++++++++-------
>  5 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index ab8042a5b895..8fbbfc9aad69 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -337,7 +337,7 @@ static int lockd_get(void)
>  		nlm_timeout = LOCKD_DFLT_TIMEO;
>  	nlmsvc_timeout = nlm_timeout * HZ;
>  
> -	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, lockd);
> +	serv = svc_create(&nlmsvc_program, NULL, LOCKD_BUFSIZE, lockd);
>  	if (!serv) {
>  		printk(KERN_WARNING "lockd_up: create service failed\n");
>  		return -ENOMEM;
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 8adfcd4c8c1a..4d56b4e73525 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -202,7 +202,7 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)
>  	if (minorversion)
>  		return ERR_PTR(-ENOTSUPP);
>  #endif
> -	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
> +	serv = svc_create(&nfs4_callback_program, NULL, NFS4_CALLBACK_BUFSIZE,
>  			  threadfn);
>  	if (!serv) {
>  		printk(KERN_ERR "nfs_callback_create_svc: create service failed\n");
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index a0b117107e86..d640f893021a 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -661,7 +661,8 @@ int nfsd_create_serv(struct net *net)
>  	if (nfsd_max_blksize == 0)
>  		nfsd_max_blksize = nfsd_get_default_max_blksize();
>  	nfsd_reset_versions(nn);
> -	serv = svc_create_pooled(&nfsd_program, nfsd_max_blksize, nfsd);
> +	serv = svc_create_pooled(&nfsd_program, &nfsd_svcstats,
> +				 nfsd_max_blksize, nfsd);
>  	if (serv == NULL)
>  		return -ENOMEM;
>  
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 67cf1c9efd80..2a1447fa5ef2 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -402,8 +402,8 @@ struct svc_procedure {
>  int svc_rpcb_setup(struct svc_serv *serv, struct net *net);
>  void svc_rpcb_cleanup(struct svc_serv *serv, struct net *net);
>  int svc_bind(struct svc_serv *serv, struct net *net);
> -struct svc_serv *svc_create(struct svc_program *, unsigned int,
> -			    int (*threadfn)(void *data));
> +struct svc_serv *svc_create(struct svc_program *, struct svc_stat *,
> +			    unsigned int, int (*threadfn)(void *data));
>  struct svc_rqst *svc_rqst_alloc(struct svc_serv *serv,
>  					struct svc_pool *pool, int node);
>  bool		   svc_rqst_replace_page(struct svc_rqst *rqstp,
> @@ -411,8 +411,8 @@ bool		   svc_rqst_replace_page(struct svc_rqst *rqstp,
>  void		   svc_rqst_release_pages(struct svc_rqst *rqstp);
>  void		   svc_rqst_free(struct svc_rqst *);
>  void		   svc_exit_thread(struct svc_rqst *);
> -struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
> -				     int (*threadfn)(void *data));
> +struct svc_serv *  svc_create_pooled(struct svc_program *, struct svc_stat *,
> +				     unsigned int, int (*threadfn)(void *data));
>  int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
>  int		   svc_pool_stats_open(struct svc_info *si, struct file *file);
>  void		   svc_process(struct svc_rqst *rqstp);
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index d2e6f3d59218..f76ef8a3dd43 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -451,8 +451,8 @@ __svc_init_bc(struct svc_serv *serv)
>   * Create an RPC service
>   */
>  static struct svc_serv *
> -__svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> -	     int (*threadfn)(void *data))
> +__svc_create(struct svc_program *prog, struct svc_stat *stats,
> +	     unsigned int bufsize, int npools, int (*threadfn)(void *data))
>  {
>  	struct svc_serv	*serv;
>  	unsigned int vers;
> @@ -463,7 +463,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  		return NULL;
>  	serv->sv_name      = prog->pg_name;
>  	serv->sv_program   = prog;
> -	serv->sv_stats     = prog->pg_stats;
> +	serv->sv_stats     = stats;
>  	if (bufsize > RPCSVC_MAXPAYLOAD)
>  		bufsize = RPCSVC_MAXPAYLOAD;
>  	serv->sv_max_payload = bufsize? bufsize : 4096;
> @@ -521,34 +521,37 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  /**
>   * svc_create - Create an RPC service
>   * @prog: the RPC program the new service will handle
> + * @stats: the stats struct if desired
>   * @bufsize: maximum message size for @prog
>   * @threadfn: a function to service RPC requests for @prog
>   *
>   * Returns an instantiated struct svc_serv object or NULL.
>   */

Here's the only minor quibble I have so far:

svc_create's callers don't use stats, so maybe you don't need
to add an @stats argument for this API.

Fwiw, I haven't gotten all the way to the end of the series yet.


> -struct svc_serv *svc_create(struct svc_program *prog, unsigned int bufsize,
> -			    int (*threadfn)(void *data))
> +struct svc_serv *svc_create(struct svc_program *prog, struct svc_stat *stats,
> +			    unsigned int bufsize, int (*threadfn)(void *data))
>  {
> -	return __svc_create(prog, bufsize, 1, threadfn);
> +	return __svc_create(prog, stats, bufsize, 1, threadfn);
>  }
>  EXPORT_SYMBOL_GPL(svc_create);
>  
>  /**
>   * svc_create_pooled - Create an RPC service with pooled threads
>   * @prog: the RPC program the new service will handle
> + * @stats: the stats struct if desired
>   * @bufsize: maximum message size for @prog
>   * @threadfn: a function to service RPC requests for @prog
>   *
>   * Returns an instantiated struct svc_serv object or NULL.
>   */
>  struct svc_serv *svc_create_pooled(struct svc_program *prog,
> +				   struct svc_stat *stats,
>  				   unsigned int bufsize,
>  				   int (*threadfn)(void *data))
>  {
>  	struct svc_serv *serv;
>  	unsigned int npools = svc_pool_map_get();
>  
> -	serv = __svc_create(prog, bufsize, npools, threadfn);
> +	serv = __svc_create(prog, stats, bufsize, npools, threadfn);
>  	if (!serv)
>  		goto out_err;
>  	return serv;
> -- 
> 2.43.0
> 
> 

-- 
Chuck Lever