Re: [PATCH v3 04/11] SUNRPC: parametrize svc creation calls withportmapper flag

[Stanislav Kinsbursky]

19.09.2011 19:07, Jeff Layton ÐÐÑÐÑ:
> On Mon, 19 Sep 2011 18:51:31 +0400
> Stanislav Kinsbursky<skinsbursky@xxxxxxxxxxxxx>  wrote:
>
> > 19.09.2011 18:08, Jeff Layton ÐÐÑÐÑ:
> > > On Tue, 13 Sep 2011 22:13:51 +0400
> > > Stanislav Kinsbursky<skinsbursky@xxxxxxxxxxxxx>   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@xxxxxxxxxxxxx>
> > > >
> > > > ---
> > > >    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.




--
Best regards,
Stanislav Kinsbursky
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/