SUNRPC: protect service sockets lists during per-net shutdown

v3:
1) rebased on 3.5-rc3 kernel.

v2: destruction of currently processing transport added:
1) Added marking of currently processing transports with XPT_CLOSE on per-net shutdown. These transports will be destroyed in svc_xprt_enqueue() (instead of enqueueing).

2) newly created temporary transport in svc_recv() will be destroyed, if it's "parent" was marked with XPT_CLOSE.
3) spin_lock(&serv->sv_lock) was replaced by spin_lock_bh() in svc_close_net(&serv->sv_lock).

Service sv_tempsocks and sv_permsocks lists are accessible by tasks with different network namespaces, and thus per-net service destruction must be protected.
These lists are protected by service sv_lock. So lets wrap list munipulations with this lock and move tranports destruction outside wrapped area to prevent deadlocks.

Signed-off-by Stanislav Kinsbursky <[email protected]>

---
 net/sunrpc/svc_xprt.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 52 insertions(+), 4 deletions(-)

net/sunrpc/svc_xprt.c +52 -4

index 88f2bf6..4af2114 100644
@@ -320,6 +320,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 	struct svc_pool *pool;
 	struct svc_rqst	*rqstp;
 	int cpu;
+	int destroy = 0;
 
 	if (!svc_xprt_has_something_to_do(xprt))
 		return;

@@ -338,6 +339,17 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 
 	pool->sp_stats.packets++;
 
+	/*
+	 * Check transport close flag. It could be marked as closed on per-net
+	 * service shutdown.
+	 */
+	if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
+		/* Don't enqueue transport if it has to be destroyed. */
+		dprintk("svc: transport %p have to be closed\n", xprt);
+		destroy++;
+		goto out_unlock;
+	}
+
 	/* Mark transport as busy. It will remain in this state until
 	 * the provider calls svc_xprt_received. We update XPT_BUSY
 	 * atomically because it also guards against trying to enqueue

@@ -374,6 +386,8 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 
 out_unlock:
 	spin_unlock_bh(&pool->sp_lock);
+	if (destroy)
+		svc_delete_xprt(xprt);
 }
 EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
 

@@ -714,6 +728,13 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			__module_get(newxpt->xpt_class->xcl_owner);
 			svc_check_conn_limits(xprt->xpt_server);
 			spin_lock_bh(&serv->sv_lock);
+			if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
+				dprintk("svc_recv: found XPT_CLOSE on listener\n");
+				set_bit(XPT_DETACHED, &newxpt->xpt_flags);
+				spin_unlock_bh(&pool->sp_lock);
+				svc_delete_xprt(newxpt);
+				goto out_closed;
+			}
 			set_bit(XPT_TEMP, &newxpt->xpt_flags);
 			list_add(&newxpt->xpt_list, &serv->sv_tempsocks);
 			serv->sv_tmpcnt++;

@@ -739,6 +760,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			len = xprt->xpt_ops->xpo_recvfrom(rqstp);
 		dprintk("svc: got len=%d\n", len);
 	}
+out_closed:
 	svc_xprt_received(xprt);
 
 	/* No data, incomplete (TCP) read, or accept() */

@@ -936,6 +958,7 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
 	struct svc_pool *pool;
 	struct svc_xprt *xprt;
 	struct svc_xprt *tmp;
+	struct svc_rqst *rqstp;
 	int i;
 
 	for (i = 0; i < serv->sv_nrpools; i++) {

@@ -947,11 +970,16 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
 				continue;
 			list_del_init(&xprt->xpt_ready);
 		}
+		list_for_each_entry(rqstp, &pool->sp_all_threads, rq_all) {
+			if (rqstp->rq_xprt && rqstp->rq_xprt->xpt_net == net)
+				set_bit(XPT_CLOSE, &rqstp->rq_xprt->xpt_flags);
+		}
 		spin_unlock_bh(&pool->sp_lock);
 	}
 }
 
