Skip to contentSkip to conversation

zfcp: convert the zfcp_erp thread to the kthread api

PATCH In review Swen Schillig 3 participants 3 messages Oct 27, 2006 – Oct 30, 2006

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 zfcp: convert the zfcp_erp thread to the kthread api

commit fc26d0531e48a7361ac7e46ead4eec2a46774413

Author: Cornelia Huck <[email protected]>
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.

drivers/s390/scsi/zfcp_def.h +1 -2
index 74c0eac..d47a0d5 100644
@@ -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;
drivers/s390/scsi/zfcp_erp.c +15 -33

99 lines not on this page · raw message

drivers/s390/scsi/zfcp_ext.h +1 -1
index b8794d7..aec24af 100644
@@ -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);
 
#1 lore raw
Quoted 9 lines · Swen Schillig

commit fc26d0531e48a7361ac7e46ead4eec2a46774413

Author: Cornelia Huck <[email protected]>
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.

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

Quoted 147 lines
drivers/s390/scsi/zfcp_def.h +1 -2
index 74c0eac..d47a0d5 100644
@@ -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;
drivers/s390/scsi/zfcp_erp.c +15 -33
index c88babc..597104a 100644
@@ -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));
drivers/s390/scsi/zfcp_ext.h +1 -2
index b8794d7..aec24af 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 8 lines · Heiko Carstens

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.

Signature · 3 lines
Cornelia Huck
Linux for zSeries Developer
Tel.: +49-7031-16-4837, Mail: [email protected]