[PATCH 1/2] fix EH thread teardown

[PATCH 1/2] fix EH thread teardown

[Christoph Hellwig]

As Rolf Eike Beer noted we might not actually get to the
kthread_should_stop() because we are waiting in the semaphore forever.
I didn't get a rmmod hang because of this, but my instrumentation showed
the thread defitiyly didn't exit.

So make sure to wake the EH thread before the kthread_stop, and to plug
the reaming race check kthead_should_stop() a second time just before
calling down_interruptible().


Index: scsi-misc-2.6/drivers/scsi/hosts.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/hosts.c	2005-09-07 14:21:44.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/hosts.c	2005-09-07 14:22:33.000000000 +0200
@@ -226,8 +226,10 @@
 	struct Scsi_Host *shost = dev_to_shost(dev);
 	struct device *parent = dev->parent;
 
-	if (shost->ehandler)
+	if (shost->ehandler) {
+		up(shost->eh_wait);
 		kthread_stop(shost->ehandler);
+	}
 	if (shost->work_q)
 		destroy_workqueue(shost->work_q);
 
Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_error.c	2005-09-07 14:21:44.000000000 +0200
+++ scsi-misc-2.6/drivers/scsi/scsi_error.c	2005-09-07 14:22:58.000000000 +0200
@@ -1591,7 +1591,7 @@
 	SCSI_LOG_ERROR_RECOVERY(3, printk("Wake up parent of"
 					  " scsi_eh_%d\n",shost->host_no));
 
-	while (1) {
+	while (!kthread_should_stop()) {
 		/*
 		 * If we get a signal, it means we are supposed to go
 		 * away and die.  This typically happens if the user is

Re: [PATCH 1/2] fix EH thread teardown

[Rolf Eike Beer]

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

Christoph Hellwig wrote:
>As Rolf Eike Beer noted we might not actually get to the
>kthread_should_stop() because we are waiting in the semaphore forever.
>I didn't get a rmmod hang because of this, but my instrumentation showed
>the thread defitiyly didn't exit.
>
>So make sure to wake the EH thread before the kthread_stop, and to plug
>the reaming race check kthead_should_stop() a second time just before
>calling down_interruptible().
>
>
>Index: scsi-misc-2.6/drivers/scsi/hosts.c
>===================================================================
>--- scsi-misc-2.6.orig/drivers/scsi/hosts.c	2005-09-07 14:21:44.000000000
> +0200 +++ scsi-misc-2.6/drivers/scsi/hosts.c	2005-09-07 14:22:33.000000000
> +0200 @@ -226,8 +226,10 @@
> 	struct Scsi_Host *shost = dev_to_shost(dev);
> 	struct device *parent = dev->parent;
>
>-	if (shost->ehandler)
>+	if (shost->ehandler) {
>+		up(shost->eh_wait);
> 		kthread_stop(shost->ehandler);
>+	}
> 	if (shost->work_q)
> 		destroy_workqueue(shost->work_q);

Wouldn't this hang the same way if rescheduling happens between the up() and 
the kthread_stop()?

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/2] fix EH thread teardown

[Rolf Eike Beer]

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

Christoph Hellwig wrote:
>As Rolf Eike Beer noted we might not actually get to the
>kthread_should_stop() because we are waiting in the semaphore forever.
>I didn't get a rmmod hang because of this, but my instrumentation showed
>the thread defitiyly didn't exit.
>
>So make sure to wake the EH thread before the kthread_stop, and to plug
>the reaming race check kthead_should_stop() a second time just before
>calling down_interruptible().

Ok, we looked a bit closer on all this. http://lwn.net/Articles/65178/ tells 
that ktread_stop() will not send a signal, so the down_interruptible() will 
never return on kthread_stop(). But calling up() directly before 
kthread_stop() adds a race condition: if reschedule happens right after the 
up() we've won nothing and it will hang again.

Using this kind of semaphore for thread wakeup looks more dangerous everytime 
I think on it. down_interruptible() may be interrupted by someone with a 
signal and return without the semaphore locked. Better would be something 
like

i = down_interruptible();

if (kthread_should_stop())
	break;

if (i)
	continue;

Or we have to block all signals, then we can just ignore the _interruptible at 
all. What about something like this instead?

while(!kthread_should_stop()) {
	if (!down_trylock(&sem)) {
		if (kthread_should_stop())
			break;
		yield();
		continue;
	}

	/* something */
}

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/2] fix EH thread teardown

