cciss rmmod/scan-thread fixes

cciss: use only one scan thread

Replace the use of one scan kthread per controller with one per driver. Use a queue to hold a list of controllers that need to be rescanned with routines to add and remove controllers from the queue.
Fix locking and completion handling to prevent a hang during rmmod.

...

drivers/block/cciss.c +1 -0

@@ -39,6 +39,7 @@
 #include <linux/hdreg.h>
 #include <linux/spinlock.h>
 #include <linux/compat.h>
+#include <linux/mutex.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 

drivers/block/cciss.c +11 -1

 static int scan_thread(void *data);
 static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c);
 

@@ -3232,20 +3239,121 @@ static irqreturn_t do_cciss_intr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int scan_thread(void *data)
+/**
+ * add_to_scan_list() - add controller to rescan queue
+ * @h:		      Pointer to the controller.
+ *
+ * Adds the controller to the rescan queue if not already on the queue.
+ *
+ * returns 1 if added to the queue, 0 if skipped (could be on the
+ * queue already, or the controller could be initializing or shutting
+ * down).
+ **/
+static int add_to_scan_list(struct ctlr_info *h)
 {

drivers/block/cciss.c +8 -4

+	if (!mutex_trylock(&h->busy_shutting_down))
+		return 0;
 
-	for (;;) {
-		rc = wait_for_completion_interruptible(&wait);
-		if (kthread_should_stop())
+	mutex_lock(&scan_mutex);
+	list_for_each_entry(test_h, &scan_q, scan_list) {
+		if (test_h == h) {
+			found = 1;
 			break;
-		if (!rc)
+		}
+	}

drivers/block/cciss.c +37 -0

+/**
+ * scan_thread() - kernel thread used to rescan controllers
+ * @data:	 Ignored.
+ *
+ * A kernel thread used scan for drive topology changes on
+ * controllers. The thread processes only one controller at a time
+ * using a queue.  Controllers are added to the queue using
+ * add_to_scan_list() and removed from the queue either after done
+ * processing or using remove_from_scan_list().
+ *
+ * returns 0.
+ **/
+static int scan_thread(void *data)
+{
+	struct ctlr_info *h;
+
+	set_current_state(TASK_INTERRUPTIBLE);
+	while (!kthread_should_stop()) {
+		mutex_lock(&scan_mutex);
+		if (list_empty(&scan_q)) {
+			h = NULL;
+		} else {
+			h = list_entry(scan_q.next,
+				       struct ctlr_info,
+				       scan_list);
+			list_del(&h->scan_list);
+			h->busy_scanning = 1;
+		}
+		mutex_unlock(&scan_mutex);
+
+		if (h) {
+			__set_current_state(TASK_RUNNING);
 			rebuild_lun_table(h, 0);
+			complete_all(&h->scan_wait);
+			mutex_lock(&scan_mutex);
+			h->busy_scanning = 0;
+			mutex_unlock(&scan_mutex);
+			set_current_state(TASK_INTERRUPTIBLE);

Quoted 24 lines · On Tue, 14 Jul 2009 16:02:50 -0600 Andrew Patterson <[email protected]>…

cciss: use only one scan thread

Replace the use of one scan kthread per controller with one per driver. Use a queue to hold a list of controllers that need to be rescanned with routines to add and remove controllers from the queue.
Fix locking and completion handling to prevent a hang during rmmod.

...

drivers/block/cciss.c +1 -0

@@ -39,6 +39,7 @@
 #include <linux/hdreg.h>
 #include <linux/spinlock.h>
 #include <linux/compat.h>
+#include <linux/mutex.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 

@@ -155,6 +156,10 @@ static struct board_type products[] = {
 
 static ctlr_info_t *hba[MAX_CTLR];
 

drivers/block/cciss.c static struct board_type products[] = { · line 161

Quoted 3 lines

drivers/block/cciss.c +3 -0

+static struct task_struct *cciss_scan_thread;
+static struct mutex scan_mutex;
+static struct list_head scan_q;

Both of the above can be initialised at compile-time. This is a bit neater and prevents reviewers from having to run around checking that they got initialised early enough.

Quoted 7 lines

drivers/block/cciss.c +0 -0

 static void do_cciss_request(struct request_queue *q);
 static irqreturn_t do_cciss_intr(int irq, void *dev_id);
 static int cciss_open(struct block_device *bdev, fmode_t mode);

@@ -189,6 +194,8 @@ static int sendcmd_withirq_core(ctlr_info_t *h, CommandList_struct *c,
 static int process_sendcmd_error(ctlr_info_t *h, CommandList_struct *c);
 

drivers/block/cciss.c static int sendcmd_withirq_core(ctlr_info_t *h, CommandList_struct *c, · line 198

Quoted 3 lines

drivers/block/cciss.c +2 -0

 static void fail_all_cmds(unsigned long ctlr);
+static int add_to_scan_list(struct ctlr_info *h);
+static void remove_from_scan_list(struct ctlr_info *h);

These forward declarations are unneeded.

Quoted 24 lines

drivers/block/cciss.c +11 -5

 static int scan_thread(void *data);
 static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c);
 

@@ -3232,20 +3239,121 @@ static irqreturn_t do_cciss_intr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int scan_thread(void *data)
+/**
+ * add_to_scan_list() - add controller to rescan queue
+ * @h:		      Pointer to the controller.
+ *
+ * Adds the controller to the rescan queue if not already on the queue.
+ *
+ * returns 1 if added to the queue, 0 if skipped (could be on the
+ * queue already, or the controller could be initializing or shutting
+ * down).
+ **/
+static int add_to_scan_list(struct ctlr_info *h)
 {
-	ctlr_info_t *h = data;
-	int rc;
-	DECLARE_COMPLETION_ONSTACK(wait);
-	h->rescan_wait = &wait;

drivers/block/cciss.c static irqreturn_t do_cciss_intr(int irq, void *dev_id) · line 3259

Quoted 6 lines

drivers/block/cciss.c +6 -0

+	struct ctlr_info *test_h;
+	int found = 0;
+	int ret = 0;
+
+	if (h->busy_initializing)
+		return 0;

This looks pretty random and racy.

For example, what stops busy_initializing from becoming true one picosecond after the test?

Quoted 19 lines

drivers/block/cciss.c +13 -4

+	if (!mutex_trylock(&h->busy_shutting_down))
+		return 0;
 
-	for (;;) {
-		rc = wait_for_completion_interruptible(&wait);
-		if (kthread_should_stop())
+	mutex_lock(&scan_mutex);
+	list_for_each_entry(test_h, &scan_q, scan_list) {
+		if (test_h == h) {
+			found = 1;
 			break;
-		if (!rc)
+		}
+	}
+	if (!found && !h->busy_scanning) {
+		INIT_COMPLETION(h->scan_wait);
+		list_add_tail(&h->scan_list, &scan_q);
+		ret = 1;
+	}

drivers/block/cciss.c static irqreturn_t do_cciss_intr(int irq, void *dev_id) · line 3280

Quoted 5 lines

drivers/block/cciss.c +5 -0

+	mutex_unlock(&scan_mutex);
+	mutex_unlock(&h->busy_shutting_down);
+
+	return ret;
+}

We already did init_completion(h->scan_wait) at startup time? Doing the INIT_COMPLETION() here looks unusual and is hopefully unnecessary.

Quoted 38 lines

drivers/block/cciss.c +37 -0

+/**
+ * scan_thread() - kernel thread used to rescan controllers
+ * @data:	 Ignored.
+ *
+ * A kernel thread used scan for drive topology changes on
+ * controllers. The thread processes only one controller at a time
+ * using a queue.  Controllers are added to the queue using
+ * add_to_scan_list() and removed from the queue either after done
+ * processing or using remove_from_scan_list().
+ *
+ * returns 0.
+ **/
+static int scan_thread(void *data)
+{
+	struct ctlr_info *h;
+
+	set_current_state(TASK_INTERRUPTIBLE);
+	while (!kthread_should_stop()) {
+		mutex_lock(&scan_mutex);
+		if (list_empty(&scan_q)) {
+			h = NULL;
+		} else {
+			h = list_entry(scan_q.next,
+				       struct ctlr_info,
+				       scan_list);
+			list_del(&h->scan_list);
+			h->busy_scanning = 1;
+		}
+		mutex_unlock(&scan_mutex);
+
+		if (h) {
+			__set_current_state(TASK_RUNNING);
 			rebuild_lun_table(h, 0);
+			complete_all(&h->scan_wait);
+			mutex_lock(&scan_mutex);
+			h->busy_scanning = 0;
+			mutex_unlock(&scan_mutex);
+			set_current_state(TASK_INTERRUPTIBLE);

drivers/block/cciss.c static irqreturn_t do_cciss_intr(int irq, void *dev_id) · line 3358

Quoted 10 lines

drivers/block/cciss.c +7 -0

+		} else {
+			schedule();
+			set_current_state(TASK_INTERRUPTIBLE);
+			continue;
+		}
 	}
+
+	__set_current_state(TASK_RUNNING);
 	return 0;
 }

This code is somewhat wrong. Look:

	set_current_state(TASK_INTERRUPTIBLE);
	...
		mutex_lock(&scan_mutex);

now, we don't know what state the task is in. If the mutex_lock() slept the we're in state TASK_RUNNING. If the mutex_lock() didn't sleep then we're in state <mumble>. Where <mumble>==TASK_INTERRUPTIBLE, but that's an implementation fluke.

So I'd say that some rethinking/restructuring is needed here.

Quoted 5 lines

Quoted 1 line

+}

We already did init_completion(h->scan_wait) at startup time? Doing the INIT_COMPLETION() here looks unusual and is hopefully unnecessary.

I am following the text in the "Linux Device Drivers 3rd Edition section 5.4:

"A completion is normally a one-shot device; it is used once then discarded. It is possible, however, to reuse completion structures if proper care is taken. If complete_all is not used, a completion structure can be reused without any problems as long as there is no ambiguity about what event is being signalled. If you use complete_all, however, you must reinitialize the completion structure before reusing it. The macro:

INIT_COMPLETION(struct completion c);

can be used to quickly perform this reinitialization."

It there are better way to do this? INIT_COMPLETION() currently just sets done = 0.

The following series fixes several problems causing hangs while rmmoding the cciss driver. In the process of fixing these hangs, I also reworked the the logical drive scanning kernel thread to use one thread per driver rather than one thread per controller. I also added a sysfs attribute to kick off a controller scan.

 .../ABI/testing/sysfs-bus-pci-devices-cciss        |    7 +
 drivers/block/cciss.c                              |  198 ++++++++++++++++++--
 drivers/block/cciss.h                              |    7 +
 3 files changed, 188 insertions(+), 24 deletions(-)

Andrew Patterson (3):
cciss: remove logical drive sysfs entries during driver cleanup.
cciss: use only one scan thread
cciss: kick off logical drive topology rescan through sysfs

Quoted 11 lines · On Tue, 14 Jul 2009 16:02:40 -0600 Andrew Patterson <[email protected]>…

The following series fixes several problems causing hangs while rmmoding the cciss driver. In the process of fixing these hangs, I also reworked the the logical drive scanning kernel thread to use one thread per driver rather than one thread per controller. I also added a sysfs attribute to kick off a controller scan.

 .../ABI/testing/sysfs-bus-pci-devices-cciss        |    7 +
 drivers/block/cciss.c                              |  198 ++++++++++++++++++--
 drivers/block/cciss.h                              |    7 +
 3 files changed, 188 insertions(+), 24 deletions(-)

Quoted 5 lines

Andrew Patterson (3):
cciss: remove logical drive sysfs entries during driver cleanup.
cciss: use only one scan thread
cciss: kick off logical drive topology rescan through sysfs

So I assume that these are intended to fix the problem which cciss-fix-the-termination-of-the-scanning-thread.patch didn't fix?

Once we get this all sorted out, which kernel version(s) are we targetting and why?

Thanks.