-static void svc_clear_list(struct list_head *xprt_list, struct net *net)
+static void svc_clear_list(struct list_head *xprt_list, struct net *net,
+			   struct list_head *kill_list)
 {
 	struct svc_xprt *xprt;
 	struct svc_xprt *tmp;

@@ -959,7 +987,8 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 	list_for_each_entry_safe(xprt, tmp, xprt_list, xpt_list) {
 		if (xprt->xpt_net != net)
 			continue;
-		svc_delete_xprt(xprt);
+		list_move(&xprt->xpt_list, kill_list);
+		set_bit(XPT_DETACHED, &xprt->xpt_flags);
 	}
 	list_for_each_entry(xprt, xprt_list, xpt_list)
 		BUG_ON(xprt->xpt_net == net);

@@ -967,6 +996,15 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 
 void svc_close_net(struct svc_serv *serv, struct net *net)
 {
+	struct svc_xprt *xprt, *tmp;
+	LIST_HEAD(kill_list);
+
+	/*
+	 * Protect the lists, since they can be by tasks with different network
+	 * namespace contexts.
+	 */
+	spin_lock_bh(&serv->sv_lock);
+
 	svc_close_list(&serv->sv_tempsocks, net);
 	svc_close_list(&serv->sv_permsocks, net);
 

@@ -976,8 +1014,18 @@ void svc_close_net(struct svc_serv *serv, struct net *net)
 	 * svc_xprt_enqueue will not add new entries without taking the
 	 * sp_lock and checking XPT_BUSY.
 	 */
-	svc_clear_list(&serv->sv_tempsocks, net);
-	svc_clear_list(&serv->sv_permsocks, net);
+	svc_clear_list(&serv->sv_tempsocks, net, &kill_list);
+	svc_clear_list(&serv->sv_permsocks, net, &kill_list);
+
+	spin_unlock_bh(&serv->sv_lock);
+
+	/*
+	 * Destroy collected transports.
+	 * Note: tranports has been marked as XPT_DETACHED on svc_clear_list(),
+	 * so no need to protect againt list_del() in svc_delete_xprt().
+	 */
+	list_for_each_entry_safe(xprt, tmp, &kill_list, xpt_list)
+		svc_delete_xprt(xprt);
 }
 
 /*

Quoted 3 lines · Stanislav Kinsbursky

v3:
1) rebased on 3.5-rc3 kernel.

Quoted 4 lines

v2: destruction of currently processing transport added:
1) Added marking of currently processing transports with XPT_CLOSE on per-net shutdown. These transports will be destroyed in svc_xprt_enqueue() (instead of enqueueing).

That worries me:

- Why did we originally defer close until svc_recv?

- Are we sure there's no risk to performing it immediately in svc_enqueue? Is it safe to call from the socket callbacks and wherever else we call svc_enqueue?

And in the past I haven't been good at testing for problems here--instead they tend to show up when a use somewhere tries shutting down a server that's under load.

I'll look more closely. Meanwhile you could split out that change as a separate patch and convince me why it's right....

--b.

Quoted 152 lines

2) newly created temporary transport in svc_recv() will be destroyed, if it's "parent" was marked with XPT_CLOSE.
3) spin_lock(&serv->sv_lock) was replaced by spin_lock_bh() in svc_close_net(&serv->sv_lock).

Service sv_tempsocks and sv_permsocks lists are accessible by tasks with different network namespaces, and thus per-net service destruction must be protected.
These lists are protected by service sv_lock. So lets wrap list munipulations with this lock and move tranports destruction outside wrapped area to prevent deadlocks.

Signed-off-by Stanislav Kinsbursky <[email protected]>

---
 net/sunrpc/svc_xprt.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 52 insertions(+), 4 deletions(-)

net/sunrpc/svc_xprt.c +52 -4

index 88f2bf6..4af2114 100644
@@ -320,6 +320,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 	struct svc_pool *pool;
 	struct svc_rqst	*rqstp;
 	int cpu;
+	int destroy = 0;
 
 	if (!svc_xprt_has_something_to_do(xprt))
 		return;

@@ -338,6 +339,17 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 
 	pool->sp_stats.packets++;
 
+	/*
+	 * Check transport close flag. It could be marked as closed on per-net
+	 * service shutdown.
+	 */
+	if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
+		/* Don't enqueue transport if it has to be destroyed. */
+		dprintk("svc: transport %p have to be closed\n", xprt);
+		destroy++;
+		goto out_unlock;
+	}
+
 	/* Mark transport as busy. It will remain in this state until
 	 * the provider calls svc_xprt_received. We update XPT_BUSY
 	 * atomically because it also guards against trying to enqueue

@@ -374,6 +386,8 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 
 out_unlock:
 	spin_unlock_bh(&pool->sp_lock);
+	if (destroy)
+		svc_delete_xprt(xprt);
 }
 EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
 

@@ -714,6 +728,13 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			__module_get(newxpt->xpt_class->xcl_owner);
 			svc_check_conn_limits(xprt->xpt_server);
 			spin_lock_bh(&serv->sv_lock);
+			if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
+				dprintk("svc_recv: found XPT_CLOSE on listener\n");
+				set_bit(XPT_DETACHED, &newxpt->xpt_flags);
+				spin_unlock_bh(&pool->sp_lock);
+				svc_delete_xprt(newxpt);
+				goto out_closed;
+			}
 			set_bit(XPT_TEMP, &newxpt->xpt_flags);
 			list_add(&newxpt->xpt_list, &serv->sv_tempsocks);
 			serv->sv_tmpcnt++;

@@ -739,6 +760,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			len = xprt->xpt_ops->xpo_recvfrom(rqstp);
 		dprintk("svc: got len=%d\n", len);
 	}
+out_closed:
 	svc_xprt_received(xprt);
 
 	/* No data, incomplete (TCP) read, or accept() */