[Alan Stern]

Rolf Eike Beer wrote:
> 
> Ok, we looked a bit closer on all this. http://lwn.net/Articles/65178/ tells 
> that ktread_stop() will not send a signal, so the down_interruptible() will 
> never return on kthread_stop(). But calling up() directly before 
> kthread_stop() adds a race condition: if reschedule happens right after the 
> up() we've won nothing and it will hang again.

This problem shows up in other ways as well.  With hot-unpluggable 
hosts, the call that does the last 'put' on the last device will hang 
waiting for the eh thread to terminate.  See

http://bugzilla.kernel.org/show_bug.cgi?id=5237

Alan Stern

Re: [PATCH 1/2] fix EH thread teardown

[Rolf Eike Beer]

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

Am Dienstag, 13. September 2005 18:50 schrieb Alan Stern:
>Rolf Eike Beer wrote:
>> Ok, we looked a bit closer on all this. http://lwn.net/Articles/65178/
>> tells that ktread_stop() will not send a signal, so the
>> down_interruptible() will never return on kthread_stop(). But calling up()
>> directly before
>> kthread_stop() adds a race condition: if reschedule happens right after
>> the up() we've won nothing and it will hang again.
>
>This problem shows up in other ways as well.  With hot-unpluggable
>hosts, the call that does the last 'put' on the last device will hang
>waiting for the eh thread to terminate.  See
>
>http://bugzilla.kernel.org/show_bug.cgi?id=5237

Looks like someone has to build a waitqueue to fix this.

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/2] fix EH thread teardown

[Alan Stern]

On Tue, 13 Sep 2005, Rolf Eike Beer wrote:

> Looks like someone has to build a waitqueue to fix this.

Tell me what you think of this patch.  I'm not sure whether that 
condition, host_busy == host_failed, is correct for the call to 
wait_event_interruptible.

Alan Stern



