[PATCH v2 0/5] SUNRPC: make rpcbind clients allocated and destroyed dynamically

[PATCH v2 0/5] SUNRPC: make rpcbind clients allocated and destroyed dynamically

[Stanislav Kinsbursky]

v2:
1) rebased to v3.1-rc4 (against branch
git://git.linux-nfs.org/projects/trondmy/nfs-2.6.git)
2) Patch 3/5 (SUNRPC: make RPC service dependable on rpcbind clients creation)
fixed: added rpcb_put_local() call in case of svc_serv creation failure.

This patch is required for further RPC layer virtualization, because rpcbind
clients have to be per network namespace.
To achive this, we have to untie network namespace from rpcbind clients sockets.
The idea of this patch set is to make rpcbind clients non-static. I.e. rpcbind
clients will be created during first RPC service creation, and destroyed when
last RPC service is stopped.
With this patch set rpcbind clients can be virtualized easely.

The following series consists of:

---

Stanislav Kinsbursky (5):
      SUNRPC: introduce helpers for reference counted rpcbind clients
      SUNRPC: use rpcbind reference counting helpers
      SUNRPC: make RPC service dependable on rpcbind clients creation
      SUNRPC: remove rpcbind clients creation during service registering
      SUNRPC: remove rpcbind clients destruction on module cleanup


 include/linux/sunrpc/clnt.h |    2 +
 net/sunrpc/rpcb_clnt.c      |   86 ++++++++++++++++++++++++++++---------------
 net/sunrpc/sunrpc_syms.c    |    3 --
 net/sunrpc/svc.c            |   13 ++++++-
 4 files changed, 69 insertions(+), 35 deletions(-)

-- 
Signature

[PATCH v2 1/5] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

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 |   51 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index e45d2fb..c84e6a3 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,54 @@ 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.

[PATCH v2 2/5] SUNRPC: use rpcbind reference counting helpers

[Stanislav Kinsbursky]

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 c84e6a3..b4cc0f1 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -256,9 +256,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;
@@ -310,9 +308,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;
@@ -327,11 +323,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)

[PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Stanislav Kinsbursky]

Create rcbind clients or increase rpcbind users counter during RPC service
creation and decrease this counter (and possibly destroy those clients) on RPC
service destruction.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 include/linux/sunrpc/clnt.h |    2 ++
 net/sunrpc/rpcb_clnt.c      |    2 +-
 net/sunrpc/svc.c            |   13 +++++++++++--
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index db7bcaf..65a8115 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -135,10 +135,12 @@ 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);
 int		rpcb_register(u32, u32, int, unsigned short);
 int		rpcb_v4_register(const u32 program, const u32 version,
 				 const struct sockaddr *address,
 				 const char *netid);
+void		rpcb_put_local(void);
 void		rpcb_getport_async(struct rpc_task *);
 
 void		rpc_call_start(struct rpc_task *);
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index b4cc0f1..437ec60 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -318,7 +318,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..9095c0e 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 	unsigned int xdrsize;
 	unsigned int i;
 
-	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
+	if (rpcb_create_local() < 0)
 		return NULL;
+
+	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
+		goto out_err;
 	serv->sv_name      = prog->pg_name;
 	serv->sv_program   = prog;
 	serv->sv_nrthreads = 1;
@@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 			GFP_KERNEL);
 	if (!serv->sv_pools) {
 		kfree(serv);
-		return NULL;
+		goto out_err;
 	}
 
 	for (i = 0; i < serv->sv_nrpools; i++) {
@@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 	svc_unregister(serv);
 
 	return serv;
+
+out_err:
+	rpcb_put_local(); 
+	return NULL;
 }
 
 struct svc_serv *
@@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
 	svc_unregister(serv);
 	kfree(serv->sv_pools);
 	kfree(serv);
+
+	rpcb_put_local();
 }
 EXPORT_SYMBOL_GPL(svc_destroy);
 

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Jeff Layton]

On Fri, 09 Sep 2011 16:08:44 +0400
Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> Create rcbind clients or increase rpcbind users counter during RPC service
> creation and decrease this counter (and possibly destroy those clients) on RPC
> service destruction.
> 
> Signed-off-by: Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> 
> ---
>  include/linux/sunrpc/clnt.h |    2 ++
>  net/sunrpc/rpcb_clnt.c      |    2 +-
>  net/sunrpc/svc.c            |   13 +++++++++++--
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index db7bcaf..65a8115 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -135,10 +135,12 @@ 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);
>  int		rpcb_register(u32, u32, int, unsigned short);
>  int		rpcb_v4_register(const u32 program, const u32 version,
>  				 const struct sockaddr *address,
>  				 const char *netid);
> +void		rpcb_put_local(void);
>  void		rpcb_getport_async(struct rpc_task *);
>  
>  void		rpc_call_start(struct rpc_task *);
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index b4cc0f1..437ec60 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -318,7 +318,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..9095c0e 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  	unsigned int xdrsize;
>  	unsigned int i;
>  
> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> +	if (rpcb_create_local() < 0)
>  		return NULL;
> +
> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> +		goto out_err;
>  	serv->sv_name      = prog->pg_name;
>  	serv->sv_program   = prog;
>  	serv->sv_nrthreads = 1;
> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  			GFP_KERNEL);
>  	if (!serv->sv_pools) {
>  		kfree(serv);
> -		return NULL;
> +		goto out_err;
>  	}
>  
>  	for (i = 0; i < serv->sv_nrpools; i++) {
> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  	svc_unregister(serv);
>  
>  	return serv;
> +
> +out_err:
> +	rpcb_put_local(); 
> +	return NULL;
>  }
>  
>  struct svc_serv *
> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
>  	svc_unregister(serv);
>  	kfree(serv->sv_pools);
>  	kfree(serv);
> +
> +	rpcb_put_local();
>  }
>  EXPORT_SYMBOL_GPL(svc_destroy);
>  
> 