@@ -936,6 +958,7 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
 	struct svc_pool *pool;
 	struct svc_xprt *xprt;
 	struct svc_xprt *tmp;
+	struct svc_rqst *rqstp;
 	int i;
 
 	for (i = 0; i < serv->sv_nrpools; i++) {

@@ -947,11 +970,16 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
 				continue;
 			list_del_init(&xprt->xpt_ready);
 		}
+		list_for_each_entry(rqstp, &pool->sp_all_threads, rq_all) {
+			if (rqstp->rq_xprt && rqstp->rq_xprt->xpt_net == net)
+				set_bit(XPT_CLOSE, &rqstp->rq_xprt->xpt_flags);
+		}
 		spin_unlock_bh(&pool->sp_lock);
 	}
 }
 
-static void svc_clear_list(struct list_head *xprt_list, struct net *net)
+static void svc_clear_list(struct list_head *xprt_list, struct net *net,
+			   struct list_head *kill_list)
 {
 	struct svc_xprt *xprt;
 	struct svc_xprt *tmp;

@@ -959,7 +987,8 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 	list_for_each_entry_safe(xprt, tmp, xprt_list, xpt_list) {
 		if (xprt->xpt_net != net)
 			continue;
-		svc_delete_xprt(xprt);
+		list_move(&xprt->xpt_list, kill_list);
+		set_bit(XPT_DETACHED, &xprt->xpt_flags);
 	}
 	list_for_each_entry(xprt, xprt_list, xpt_list)
 		BUG_ON(xprt->xpt_net == net);

@@ -967,6 +996,15 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 
 void svc_close_net(struct svc_serv *serv, struct net *net)
 {
+	struct svc_xprt *xprt, *tmp;
+	LIST_HEAD(kill_list);
+
+	/*
+	 * Protect the lists, since they can be by tasks with different network
+	 * namespace contexts.
+	 */
+	spin_lock_bh(&serv->sv_lock);
+
 	svc_close_list(&serv->sv_tempsocks, net);
 	svc_close_list(&serv->sv_permsocks, net);
 

@@ -976,8 +1014,18 @@ void svc_close_net(struct svc_serv *serv, struct net *net)
 	 * svc_xprt_enqueue will not add new entries without taking the
 	 * sp_lock and checking XPT_BUSY.
 	 */
-	svc_clear_list(&serv->sv_tempsocks, net);
-	svc_clear_list(&serv->sv_permsocks, net);
+	svc_clear_list(&serv->sv_tempsocks, net, &kill_list);
+	svc_clear_list(&serv->sv_permsocks, net, &kill_list);
+
+	spin_unlock_bh(&serv->sv_lock);
+
+	/*
+	 * Destroy collected transports.
+	 * Note: tranports has been marked as XPT_DETACHED on svc_clear_list(),
+	 * so no need to protect againt list_del() in svc_delete_xprt().
+	 */
+	list_for_each_entry_safe(xprt, tmp, &kill_list, xpt_list)
+		svc_delete_xprt(xprt);
 }
 
 /*

Quoted 7 lines · Stanislav Kinsbursky

v3:
1) rebased on 3.5-rc3 kernel.

v2: destruction of currently processing transport added:
1) Added marking of currently processing transports with XPT_CLOSE on per-net shutdown. These transports will be destroyed in svc_xprt_enqueue() (instead of enqueueing).

That worries me:

- Why did we originally defer close until svc_recv?
- Are we sure there's no risk to performing it immediately in svc_enqueue? Is it safe to call from the socket callbacks and wherever else we call svc_enqueue?

And in the past I haven't been good at testing for problems here--instead they tend to show up when a use somewhere tries shutting down a server that's under load.

I'll look more closely. Meanwhile you could split out that change as a separate patch and convince me why it's right....

--b.

Quoted 152 lines

2) newly created temporary transport in svc_recv() will be destroyed, if it's "parent" was marked with XPT_CLOSE.
3) spin_lock(&serv->sv_lock) was replaced by spin_lock_bh() in svc_close_net(&serv->sv_lock).

Service sv_tempsocks and sv_permsocks lists are accessible by tasks with different network namespaces, and thus per-net service destruction must be protected.
These lists are protected by service sv_lock. So lets wrap list munipulations with this lock and move tranports destruction outside wrapped area to prevent deadlocks.

Signed-off-by Stanislav Kinsbursky <[email protected]>

---
 net/sunrpc/svc_xprt.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 52 insertions(+), 4 deletions(-)

net/sunrpc/svc_xprt.c +52 -4

index 88f2bf6..4af2114 100644
@@ -320,6 +320,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 	struct svc_pool *pool;
 	struct svc_rqst	*rqstp;
 	int cpu;
+	int destroy = 0;
 
 	if (!svc_xprt_has_something_to_do(xprt))
 		return;

@@ -338,6 +339,17 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 
 	pool->sp_stats.packets++;
 
+	/*
+	 * Check transport close flag. It could be marked as closed on per-net
+	 * service shutdown.
+	 */
+	if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
+		/* Don't enqueue transport if it has to be destroyed. */
+		dprintk("svc: transport %p have to be closed\n", xprt);
+		destroy++;
+		goto out_unlock;
+	}
+
 	/* Mark transport as busy. It will remain in this state until
 	 * the provider calls svc_xprt_received. We update XPT_BUSY
 	 * atomically because it also guards against trying to enqueue

@@ -374,6 +386,8 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 
 out_unlock:
 	spin_unlock_bh(&pool->sp_lock);
+	if (destroy)
+		svc_delete_xprt(xprt);
 }
 EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
 

@@ -714,6 +728,13 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			__module_get(newxpt->xpt_class->xcl_owner);
 			svc_check_conn_limits(xprt->xpt_server);
 			spin_lock_bh(&serv->sv_lock);
+			if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
+				dprintk("svc_recv: found XPT_CLOSE on listener\n");
+				set_bit(XPT_DETACHED, &newxpt->xpt_flags);
+				spin_unlock_bh(&pool->sp_lock);
+				svc_delete_xprt(newxpt);
+				goto out_closed;
+			}
 			set_bit(XPT_TEMP, &newxpt->xpt_flags);
 			list_add(&newxpt->xpt_list, &serv->sv_tempsocks);
 			serv->sv_tmpcnt++;

@@ -739,6 +760,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			len = xprt->xpt_ops->xpo_recvfrom(rqstp);
 		dprintk("svc: got len=%d\n", len);
 	}
+out_closed:
 	svc_xprt_received(xprt);
 
 	/* No data, incomplete (TCP) read, or accept() */

@@ -936,6 +958,7 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
 	struct svc_pool *pool;
 	struct svc_xprt *xprt;
 	struct svc_xprt *tmp;
+	struct svc_rqst *rqstp;
 	int i;
 
 	for (i = 0; i < serv->sv_nrpools; i++) {

@@ -947,11 +970,16 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
 				continue;
 			list_del_init(&xprt->xpt_ready);
 		}
+		list_for_each_entry(rqstp, &pool->sp_all_threads, rq_all) {
+			if (rqstp->rq_xprt && rqstp->rq_xprt->xpt_net == net)
+				set_bit(XPT_CLOSE, &rqstp->rq_xprt->xpt_flags);
+		}
 		spin_unlock_bh(&pool->sp_lock);
 	}
 }
 
-static void svc_clear_list(struct list_head *xprt_list, struct net *net)
+static void svc_clear_list(struct list_head *xprt_list, struct net *net,
+			   struct list_head *kill_list)
 {
 	struct svc_xprt *xprt;
 	struct svc_xprt *tmp;

@@ -959,7 +987,8 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 	list_for_each_entry_safe(xprt, tmp, xprt_list, xpt_list) {
 		if (xprt->xpt_net != net)
 			continue;
-		svc_delete_xprt(xprt);
+		list_move(&xprt->xpt_list, kill_list);
+		set_bit(XPT_DETACHED, &xprt->xpt_flags);
 	}
 	list_for_each_entry(xprt, xprt_list, xpt_list)
 		BUG_ON(xprt->xpt_net == net);

@@ -967,6 +996,15 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 
 void svc_close_net(struct svc_serv *serv, struct net *net)
 {
+	struct svc_xprt *xprt, *tmp;
+	LIST_HEAD(kill_list);
+
+	/*
+	 * Protect the lists, since they can be by tasks with different network
+	 * namespace contexts.
+	 */
+	spin_lock_bh(&serv->sv_lock);
+
 	svc_close_list(&serv->sv_tempsocks, net);
 	svc_close_list(&serv->sv_permsocks, net);
 

@@ -976,8 +1014,18 @@ void svc_close_net(struct svc_serv *serv, struct net *net)
 	 * svc_xprt_enqueue will not add new entries without taking the
 	 * sp_lock and checking XPT_BUSY.
 	 */
-	svc_clear_list(&serv->sv_tempsocks, net);
-	svc_clear_list(&serv->sv_permsocks, net);
+	svc_clear_list(&serv->sv_tempsocks, net, &kill_list);
+	svc_clear_list(&serv->sv_permsocks, net, &kill_list);
+
+	spin_unlock_bh(&serv->sv_lock);
+
+	/*
+	 * Destroy collected transports.
+	 * Note: tranports has been marked as XPT_DETACHED on svc_clear_list(),
+	 * so no need to protect againt list_del() in svc_delete_xprt().
+	 */
+	list_for_each_entry_safe(xprt, tmp, &kill_list, xpt_list)
+		svc_delete_xprt(xprt);
 }
 
 /*

Quoted 4 lines · Stanislav Kinsbursky

Quoted 3 lines

v3:
1) rebased on 3.5-rc3 kernel.

Quoted 8 lines

Quoted 4 lines

v2: destruction of currently processing transport added:
1) Added marking of currently processing transports with XPT_CLOSE on per-net shutdown. These transports will be destroyed in svc_xprt_enqueue() (instead of enqueueing).

That worries me:

- Why did we originally defer close until svc_recv?

Quoted 10 lines

- Are we sure there's no risk to performing it immediately in svc_enqueue? Is it safe to call from the socket callbacks and wherever else we call svc_enqueue?

And in the past I haven't been good at testing for problems here--instead they tend to show up when a use somewhere tries shutting down a server that's under load.

I'll look more closely. Meanwhile you could split out that change as a separate patch and convince me why it's right....

Looking back at this:

- adding the sv_lock looks like the right thing to do anyway independent of containers, because svc_age_temp_xprts may still be running.

- I'm increasingly unhappy about sharing rpc servers between network namespaces. Everything would be easier to understand if they were independent. Can we figure out how to do that?

Quoted 155 lines

--b.

Quoted 152 lines

2) newly created temporary transport in svc_recv() will be destroyed, if it's "parent" was marked with XPT_CLOSE.
3) spin_lock(&serv->sv_lock) was replaced by spin_lock_bh() in svc_close_net(&serv->sv_lock).

Service sv_tempsocks and sv_permsocks lists are accessible by tasks with different network namespaces, and thus per-net service destruction must be protected.
These lists are protected by service sv_lock. So lets wrap list munipulations with this lock and move tranports destruction outside wrapped area to prevent deadlocks.

Signed-off-by Stanislav Kinsbursky <[email protected]>

---
  net/sunrpc/svc_xprt.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++---
  1 files changed, 52 insertions(+), 4 deletions(-)

net/sunrpc/svc_xprt.c +52 -4

index 88f2bf6..4af2114 100644
@@ -320,6 +320,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
  	struct svc_pool *pool;
  	struct svc_rqst	*rqstp;
  	int cpu;
+	int destroy = 0;
 
  	if (!svc_xprt_has_something_to_do(xprt))
  		return;

@@ -338,6 +339,17 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 
  	pool->sp_stats.packets++;
 
+	/*
+	 * Check transport close flag. It could be marked as closed on per-net
+	 * service shutdown.
+	 */
+	if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
+		/* Don't enqueue transport if it has to be destroyed. */
+		dprintk("svc: transport %p have to be closed\n", xprt);
+		destroy++;
+		goto out_unlock;
+	}
+
  	/* Mark transport as busy. It will remain in this state until
  	 * the provider calls svc_xprt_received. We update XPT_BUSY
  	 * atomically because it also guards against trying to enqueue

@@ -374,6 +386,8 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
 
  out_unlock:
  	spin_unlock_bh(&pool->sp_lock);
+	if (destroy)
+		svc_delete_xprt(xprt);
  }
  EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
 

@@ -714,6 +728,13 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
  			__module_get(newxpt->xpt_class->xcl_owner);
  			svc_check_conn_limits(xprt->xpt_server);
  			spin_lock_bh(&serv->sv_lock);
+			if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
+				dprintk("svc_recv: found XPT_CLOSE on listener\n");
+				set_bit(XPT_DETACHED, &newxpt->xpt_flags);
+				spin_unlock_bh(&pool->sp_lock);
+				svc_delete_xprt(newxpt);
+				goto out_closed;
+			}
  			set_bit(XPT_TEMP, &newxpt->xpt_flags);
  			list_add(&newxpt->xpt_list, &serv->sv_tempsocks);
  			serv->sv_tmpcnt++;

@@ -739,6 +760,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
  			len = xprt->xpt_ops->xpo_recvfrom(rqstp);
  		dprintk("svc: got len=%d\n", len);
  	}
+out_closed:
  	svc_xprt_received(xprt);
 
  	/* No data, incomplete (TCP) read, or accept() */

@@ -936,6 +958,7 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
  	struct svc_pool *pool;
  	struct svc_xprt *xprt;
  	struct svc_xprt *tmp;
+	struct svc_rqst *rqstp;
  	int i;
 
  	for (i = 0; i < serv->sv_nrpools; i++) {

@@ -947,11 +970,16 @@ static void svc_clear_pools(struct svc_serv *serv, struct net *net)
  				continue;
  			list_del_init(&xprt->xpt_ready);
  		}
+		list_for_each_entry(rqstp, &pool->sp_all_threads, rq_all) {
+			if (rqstp->rq_xprt && rqstp->rq_xprt->xpt_net == net)
+				set_bit(XPT_CLOSE, &rqstp->rq_xprt->xpt_flags);
+		}
  		spin_unlock_bh(&pool->sp_lock);
  	}
  }
 
-static void svc_clear_list(struct list_head *xprt_list, struct net *net)
+static void svc_clear_list(struct list_head *xprt_list, struct net *net,
+			   struct list_head *kill_list)
  {
  	struct svc_xprt *xprt;
  	struct svc_xprt *tmp;

@@ -959,7 +987,8 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
  	list_for_each_entry_safe(xprt, tmp, xprt_list, xpt_list) {
  		if (xprt->xpt_net != net)
  			continue;
-		svc_delete_xprt(xprt);
+		list_move(&xprt->xpt_list, kill_list);
+		set_bit(XPT_DETACHED, &xprt->xpt_flags);
  	}
  	list_for_each_entry(xprt, xprt_list, xpt_list)
  		BUG_ON(xprt->xpt_net == net);

@@ -967,6 +996,15 @@ static void svc_clear_list(struct list_head *xprt_list, struct net *net)
 
  void svc_close_net(struct svc_serv *serv, struct net *net)
  {
+	struct svc_xprt *xprt, *tmp;
+	LIST_HEAD(kill_list);
+
+	/*
+	 * Protect the lists, since they can be by tasks with different network
+	 * namespace contexts.
+	 */
+	spin_lock_bh(&serv->sv_lock);
+
  	svc_close_list(&serv->sv_tempsocks, net);
  	svc_close_list(&serv->sv_permsocks, net);
 

@@ -976,8 +1014,18 @@ void svc_close_net(struct svc_serv *serv, struct net *net)
  	 * svc_xprt_enqueue will not add new entries without taking the
  	 * sp_lock and checking XPT_BUSY.
  	 */
-	svc_clear_list(&serv->sv_tempsocks, net);
-	svc_clear_list(&serv->sv_permsocks, net);
+	svc_clear_list(&serv->sv_tempsocks, net, &kill_list);
+	svc_clear_list(&serv->sv_permsocks, net, &kill_list);
+
+	spin_unlock_bh(&serv->sv_lock);
+
+	/*
+	 * Destroy collected transports.
+	 * Note: tranports has been marked as XPT_DETACHED on svc_clear_list(),
+	 * so no need to protect againt list_del() in svc_delete_xprt().
+	 */
+	list_for_each_entry_safe(xprt, tmp, &kill_list, xpt_list)
+		svc_delete_xprt(xprt);
  }
 
  /*

16.08.2012 23:29, J. Bruce Fields пишет:

Quoted 6 lines

Looking back at this:

- adding the sv_lock looks like the right thing to do anyway independent of containers, because svc_age_temp_xprts may still be running.

Quoted 4 lines

- I'm increasingly unhappy about sharing rpc servers between network namespaces. Everything would be easier to understand if they were independent. Can we figure out how to do that?

Could you, please, elaborate on your your unhappiness?

I.e. I don't like it too. But the problem here, is that rpc server is tied with kernel threads creation and destruction. And these threads can be only a part of initial pid namespace (because we have only one kthreadd). And we decided do not create new kernel thread per container when were discussing the problem last time.

Quoted 7 lines

16.08.2012 23:29, J. Bruce Fields пишет:

Quoted 6 lines

Looking back at this:

- adding the sv_lock looks like the right thing to do anyway independent of containers, because svc_age_temp_xprts may still be running.

Quoted 6 lines

Quoted 4 lines

- I'm increasingly unhappy about sharing rpc servers between network namespaces. Everything would be easier to understand if they were independent. Can we figure out how to do that?

Could you, please, elaborate on your your unhappiness?

It seems like you're having to do a lot of work on each individual rpc server (callback server, lockd, etc.) to make per-net startup/shutdown work. And then we still don't have it quite right (see the shutdown races).)

In general whenever we have the opportunity to have entirely separate data structures, I'd expect that to simplify things: it should eliminate some locking and reference-counting issues.

Quoted 5 lines

I.e. I don't like it too. But the problem here, is that rpc server is tied with kernel threads creation and destruction. And these threads can be only a part of initial pid namespace (because we have only one kthreadd). And we decided do not create new kernel thread per container when were discussing the problem last time.

There really should be some way to create a kernel thread in a specific namespace, shouldn't there?

Until we have that, could the threads be taught to fix their namespace on startup?

--b.

20.08.2012 18:56, J. Bruce Fields пишет:

Quoted 15 lines · Stanislav Kinsbursky

Quoted 7 lines

16.08.2012 23:29, J. Bruce Fields пишет:

Quoted 6 lines

Looking back at this:

- adding the sv_lock looks like the right thing to do anyway independent of containers, because svc_age_temp_xprts may still be running.

Quoted 6 lines

Quoted 4 lines

- I'm increasingly unhappy about sharing rpc servers between network namespaces. Everything would be easier to understand if they were independent. Can we figure out how to do that?

Could you, please, elaborate on your your unhappiness?

Quoted 9 lines

It seems like you're having to do a lot of work on each individual rpc server (callback server, lockd, etc.) to make per-net startup/shutdown work. And then we still don't have it quite right (see the shutdown races).)

In general whenever we have the opportunity to have entirely separate data structures, I'd expect that to simplify things: it should eliminate some locking and reference-counting issues.

Agreed. But current solution still looks like the easies way to me to implement desired functionality.

Quoted 6 lines

Quoted 5 lines

I.e. I don't like it too. But the problem here, is that rpc server is tied with kernel threads creation and destruction. And these threads can be only a part of initial pid namespace (because we have only one kthreadd). And we decided do not create new kernel thread per container when were discussing the problem last time.

Quoted 3 lines

There really should be some way to create a kernel thread in a specific namespace, shouldn't there?

Kthreads support in a container is rather a "political" problem, than an implementation problem.

Currently, when you call kthread_create(), you add new job to kthreadd queue. Kthreadd is unique, starts right after init and lives in global initial environment. So, any kthread inherits namespaces from it.
Of course, we can start one kthread per environment and change it's root or even network namespace in kthread function. But pid namespace of this kthread will remain global.

It looks like not a big problem, when we shutdown kthread by some variable. But what about killable nfsd kthreads?

1) We can't kill them from nested pid namespace.
2) How we will differ nfsd kthreads in initial pid namespace?

In OpenVZ we have kthreadd per pid hamespace and it allows us to create kthreads (and thus services) per pid namespace.

Quoted 40 lines

20.08.2012 18:56, J. Bruce Fields пишет:

Quoted 15 lines · Stanislav Kinsbursky

Quoted 7 lines

16.08.2012 23:29, J. Bruce Fields пишет:

Quoted 6 lines

Looking back at this:

- adding the sv_lock looks like the right thing to do anyway independent of containers, because svc_age_temp_xprts may still be running.

Quoted 6 lines

Quoted 4 lines

- I'm increasingly unhappy about sharing rpc servers between network namespaces. Everything would be easier to understand if they were independent. Can we figure out how to do that?

Could you, please, elaborate on your your unhappiness?

Quoted 9 lines

It seems like you're having to do a lot of work on each individual rpc server (callback server, lockd, etc.) to make per-net startup/shutdown work. And then we still don't have it quite right (see the shutdown races).)

In general whenever we have the opportunity to have entirely separate data structures, I'd expect that to simplify things: it should eliminate some locking and reference-counting issues.

Agreed. But current solution still looks like the easies way to me to implement desired functionality.

Quoted 9 lines

Quoted 5 lines

I.e. I don't like it too. But the problem here, is that rpc server is tied with kernel threads creation and destruction. And these threads can be only a part of initial pid namespace (because we have only one kthreadd). And we decided do not create new kernel thread per container when were discussing the problem last time.

There really should be some way to create a kernel thread in a specific namespace, shouldn't there?

Quoted 2 lines

Kthreads support in a container is rather a "political" problem, than an implementation problem.

Is there a mail thread somewhere with a summary of the objections?

Quoted 7 lines

Currently, when you call kthread_create(), you add new job to kthreadd queue. Kthreadd is unique, starts right after init and lives in global initial environment. So, any kthread inherits namespaces from it.
Of course, we can start one kthread per environment and change it's root or even network namespace in kthread function. But pid namespace of this kthread will remain global.

OK. But the current implementation will leave all the server threads in the initial pid namespace, too.

Quoted 2 lines

It looks like not a big problem, when we shutdown kthread by some variable. But what about killable nfsd kthreads?

And we're stuck with that problem either way too, aren't we?

Quoted 2 lines

1) We can't kill them from nested pid namespace.
2) How we will differ nfsd kthreads in initial pid namespace?

I have to admit for my purposes I don't care too much about pid namespaces or about signalling server threads. It'd be nice to get those things right but it wouldn't bother me that much not to.

Another stupid idea: can we do our own implementation of something like kthreadd just for the purpose of starting rpc server threads? It doesn't seem that complicated.

--b.

Quoted 2 lines

In OpenVZ we have kthreadd per pid hamespace and it allows us to create kthreads (and thus services) per pid namespace.

20.08.2012 20:58, J. Bruce Fields пишет:

Quoted 9 lines · Stanislav Kinsbursky

Quoted 7 lines

Currently, when you call kthread_create(), you add new job to kthreadd queue. Kthreadd is unique, starts right after init and lives in global initial environment. So, any kthread inherits namespaces from it.
Of course, we can start one kthread per environment and change it's root or even network namespace in kthread function. But pid namespace of this kthread will remain global.

Quoted 8 lines

OK. But the current implementation will leave all the server threads in the initial pid namespace, too.

Quoted 2 lines

It looks like not a big problem, when we shutdown kthread by some variable. But what about killable nfsd kthreads?

And we're stuck with that problem either way too, aren't we?

Yes, we are. But at least we are avoiding patching of task subsystem.

Quoted 11 lines

Quoted 2 lines

1) We can't kill them from nested pid namespace.
2) How we will differ nfsd kthreads in initial pid namespace?

I have to admit for my purposes I don't care too much about pid namespaces or about signalling server threads. It'd be nice to get those things right but it wouldn't bother me that much not to.

Another stupid idea: can we do our own implementation of something like kthreadd just for the purpose of starting rpc server threads? It doesn't seem that complicated.

Gm...
This idea is not stupid. If I understand you right, you suggest to implement a service per network namespace (i.e. not only data, but also threads)?

Looking back at this:

- adding the sv_lock looks like the right thing to do anyway independent of containers, because svc_age_temp_xprts may still be running.