| Message ID | 20110913181351.3961.70207.stgit@localhost6.localdomain6 |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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. 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
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. > > 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@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > >
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.
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.
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.
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, 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;
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(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html