I don't get it -- what's the advantage of creating rpcbind clients in
__svc_create vs. the old way of creating them just before we plan to
use them?

With this scheme, won't we end up creating rpcbind sockets even when we
don't need them? For instance, if I create a callback socket for NFSv4
then I don't really need to talk to rpcbind. With this patch I'll still
get the rpcbind clients created though.

It would seem to me to make more sense to create the rpcbind clients
somewhere closer to svc_setup_socket, and only if SVC_SOCK_ANONYMOUS is
not set.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Stanislav Kinsbursky]

09.09.2011 18:07, Jeff Layton пишет:
> On Fri, 09 Sep 2011 16:08:44 +0400
> Stanislav Kinsbursky<skinsbursky@parallels.com>  wrote:
>
>> Create rcbind clients or increase rpcbind users counter during RPC service
>> creation and decrease this counter (and possibly destroy those clients) on RPC
>> service destruction.
>>
>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>
>> ---
>>   include/linux/sunrpc/clnt.h |    2 ++
>>   net/sunrpc/rpcb_clnt.c      |    2 +-
>>   net/sunrpc/svc.c            |   13 +++++++++++--
>>   3 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>> index db7bcaf..65a8115 100644
>> --- a/include/linux/sunrpc/clnt.h
>> +++ b/include/linux/sunrpc/clnt.h
>> @@ -135,10 +135,12 @@ 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);
>>   int		rpcb_register(u32, u32, int, unsigned short);
>>   int		rpcb_v4_register(const u32 program, const u32 version,
>>   				 const struct sockaddr *address,
>>   				 const char *netid);
>> +void		rpcb_put_local(void);
>>   void		rpcb_getport_async(struct rpc_task *);
>>
>>   void		rpc_call_start(struct rpc_task *);
>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>> index b4cc0f1..437ec60 100644
>> --- a/net/sunrpc/rpcb_clnt.c
>> +++ b/net/sunrpc/rpcb_clnt.c
>> @@ -318,7 +318,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..9095c0e 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>   	unsigned int xdrsize;
>>   	unsigned int i;
>>
>> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
>> +	if (rpcb_create_local()<  0)
>>   		return NULL;
>> +
>> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
>> +		goto out_err;
>>   	serv->sv_name      = prog->pg_name;
>>   	serv->sv_program   = prog;
>>   	serv->sv_nrthreads = 1;
>> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>   			GFP_KERNEL);
>>   	if (!serv->sv_pools) {
>>   		kfree(serv);
>> -		return NULL;
>> +		goto out_err;
>>   	}
>>
>>   	for (i = 0; i<  serv->sv_nrpools; i++) {
>> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>   	svc_unregister(serv);
>>
>>   	return serv;
>> +
>> +out_err:
>> +	rpcb_put_local();
>> +	return NULL;
>>   }
>>
>>   struct svc_serv *
>> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
>>   	svc_unregister(serv);
>>   	kfree(serv->sv_pools);
>>   	kfree(serv);
>> +
>> +	rpcb_put_local();
>>   }
>>   EXPORT_SYMBOL_GPL(svc_destroy);
>>
>>
>
> I don't get it -- what's the advantage of creating rpcbind clients in
> __svc_create vs. the old way of creating them just before we plan to
> use them?
>

The main problem here is not in creation, but in destroying those clients.
Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.

> With this scheme, won't we end up creating rpcbind sockets even when we
> don't need them? For instance, if I create a callback socket for NFSv4
> then I don't really need to talk to rpcbind. With this patch I'll still
> get the rpcbind clients created though.
>

Yep, you right. This is not a real problem from my pow.
But I'll think about how to avoid this creation for nfs callbacks.

> It would seem to me to make more sense to create the rpcbind clients
> somewhere closer to svc_setup_socket, and only if SVC_SOCK_ANONYMOUS is
> not set.
>

Probably. Will try to find better place.

-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Trond Myklebust]

On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote: 
> 09.09.2011 18:07, Jeff Layton пишет:
> > On Fri, 09 Sep 2011 16:08:44 +0400
> > Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  wrote:
> >
> >> Create rcbind clients or increase rpcbind users counter during RPC service
> >> creation and decrease this counter (and possibly destroy those clients) on RPC
> >> service destruction.
> >>
> >> Signed-off-by: Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> >>
> >> ---
> >>   include/linux/sunrpc/clnt.h |    2 ++
> >>   net/sunrpc/rpcb_clnt.c      |    2 +-
> >>   net/sunrpc/svc.c            |   13 +++++++++++--
> >>   3 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> >> index db7bcaf..65a8115 100644
> >> --- a/include/linux/sunrpc/clnt.h
> >> +++ b/include/linux/sunrpc/clnt.h
> >> @@ -135,10 +135,12 @@ 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);
> >>   int		rpcb_register(u32, u32, int, unsigned short);
> >>   int		rpcb_v4_register(const u32 program, const u32 version,
> >>   				 const struct sockaddr *address,
> >>   				 const char *netid);
> >> +void		rpcb_put_local(void);
> >>   void		rpcb_getport_async(struct rpc_task *);
> >>
> >>   void		rpc_call_start(struct rpc_task *);
> >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> >> index b4cc0f1..437ec60 100644
> >> --- a/net/sunrpc/rpcb_clnt.c
> >> +++ b/net/sunrpc/rpcb_clnt.c
> >> @@ -318,7 +318,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..9095c0e 100644
> >> --- a/net/sunrpc/svc.c
> >> +++ b/net/sunrpc/svc.c
> >> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >>   	unsigned int xdrsize;
> >>   	unsigned int i;
> >>
> >> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> >> +	if (rpcb_create_local()<  0)
> >>   		return NULL;
> >> +
> >> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> >> +		goto out_err;
> >>   	serv->sv_name      = prog->pg_name;
> >>   	serv->sv_program   = prog;
> >>   	serv->sv_nrthreads = 1;
> >> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >>   			GFP_KERNEL);
> >>   	if (!serv->sv_pools) {
> >>   		kfree(serv);
> >> -		return NULL;
> >> +		goto out_err;
> >>   	}
> >>
> >>   	for (i = 0; i<  serv->sv_nrpools; i++) {
> >> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >>   	svc_unregister(serv);
> >>
> >>   	return serv;
> >> +
> >> +out_err:
> >> +	rpcb_put_local();
> >> +	return NULL;
> >>   }
> >>
> >>   struct svc_serv *
> >> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
> >>   	svc_unregister(serv);
> >>   	kfree(serv->sv_pools);
> >>   	kfree(serv);
> >> +
> >> +	rpcb_put_local();
> >>   }
> >>   EXPORT_SYMBOL_GPL(svc_destroy);
> >>
> >>
> >
> > I don't get it -- what's the advantage of creating rpcbind clients in
> > __svc_create vs. the old way of creating them just before we plan to
> > use them?
> >
> 
> The main problem here is not in creation, but in destroying those clients.
> Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
> But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.

Could we perhaps set up a 'struct pernet_operations' to create a
destructor for them?

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Jeff Layton]

On Fri, 09 Sep 2011 14:44:34 -0400
Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote:

> On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote: 
> > 09.09.2011 18:07, Jeff Layton пишет:
> > > On Fri, 09 Sep 2011 16:08:44 +0400
> > > Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  wrote:
> > >
> > >> Create rcbind clients or increase rpcbind users counter during RPC service
> > >> creation and decrease this counter (and possibly destroy those clients) on RPC
> > >> service destruction.
> > >>
> > >> Signed-off-by: Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> > >>
> > >> ---
> > >>   include/linux/sunrpc/clnt.h |    2 ++
> > >>   net/sunrpc/rpcb_clnt.c      |    2 +-
> > >>   net/sunrpc/svc.c            |   13 +++++++++++--
> > >>   3 files changed, 14 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> > >> index db7bcaf..65a8115 100644
> > >> --- a/include/linux/sunrpc/clnt.h
> > >> +++ b/include/linux/sunrpc/clnt.h
> > >> @@ -135,10 +135,12 @@ 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);
> > >>   int		rpcb_register(u32, u32, int, unsigned short);
> > >>   int		rpcb_v4_register(const u32 program, const u32 version,
> > >>   				 const struct sockaddr *address,
> > >>   				 const char *netid);
> > >> +void		rpcb_put_local(void);
> > >>   void		rpcb_getport_async(struct rpc_task *);
> > >>
> > >>   void		rpc_call_start(struct rpc_task *);
> > >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> > >> index b4cc0f1..437ec60 100644
> > >> --- a/net/sunrpc/rpcb_clnt.c
> > >> +++ b/net/sunrpc/rpcb_clnt.c
> > >> @@ -318,7 +318,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..9095c0e 100644
> > >> --- a/net/sunrpc/svc.c
> > >> +++ b/net/sunrpc/svc.c
> > >> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > >>   	unsigned int xdrsize;
> > >>   	unsigned int i;
> > >>
> > >> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > >> +	if (rpcb_create_local()<  0)
> > >>   		return NULL;
> > >> +
> > >> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > >> +		goto out_err;
> > >>   	serv->sv_name      = prog->pg_name;
> > >>   	serv->sv_program   = prog;
> > >>   	serv->sv_nrthreads = 1;
> > >> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > >>   			GFP_KERNEL);
> > >>   	if (!serv->sv_pools) {
> > >>   		kfree(serv);
> > >> -		return NULL;
> > >> +		goto out_err;
> > >>   	}
> > >>
> > >>   	for (i = 0; i<  serv->sv_nrpools; i++) {
> > >> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > >>   	svc_unregister(serv);
> > >>
> > >>   	return serv;
> > >> +
> > >> +out_err:
> > >> +	rpcb_put_local();
> > >> +	return NULL;
> > >>   }
> > >>
> > >>   struct svc_serv *
> > >> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
> > >>   	svc_unregister(serv);
> > >>   	kfree(serv->sv_pools);
> > >>   	kfree(serv);
> > >> +
> > >> +	rpcb_put_local();
> > >>   }
> > >>   EXPORT_SYMBOL_GPL(svc_destroy);
> > >>
> > >>
> > >
> > > I don't get it -- what's the advantage of creating rpcbind clients in
> > > __svc_create vs. the old way of creating them just before we plan to
> > > use them?
> > >
> > 
> > The main problem here is not in creation, but in destroying those clients.
> > Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
> > But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.
> 
> Could we perhaps set up a 'struct pernet_operations' to create a
> destructor for them?
> 

