[PATCH] zfcp: convert the zfcp_erp thread to the kthread api

[PATCH] zfcp: convert the zfcp_erp thread to the kthread api

[Swen Schillig]

commit fc26d0531e48a7361ac7e46ead4eec2a46774413
Author: Cornelia Huck <cornelia.huck@de.ibm.com>
Date:   Fri Oct 27 11:31:05 2006 +0200

    Convert the zfcp_erp thread to the kthread api. Also make zfcp_erp_thread_killdrivers
    return void since it cannot fail.
    
    Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> 
    Signed-off-by: Swen Schillig <swen@vnet.ibm.com>

diff --git a/drivers/s390/scsi/zfcp_def.h b/drivers/s390/scsi/zfcp_def.h
index 74c0eac..d47a0d5 100644
--- a/drivers/s390/scsi/zfcp_def.h
+++ b/drivers/s390/scsi/zfcp_def.h
@@ -605,8 +605,6 @@ #define ZFCP_STATUS_ADAPTER_QDIOUP		0x00
 #define ZFCP_STATUS_ADAPTER_REGISTERED		0x00000004
 #define ZFCP_STATUS_ADAPTER_XCONFIG_OK		0x00000008
 #define ZFCP_STATUS_ADAPTER_HOST_CON_INIT	0x00000010
-#define ZFCP_STATUS_ADAPTER_ERP_THREAD_UP	0x00000020
-#define ZFCP_STATUS_ADAPTER_ERP_THREAD_KILL	0x00000080
 #define ZFCP_STATUS_ADAPTER_ERP_PENDING		0x00000100
 #define ZFCP_STATUS_ADAPTER_LINK_UNPLUGGED	0x00000200
 #define ZFCP_STATUS_ADAPTER_XPORT_OK		0x00000800
@@ -900,6 +898,7 @@ struct zfcp_adapter {
 						      completion races */
 	u16			status_read_failed; /* # failed status reads */
 	atomic_t		status;	           /* status of this adapter */
+	struct task_struct      *erp_thread;       /* error recovery thread */
 	struct list_head	erp_ready_head;	   /* error recovery for this
 						      adapter/devices */
 	struct list_head	erp_running_head;
diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
index c88babc..597104a 100644
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -22,6 +22,7 @@
 #define ZFCP_LOG_AREA			ZFCP_LOG_AREA_ERP
 
 #include "zfcp_ext.h"
+#include <linux/kthread.h>
 
 static int zfcp_erp_adisc(struct zfcp_port *);
 static void zfcp_erp_adisc_handler(unsigned long);
@@ -991,24 +992,18 @@ static void zfcp_erp_action_dismiss(stru
 int
 zfcp_erp_thread_setup(struct zfcp_adapter *adapter)
 {
-	int retval = 0;
-
-	atomic_clear_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
-
-	retval = kernel_thread(zfcp_erp_thread, adapter, SIGCHLD);
-	if (retval < 0) {
+	adapter->erp_thread = kthread_run(zfcp_erp_thread, adapter, "zfcperp%s",
+					  zfcp_get_busid_by_adapter(adapter));
+	if (IS_ERR(adapter->erp_thread)) {
 		ZFCP_LOG_NORMAL("error: creation of erp thread failed for "
 				"adapter %s\n",
 				zfcp_get_busid_by_adapter(adapter));
 		debug_text_event(adapter->erp_dbf, 5, "a_thset_fail");
-	} else {
-		wait_event(adapter->erp_thread_wqh,
-			   atomic_test_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP,
-					    &adapter->status));
+		adapter->erp_thread = NULL;
+	} else
 		debug_text_event(adapter->erp_dbf, 5, "a_thset_ok");
-	}
 
-	return (retval < 0);
+	return (!adapter->erp_thread);
 }
 
 /*
@@ -1025,24 +1020,16 @@ zfcp_erp_thread_setup(struct zfcp_adapte
  *		has been completed. Thus, there are no pending erp_actions
  *		which would need to be handled here.
  */
-int
-zfcp_erp_thread_kill(struct zfcp_adapter *adapter)
+void zfcp_erp_thread_kill(struct zfcp_adapter *adapter)
 {
-	int retval = 0;
+	struct task_struct *erp_thread;
 
-	atomic_set_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_KILL, &adapter->status);
+	erp_thread = adapter->erp_thread;
+	adapter->erp_thread = NULL;
+	if (erp_thread)
+		kthread_stop(erp_thread);
 	up(&adapter->erp_ready_sem);
-
-	wait_event(adapter->erp_thread_wqh,
-		   !atomic_test_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP,
-				     &adapter->status));
-
-	atomic_clear_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_KILL,
-			  &adapter->status);
-
 	debug_text_event(adapter->erp_dbf, 5, "a_thki_ok");
-
-	return retval;
 }
 
 /*
@@ -1060,15 +1047,12 @@ zfcp_erp_thread(void *data)
 	struct zfcp_erp_action *erp_action;
 	unsigned long flags;
 
-	daemonize("zfcperp%s", zfcp_get_busid_by_adapter(adapter));
 	/* Block all signals */
 	siginitsetinv(&current->blocked, 0);
-	atomic_set_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
 	debug_text_event(adapter->erp_dbf, 5, "a_th_run");
 	wake_up(&adapter->erp_thread_wqh);
 
-	while (!atomic_test_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_KILL,
-				 &adapter->status)) {
+	while (!kthread_should_stop()) {
 
 		write_lock_irqsave(&adapter->erp_lock, flags);
 		next = adapter->erp_ready_head.prev;
@@ -1093,7 +1077,6 @@ zfcp_erp_thread(void *data)
 		debug_text_event(adapter->erp_dbf, 5, "a_th_woken");
 	}
 
-	atomic_clear_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
 	debug_text_event(adapter->erp_dbf, 5, "a_th_stop");
 	wake_up(&adapter->erp_thread_wqh);
 
@@ -2886,8 +2869,7 @@ zfcp_erp_action_enqueue(int action,
 	 * efficient.
 	 */
 
-	if (!atomic_test_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP,
-			      &adapter->status))
+	if (!adapter->erp_thread)
 		return -EIO;
 
 	debug_event(adapter->erp_dbf, 4, &action, sizeof (int));
diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
index b8794d7..aec24af 100644
--- a/drivers/s390/scsi/zfcp_ext.h
+++ b/drivers/s390/scsi/zfcp_ext.h
@@ -148,7 +148,7 @@ extern int  zfcp_erp_unit_shutdown(struc
 extern void zfcp_erp_unit_failed(struct zfcp_unit *);
 
 extern int  zfcp_erp_thread_setup(struct zfcp_adapter *);
-extern int  zfcp_erp_thread_kill(struct zfcp_adapter *);
+extern void zfcp_erp_thread_kill(struct zfcp_adapter *);
 extern int  zfcp_erp_wait(struct zfcp_adapter *);
 extern void zfcp_erp_async_handler(struct zfcp_erp_action *, unsigned long);
 

Re: [PATCH] zfcp: convert the zfcp_erp thread to the kthread api

[Heiko Carstens]

On Fri, Oct 27, 2006 at 11:38:49AM +0200, Swen Schillig wrote:
> commit fc26d0531e48a7361ac7e46ead4eec2a46774413
> Author: Cornelia Huck <cornelia.huck@de.ibm.com>
> Date:   Fri Oct 27 11:31:05 2006 +0200
> 
>     Convert the zfcp_erp thread to the kthread api. Also make zfcp_erp_thread_killdrivers
>     return void since it cannot fail.
>     
>     Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> 
>     Signed-off-by: Swen Schillig <swen@vnet.ibm.com>

Would you mind reading Documentation/SubmittingPatches and
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt ?

Besides that this patch is broken and might lead to a deadlock. I think
somebody else tried already a similar approach and Christoph pointed out
that it could deadlock...

What could happen:
- code is in erp thread loop before down_interruptible(erp_ready_sem)
- thread should be killed: somebody calls kthread_stop() hence
  kthread_should_stop() gets true. But since the thread is not sleeping
  (yet) there is no need to wake it. So kthread_stop() just waits.
- erp thread comes to down_interruptible(erp_ready_sem) and sleeps.
- the only "up" that would wake up the erp_thread comes after your
  kthread_stop() which is waits that the thread terminates -> deadlock.

For reference I quote the whole patch (Cornelia now on cc).

> diff --git a/drivers/s390/scsi/zfcp_def.h b/drivers/s390/scsi/zfcp_def.h
> index 74c0eac..d47a0d5 100644
> --- a/drivers/s390/scsi/zfcp_def.h
> +++ b/drivers/s390/scsi/zfcp_def.h
> @@ -605,8 +605,6 @@ #define ZFCP_STATUS_ADAPTER_QDIOUP		0x00
>  #define ZFCP_STATUS_ADAPTER_REGISTERED		0x00000004
>  #define ZFCP_STATUS_ADAPTER_XCONFIG_OK		0x00000008
>  #define ZFCP_STATUS_ADAPTER_HOST_CON_INIT	0x00000010
> -#define ZFCP_STATUS_ADAPTER_ERP_THREAD_UP	0x00000020
> -#define ZFCP_STATUS_ADAPTER_ERP_THREAD_KILL	0x00000080
>  #define ZFCP_STATUS_ADAPTER_ERP_PENDING		0x00000100
>  #define ZFCP_STATUS_ADAPTER_LINK_UNPLUGGED	0x00000200
>  #define ZFCP_STATUS_ADAPTER_XPORT_OK		0x00000800
> @@ -900,6 +898,7 @@ struct zfcp_adapter {
>  						      completion races */
>  	u16			status_read_failed; /* # failed status reads */
>  	atomic_t		status;	           /* status of this adapter */
> +	struct task_struct      *erp_thread;       /* error recovery thread */
>  	struct list_head	erp_ready_head;	   /* error recovery for this
>  						      adapter/devices */
>  	struct list_head	erp_running_head;
> diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
> index c88babc..597104a 100644
> --- a/drivers/s390/scsi/zfcp_erp.c
> +++ b/drivers/s390/scsi/zfcp_erp.c
> @@ -22,6 +22,7 @@
>  #define ZFCP_LOG_AREA			ZFCP_LOG_AREA_ERP
>  
>  #include "zfcp_ext.h"
> +#include <linux/kthread.h>
>  
>  static int zfcp_erp_adisc(struct zfcp_port *);
>  static void zfcp_erp_adisc_handler(unsigned long);
> @@ -991,24 +992,18 @@ static void zfcp_erp_action_dismiss(stru
>  int
>  zfcp_erp_thread_setup(struct zfcp_adapter *adapter)
>  {
> -	int retval = 0;
> -
> -	atomic_clear_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
> -
> -	retval = kernel_thread(zfcp_erp_thread, adapter, SIGCHLD);
> -	if (retval < 0) {
> +	adapter->erp_thread = kthread_run(zfcp_erp_thread, adapter, "zfcperp%s",
> +					  zfcp_get_busid_by_adapter(adapter));
> +	if (IS_ERR(adapter->erp_thread)) {
>  		ZFCP_LOG_NORMAL("error: creation of erp thread failed for "
>  				"adapter %s\n",
>  				zfcp_get_busid_by_adapter(adapter));
>  		debug_text_event(adapter->erp_dbf, 5, "a_thset_fail");
> -	} else {
> -		wait_event(adapter->erp_thread_wqh,
> -			   atomic_test_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP,
> -					    &adapter->status));
> +		adapter->erp_thread = NULL;
> +	} else
>  		debug_text_event(adapter->erp_dbf, 5, "a_thset_ok");
> -	}
>  
> -	return (retval < 0);
> +	return (!adapter->erp_thread);
>  }
>  
>  /*
> @@ -1025,24 +1020,16 @@ zfcp_erp_thread_setup(struct zfcp_adapte
>   *		has been completed. Thus, there are no pending erp_actions
>   *		which would need to be handled here.
>   */
> -int
> -zfcp_erp_thread_kill(struct zfcp_adapter *adapter)
> +void zfcp_erp_thread_kill(struct zfcp_adapter *adapter)
>  {
> -	int retval = 0;
> +	struct task_struct *erp_thread;
>  
> -	atomic_set_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_KILL, &adapter->status);
> +	erp_thread = adapter->erp_thread;
> +	adapter->erp_thread = NULL;
> +	if (erp_thread)
> +		kthread_stop(erp_thread);
>  	up(&adapter->erp_ready_sem);
> -
> -	wait_event(adapter->erp_thread_wqh,
> -		   !atomic_test_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP,
> -				     &adapter->status));
> -
> -	atomic_clear_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_KILL,
> -			  &adapter->status);
> -
>  	debug_text_event(adapter->erp_dbf, 5, "a_thki_ok");
> -
> -	return retval;
>  }
>  
>  /*
> @@ -1060,15 +1047,12 @@ zfcp_erp_thread(void *data)
>  	struct zfcp_erp_action *erp_action;
>  	unsigned long flags;
>  
> -	daemonize("zfcperp%s", zfcp_get_busid_by_adapter(adapter));
>  	/* Block all signals */
>  	siginitsetinv(&current->blocked, 0);
> -	atomic_set_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
>  	debug_text_event(adapter->erp_dbf, 5, "a_th_run");
>  	wake_up(&adapter->erp_thread_wqh);
>  
> -	while (!atomic_test_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_KILL,
> -				 &adapter->status)) {
> +	while (!kthread_should_stop()) {
>  
>  		write_lock_irqsave(&adapter->erp_lock, flags);
>  		next = adapter->erp_ready_head.prev;
> @@ -1093,7 +1077,6 @@ zfcp_erp_thread(void *data)
>  		debug_text_event(adapter->erp_dbf, 5, "a_th_woken");
>  	}
>  
> -	atomic_clear_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP, &adapter->status);
>  	debug_text_event(adapter->erp_dbf, 5, "a_th_stop");
>  	wake_up(&adapter->erp_thread_wqh);
>  
> @@ -2886,8 +2869,7 @@ zfcp_erp_action_enqueue(int action,
>  	 * efficient.
>  	 */
>  
> -	if (!atomic_test_mask(ZFCP_STATUS_ADAPTER_ERP_THREAD_UP,
> -			      &adapter->status))
> +	if (!adapter->erp_thread)
>  		return -EIO;
>  
>  	debug_event(adapter->erp_dbf, 4, &action, sizeof (int));
> diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
> index b8794d7..aec24af 100644
> --- a/drivers/s390/scsi/zfcp_ext.h
> +++ b/drivers/s390/scsi/zfcp_ext.h
> @@ -148,7 +148,7 @@ extern int  zfcp_erp_unit_shutdown(struc
>  extern void zfcp_erp_unit_failed(struct zfcp_unit *);
>  
>  extern int  zfcp_erp_thread_setup(struct zfcp_adapter *);
> -extern int  zfcp_erp_thread_kill(struct zfcp_adapter *);
> +extern void zfcp_erp_thread_kill(struct zfcp_adapter *);
>  extern int  zfcp_erp_wait(struct zfcp_adapter *);
>  extern void zfcp_erp_async_handler(struct zfcp_erp_action *, unsigned long);
>  
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] zfcp: convert the zfcp_erp thread to the kthread api

[Cornelia Huck]

On Sun, 29 Oct 2006 23:26:56 +0100,
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> What could happen:
> - code is in erp thread loop before down_interruptible(erp_ready_sem)
> - thread should be killed: somebody calls kthread_stop() hence
>   kthread_should_stop() gets true. But since the thread is not sleeping
>   (yet) there is no need to wake it. So kthread_stop() just waits.
> - erp thread comes to down_interruptible(erp_ready_sem) and sleeps.
> - the only "up" that would wake up the erp_thread comes after your
>   kthread_stop() which is waits that the thread terminates -> deadlock.

Grmpf, you're right. And even checking kthread_should_stop()
immediately before going to sleep would be racy. We need to up() after
we set should_stop and wait afterwards... Please drop this patch for
now until I have time to sort it out.

-- 
Cornelia Huck
Linux for zSeries Developer
Tel.: +49-7031-16-4837, Mail: cornelia.huck@de.ibm.com