Re: [PATCH v12 12/24] SUNRPC: replace program list with program array

[Mike Snitzer <snitzer@kernel.org>]

On Wed, Aug 21, 2024 at 04:40:10PM -0400, Mike Snitzer wrote:
> On Wed, Aug 21, 2024 at 02:31:07PM -0400, Jeff Layton wrote:
> > On Mon, 2024-08-19 at 14:17 -0400, Mike Snitzer wrote:
> > > From: NeilBrown <neil@brown.name>
> > > 
> > > A service created with svc_create_pooled() can be given a linked list of
> > > programs and all of these will be served.
> > > 
> > > Using a linked list makes it cumbersome when there are several programs
> > > that can be optionally selected with CONFIG settings.
> > > 
> > > After this patch is applied, API consumers must use only
> > > svc_create_pooled() when creating an RPC service that listens for more
> > > than one RPC program.
> > > 
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > ---
> > >  fs/nfsd/nfsctl.c           |  2 +-
> > >  fs/nfsd/nfsd.h             |  2 +-
> > >  fs/nfsd/nfssvc.c           | 67 +++++++++++++++++--------------------
> > >  include/linux/sunrpc/svc.h |  7 ++--
> > >  net/sunrpc/svc.c           | 68 ++++++++++++++++++++++----------------
> > >  net/sunrpc/svc_xprt.c      |  2 +-
> > >  net/sunrpc/svcauth_unix.c  |  3 +-
> > >  7 files changed, 79 insertions(+), 72 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > > index 1c9e5b4bcb0a..64c1b4d649bc 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -2246,7 +2246,7 @@ static __net_init int nfsd_net_init(struct net *net)
> > >  	if (retval)
> > >  		goto out_repcache_error;
> > >  	memset(&nn->nfsd_svcstats, 0, sizeof(nn->nfsd_svcstats));
> > > -	nn->nfsd_svcstats.program = &nfsd_program;
> > > +	nn->nfsd_svcstats.program = &nfsd_programs[0];
> > >  	for (i = 0; i < sizeof(nn->nfsd_versions); i++)
> > >  		nn->nfsd_versions[i] = nfsd_support_version(i);
> > >  	for (i = 0; i < sizeof(nn->nfsd4_minorversions); i++)
> > > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > > index f87a359d968f..232a873dc53a 100644
> > > --- a/fs/nfsd/nfsd.h
> > > +++ b/fs/nfsd/nfsd.h
> > > @@ -85,7 +85,7 @@ struct nfsd_genl_rqstp {
> > >  	u32			rq_opnum[NFSD_MAX_OPS_PER_COMPOUND];
> > >  };
> > >  
> > > -extern struct svc_program	nfsd_program;
> > > +extern struct svc_program	nfsd_programs[];
> > >  extern const struct svc_version	nfsd_version2, nfsd_version3, nfsd_version4;
> > >  extern struct mutex		nfsd_mutex;
> > >  extern spinlock_t		nfsd_drc_lock;
> > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > > index 1bec3a53e35f..5f8680ab1013 100644
> > > --- a/fs/nfsd/nfssvc.c
> > > +++ b/fs/nfsd/nfssvc.c
> > > @@ -35,7 +35,6 @@
> > >  #define NFSDDBG_FACILITY	NFSDDBG_SVC
> > >  
> > >  atomic_t			nfsd_th_cnt = ATOMIC_INIT(0);
> > > -extern struct svc_program	nfsd_program;
> > >  static int			nfsd(void *vrqstp);
> > >  #if defined(CONFIG_NFSD_V2_ACL) || defined(CONFIG_NFSD_V3_ACL)
> > >  static int			nfsd_acl_rpcbind_set(struct net *,
> > > @@ -87,16 +86,6 @@ static const struct svc_version *localio_versions[] = {
> > >  
> > >  #define NFSD_LOCALIO_NRVERS		ARRAY_SIZE(localio_versions)
> > >  
> > > -static struct svc_program	nfsd_localio_program = {
> > > -	.pg_prog		= NFS_LOCALIO_PROGRAM,
> > > -	.pg_nvers		= NFSD_LOCALIO_NRVERS,
> > > -	.pg_vers		= localio_versions,
> > > -	.pg_name		= "nfslocalio",
> > > -	.pg_class		= "nfsd",
> > > -	.pg_authenticate	= &svc_set_client,
> > > -	.pg_init_request	= svc_generic_init_request,
> > > -	.pg_rpcbind_set		= svc_generic_rpcbind_set,
> > > -};
> > >  #endif /* CONFIG_NFSD_LOCALIO */
> > >  
> > >  #if defined(CONFIG_NFSD_V2_ACL) || defined(CONFIG_NFSD_V3_ACL)
> > > @@ -109,23 +98,9 @@ static const struct svc_version *nfsd_acl_version[] = {
> > >  # endif
> > >  };
> > >  
> > > -#define NFSD_ACL_MINVERS            2
> > > +#define NFSD_ACL_MINVERS	2
> > >  #define NFSD_ACL_NRVERS		ARRAY_SIZE(nfsd_acl_version)
> > >  
> > > -static struct svc_program	nfsd_acl_program = {
> > > -#if IS_ENABLED(CONFIG_NFSD_LOCALIO)
> > > -	.pg_next		= &nfsd_localio_program,
> > > -#endif /* CONFIG_NFSD_LOCALIO */
> > > -	.pg_prog		= NFS_ACL_PROGRAM,
> > > -	.pg_nvers		= NFSD_ACL_NRVERS,
> > > -	.pg_vers		= nfsd_acl_version,
> > > -	.pg_name		= "nfsacl",
> > > -	.pg_class		= "nfsd",
> > > -	.pg_authenticate	= &svc_set_client,
> > > -	.pg_init_request	= nfsd_acl_init_request,
> > > -	.pg_rpcbind_set		= nfsd_acl_rpcbind_set,
> > > -};
> > > -
> > 
> > You just added this code in patch 11.
> > 
> > I think it'd be clearer to reverse the order of patches 11 and 12. That
> > way you don't have this interim version of the localio program that
> > lives on the linked list.
> 
> You may recall that these changes developed over time.  This patch
> from Neil came after because he saw a way to make things better.
> Inverting/inserting patches earlier to reduce "churn" loses the
> development history.  (It also introduces possibility to cause some
> regression or break bisect-ability.)
> 
> It'd be one thing if I wrote every patch in this series, but I built
> on others' work and then others upon that.  Preserving development
> history and attribution is something I've always tried to do.
> Otherwise changes get attributed to the wrong person.
> 
> SO in this instance, I'd prefer to keep Neil's contribution as-is and
> avoid switching patch 12 and 11.

Looked closer, Neil is a co-developer on the preceding patch (happened
as side-effect of a different rebase some iterations ago).  So I'm
fine with switching patches 11 and 12 like you suggested.  It does
make things cleaner.

Thanks,
Mike