An even easier idea might be to just not take a reference to the
rpcbind client for svc_programs that have vs_hidden set on every
version.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Trond Myklebust]

On Fri, 2011-09-09 at 15:01 -0400, Jeff Layton wrote: 
> On Fri, 09 Sep 2011 14:44:34 -0400
> Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote: 
> > > 09.09.2011 18:07, Jeff Layton пишет:
> > > > On Fri, 09 Sep 2011 16:08:44 +0400
> > > > Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  wrote:
> > > >
> > > >> Create rcbind clients or increase rpcbind users counter during RPC service
> > > >> creation and decrease this counter (and possibly destroy those clients) on RPC
> > > >> service destruction.
> > > >>
> > > >> Signed-off-by: Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> > > >>
> > > >> ---
> > > >>   include/linux/sunrpc/clnt.h |    2 ++
> > > >>   net/sunrpc/rpcb_clnt.c      |    2 +-
> > > >>   net/sunrpc/svc.c            |   13 +++++++++++--
> > > >>   3 files changed, 14 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> > > >> index db7bcaf..65a8115 100644
> > > >> --- a/include/linux/sunrpc/clnt.h
> > > >> +++ b/include/linux/sunrpc/clnt.h
> > > >> @@ -135,10 +135,12 @@ 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);
> > > >>   int		rpcb_register(u32, u32, int, unsigned short);
> > > >>   int		rpcb_v4_register(const u32 program, const u32 version,
> > > >>   				 const struct sockaddr *address,
> > > >>   				 const char *netid);
> > > >> +void		rpcb_put_local(void);
> > > >>   void		rpcb_getport_async(struct rpc_task *);
> > > >>
> > > >>   void		rpc_call_start(struct rpc_task *);
> > > >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> > > >> index b4cc0f1..437ec60 100644
> > > >> --- a/net/sunrpc/rpcb_clnt.c
> > > >> +++ b/net/sunrpc/rpcb_clnt.c
> > > >> @@ -318,7 +318,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..9095c0e 100644
> > > >> --- a/net/sunrpc/svc.c
> > > >> +++ b/net/sunrpc/svc.c
> > > >> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > >>   	unsigned int xdrsize;
> > > >>   	unsigned int i;
> > > >>
> > > >> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > > >> +	if (rpcb_create_local()<  0)
> > > >>   		return NULL;
> > > >> +
> > > >> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > > >> +		goto out_err;
> > > >>   	serv->sv_name      = prog->pg_name;
> > > >>   	serv->sv_program   = prog;
> > > >>   	serv->sv_nrthreads = 1;
> > > >> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > >>   			GFP_KERNEL);
> > > >>   	if (!serv->sv_pools) {
> > > >>   		kfree(serv);
> > > >> -		return NULL;
> > > >> +		goto out_err;
> > > >>   	}
> > > >>
> > > >>   	for (i = 0; i<  serv->sv_nrpools; i++) {
> > > >> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > >>   	svc_unregister(serv);
> > > >>
> > > >>   	return serv;
> > > >> +
> > > >> +out_err:
> > > >> +	rpcb_put_local();
> > > >> +	return NULL;
> > > >>   }
> > > >>
> > > >>   struct svc_serv *
> > > >> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
> > > >>   	svc_unregister(serv);
> > > >>   	kfree(serv->sv_pools);
> > > >>   	kfree(serv);
> > > >> +
> > > >> +	rpcb_put_local();
> > > >>   }
> > > >>   EXPORT_SYMBOL_GPL(svc_destroy);
> > > >>
> > > >>
> > > >
> > > > I don't get it -- what's the advantage of creating rpcbind clients in
> > > > __svc_create vs. the old way of creating them just before we plan to
> > > > use them?
> > > >
> > > 
> > > The main problem here is not in creation, but in destroying those clients.
> > > Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
> > > But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.
> > 
> > Could we perhaps set up a 'struct pernet_operations' to create a
> > destructor for them?
> > 
> 
> An even easier idea might be to just not take a reference to the
> rpcbind client for svc_programs that have vs_hidden set on every
> version.

Isn't the problem that Stanislav is trying to solve that we need to be
able to register and unregister RPC services to the correct rpcbind
server, depending on which net namespace we are in?

My understanding is that the current code will register everything to
whatever rpcbind server is running in the init net namespace because
that's what rpcb_create_local() uses.

My suggestion is to use a struct pernet_operations to detect when a net
namespace is being created or destroyed, so that the rpcbind client code
knows when to create or destroy a connection to the server that is
running in that namespace.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Pavel Emelyanov]

> Isn't the problem that Stanislav is trying to solve that we need to be
> able to register and unregister RPC services to the correct rpcbind
> server, depending on which net namespace we are in?
> 
> My understanding is that the current code will register everything to
> whatever rpcbind server is running in the init net namespace because
> that's what rpcb_create_local() uses.
> 
> My suggestion is to use a struct pernet_operations to detect when a net
> namespace is being created or destroyed, so that the rpcbind client code
> knows when to create or destroy a connection to the server that is
> running in that namespace.

The problem is that the client has a reference on transport, the transport gets
a socket and the socket holds the net namesace and thus we will not receive the
net->exit event with this approach.

Another option is to break the client's socket reference and kill the rpcb client
on netns stop (i.e. as you propose), but in that case we have another problem - 
how to destroy _other_ clients (not rpc bind ones) when the net namespace stops.

If you prefer either way just let us know, we'll fix the patches.

> Cheers
>   Trond

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Stanislav Kinsbursky]

09.09.2011 23:25, Trond Myklebust пишет:
> On Fri, 2011-09-09 at 15:01 -0400, Jeff Layton wrote:
>> On Fri, 09 Sep 2011 14:44:34 -0400
>> Trond Myklebust<Trond.Myklebust@netapp.com>  wrote:
>>
>>> On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote:
>>>> 09.09.2011 18:07, Jeff Layton пишет:
>>>>> On Fri, 09 Sep 2011 16:08:44 +0400
>>>>> Stanislav Kinsbursky<skinsbursky@parallels.com>   wrote:
>>>>>
>>>>>> Create rcbind clients or increase rpcbind users counter during RPC service
>>>>>> creation and decrease this counter (and possibly destroy those clients) on RPC
>>>>>> service destruction.
>>>>>>
>>>>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>>>>
>>>>>> ---
>>>>>>    include/linux/sunrpc/clnt.h |    2 ++
>>>>>>    net/sunrpc/rpcb_clnt.c      |    2 +-
>>>>>>    net/sunrpc/svc.c            |   13 +++++++++++--
>>>>>>    3 files changed, 14 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>>>>>> index db7bcaf..65a8115 100644
>>>>>> --- a/include/linux/sunrpc/clnt.h
>>>>>> +++ b/include/linux/sunrpc/clnt.h
>>>>>> @@ -135,10 +135,12 @@ 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);
>>>>>>    int		rpcb_register(u32, u32, int, unsigned short);
>>>>>>    int		rpcb_v4_register(const u32 program, const u32 version,
>>>>>>    				 const struct sockaddr *address,
>>>>>>    				 const char *netid);
>>>>>> +void		rpcb_put_local(void);
>>>>>>    void		rpcb_getport_async(struct rpc_task *);
>>>>>>
>>>>>>    void		rpc_call_start(struct rpc_task *);
>>>>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>>>>>> index b4cc0f1..437ec60 100644
>>>>>> --- a/net/sunrpc/rpcb_clnt.c
>>>>>> +++ b/net/sunrpc/rpcb_clnt.c
>>>>>> @@ -318,7 +318,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..9095c0e 100644
>>>>>> --- a/net/sunrpc/svc.c
>>>>>> +++ b/net/sunrpc/svc.c
>>>>>> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>>>>>    	unsigned int xdrsize;
>>>>>>    	unsigned int i;
>>>>>>
>>>>>> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
>>>>>> +	if (rpcb_create_local()<   0)
>>>>>>    		return NULL;
>>>>>> +
>>>>>> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
>>>>>> +		goto out_err;
>>>>>>    	serv->sv_name      = prog->pg_name;
>>>>>>    	serv->sv_program   = prog;
>>>>>>    	serv->sv_nrthreads = 1;
>>>>>> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>>>>>    			GFP_KERNEL);
>>>>>>    	if (!serv->sv_pools) {
>>>>>>    		kfree(serv);
>>>>>> -		return NULL;
>>>>>> +		goto out_err;
>>>>>>    	}
>>>>>>
>>>>>>    	for (i = 0; i<   serv->sv_nrpools; i++) {
>>>>>> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>>>>>    	svc_unregister(serv);
>>>>>>
>>>>>>    	return serv;
>>>>>> +
>>>>>> +out_err:
>>>>>> +	rpcb_put_local();
>>>>>> +	return NULL;
>>>>>>    }
>>>>>>
>>>>>>    struct svc_serv *
>>>>>> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
>>>>>>    	svc_unregister(serv);
>>>>>>    	kfree(serv->sv_pools);
>>>>>>    	kfree(serv);
>>>>>> +
>>>>>> +	rpcb_put_local();
>>>>>>    }
>>>>>>    EXPORT_SYMBOL_GPL(svc_destroy);
>>>>>>
>>>>>>
>>>>>
>>>>> I don't get it -- what's the advantage of creating rpcbind clients in
>>>>> __svc_create vs. the old way of creating them just before we plan to
>>>>> use them?
>>>>>
>>>>
>>>> The main problem here is not in creation, but in destroying those clients.
>>>> Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
>>>> But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.
>>>
>>> Could we perhaps set up a 'struct pernet_operations' to create a
>>> destructor for them?
>>>
>>
>> An even easier idea might be to just not take a reference to the
>> rpcbind client for svc_programs that have vs_hidden set on every
>> version.
>
> Isn't the problem that Stanislav is trying to solve that we need to be
> able to register and unregister RPC services to the correct rpcbind
> server, depending on which net namespace we are in?
>

Yes, it is.
I'm going to make rpcbind clients per net namespace.

> My understanding is that the current code will register everything to
> whatever rpcbind server is running in the init net namespace because
> that's what rpcb_create_local() uses.
>
> My suggestion is to use a struct pernet_operations to detect when a net
> namespace is being created or destroyed, so that the rpcbind client code
> knows when to create or destroy a connection to the server that is
> running in that namespace.
>

But as Pavel mentioned already, we can't use netns destructor for rpcbind 
clients because they holds netns reference.
That's why we have to untie netns from rpbind clients first.
Another solution is to not increment netns ref counter for rpcbind sockets as, 
again, Pavel already mentioned.
But first approach looks clearer from my pow. That's why I'm trying to make 
rcpbind client's self-destructible.After achieving this we can just make this 
rpcbind clients per netns and then we can virtualize lockd.

