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

[Heiko Carstens <heiko.carstens@de.ibm.com>]

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