* [PATCH v3 01/11] SUNRPC: introduce helpers for reference counted rpcbind clients
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
@ 2011-09-13 18:13 ` Stanislav Kinsbursky
2011-09-13 18:13 ` [PATCH v3 02/11] SUNRPC: use rpcbind reference counting helpers Stanislav Kinsbursky
` (9 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:13 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
This helpers will be used for dynamical creation and destruction of rpcbind
clients.
Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
clients has been created already, then we just increase rpcb_users.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
net/sunrpc/rpcb_clnt.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 50 insertions(+), 0 deletions(-)
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index e45d2fb..3f2bb8a 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
static struct rpc_clnt * rpcb_local_clnt;
static struct rpc_clnt * rpcb_local_clnt4;
+DEFINE_SPINLOCK(rpcb_clnt_lock);
+unsigned int rpcb_users;
+
struct rpcbind_args {
struct rpc_xprt * r_xprt;
@@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
kfree(map);
}
+static int rpcb_get_local(void)
+{
+ spin_lock(&rpcb_clnt_lock);
+ if (rpcb_users)
+ rpcb_users++;
+ spin_unlock(&rpcb_clnt_lock);
+
+ return rpcb_users;
+}
+
+void rpcb_put_local(void)
+{
+ struct rpc_clnt *clnt = rpcb_local_clnt;
+ struct rpc_clnt *clnt4 = rpcb_local_clnt4;
+ int shutdown;
+
+ spin_lock(&rpcb_clnt_lock);
+ if (--rpcb_users == 0) {
+ rpcb_local_clnt = NULL;
+ rpcb_local_clnt4 = NULL;
+ }
+ shutdown = !rpcb_users;
+ spin_unlock(&rpcb_clnt_lock);
+
+ if (shutdown) {
+ /*
+ * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
+ */
+ if (clnt4)
+ rpc_shutdown_client(clnt4);
+ if (clnt)
+ rpc_shutdown_client(clnt);
+ }
+ return;
+}
+
+static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
+{
+ /* Protected by rpcb_create_local_mutex */
+ rpcb_local_clnt = clnt;
+ rpcb_local_clnt4 = clnt4;
+ rpcb_users++;
+ dprintk("RPC: created new rpcb local clients (rpcb_local_clnt: "
+ "0x%p, rpcb_local_clnt4: 0x%p)\n", rpcb_local_clnt,
+ rpcb_local_clnt4);
+}
+
/*
* Returns zero on success, otherwise a negative errno value
* is returned.
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 02/11] SUNRPC: use rpcbind reference counting helpers
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
2011-09-13 18:13 ` [PATCH v3 01/11] SUNRPC: introduce helpers for reference counted rpcbind clients Stanislav Kinsbursky
@ 2011-09-13 18:13 ` Stanislav Kinsbursky
2011-09-13 18:13 ` [PATCH v3 03/11] SUNRPC: introduce svc helpers for prepairing rpcbind infrastructure Stanislav Kinsbursky
` (8 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:13 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
All is simple: we just increase users counter if rpcbind clients has been
created already. Otherwise we create them and set users counter to 1.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
net/sunrpc/rpcb_clnt.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 3f2bb8a..b3ac194 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -255,9 +255,7 @@ static int rpcb_create_local_unix(void)
clnt4 = NULL;
}
- /* Protected by rpcb_create_local_mutex */
- rpcb_local_clnt = clnt;
- rpcb_local_clnt4 = clnt4;
+ rpcb_set_local(clnt, clnt4);
out:
return result;
@@ -309,9 +307,7 @@ static int rpcb_create_local_net(void)
clnt4 = NULL;
}
- /* Protected by rpcb_create_local_mutex */
- rpcb_local_clnt = clnt;
- rpcb_local_clnt4 = clnt4;
+ rpcb_set_local(clnt, clnt4);
out:
return result;
@@ -326,11 +322,11 @@ static int rpcb_create_local(void)
static DEFINE_MUTEX(rpcb_create_local_mutex);
int result = 0;
- if (rpcb_local_clnt)
+ if (rpcb_get_local())
return result;
mutex_lock(&rpcb_create_local_mutex);
- if (rpcb_local_clnt)
+ if (rpcb_get_local())
goto out;
if (rpcb_create_local_unix() != 0)
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 03/11] SUNRPC: introduce svc helpers for prepairing rpcbind infrastructure
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
2011-09-13 18:13 ` [PATCH v3 01/11] SUNRPC: introduce helpers for reference counted rpcbind clients Stanislav Kinsbursky
2011-09-13 18:13 ` [PATCH v3 02/11] SUNRPC: use rpcbind reference counting helpers Stanislav Kinsbursky
@ 2011-09-13 18:13 ` Stanislav Kinsbursky
2011-09-13 18:13 ` [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag Stanislav Kinsbursky
` (7 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:13 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
This helpers will be used only for those services, that will send portmapper
registration calls.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
include/linux/sunrpc/clnt.h | 2 ++
net/sunrpc/rpcb_clnt.c | 2 +-
net/sunrpc/svc.c | 18 ++++++++++++++++++
3 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index db7bcaf..1eb437d 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -135,6 +135,8 @@ void rpc_shutdown_client(struct rpc_clnt *);
void rpc_release_client(struct rpc_clnt *);
void rpc_task_release_client(struct rpc_task *);
+int rpcb_create_local(void);
+void rpcb_put_local(void);
int rpcb_register(u32, u32, int, unsigned short);
int rpcb_v4_register(const u32 program, const u32 version,
const struct sockaddr *address,
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index b3ac194..d42b642 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -317,7 +317,7 @@ out:
* Returns zero on success, otherwise a negative errno value
* is returned.
*/
-static int rpcb_create_local(void)
+int rpcb_create_local(void)
{
static DEFINE_MUTEX(rpcb_create_local_mutex);
int result = 0;
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 6a69a11..f31e5cc 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -354,6 +354,24 @@ svc_pool_for_cpu(struct svc_serv *serv, int cpu)
return &serv->sv_pools[pidx % serv->sv_nrpools];
}
+static int svc_rpcb_setup(struct svc_serv *serv)
+{
+ int err;
+
+ err = rpcb_create_local();
+ if (err)
+ return err;
+
+ /* Remove any stale portmap registrations */
+ svc_unregister(serv);
+ return 0;
+}
+
+static void svc_rpcb_cleanup(struct svc_serv *serv)
+{
+ svc_unregister(serv);
+ rpcb_put_local();
+}
/*
* Create an RPC service
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
` (2 preceding siblings ...)
2011-09-13 18:13 ` [PATCH v3 03/11] SUNRPC: introduce svc helpers for prepairing rpcbind infrastructure Stanislav Kinsbursky
@ 2011-09-13 18:13 ` Stanislav Kinsbursky
2011-09-19 14:08 ` Jeff Layton
2011-09-13 18:13 ` [PATCH v3 05/11] SUNRPC: cleanup service destruction Stanislav Kinsbursky
` (6 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:13 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
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,
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;
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag
2011-09-13 18:13 ` [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag Stanislav Kinsbursky
@ 2011-09-19 14:08 ` Jeff Layton
2011-09-19 14:51 ` Stanislav Kinsbursky
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Layton @ 2011-09-19 14:08 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Trond.Myklebust, linux-nfs, xemul, neilb, netdev, linux-kernel,
bfields, davem
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
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag
2011-09-19 14:08 ` Jeff Layton
@ 2011-09-19 14:51 ` Stanislav Kinsbursky
2011-09-19 15:07 ` Jeff Layton
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-19 14:51 UTC (permalink / raw)
To: Jeff Layton
Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org,
Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
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
>
>
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag
2011-09-19 14:51 ` Stanislav Kinsbursky
@ 2011-09-19 15:07 ` Jeff Layton
2011-09-19 15:42 ` Stanislav Kinsbursky
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Layton @ 2011-09-19 15:07 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org,
Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
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
> >
> >
>
>
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag
2011-09-19 15:07 ` Jeff Layton
@ 2011-09-19 15:42 ` Stanislav Kinsbursky
2011-09-19 18:11 ` Jeff Layton
0 siblings, 1 reply; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-19 15:42 UTC (permalink / raw)
To: Jeff Layton
Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org,
Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
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.
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag
2011-09-19 15:42 ` Stanislav Kinsbursky
@ 2011-09-19 18:11 ` Jeff Layton
2011-09-20 10:14 ` Stanislav Kinsbursky
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Layton @ 2011-09-19 18:11 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org,
Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
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.
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag
2011-09-19 18:11 ` Jeff Layton
@ 2011-09-20 10:14 ` Stanislav Kinsbursky
0 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-20 10:14 UTC (permalink / raw)
To: Jeff Layton
Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org,
Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 05/11] SUNRPC: cleanup service destruction
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
` (3 preceding siblings ...)
2011-09-13 18:13 ` [PATCH v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag Stanislav Kinsbursky
@ 2011-09-13 18:13 ` Stanislav Kinsbursky
2011-09-13 18:14 ` [PATCH v3 06/11] Lockd: force creation of rpcbind clients during service creation Stanislav Kinsbursky
` (5 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:13 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
svc_unregister() call have to be removed from svc_destroy() since it will be
called in sv_shutdown callback.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
net/sunrpc/svc.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 03231d5..1bf1b1a 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -513,7 +513,6 @@ svc_destroy(struct svc_serv *serv)
if (svc_serv_is_pooled(serv))
svc_pool_map_put();
- svc_unregister(serv);
kfree(serv->sv_pools);
kfree(serv);
}
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 06/11] Lockd: force creation of rpcbind clients during service creation
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
` (4 preceding siblings ...)
2011-09-13 18:13 ` [PATCH v3 05/11] SUNRPC: cleanup service destruction Stanislav Kinsbursky
@ 2011-09-13 18:14 ` Stanislav Kinsbursky
2011-09-13 18:14 ` [PATCH v3 07/11] NFS: avoid rpcbind clients creation " Stanislav Kinsbursky
` (4 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:14 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
Lockd sends requests to portmapper and thus requires rpcbind clients.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
fs/lockd/svc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index abfff9d..bdf79dc 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -269,7 +269,7 @@ int lockd_up(void)
"lockd_up: no pid, %d users??\n", nlmsvc_users);
error = -ENOMEM;
- serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, NULL);
+ serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, 1, NULL);
if (!serv) {
printk(KERN_WARNING "lockd_up: create service failed\n");
goto out;
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 07/11] NFS: avoid rpcbind clients creation during service creation
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
` (5 preceding siblings ...)
2011-09-13 18:14 ` [PATCH v3 06/11] Lockd: force creation of rpcbind clients during service creation Stanislav Kinsbursky
@ 2011-09-13 18:14 ` Stanislav Kinsbursky
2011-09-13 18:14 ` [PATCH v3 08/11] NFSd: force creation of rpcbind clients " Stanislav Kinsbursky
` (3 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:14 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
NFS callbacks doesn't communicate portmapper and thus doesn't need rpcbind
clients.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
fs/nfs/callback.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index e3d2942..2715c02 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -259,7 +259,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
nfs_callback_bc_serv(minorversion, xprt, cb_info);
goto out;
}
- serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, NULL);
+ serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE, 0, NULL);
if (!serv) {
ret = -ENOMEM;
goto out_err;
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 08/11] NFSd: force creation of rpcbind clients during service creation
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
` (6 preceding siblings ...)
2011-09-13 18:14 ` [PATCH v3 07/11] NFS: avoid rpcbind clients creation " Stanislav Kinsbursky
@ 2011-09-13 18:14 ` Stanislav Kinsbursky
2011-09-13 18:14 ` [PATCH v3 09/11] NFSd: call svc rpcbind cleanup explicitly Stanislav Kinsbursky
` (2 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:14 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
NFSd sends requests to portmapper and thus requires rpcbind clients.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
fs/nfsd/nfssvc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index dc5a1bf..88faab8 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -332,7 +332,7 @@ int nfsd_create_serv(void)
nfsd_reset_versions();
nfsd_serv = svc_create_pooled(&nfsd_program, nfsd_max_blksize,
- nfsd_last_thread, nfsd, THIS_MODULE);
+ 1, nfsd_last_thread, nfsd, THIS_MODULE);
if (nfsd_serv == NULL)
return -ENOMEM;
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 09/11] NFSd: call svc rpcbind cleanup explicitly
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
` (7 preceding siblings ...)
2011-09-13 18:14 ` [PATCH v3 08/11] NFSd: force creation of rpcbind clients " Stanislav Kinsbursky
@ 2011-09-13 18:14 ` Stanislav Kinsbursky
2011-09-13 18:14 ` [PATCH v3 10/11] SUNRPC: remove rpcbind clients creation during service registering Stanislav Kinsbursky
2011-09-13 18:14 ` [PATCH v3 11/11] SUNRPC: remove rpcbind clients destruction on module cleanup Stanislav Kinsbursky
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:14 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
We have to call svc_rpcb_cleanup() explicitly from nfsd_last_thread() since
this function is registered as service shutdown callback and thus nobody else
will done it for us.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
fs/nfsd/nfssvc.c | 2 ++
include/linux/sunrpc/svc.h | 1 +
net/sunrpc/svc.c | 3 ++-
3 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 88faab8..fe14cd2 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -256,6 +256,8 @@ static void nfsd_last_thread(struct svc_serv *serv)
nfsd_serv = NULL;
nfsd_shutdown();
+ svc_rpcb_cleanup(serv);
+
printk(KERN_WARNING "nfsd: last server has exited, flushing export "
"cache\n");
nfsd_export_flush();
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 528952a..35fa9ca 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -401,6 +401,7 @@ struct svc_procedure {
/*
* Function prototypes.
*/
+void svc_rpcb_cleanup(struct svc_serv *serv);
struct svc_serv *svc_create(struct svc_program *, unsigned int,
int setup_rpcbind,
void (*shutdown)(struct svc_serv *));
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 1bf1b1a..71dd439 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -367,11 +367,12 @@ static int svc_rpcb_setup(struct svc_serv *serv)
return 0;
}
-static void svc_rpcb_cleanup(struct svc_serv *serv)
+void svc_rpcb_cleanup(struct svc_serv *serv)
{
svc_unregister(serv);
rpcb_put_local();
}
+EXPORT_SYMBOL_GPL(svc_rpcb_cleanup);
/*
* Create an RPC service
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 10/11] SUNRPC: remove rpcbind clients creation during service registering
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
` (8 preceding siblings ...)
2011-09-13 18:14 ` [PATCH v3 09/11] NFSd: call svc rpcbind cleanup explicitly Stanislav Kinsbursky
@ 2011-09-13 18:14 ` Stanislav Kinsbursky
2011-09-13 18:14 ` [PATCH v3 11/11] SUNRPC: remove rpcbind clients destruction on module cleanup Stanislav Kinsbursky
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:14 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
We don't need this code since rpcbind clients are creating during RPC service
creation.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
net/sunrpc/rpcb_clnt.c | 9 ---------
1 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index d42b642..e5c936d 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -428,11 +428,6 @@ int rpcb_register(u32 prog, u32 vers, int prot, unsigned short port)
struct rpc_message msg = {
.rpc_argp = &map,
};
- int error;
-
- error = rpcb_create_local();
- if (error)
- return error;
dprintk("RPC: %sregistering (%u, %u, %d, %u) with local "
"rpcbind\n", (port ? "" : "un"),
@@ -568,11 +563,7 @@ int rpcb_v4_register(const u32 program, const u32 version,
struct rpc_message msg = {
.rpc_argp = &map,
};
- int error;
- error = rpcb_create_local();
- if (error)
- return error;
if (rpcb_local_clnt4 == NULL)
return -EPROTONOSUPPORT;
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 11/11] SUNRPC: remove rpcbind clients destruction on module cleanup
2011-09-13 18:13 [PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically Stanislav Kinsbursky
` (9 preceding siblings ...)
2011-09-13 18:14 ` [PATCH v3 10/11] SUNRPC: remove rpcbind clients creation during service registering Stanislav Kinsbursky
@ 2011-09-13 18:14 ` Stanislav Kinsbursky
10 siblings, 0 replies; 18+ messages in thread
From: Stanislav Kinsbursky @ 2011-09-13 18:14 UTC (permalink / raw)
To: Trond.Myklebust
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
Rpcbind clients destruction during SUNRPC module removing is obsolete since now
those clients are destroying during last RPC service shutdown.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
net/sunrpc/rpcb_clnt.c | 12 ------------
net/sunrpc/sunrpc_syms.c | 3 ---
2 files changed, 0 insertions(+), 15 deletions(-)
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index e5c936d..fc6c59b 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -1097,15 +1097,3 @@ static struct rpc_program rpcb_program = {
.version = rpcb_version,
.stats = &rpcb_stats,
};
-
-/**
- * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
- *
- */
-void cleanup_rpcb_clnt(void)
-{
- if (rpcb_local_clnt4)
- rpc_shutdown_client(rpcb_local_clnt4);
- if (rpcb_local_clnt)
- rpc_shutdown_client(rpcb_local_clnt);
-}
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index 9d08091..8ec9778 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -61,8 +61,6 @@ static struct pernet_operations sunrpc_net_ops = {
extern struct cache_detail unix_gid_cache;
-extern void cleanup_rpcb_clnt(void);
-
static int __init
init_sunrpc(void)
{
@@ -102,7 +100,6 @@ out:
static void __exit
cleanup_sunrpc(void)
{
- cleanup_rpcb_clnt();
rpcauth_remove_module();
cleanup_socket_xprt();
svc_cleanup_xprt_sock();
^ permalink raw reply related [flat|nested] 18+ messages in thread