I've tried to find some better place for creating rpcbind clients (instead of 
__svc_create()). But this place looks like the best one for current solution.

About avoiding of creation of rpcbind clients for nfs 4.* callbacks.
Probably, we can implement init-fini calls for svc_program structure 
(rpcb_create_local() and rpcb_put_local() will be used in our case) and then 
inherit them for svc_serv. This will allow us to call this hooks only if they 
defined and thus avoid rpcbind clients creation for nfs callbacks.

What all of you think about this hook's idea?

> Cheers
>    Trond


-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Stanislav Kinsbursky]

12.09.2011 15:22, Stanislav Kinsbursky пишет:
> 09.09.2011 23:25, Trond Myklebust пишет:
>> On Fri, 2011-09-09 at 15:01 -0400, Jeff Layton wrote:
>>> On Fri, 09 Sep 2011 14:44:34 -0400
>>> Trond Myklebust<Trond.Myklebust@netapp.com>   wrote:
>>>
>>>> On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote:
>>>>> 09.09.2011 18:07, Jeff Layton пишет:
>>>>>> On Fri, 09 Sep 2011 16:08:44 +0400
>>>>>> Stanislav Kinsbursky<skinsbursky@parallels.com>    wrote:
>>>>>>
>>>>>>> Create rcbind clients or increase rpcbind users counter during RPC service
>>>>>>> creation and decrease this counter (and possibly destroy those clients) on RPC
>>>>>>> service destruction.
>>>>>>>
>>>>>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>>>>>
>>>>>>> ---
>>>>>>>     include/linux/sunrpc/clnt.h |    2 ++
>>>>>>>     net/sunrpc/rpcb_clnt.c      |    2 +-
>>>>>>>     net/sunrpc/svc.c            |   13 +++++++++++--
>>>>>>>     3 files changed, 14 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
>>>>>>> index db7bcaf..65a8115 100644
>>>>>>> --- a/include/linux/sunrpc/clnt.h
>>>>>>> +++ b/include/linux/sunrpc/clnt.h
>>>>>>> @@ -135,10 +135,12 @@ 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);
>>>>>>>     int		rpcb_register(u32, u32, int, unsigned short);
>>>>>>>     int		rpcb_v4_register(const u32 program, const u32 version,
>>>>>>>     				 const struct sockaddr *address,
>>>>>>>     				 const char *netid);
>>>>>>> +void		rpcb_put_local(void);
>>>>>>>     void		rpcb_getport_async(struct rpc_task *);
>>>>>>>
>>>>>>>     void		rpc_call_start(struct rpc_task *);
>>>>>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>>>>>>> index b4cc0f1..437ec60 100644
>>>>>>> --- a/net/sunrpc/rpcb_clnt.c
>>>>>>> +++ b/net/sunrpc/rpcb_clnt.c
>>>>>>> @@ -318,7 +318,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..9095c0e 100644
>>>>>>> --- a/net/sunrpc/svc.c
>>>>>>> +++ b/net/sunrpc/svc.c
>>>>>>> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>>>>>>     	unsigned int xdrsize;
>>>>>>>     	unsigned int i;
>>>>>>>
>>>>>>> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
>>>>>>> +	if (rpcb_create_local()<    0)
>>>>>>>     		return NULL;
>>>>>>> +
>>>>>>> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
>>>>>>> +		goto out_err;
>>>>>>>     	serv->sv_name      = prog->pg_name;
>>>>>>>     	serv->sv_program   = prog;
>>>>>>>     	serv->sv_nrthreads = 1;
>>>>>>> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>>>>>>     			GFP_KERNEL);
>>>>>>>     	if (!serv->sv_pools) {
>>>>>>>     		kfree(serv);
>>>>>>> -		return NULL;
>>>>>>> +		goto out_err;
>>>>>>>     	}
>>>>>>>
>>>>>>>     	for (i = 0; i<    serv->sv_nrpools; i++) {
>>>>>>> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>>>>>>>     	svc_unregister(serv);
>>>>>>>
>>>>>>>     	return serv;
>>>>>>> +
>>>>>>> +out_err:
>>>>>>> +	rpcb_put_local();
>>>>>>> +	return NULL;
>>>>>>>     }
>>>>>>>
>>>>>>>     struct svc_serv *
>>>>>>> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
>>>>>>>     	svc_unregister(serv);
>>>>>>>     	kfree(serv->sv_pools);
>>>>>>>     	kfree(serv);
>>>>>>> +
>>>>>>> +	rpcb_put_local();
>>>>>>>     }
>>>>>>>     EXPORT_SYMBOL_GPL(svc_destroy);
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> I don't get it -- what's the advantage of creating rpcbind clients in
>>>>>> __svc_create vs. the old way of creating them just before we plan to
>>>>>> use them?
>>>>>>
>>>>>
>>>>> The main problem here is not in creation, but in destroying those clients.
>>>>> Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
>>>>> But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.
>>>>
>>>> Could we perhaps set up a 'struct pernet_operations' to create a
>>>> destructor for them?
>>>>
>>>
>>> An even easier idea might be to just not take a reference to the
>>> rpcbind client for svc_programs that have vs_hidden set on every
>>> version.
>>
>> Isn't the problem that Stanislav is trying to solve that we need to be
>> able to register and unregister RPC services to the correct rpcbind
>> server, depending on which net namespace we are in?
>>
>
> Yes, it is.
> I'm going to make rpcbind clients per net namespace.
>
>> My understanding is that the current code will register everything to
>> whatever rpcbind server is running in the init net namespace because
>> that's what rpcb_create_local() uses.
>>
>> My suggestion is to use a struct pernet_operations to detect when a net
>> namespace is being created or destroyed, so that the rpcbind client code
>> knows when to create or destroy a connection to the server that is
>> running in that namespace.
>>
>
> But as Pavel mentioned already, we can't use netns destructor for rpcbind
> clients because they holds netns reference.
> That's why we have to untie netns from rpbind clients first.
> Another solution is to not increment netns ref counter for rpcbind sockets as,
> again, Pavel already mentioned.
> But first approach looks clearer from my pow. That's why I'm trying to make
> rcpbind client's self-destructible.After achieving this we can just make this
> rpcbind clients per netns and then we can virtualize lockd.
>
> I've tried to find some better place for creating rpcbind clients (instead of
> __svc_create()). But this place looks like the best one for current solution.
>
> About avoiding of creation of rpcbind clients for nfs 4.* callbacks.
> Probably, we can implement init-fini calls for svc_program structure
> (rpcb_create_local() and rpcb_put_local() will be used in our case) and then
> inherit them for svc_serv. This will allow us to call this hooks only if they
> defined and thus avoid rpcbind clients creation for nfs callbacks.
>
> What all of you think about this hook's idea?
>

Since we already have sv_shutdown callback, we can use it to put rpcbind clients.
Creation of rpcbind clients can be performed in lockd_up() and 
nfsd_create_serv() before calling svc_create(_pooled)().

>> Cheers
>>     Trond
>
>


-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Jeff Layton]

On Fri, 09 Sep 2011 15:25:08 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Fri, 2011-09-09 at 15:01 -0400, Jeff Layton wrote: 
> > On Fri, 09 Sep 2011 14:44:34 -0400
> > Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > 
> > > On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote: 
> > > > 09.09.2011 18:07, Jeff Layton пишет:
> > > > > On Fri, 09 Sep 2011 16:08:44 +0400
> > > > > Stanislav Kinsbursky<skinsbursky@parallels.com>  wrote:
> > > > >
> > > > >> Create rcbind clients or increase rpcbind users counter during RPC service
> > > > >> creation and decrease this counter (and possibly destroy those clients) on RPC
> > > > >> service destruction.
> > > > >>
> > > > >> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
> > > > >>
> > > > >> ---
> > > > >>   include/linux/sunrpc/clnt.h |    2 ++
> > > > >>   net/sunrpc/rpcb_clnt.c      |    2 +-
> > > > >>   net/sunrpc/svc.c            |   13 +++++++++++--
> > > > >>   3 files changed, 14 insertions(+), 3 deletions(-)
> > > > >>
> > > > >> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> > > > >> index db7bcaf..65a8115 100644
> > > > >> --- a/include/linux/sunrpc/clnt.h
> > > > >> +++ b/include/linux/sunrpc/clnt.h
> > > > >> @@ -135,10 +135,12 @@ 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);
> > > > >>   int		rpcb_register(u32, u32, int, unsigned short);
> > > > >>   int		rpcb_v4_register(const u32 program, const u32 version,
> > > > >>   				 const struct sockaddr *address,
> > > > >>   				 const char *netid);
> > > > >> +void		rpcb_put_local(void);
> > > > >>   void		rpcb_getport_async(struct rpc_task *);
> > > > >>
> > > > >>   void		rpc_call_start(struct rpc_task *);
> > > > >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> > > > >> index b4cc0f1..437ec60 100644
> > > > >> --- a/net/sunrpc/rpcb_clnt.c
> > > > >> +++ b/net/sunrpc/rpcb_clnt.c
> > > > >> @@ -318,7 +318,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..9095c0e 100644
> > > > >> --- a/net/sunrpc/svc.c
> > > > >> +++ b/net/sunrpc/svc.c
> > > > >> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > > >>   	unsigned int xdrsize;
> > > > >>   	unsigned int i;
> > > > >>
> > > > >> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > > > >> +	if (rpcb_create_local()<  0)
> > > > >>   		return NULL;
> > > > >> +
> > > > >> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > > > >> +		goto out_err;
> > > > >>   	serv->sv_name      = prog->pg_name;
> > > > >>   	serv->sv_program   = prog;
> > > > >>   	serv->sv_nrthreads = 1;
> > > > >> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > > >>   			GFP_KERNEL);
> > > > >>   	if (!serv->sv_pools) {
> > > > >>   		kfree(serv);
> > > > >> -		return NULL;
> > > > >> +		goto out_err;
> > > > >>   	}
> > > > >>
> > > > >>   	for (i = 0; i<  serv->sv_nrpools; i++) {
> > > > >> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > > >>   	svc_unregister(serv);
> > > > >>
> > > > >>   	return serv;
> > > > >> +
> > > > >> +out_err:
> > > > >> +	rpcb_put_local();
> > > > >> +	return NULL;
> > > > >>   }
> > > > >>
> > > > >>   struct svc_serv *
> > > > >> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
> > > > >>   	svc_unregister(serv);
> > > > >>   	kfree(serv->sv_pools);
> > > > >>   	kfree(serv);
> > > > >> +
> > > > >> +	rpcb_put_local();
> > > > >>   }
> > > > >>   EXPORT_SYMBOL_GPL(svc_destroy);
> > > > >>
> > > > >>
> > > > >
> > > > > I don't get it -- what's the advantage of creating rpcbind clients in
> > > > > __svc_create vs. the old way of creating them just before we plan to
> > > > > use them?
> > > > >
> > > > 
> > > > The main problem here is not in creation, but in destroying those clients.
> > > > Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
> > > > But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.
> > > 
> > > Could we perhaps set up a 'struct pernet_operations' to create a
> > > destructor for them?
> > > 
> > 
> > An even easier idea might be to just not take a reference to the
> > rpcbind client for svc_programs that have vs_hidden set on every
> > version.
> 
> Isn't the problem that Stanislav is trying to solve that we need to be
> able to register and unregister RPC services to the correct rpcbind
> server, depending on which net namespace we are in?
> 
> My understanding is that the current code will register everything to
> whatever rpcbind server is running in the init net namespace because
> that's what rpcb_create_local() uses.
> 

My assumption in reading this set (maybe wrong) was that this was a
preliminary set for now that just plops in function calls in the places
that do this sort of thing now. I figured that eventually he'd convert
rpcb_create_local, et. al. to do the same thing but within the correct
namespace for the calling task.

> My suggestion is to use a struct pernet_operations to detect when a net
> namespace is being created or destroyed, so that the rpcbind client code
> knows when to create or destroy a connection to the server that is
> running in that namespace.
> 

I'm not sure that solves anything. I'd also ass|u|me that something
here (the xprt?) will hold a reference to its netns. If we try to do
this with pernet ops, then I think we'd end up with a chicken-and-egg
problem...

I think the simplest solution would be to basically call these
functions closer to where the rpcbind calls happen today, and just
don't do them when the svc_program has vs_hidden set or if the xprt is
being created with SVC_SOCK_ANONYMOUS set.

In fact, a rethink of the vs_hidden/SVC_SOCK_ANONYMOUS scheme would
probably be a good preliminary patch here. There's a lot of overlap
between those two flags, and somehow consolidating them would probably
be a good thing.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Stanislav Kinsbursky]

13.09.2011 16:51, Jeff Layton пишет:
> My assumption in reading this set (maybe wrong) was that this was a
> preliminary set for now that just plops in function calls in the places
> that do this sort of thing now. I figured that eventually he'd convert
> rpcb_create_local, et. al. to do the same thing but within the correct
> namespace for the calling task.
>

You have a right assumption. This is exactly what I'm going to do next.

>
> I think the simplest solution would be to basically call these
> functions closer to where the rpcbind calls happen today, and just
> don't do them when the svc_program has vs_hidden set or if the xprt is
> being created with SVC_SOCK_ANONYMOUS set.
>

This solution is not the simplest one since we call svc_register() for every svc 
socket if it's not anonymous. But svc_unregister() is called only once for all 
inet families and protocols.

Also I've noticed, that we call svc_unregister in __svc_create(). I.e. we call 
it for nfs callbacks as well (in spite of that we don't need this). Thus, for 
now, nfs callbacks service creation depends on rpcbind clients presence.

So, for my pow, we need something like startup() callback, passed to 
svc_create(_pooled)() to clean up this mess.
This callback will be defined only for lockd and nfsd and will create rpcbind 
clients and remove any stale portmap registrations.

-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation

[Jeff Layton]

On Tue, 13 Sep 2011 17:39:59 +0400
Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> 13.09.2011 16:51, Jeff Layton пишет:
> > My assumption in reading this set (maybe wrong) was that this was a
> > preliminary set for now that just plops in function calls in the places
> > that do this sort of thing now. I figured that eventually he'd convert
> > rpcb_create_local, et. al. to do the same thing but within the correct
> > namespace for the calling task.
> >
> 
> You have a right assumption. This is exactly what I'm going to do next.
> 
> >
> > I think the simplest solution would be to basically call these
> > functions closer to where the rpcbind calls happen today, and just
> > don't do them when the svc_program has vs_hidden set or if the xprt is
> > being created with SVC_SOCK_ANONYMOUS set.
> >
> 
> This solution is not the simplest one since we call svc_register() for every svc 
> socket if it's not anonymous. But svc_unregister() is called only once for all 
> inet families and protocols.
> 

Ahh ok, good point.

> Also I've noticed, that we call svc_unregister in __svc_create(). I.e. we call 
> it for nfs callbacks as well (in spite of that we don't need this). Thus, for 
> now, nfs callbacks service creation depends on rpcbind clients presence.
> 

Yeah, that's just to remove the any existing registration before we set
up the new one. In the case of a "hidden" service that can probably be
skipped if it makes things easier.

> So, for my pow, we need something like startup() callback, passed to 
> svc_create(_pooled)() to clean up this mess.
> This callback will be defined only for lockd and nfsd and will create rpcbind 
> clients and remove any stale portmap registrations.
> 

That sounds like a reasonable scheme. I'll wait to see the patches.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/5] SUNRPC: remove rpcbind clients creation during service registering

[Stanislav Kinsbursky]

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 437ec60..f363efe 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -429,11 +429,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"),
@@ -569,11 +564,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;
 

[PATCH v2 5/5] SUNRPC: remove rpcbind clients destruction on module cleanup

[Stanislav Kinsbursky]

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 f363efe..94a310d 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -1098,15 +1098,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();