[PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically

[PATCH v3 00/11] SUNRPC: make rpcbind clients allocated and destroyed dynamically

[Stanislav Kinsbursky]

v3:
1) rebased to v3.1-rc6 (against branch
git://git.linux-nfs.org/projects/trondmy/nfs-2.6.git)
2) creation and destruction on rpcbind clients now depends on service type

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 (11):
      SUNRPC: introduce helpers for reference counted rpcbind clients
      SUNRPC: use rpcbind reference counting helpers
      SUNRPC: introduce svc helpers for prepairing rpcbind infrastructure
      SUNRPC: parametrize svc creation calls with portmapper flag
      SUNRPC: cleanup service destruction
      Lockd: force creation of rpcbind clients during service creation
      NFS: avoid rpcbind clients creation during service creation
      NFSd: force creation of rpcbind clients during service creation
      NFSd: call svc rpcbind cleanup explicitly
      SUNRPC: remove rpcbind clients creation during service registering
      SUNRPC: remove rpcbind clients destruction on module cleanup


 fs/lockd/svc.c              |    2 +
 fs/nfs/callback.c           |    2 +
 fs/nfsd/nfssvc.c            |    4 ++
 include/linux/sunrpc/clnt.h |    2 +
 include/linux/sunrpc/svc.h  |    3 ++
 net/sunrpc/rpcb_clnt.c      |   85 ++++++++++++++++++++++++++++---------------
 net/sunrpc/sunrpc_syms.c    |    3 --
 net/sunrpc/svc.c            |   41 +++++++++++++++++----
 8 files changed, 98 insertions(+), 44 deletions(-)

-- 
Signature

[PATCH v3 01/11] 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 |   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.

[PATCH v3 02/11] 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 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)

[PATCH v3 03/11] SUNRPC: introduce svc helpers for prepairing rpcbind infrastructure

[Stanislav Kinsbursky]

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

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

[Stanislav Kinsbursky]

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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

---
 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;

--
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 v3 05/11] SUNRPC: cleanup service destruction

[Stanislav Kinsbursky]

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);
 }

[PATCH v3 06/11] Lockd: force creation of rpcbind clients during service creation

[Stanislav Kinsbursky]

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;

[PATCH v3 07/11] NFS: avoid rpcbind clients creation during service creation

[Stanislav Kinsbursky]

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;

[PATCH v3 08/11] NFSd: force creation of rpcbind clients during service creation

[Stanislav Kinsbursky]

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;
 

[PATCH v3 09/11] NFSd: call svc rpcbind cleanup explicitly

[Stanislav Kinsbursky]

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

[PATCH v3 10/11] 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 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;
 

[PATCH v3 11/11] 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 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();

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

[Jeff Layton]

On Tue, 13 Sep 2011 22:13:51 +0400
Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> 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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> 
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
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 v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag

[Stanislav Kinsbursky]

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

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

[Jeff Layton]

On Mon, 19 Sep 2011 18:51:31 +0400
Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> 19.09.2011 18:08, Jeff Layton пишет:
> > On Tue, 13 Sep 2011 22:13:51 +0400
> > Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> >>
> >> ---
> >>   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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> 
> 


-- 
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 v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag

[Stanislav Kinsbursky]

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

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

[Jeff Layton]

On Mon, 19 Sep 2011 19:42:12 +0400
Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> 19.09.2011 19:07, Jeff Layton пишет:
> > On Mon, 19 Sep 2011 18:51:31 +0400
> > Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  wrote:
> >
> >> 19.09.2011 18:08, Jeff Layton пишет:
> >>> On Tue, 13 Sep 2011 22:13:51 +0400
> >>> Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>   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-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> >>>>
> >>>> ---
> >>>>    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-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 v3 04/11] SUNRPC: parametrize svc creation calls with portmapper flag

[Stanislav Kinsbursky]

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