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

[Stanislav Kinsbursky <skinsbursky@parallels.com>]

19.09.2011 22:11, Jeff Layton пишет:
> On Mon, 19 Sep 2011 19:42:12 +0400
> Stanislav Kinsbursky<skinsbursky@parallels.com>  wrote:
>
>> 19.09.2011 19:07, Jeff Layton пишет:
>>> On Mon, 19 Sep 2011 18:51:31 +0400
>>> Stanislav Kinsbursky<skinsbursky@parallels.com>   wrote:
>>>
>>>> 19.09.2011 18:08, Jeff Layton пишет:
>>>>> On Tue, 13 Sep 2011 22:13:51 +0400
>>>>> Stanislav Kinsbursky<skinsbursky@parallels.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@parallels.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.
>

Done. I've sent v4 patch-set.

-- 
Best regards,
Stanislav Kinsbursky