Message-ID: <20110919110729.33a876ca@tlielax.poochiereds.net>
Date:	Mon, 19 Sep 2011 11:07:29 -0400
From:	Jeff Layton <jlayton@...hat.com>
To:	Stanislav Kinsbursky <skinsbursky@...allels.com>
Cc:	"Trond.Myklebust@...app.com" <Trond.Myklebust@...app.com>,
	"linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
	Pavel Emelianov <xemul@...allels.com>,
	"neilb@...e.de" <neilb@...e.de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"bfields@...ldses.org" <bfields@...ldses.org>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with
 portmapper flag

On Mon, 19 Sep 2011 18:51:31 +0400
Stanislav Kinsbursky <skinsbursky@...allels.com> wrote:

> 19.09.2011 18:08, Jeff Layton пишет:
> > On Tue, 13 Sep 2011 22:13:51 +0400
> > Stanislav Kinsbursky<skinsbursky@...allels.com>  wrote:
> >
> >> This new flag ("setup_rpcbind) will be used to detect, that new service will
> >> send portmapper register calls. For such services we will create rpcbind
> >> clients and remove all stale portmap registrations.
> >> Also, svc_rpcb_cleanup() will be set as sv_shutdown callback for such services
> >> in case of this field wasn't initialized earlier. This will allow to destroy
> >> rpcbind clients when no other users of them left.
> >>
> >> Signed-off-by: Stanislav Kinsbursky<skinsbursky@...allels.com>
> >>
> >> ---
> >>   include/linux/sunrpc/svc.h |    2 ++
> >>   net/sunrpc/svc.c           |   21 ++++++++++++++-------
> >>   2 files changed, 16 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> >> index 223588a..528952a 100644
> >> --- a/include/linux/sunrpc/svc.h
> >> +++ b/include/linux/sunrpc/svc.h
> >> @@ -402,11 +402,13 @@ struct svc_procedure {
> >>    * Function prototypes.
> >>    */
> >>   struct svc_serv *svc_create(struct svc_program *, unsigned int,
> >> +			    int setup_rpcbind,
> > 				^^^
> > 			Instead of adding this parameter, why not
> > 			base this on the vs_hidden flag in the
> > 			svc_version? IOW, have a function that looks at
> > 			all the svc_versions for a particular
> > 			svc_program, and returns "true" if any of them
> > 			have vs_hidden unset? The mechanism you're
> > 			proposing here has the potential to be out of
> > 			sync with the vs_hidden flag.
> >
> 
> Could you, please, clarify me this vs_hidden flag?
> I understand, that it's used to avoid portmap registration.
> But as I see, it's set only for nfs_callback_version1. But this svc_version is a 
> part of nfs4_callback_program with nfs_callback_version4, which is not hidden.
> Does this flag is missed here? If not, how we can return "true" from your 
> proposed function if any of them have vs_hidden unset?
> 
> Also sockets for this program are created with SVC_SOCK_ANONYMOUS flag and we 
> will not register any of this program versions with portmapper.
> Thus, from my pow, this vs_hidden flag affects only svc_unregister. And only 
> nfs_callback_version1. This looks really strange.
> 
> I.e. if we use this flag only for passing through this versions during 
> svc_(un)register, and we actually also want to pass through 
> nfs_callback_version4 as well (but just missed this vs_hidden flag for it), then 
> with current patch-set we can move this flag from (vs_hidden) svc_version to 
> svc_program and check it during svc_create instead of my home-brew 
> "setup_rpcbind" variable.
> 

Agreed. The current situation is a mess, which is why I suggested a
cleanup and overhaul before you do this...

The vs_hidden flag is intended to show that a particular program
version should not be registered with (or unregistered from) the
portmapper. Unfortunately, nothing looks at vs_hidden during
registration time, only when unregistering (as you mention).

It's quite possible that several svc_versions declared in the kernel do
not have this set correctly. One thing that would be good is to audit
each of those.

We currently rely on SVC_SOCK_ANONYMOUS for registration, but that
wasn't its original intent. It's was just convenient to use it there
too.

SVC_SOCK_ANONYMOUS was (as best I can tell) originally intended for use
on temporary sockets that we establish on receive. So for
instance...when a client connects to nfsd, we need to create a new
socket for nfsd, but obviously we don't want to register that socket
with the portmapper (since nfsd should already be registered there).
SVC_SOCK_ANONYMOUS ensures that that socket is not registered.

The whole scheme could probably use a fundamental re-think. I'm not
sure I have a great idea to propose in lieu of it, but I think adding
yet another flag here is probably not the best way to go.

> > 			Also, if you're adding an argument to a
> > 			function like this, you you really ought to
> > 			change the callers in the same patch. Otherwise
> > 			you'll cause a build break if someone tries to
> > 			bisect and ends up between the patch that
> > 			changes the function and the one that changes
> > 			the callers.
> >
> >>   			void (*shutdown)(struct svc_serv *));
> >>   struct svc_rqst *svc_prepare_thread(struct svc_serv *serv,
> >>   					struct svc_pool *pool);
> >>   void		   svc_exit_thread(struct svc_rqst *);
> >>   struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
> >> +			int setup_rpcbind,
> >>   			void (*shutdown)(struct svc_serv *),
> >>   			svc_thread_fn, struct module *);
> >>   int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
> >> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> >> index f31e5cc..03231d5 100644
> >> --- a/net/sunrpc/svc.c
> >> +++ b/net/sunrpc/svc.c
> >> @@ -378,7 +378,7 @@ static void svc_rpcb_cleanup(struct svc_serv *serv)
> >>    */
> >>   static struct svc_serv *
> >>   __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >> -	     void (*shutdown)(struct svc_serv *serv))
> >> +	     int setup_rpcbind, void (*shutdown)(struct svc_serv *serv))
> >>   {
> >>   	struct svc_serv	*serv;
> >>   	unsigned int vers;
> >> @@ -437,29 +437,36 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >>   		spin_lock_init(&pool->sp_lock);
> >>   	}
> >>
> >> -	/* Remove any stale portmap registrations */
> >> -	svc_unregister(serv);
> >> +	if (setup_rpcbind) {
> >> +	       	if (svc_rpcb_setup(serv)<  0) {
> >> +			kfree(serv->sv_pools);
> >> +			kfree(serv);
> >> +			return NULL;
> >> +		}
> >> +		if (!serv->sv_shutdown)
> >> +			serv->sv_shutdown = svc_rpcb_cleanup;
> >> +	}
> >>
> >>   	return serv;
> >>   }
> >>
> >>   struct svc_serv *
> >>   svc_create(struct svc_program *prog, unsigned int bufsize,
> >> -	   void (*shutdown)(struct svc_serv *serv))
> >> +	   int setup_rpcbind, void (*shutdown)(struct svc_serv *serv))
> >>   {
> >> -	return __svc_create(prog, bufsize, /*npools*/1, shutdown);
> >> +	return __svc_create(prog, bufsize, /*npools*/1, setup_rpcbind, shutdown);
> >>   }
> >>   EXPORT_SYMBOL_GPL(svc_create);
> >>
> >>   struct svc_serv *
> >>   svc_create_pooled(struct svc_program *prog, unsigned int bufsize,
> >> -		  void (*shutdown)(struct svc_serv *serv),
> >> +		  int setup_rpcbind, void (*shutdown)(struct svc_serv *serv),
> >>   		  svc_thread_fn func, struct module *mod)
> >>   {
> >>   	struct svc_serv *serv;
> >>   	unsigned int npools = svc_pool_map_get();
> >>
> >> -	serv = __svc_create(prog, bufsize, npools, shutdown);
> >> +	serv = __svc_create(prog, bufsize, npools, setup_rpcbind, shutdown);
> >>
> >>   	if (serv != NULL) {
> >>   		serv->sv_function = func;
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> >> the body of a message to majordomo@...r.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> 
> 


-- 
Jeff Layton <jlayton@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists