[PATCH] prevent sunrpc deadlock with debug on

[PATCH] prevent sunrpc deadlock with debug on

[Olaf Kirch]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

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

Olaf
-- 
Olaf Kirch     | Things that make Monday morning interesting, #2:
okir@suse.de   |        "We have 8,000 NFS mount points, why do we keep
---------------+ 	 running out of privileged ports?"

[-- Attachment #2: sunrpc-sched-deadlock --]
[-- Type: text/plain, Size: 1293 bytes --]

From: Shirly Ma <xma@us.ibm.com>
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 <okir@suse.de>

diff -urN linux-2.6.5-7.13/net/sunrpc/sched.c linux-2.6.5-7.13p/net/sunrpc/sched.c
--- linux-2.6.5-7.13/net/sunrpc/sched.c	2004-04-30 10:46:58.000000000 -0500
+++ linux-2.6.5-7.13p/net/sunrpc/sched.c	2004-04-30 17:21:05.000000000 -0500
@@ -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);
 }

Re: [PATCH] prevent sunrpc deadlock with debug on

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

to den 16.12.2004 Klokka 17:57 (+0100) skreiv Olaf Kirch:
> 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

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>

[-- Attachment #2: linux-2.6.10-03-fix_rpc_show_task.dif --]
[-- Type: text/plain, Size: 2677 bytes --]

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 <Trond.Myklebust@netapp.com>
---
 sched.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

Index: linux-2.6.10/net/sunrpc/sched.c
===================================================================
--- linux-2.6.10.orig/net/sunrpc/sched.c	2004-12-26 01:34:01.000000000 +0100
+++ linux-2.6.10/net/sunrpc/sched.c	2004-12-29 00:41:16.223908500 +0100
@@ -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);
 }
 

Re: [PATCH] prevent sunrpc deadlock with debug on

[Olaf Kirch]

Hi Trond,

On Wed, Dec 29, 2004 at 12:43:16AM +0100, Trond Myklebust wrote:
> 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
-- 
Olaf Kirch     | Things that make Monday morning interesting, #2:
okir@suse.de   |        "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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] prevent sunrpc deadlock with debug on

[Trond Myklebust]

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

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

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

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs