Skip to contentSkip to conversation

prevent sunrpc deadlock with debug on

PATCH In review Olaf Kirch 2 participants 4 messages Dec 16, 2004 – Jan 5, 2005

Subsystems (3)
Networking NFS client NFS server
lore
Share & tools
lore

Olaf Kirch

author
#0 lore raw

PATCH 1/1 prevent sunrpc deadlock with debug on

Here's a patch we added to our kernel a while ago, but which I forgot to submit... here it is.

Olaf

Signature · 3 lines
Olaf Kirch     | Things that make Monday morning interesting, #2:
[email protected]   |        "We have 8,000 NFS mount points, why do we keep
---------------+ 	 running out of privileged ports?"

From: Shirly Ma <[email protected]>
Subject: Prevent sunrpc deadlock with debugging turned on
References: 38960

This patch fixes a deadlock in the RPC code when debugging is enabled.

CPU 1:

	rpc_wake_up_task
		spin_lock_bh(&rpc_queue_lock);
		-> __rpc_wake_up_task
			-> rpc_show_tasks
				-> spin_lock(&rpc_sched_lock);

CPU 2:

	rpc_killall_tasks
		spin_lock(&rpc_sched_lock);
		-> rpc_wake_up_task()
			spin_lock_bh(&rpc_queue_lock);

This is a very rare condition, but it was actually observed several times during test runs on ppc64.

Signed-off-by Olaf Kirch <[email protected]>
net/sunrpc/sched.c +5 -1
diff -urN linux-2.6.5-7.13/net/sunrpc/sched.c linux-2.6.5-7.13p/net/sunrpc/sched.c
@@ -931,12 +931,16 @@
 	/*
 	 * Spin lock all_tasks to prevent changes...
 	 */
+again:
 	spin_lock(&rpc_sched_lock);
 	alltask_for_each(rovr, le, &all_tasks)
-		if (!clnt || rovr->tk_client == clnt) {
+		if (!(rovr->tk_flags & RPC_TASK_KILLED) && 
+		    (!clnt || rovr->tk_client == clnt)) {
 			rovr->tk_flags |= RPC_TASK_KILLED;
 			rpc_exit(rovr, -EIO);
+			spin_unlock(&rpc_sched_lock);
 			rpc_wake_up_task(rovr);
+			goto again;
 		}
 	spin_unlock(&rpc_sched_lock);
 }
#1 lore raw

to den 16.12.2004 Klokka 17:57 (+0100) skreiv Olaf Kirch:

Quoted 2 lines

Here's a patch we added to our kernel a while ago, but which I forgot to submit... here it is.

Olaf,

The fix here is blatantly wrong, since you are releasing the lock and
then referencing rpc_wake_up_task() on the pointer it was protecting. That is far worse than the "problem" it is trying to fix (which is after all debugging code).

I think I've found the real bug that Shirly was seeing. Can you ask her to test if this change fixes the actual problem?

Cheers,
Trond

Signature · 1 line
Trond Myklebust <[email protected]>

RPC: rpc_tasks on the "all_tasks" list must be initialized first.

Shirly Ma reported seeing problems with rpc_killall_tasks() causing the task->tk_magic debugging test to trigger.

Signed-off-by Trond Myklebust <[email protected]>
---
 sched.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)
net/sunrpc/sched.c +13 -9
Index: linux-2.6.10/net/sunrpc/sched.c
===================================================================
@@ -25,6 +25,7 @@
 
 #ifdef RPC_DEBUG
 #define RPCDBG_FACILITY		RPCDBG_SCHED
+#define RPC_TASK_MAGIC_ID	0xf00baa
 static int			rpc_task_id;
 #endif
 

