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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > > ---
> > > 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
