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.
Changelog
v2
Staticially initialize scan_mutex and scan_q.
Remove unneeded function declarations.
Added a patch that uses a mutex to indicate we are busy initializing
rather than a integer flag.
Remove a possible scan race by moving the busy_initializing unlock after
the call to rebuild_luntable in cciss_init_one.
Reworked the scan thread to use a more traditional loop with a
set_current_state(TASK_INTERRUPTIBLE); schedule() at the top of the
loop.
.../ABI/testing/sysfs-bus-pci-devices-cciss | 7 +
drivers/block/cciss.c | 204 +++++++++++++++++---
drivers/block/cciss.h | 9 +
3 files changed, 191 insertions(+), 29 deletions(-)
Andrew Patterson (4):
cciss: remove logical drive sysfs entries during driver cleanup.
cciss: use mutex instead of flag to indicate busy initializing
cciss: use only one scan thread
cciss: kick off logical drive topology rescan through sysfs
--
Andrew Patterson
cciss: kick off logical drive topology rescan through sysfs
Added /sys/bus/pci/devices/<dev>/ccissX/rescan sysfs entry used
to kick off a rescan that discovers logical drive topology changes.
Acked-by: Mike Miller <[email protected]>
Signed-off-by: Andrew Patterson <[email protected]>
---
diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-cciss b/Documentation/ABI/testing/sysfs-bus-pci-devices-cciss
index 0a92a7c..4050606 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci-devices-cciss
+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-cciss
@@ -31,3 +31,10 @@ Date: March 2009
Kernel Version: 2.6.30
Contact: [email protected]
Description: A symbolic link to /sys/block/cciss!cXdY
+
+Where: /sys/bus/pci/devices/<dev>/ccissX/rescan
+Date: July 2009
+Kernel Version: 2.6.31
+Contact: [email protected]
+Description: Kicks of a rescan of the controller to discovery logical
+ drive topology changes.
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 0ce90c0..12f0638 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -194,6 +194,7 @@ static int sendcmd_withirq_core(ctlr_info_t *h, CommandList_struct *c,
static int process_sendcmd_error(ctlr_info_t *h, CommandList_struct *c);
static void fail_all_cmds(unsigned long ctlr);
+static int add_to_scan_list(struct ctlr_info *h);
static int scan_thread(void *data);
static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c);
@@ -459,9 +460,19 @@ static void __devinit cciss_procinit(int i)
#define to_hba(n) container_of(n, struct ctlr_info, dev)
#define to_drv(n) container_of(n, drive_info_struct, dev)
-static struct device_type cciss_host_type = {
- .name = "cciss_host",
-};
+static ssize_t host_store_rescan(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ctlr_info *h = to_hba(dev);
+
+ add_to_scan_list(h);
+ wake_up_process(cciss_scan_thread);
+ wait_for_completion_interruptible(&h->scan_wait);
+
+ return count;
+}
+DEVICE_ATTR(rescan, S_IWUSR, NULL, host_store_rescan);
static ssize_t dev_show_unique_id(struct device *dev,
struct device_attribute *attr,
@@ -565,6 +576,25 @@ static ssize_t dev_show_rev(struct device *dev,
}
DEVICE_ATTR(rev, S_IRUGO, dev_show_rev, NULL);
+static struct attribute *cciss_host_attrs[] = {
+ &dev_attr_rescan.attr,
+ NULL
+};
+
+static struct attribute_group cciss_host_attr_group = {
+ .attrs = cciss_host_attrs,
+};
+
+static struct attribute_group *cciss_host_attr_groups[] = {
+ &cciss_host_attr_group,
+ NULL
+};
+
+static struct device_type cciss_host_type = {
+ .name = "cciss_host",
+ .groups = cciss_host_attr_groups,
+};
+
static struct attribute *cciss_dev_attrs[] = {
&dev_attr_unique_id.attr,
&dev_attr_model.attr,
cciss: use mutex instead of flag to indicate busy initializing
Convert busy_initializing from simple integer flag to mutex, so we
can use it to block and thus avoid some race conditions.
Acked-by: Mike Miller <[email protected]>
Signed-off-by: Andrew Patterson <[email protected]>
---
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 970c896..085ab11 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -751,7 +751,7 @@ static int cciss_open(struct block_device *bdev, fmode_t mode)
printk(KERN_DEBUG "cciss_open %s\n", bdev->bd_disk->disk_name);
#endif /* CCISS_DEBUG */
- if (host->busy_initializing || drv->busy_configuring)
+ if (mutex_is_locked(&host->busy_initializing) || drv->busy_configuring)
return -EBUSY;
/*
* Root is allowed to open raw volume zero even if it's not configured
@@ -3915,7 +3915,8 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
if (i < 0)
return -1;
- hba[i]->busy_initializing = 1;
+ mutex_init(&hba[i]->busy_initializing);
+ mutex_lock(&hba[i]->busy_initializing);
INIT_HLIST_HEAD(&hba[i]->cmpQ);
INIT_HLIST_HEAD(&hba[i]->reqQ);
@@ -4034,7 +4035,7 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
hba[i]->cciss_max_sectors = 2048;
- hba[i]->busy_initializing = 0;
+ mutex_unlock(&hba[i]->busy_initializing);
rebuild_lun_table(hba[i], 1);
hba[i]->cciss_scan_thread = kthread_run(scan_thread, hba[i],
@@ -4062,7 +4063,7 @@ clean2:
clean1:
cciss_destroy_hba_sysfs_entry(hba[i]);
clean0:
- hba[i]->busy_initializing = 0;
+ mutex_unlock(&hba[i]->busy_initializing);
/* cleanup any queues that may have been initialized */
for (j=0; j <= hba[i]->highest_lun; j++){
drive_info_struct *drv = &(hba[i]->drv[j]);
diff --git a/drivers/block/cciss.h b/drivers/block/cciss.h
index 06a5db2..bf1699c 100644
--- a/drivers/block/cciss.h
+++ b/drivers/block/cciss.h
@@ -107,7 +107,7 @@ struct ctlr_info
int nr_allocs;
int nr_frees;
int busy_configuring;
- int busy_initializing;
+ struct mutex busy_initializing;
/* This element holds the zero based queue number of the last
* queue to be started. It is used for fairness.
cciss: remove logical drive sysfs entries during driver cleanup.
Sysfs entries for logical drives need to be removed when a drive is
deleted during driver cleanup.
Acked-by: Mike Miller <[email protected]>
Signed-off-by: Andrew Patterson <[email protected]>
---
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index a52cc7f..970c896 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1977,7 +1977,6 @@ static int rebuild_lun_table(ctlr_info_t *h, int first_time)
h->drv[i].busy_configuring = 1;
spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
return_code = deregister_disk(h, i, 1);
- cciss_destroy_ld_sysfs_entry(&h->drv[i]);
h->drv[i].busy_configuring = 0;
}
}
@@ -2118,6 +2117,7 @@ static int deregister_disk(ctlr_info_t *h, int drv_index,
* indicate that this element of the drive
* array is free.
*/
+ cciss_destroy_ld_sysfs_entry(drv);
if (clear_all) {
/* check to see if it was the last disk */
@@ -4141,6 +4141,9 @@ static void __devexit cciss_remove_one(struct pci_dev *pdev)
if (q)
blk_cleanup_queue(q);
}
+ if (hba[i]->drv[j].raid_level != -1)
+ cciss_destroy_ld_sysfs_entry(&hba[i]->drv[j]);
+
}
#ifdef CONFIG_CISS_SCSI_TAPE
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.
Acked-by: Mike Miller <[email protected]>
Signed-off-by: Andrew Patterson <[email protected]>
---
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 085ab11..0ce90c0 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -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];
+static struct task_struct *cciss_scan_thread;
+static DEFINE_MUTEX(scan_mutex);
+static LIST_HEAD(scan_q);
+
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);
@@ -3232,20 +3237,121 @@ static irqreturn_t do_cciss_intr(int irq, void *dev_id)
return IRQ_HANDLED;
}
+/**
+ * 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)
+{
+ struct ctlr_info *test_h;
+ int found = 0;
+ int ret = 0;
+
+ if (mutex_is_locked(&h->busy_initializing))
+ return 0;
+
+ if (!mutex_trylock(&h->busy_shutting_down))
+ return 0;
+
+ mutex_lock(&scan_mutex);
+ list_for_each_entry(test_h, &scan_q, scan_list) {
+ if (test_h == h) {
+ found = 1;
+ break;
+ }
+ }
+ if (!found && !h->busy_scanning) {
+ INIT_COMPLETION(h->scan_wait);
+ list_add_tail(&h->scan_list, &scan_q);
+ ret = 1;
+ }
+ mutex_unlock(&scan_mutex);
+ mutex_unlock(&h->busy_shutting_down);
+
+ return ret;
+}
+
+/**
+ * remove_from_scan_list() - remove controller from rescan queue
+ * @h: Pointer to the controller.
+ *
+ * Removes the controller from the rescan queue if present. Blocks if
+ * the controller is currently conducting a rescan.
+ **/
+static void remove_from_scan_list(struct ctlr_info *h)
+{
+ struct ctlr_info *test_h, *tmp_h;
+ int scanning = 0;
+
+ mutex_lock(&scan_mutex);
+ list_for_each_entry_safe(test_h, tmp_h, &scan_q, scan_list) {
+ if (test_h == h) {
+ list_del(&h->scan_list);
+ complete_all(&h->scan_wait);
+ mutex_unlock(&scan_mutex);
+ return;
+ }
+ }
+ if (&h->busy_scanning)
+ scanning = 0;
+ mutex_unlock(&scan_mutex);
+
+ if (scanning)
+ wait_for_completion(&h->scan_wait);
+}
+
+/**
+ * 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)
{
- ctlr_info_t *h = data;
- int rc;
- DECLARE_COMPLETION_ONSTACK(wait);
- h->rescan_wait = &wait;
+ struct ctlr_info *h;
- for (;;) {
- rc = wait_for_completion_interruptible(&wait);
+ while (1) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule();
if (kthread_should_stop())
break;
- if (!rc)
- rebuild_lun_table(h, 0);
+
+ while (1) {
+ mutex_lock(&scan_mutex);
+ if (list_empty(&scan_q)) {
+ mutex_unlock(&scan_mutex);
+ break;
+ }
+
+ 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) {
+ rebuild_lun_table(h, 0);
+ complete_all(&h->scan_wait);
+ mutex_lock(&scan_mutex);
+ h->busy_scanning = 0;
+ mutex_unlock(&scan_mutex);
+ }
+ }
}
+
return 0;
}
@@ -3268,8 +3374,8 @@ static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c)
case REPORT_LUNS_CHANGED:
printk(KERN_WARNING "cciss%d: report LUN data "
"changed\n", h->ctlr);
- if (h->rescan_wait)
- complete(h->rescan_wait);
+ add_to_scan_list(h);
+ wake_up_process(cciss_scan_thread);
return 1;
break;
case POWER_OR_RESET:
@@ -3919,6 +4025,7 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
mutex_lock(&hba[i]->busy_initializing);
INIT_HLIST_HEAD(&hba[i]->cmpQ);
INIT_HLIST_HEAD(&hba[i]->reqQ);
+ mutex_init(&hba[i]->busy_shutting_down);
if (cciss_pci_init(hba[i], pdev) != 0)
goto clean0;
@@ -3927,6 +4034,8 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
hba[i]->ctlr = i;
hba[i]->pdev = pdev;
+ init_completion(&hba[i]->scan_wait);
+
if (cciss_create_hba_sysfs_entry(hba[i]))
goto clean0;
@@ -4035,13 +4144,9 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
hba[i]->cciss_max_sectors = 2048;
- mutex_unlock(&hba[i]->busy_initializing);
-
rebuild_lun_table(hba[i], 1);
- hba[i]->cciss_scan_thread = kthread_run(scan_thread, hba[i],
- "cciss_scan%02d", i);
- if (IS_ERR(hba[i]->cciss_scan_thread))
- return PTR_ERR(hba[i]->cciss_scan_thread);
+
+ mutex_unlock(&hba[i]->busy_initializing);
return 1;
@@ -4126,8 +4231,9 @@ static void __devexit cciss_remove_one(struct pci_dev *pdev)
return;
}
- kthread_stop(hba[i]->cciss_scan_thread);
+ mutex_lock(&hba[i]->busy_shutting_down);
+ remove_from_scan_list(hba[i]);
remove_proc_entry(hba[i]->devname, proc_cciss);
unregister_blkdev(hba[i]->major, hba[i]->devname);
@@ -4174,6 +4280,7 @@ static void __devexit cciss_remove_one(struct pci_dev *pdev)
pci_release_regions(pdev);
pci_set_drvdata(pdev, NULL);
cciss_destroy_hba_sysfs_entry(hba[i]);
+ mutex_unlock(&hba[i]->busy_shutting_down);
free_hba(i);
}
@@ -4206,15 +4313,25 @@ static int __init cciss_init(void)
if (err)
return err;
+ /* Start the scan thread */
+ cciss_scan_thread = kthread_run(scan_thread, NULL, "cciss_scan");
+ if (IS_ERR(cciss_scan_thread)) {
+ err = PTR_ERR(cciss_scan_thread);
+ goto err_bus_unregister;
+ }
+
/* Register for our PCI devices */
err = pci_register_driver(&cciss_pci_driver);
if (err)
- goto err_bus_register;
+ goto err_thread_stop;
return 0;
-err_bus_register:
+err_thread_stop:
+ kthread_stop(cciss_scan_thread);
+err_bus_unregister:
bus_unregister(&cciss_bus_type);
+
return err;
}
@@ -4231,6 +4348,7 @@ static void __exit cciss_cleanup(void)
cciss_remove_one(hba[i]->pdev);
}
}
+ kthread_stop(cciss_scan_thread);
remove_proc_entry("driver/cciss", NULL);
bus_unregister(&cciss_bus_type);
}
diff --git a/drivers/block/cciss.h b/drivers/block/cciss.h
index bf1699c..6fb9dcc 100644
--- a/drivers/block/cciss.h
+++ b/drivers/block/cciss.h
@@ -2,6 +2,7 @@
#define CCISS_H
#include <linux/genhd.h>
+#include <linux/mutex.h>
#include "cciss_cmd.h"
@@ -108,6 +109,8 @@ struct ctlr_info
int nr_frees;
int busy_configuring;
struct mutex busy_initializing;
+ int busy_scanning;
+ struct mutex busy_shutting_down;
/* This element holds the zero based queue number of the last
* queue to be started. It is used for fairness.
@@ -122,8 +125,8 @@ struct ctlr_info
/* and saved for later processing */
#endif
unsigned char alive;
- struct completion *rescan_wait;
- struct task_struct *cciss_scan_thread;
+ struct list_head scan_list;
+ struct completion scan_wait;
struct device dev;
};
On Tue, 21 Jul 2009 13:55:27 -0600
Andrew Patterson <[email protected]> wrote:
> cciss: use mutex instead of flag to indicate busy initializing
>
> Convert busy_initializing from simple integer flag to mutex, so we
> can use it to block and thus avoid some race conditions.
>
But the code never blocks on busy_initializing. The only place where
we do a mutex_lock() is in cciss_init_one(). Everywhere else we're
using the mutex as a boolean flag in weird ways.
I really don't have the time to work out what we're trying to do here.
Does the code actually work? If so I'm thinking we should merge it and
pretend that someone else did it :(