Message-ID: <20110919141101.46303e53@tlielax.poochiereds.net>
Date:	Mon, 19 Sep 2011 14:11:01 -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 19:42:12 +0400
Stanislav Kinsbursky <skinsbursky@...allels.com> wrote:

> 19.09.2011 19:07, Jeff Layton пишет:
> > 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.
> >
> 
> Ok, thank you, Jeff.
> It looks like no mentions about portmapper are present in RFC's for NFS versions 
> 4.* after a brief look.
> This SVC_SOCK_ANONYMOUS is understandable and can't be removed with this 
> patch-set from my pow.
> But now I strongly believe, that we can move this vs_hidden flag from 
> svc_version to svc_program structure and set it for both NFSv4.* programs.
> Hope, someone else will confirm of refute this statement.
> 

The problem is nfsd. In principle, there's no real reason we have to
register NFSv4 with the portmapper at all. One could envision a
setup where a v4-only server doesn't need to run rpcbind at all. Making
it per-program may hamstring you from doing that later.

It think it would be a good thing to keep it per-version, and it's
trivial to write a routine to do what I've described. svc_creates only
happen rarely.

Just walk the pg_vers array for the program and look at each vs_hidden
value. Return true when you hit one that has vs_hidden unset. Return
false if none do. Then use that return value to replace your new flag
in this patch.

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