Index: usb-2.6/drivers/scsi/scsi_error.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_error.c
+++ usb-2.6/drivers/scsi/scsi_error.c
@@ -50,7 +50,7 @@
 void scsi_eh_wakeup(struct Scsi_Host *shost)
 {
 	if (shost->host_busy == shost->host_failed) {
-		up(shost->eh_wait);
+		wake_up(&shost->eh_wait);
 		SCSI_LOG_ERROR_RECOVERY(5,
 				printk("Waking error handler thread\n"));
 	}
@@ -69,7 +69,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *s
 	struct Scsi_Host *shost = scmd->device->host;
 	unsigned long flags;
 
-	if (shost->eh_wait == NULL)
+	if (shost->eh_alive == 0)	/* Not 100% reliable */
 		return 0;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -1582,37 +1582,24 @@ int scsi_error_handler(void *data)
 {
 	struct Scsi_Host *shost = (struct Scsi_Host *) data;
 	int rtn;
-	DECLARE_MUTEX_LOCKED(sem);
 
 	current->flags |= PF_NOFREEZE;
-	shost->eh_wait = &sem;
-
-	/*
-	 * Wake up the thread that created us.
-	 */
-	SCSI_LOG_ERROR_RECOVERY(3, printk("Wake up parent of"
-					  " scsi_eh_%d\n",shost->host_no));
 
 	while (1) {
-		/*
-		 * If we get a signal, it means we are supposed to go
-		 * away and die.  This typically happens if the user is
-		 * trying to unload a module.
-		 */
 		SCSI_LOG_ERROR_RECOVERY(1, printk("Error handler"
 						  " scsi_eh_%d"
 						  " sleeping\n",shost->host_no));
 
 		/*
-		 * Note - we always use down_interruptible with the semaphore
-		 * even if the module was loaded as part of the kernel.  The
-		 * reason is that down() will cause this thread to be counted
-		 * in the load average as a running process, and down
-		 * interruptible doesn't.  Given that we need to allow this
-		 * thread to die if the driver was loaded as a module, using
-		 * semaphores isn't unreasonable.
-		 */
-		down_interruptible(&sem);
+		 * Note - we always use wait_event_interruptible even if
+		 * the module was loaded as part of the kernel.  The reason
+		 * is that wait_event() will cause this thread to be counted
+		 * in the load average as a running process, and an
+		 * interruptible wait doesn't.
+		 */
+		wait_event_interruptible(shost->eh_wait,
+				shost->host_busy == shost->host_failed ||
+				kthread_should_stop());
 		if (kthread_should_stop())
 			break;
 
@@ -1651,7 +1638,7 @@ int scsi_error_handler(void *data)
 	/*
 	 * Make sure that nobody tries to wake us up again.
 	 */
-	shost->eh_wait = NULL;
+	shost->eh_alive = 0;
 	return 0;
 }
 
Index: usb-2.6/include/scsi/scsi_host.h
===================================================================
--- usb-2.6.orig/include/scsi/scsi_host.h
+++ usb-2.6/include/scsi/scsi_host.h
@@ -465,10 +465,11 @@ struct Scsi_Host {
 
 	struct list_head	eh_cmd_q;
 	struct task_struct    * ehandler;  /* Error recovery thread. */
-	struct semaphore      * eh_wait;   /* The error recovery thread waits
+	wait_queue_head_t	eh_wait;   /* The error recovery thread waits
 					      on this. */
 	struct semaphore      * eh_action; /* Wait for specific actions on the
                                           host. */
+	unsigned int		eh_alive:1;
 	unsigned int            eh_active:1; /* Indicates the eh thread is awake and active if
                                           this is true. */
 	wait_queue_head_t       host_wait;
Index: usb-2.6/drivers/scsi/hosts.c
===================================================================
--- usb-2.6.orig/drivers/scsi/hosts.c
+++ usb-2.6/drivers/scsi/hosts.c
@@ -285,6 +285,7 @@ struct Scsi_Host *scsi_host_alloc(struct
 	INIT_LIST_HEAD(&shost->__targets);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
+	init_waitqueue_head(&shost->eh_wait);
 	init_waitqueue_head(&shost->host_wait);
 
 	init_MUTEX(&shost->scan_mutex);
@@ -368,6 +369,7 @@ struct Scsi_Host *scsi_host_alloc(struct
 		rval = PTR_ERR(shost->ehandler);
 		goto fail_destroy_freelist;
 	}
+	shost->eh_alive = 1;
 
 	scsi_proc_hostdir_add(shost->hostt);
 	return shost;

Re: [PATCH 1/2] fix EH thread teardown

[Christoph Hellwig]

On Tue, Sep 13, 2005 at 03:03:19PM -0400, Alan Stern wrote:
> On Tue, 13 Sep 2005, Rolf Eike Beer wrote:
> 
> > Looks like someone has to build a waitqueue to fix this.
> 
> Tell me what you think of this patch.  I'm not sure whether that 
> condition, host_busy == host_failed, is correct for the call to 
> wait_event_interruptible.

I don't think we even need the waitque, a wake_up_process can replace
it when there's only a single process waiting on it ever.  Similarly
we don't need eh_alive - EH must be alive after scsi_add_host returned,
and no command may be in flight before that.

Re: [PATCH 1/2] fix EH thread teardown

[Alan Stern]

On Tue, 13 Sep 2005, Christoph Hellwig wrote:

> > Tell me what you think of this patch.  I'm not sure whether that 
> > condition, host_busy == host_failed, is correct for the call to 
> > wait_event_interruptible.
> 
> I don't think we even need the waitque, a wake_up_process can replace
> it when there's only a single process waiting on it ever.  Similarly
> we don't need eh_alive - EH must be alive after scsi_add_host returned,
> and no command may be in flight before that.

You're right about the wait_queue; I'll take it out.  The reason for 
adding eh_alive was to prevent scsi_eh_scmd_add from calling 
scsi_eh_wakeup after the error handler thread had exited.  The test that 
used to be there, shost->eh_wait == NULL, no longer applies.  On the other 
hand, since we're no longer checking for signals maybe this test isn't 
needed at all.  The error handler thread won't exit until the last 
reference to the host is gone.

Alan Stern

Re: [PATCH 1/2] fix EH thread teardown

[Alan Stern]

On Tue, 13 Sep 2005, Christoph Hellwig wrote:

> I don't think we even need the waitque, a wake_up_process can replace
> it when there's only a single process waiting on it ever.  Similarly
> we don't need eh_alive - EH must be alive after scsi_add_host returned,
> and no command may be in flight before that.
        
Okay, how do you like this patch instead?  I haven't tested it yet.
  
Alan Stern



Index: usb-2.6/drivers/scsi/scsi_error.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_error.c
+++ usb-2.6/drivers/scsi/scsi_error.c
@@ -50,7 +50,7 @@
 void scsi_eh_wakeup(struct Scsi_Host *shost)
 {
 	if (shost->host_busy == shost->host_failed) {
-		up(shost->eh_wait);
+		wake_up_process(shost->ehandler);
 		SCSI_LOG_ERROR_RECOVERY(5,
 				printk("Waking error handler thread\n"));
 	}
@@ -69,9 +69,6 @@ int scsi_eh_scmd_add(struct scsi_cmnd *s
 	struct Scsi_Host *shost = scmd->device->host;
 	unsigned long flags;
 
-	if (shost->eh_wait == NULL)
-		return 0;
-
 	spin_lock_irqsave(shost->host_lock, flags);
 
 	scmd->eh_eflags |= eh_flag;
@@ -1582,37 +1579,33 @@ int scsi_error_handler(void *data)
 {
 	struct Scsi_Host *shost = (struct Scsi_Host *) data;
 	int rtn;
-	DECLARE_MUTEX_LOCKED(sem);
 
 	current->flags |= PF_NOFREEZE;
-	shost->eh_wait = &sem;
-
-	/*
-	 * Wake up the thread that created us.
-	 */
-	SCSI_LOG_ERROR_RECOVERY(3, printk("Wake up parent of"
-					  " scsi_eh_%d\n",shost->host_no));
 
 	while (1) {
-		/*
-		 * If we get a signal, it means we are supposed to go
-		 * away and die.  This typically happens if the user is
-		 * trying to unload a module.
-		 */
 		SCSI_LOG_ERROR_RECOVERY(1, printk("Error handler"
 						  " scsi_eh_%d"
 						  " sleeping\n",shost->host_no));
 
 		/*
-		 * Note - we always use down_interruptible with the semaphore
-		 * even if the module was loaded as part of the kernel.  The
-		 * reason is that down() will cause this thread to be counted
-		 * in the load average as a running process, and down
-		 * interruptible doesn't.  Given that we need to allow this
-		 * thread to die if the driver was loaded as a module, using
-		 * semaphores isn't unreasonable.
-		 */
-		down_interruptible(&sem);
+		 * Note - we always use TASK_INTERRUPTIBLE even if the
+		 * module was loaded as part of the kernel.  The reason
+		 * is that UNINTERRUPTIBLE would cause this thread to be
+		 * counted in the load average as a running process, and
+		 * an interruptible wait doesn't.
+		 */
+		for (;;) {
+			set_current_state(TASK_INTERRUPTIBLE);
+			if (shost->host_busy == shost->host_failed ||
+					kthread_should_stop()) {
+				set_current_state(TASK_RUNNING);
+				break;
+			}
+
+			/* Even though we are INTERRUPTIBLE, ignore signals */
+			flush_signals(current);
+			schedule();
+		}
 		if (kthread_should_stop())
 			break;
 
@@ -1648,10 +1641,6 @@ int scsi_error_handler(void *data)
 	SCSI_LOG_ERROR_RECOVERY(1, printk("Error handler scsi_eh_%d"
 					  " exiting\n",shost->host_no));
 
-	/*
-	 * Make sure that nobody tries to wake us up again.
-	 */
-	shost->eh_wait = NULL;
 	return 0;
 }
 
Index: usb-2.6/include/scsi/scsi_host.h
===================================================================
--- usb-2.6.orig/include/scsi/scsi_host.h
+++ usb-2.6/include/scsi/scsi_host.h
@@ -465,8 +465,6 @@ struct Scsi_Host {
 
 	struct list_head	eh_cmd_q;
 	struct task_struct    * ehandler;  /* Error recovery thread. */
-	struct semaphore      * eh_wait;   /* The error recovery thread waits
-					      on this. */
 	struct semaphore      * eh_action; /* Wait for specific actions on the
                                           host. */
 	unsigned int            eh_active:1; /* Indicates the eh thread is awake and active if