@@ -352,7 +353,7 @@ static void __rpc_do_wake_up_task(struct
 	dprintk("RPC: %4d __rpc_wake_up_task (now %ld)\n", task->tk_pid, jiffies);
 
 #ifdef RPC_DEBUG
-	if (task->tk_magic != 0xf00baa) {
+	if (unlikely(task->tk_magic != RPC_TASK_MAGIC_ID)) {
 		printk(KERN_ERR "RPC: attempt to wake up non-existing task!\n");
 		rpc_debug = ~0;
 		rpc_show_tasks();

@@ -766,11 +767,6 @@ void rpc_init_task(struct rpc_task *task
 	if (!RPC_IS_ASYNC(task))
 		init_waitqueue_head(&task->u.tk_wait.waitq);
 
-	/* Add to global list of all tasks */
-	spin_lock(&rpc_sched_lock);
-	list_add(&task->tk_task, &all_tasks);
-	spin_unlock(&rpc_sched_lock);
-
 	if (clnt) {
 		atomic_inc(&clnt->cl_users);
 		if (clnt->cl_softrtry)

@@ -780,9 +776,14 @@ void rpc_init_task(struct rpc_task *task
 	}
 
 #ifdef RPC_DEBUG
-	task->tk_magic = 0xf00baa;
+	task->tk_magic = RPC_TASK_MAGIC_ID;
 	task->tk_pid = rpc_task_id++;
 #endif
+	/* Add to global list of all tasks */
+	spin_lock(&rpc_sched_lock);
+	list_add_tail(&task->tk_task, &all_tasks);
+	spin_unlock(&rpc_sched_lock);
+
 	dprintk("RPC: %4d new task procpid %d\n", task->tk_pid,
 				current->pid);
 }

@@ -840,7 +841,7 @@ void rpc_release_task(struct rpc_task *t
 	dprintk("RPC: %4d release task\n", task->tk_pid);
 
 #ifdef RPC_DEBUG
-	if (task->tk_magic != 0xf00baa) {
+	if (unlikely(task->tk_magic != RPC_TASK_MAGIC_ID)) {
 		printk(KERN_ERR "RPC: attempt to release a non-existing task!\n");
 		rpc_debug = ~0;
 		rpc_show_tasks();

@@ -956,12 +957,15 @@ void rpc_killall_tasks(struct rpc_clnt *
 	 * Spin lock all_tasks to prevent changes...
 	 */
 	spin_lock(&rpc_sched_lock);
-	alltask_for_each(rovr, le, &all_tasks)
+	alltask_for_each(rovr, le, &all_tasks) {
+		if (! RPC_IS_ACTIVATED(rovr))
+			continue;
 		if (!clnt || rovr->tk_client == clnt) {
 			rovr->tk_flags |= RPC_TASK_KILLED;
 			rpc_exit(rovr, -EIO);
 			rpc_wake_up_task(rovr);
 		}
+	}
 	spin_unlock(&rpc_sched_lock);
 }
author
#2 lore raw

Hi Trond,

Quoted 2 lines · Trond Myklebust

I think I've found the real bug that Shirly was seeing. Can you ask her to test if this change fixes the actual problem?

I don't think that will be possible, but I'll ask her. The patch does look correct though.

Still, if we ever run into a corrupted task struct, we will still deadlock. How about this:

  show_tasks(void)
  {
        struct list_head *le;
        struct rpc_task *t;

- spin_lock(&rpc_sched_lock);
+ if (!spin_trylock(&rpc_sched_lock))

+		return;
        if (list_empty(&all_tasks)) {
                spin_unlock(&rpc_sched_lock);

Olaf

Signature · 12 lines
Olaf Kirch     | Things that make Monday morning interesting, #2:
[email protected]   |        "We have 8,000 NFS mount points, why do we keep
---------------+ 	 running out of privileged ports?"


-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#3 lore raw

on den 05.01.2005 Klokka 17:44 (+0100) skreiv Olaf Kirch:

Review

Quoted 13 lines

Still, if we ever run into a corrupted task struct, we will still deadlock. How about this:

  show_tasks(void)
  {
        struct list_head *le;
        struct rpc_task *t;
(quoted diff) +2 -1
-       spin_lock(&rpc_sched_lock);
+	if (!spin_trylock(&rpc_sched_lock))
+		return;
        if (list_empty(&all_tasks)) {
                spin_unlock(&rpc_sched_lock);

In the end I opted to convert that deadlock into a BUG_ON() (see the latest Bitkeeper kernel). The whole point of that code is to ensure that people report real bugs in which the RPC state machine is being corrupted. Its entire reason for existence disappears if people start worrying about the debugging deadlocks, but not the underlying bugs.

BUG_ON() is a more definite obstruction, and so tends to get people annoyed enough to file reports. 8-)

Cheers,
Trond

Signature · 11 lines
Trond Myklebust <[email protected]>



-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs