resubmit cciss: kernel thread to detect changes on MSA2012

PATCH In review Mike Miller 4 participants 7 messages Mar 6, 2009 – Mar 7, 2009

Subsystems (2)

SCSI Block layer

lore

Share & tools

lore

Mike Miller

author

2009-03-06 18:16 UTC #0 lore raw

PATCH 1/2 resubmit cciss: kernel thread to detect changes on MSA2012

Patch 1 of 2

This is a resubmission of yesterdays patch to detect changes on the MSA2012. I hope I've addressed all concerns. This patch rearranges some of the code so we also have coverage in the sg and the ioctl paths as well as the main data path.

The MSA2012 cannot inform the driver of configuration changes since all management is out of band. This is a departure from any storage we have supported in the past. We need some way to detect changes on the topology so we implement this kernel thread. In some instances there's nothing we can do from the driver (like LUN failure) so just print out a message. In the case where logical volumes are added or deleted we call rebuild_lun_table to refreash the driver's view of the world.

Please consider this for inclusion.

Signed-off-by Mike Miller <[email protected]>

drivers/block/cciss.c +84 -2

index d2cb67b..f51545e 100644
@@ -51,6 +51,7 @@
 #include <scsi/scsi_ioctl.h>
 #include <linux/cdrom.h>
 #include <linux/scatterlist.h>
+#include <linux/kthread.h>
 
 #define CCISS_DRIVER_VERSION(maj,min,submin) ((maj<<16)|(min<<8)|(submin))
 #define DRIVER_NAME "HP CISS Driver (v 3.6.20)"

@@ -186,6 +187,8 @@ static int sendcmd_withirq(__u8 cmd, int ctlr, void *buff, size_t size,
 			   __u8 page_code, int cmd_type);
 
 static void fail_all_cmds(unsigned long ctlr);
+static int scan_thread(ctlr_info_t *h);
+static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c);
 
 #ifdef CONFIG_PROC_FS
 static void cciss_procinit(int i);

@@ -735,6 +738,12 @@ static int cciss_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
+static void check_ioctl_unit_attention(ctlr_info_t *host, CommandList_struct *c)
+{
+	if (c->err_info->CommandStatus == CMD_TARGET_STATUS &&
+			c->err_info->ScsiStatus != SAM_STAT_CHECK_CONDITION)
+		(void)check_for_unit_attention(host, c);
+}
 /*
  * ioctl
  */

@@ -1029,6 +1038,8 @@ static int cciss_ioctl(struct block_device *bdev, fmode_t mode,
 					 iocommand.buf_size,
 					 PCI_DMA_BIDIRECTIONAL);
 
+			check_ioctl_unit_attention(host, c);
+
 			/* Copy the error information out */
 			iocommand.error_info = *(c->err_info);
 			if (copy_to_user

@@ -1180,6 +1191,7 @@ static int cciss_ioctl(struct block_device *bdev, fmode_t mode,
 					(dma_addr_t) temp64.val, buff_size[i],
 					PCI_DMA_BIDIRECTIONAL);
 			}
+			check_ioctl_unit_attention(host, c);
 			/* Copy the error information out */
 			ioc->error_info = *(c->err_info);
 			if (copy_to_user(argp, ioc, sizeof(*ioc))) {

@@ -2585,12 +2597,14 @@ static inline unsigned int make_status_bytes(unsigned int scsi_status_byte,
 		((driver_byte & 0xff) << 24);
 }
 
-static inline int evaluate_target_status(CommandList_struct *cmd)
+static inline int evaluate_target_status(ctlr_info_t *h,
+			CommandList_struct *cmd, int *retry_cmd)
 {
 	unsigned char sense_key;
 	unsigned char status_byte, msg_byte, host_byte, driver_byte;
 	int error_value;
 
+	*retry_cmd = 0;
 	/* If we get in here, it means we got "target status", that is, scsi status */
 	status_byte = cmd->err_info->ScsiStatus;
 	driver_byte = DRIVER_OK;

@@ -2618,6 +2632,11 @@ static inline int evaluate_target_status(CommandList_struct *cmd)
 	if (((sense_key == 0x0) || (sense_key == 0x1)) && !blk_pc_request(cmd->rq))
 		error_value = 0;
 
+	if (check_for_unit_attention(h, cmd)) {
+		*retry_cmd = !blk_pc_request(cmd->rq);
+		return 0;
+	}
+
 	if (!blk_pc_request(cmd->rq)) { /* Not SG_IO or similar? */
 		if (error_value != 0)
 			printk(KERN_WARNING "cciss: cmd %p has CHECK CONDITION"

@@ -2647,6 +2666,7 @@ static inline void complete_command(ctlr_info_t *h, CommandList_struct *cmd,
 	int retry_cmd = 0;
 	struct request *rq = cmd->rq;
 
+
 	rq->errors = 0;
 
 	if (timeout)

@@ -2657,7 +2677,7 @@ static inline void complete_command(ctlr_info_t *h, CommandList_struct *cmd,
 
 	switch (cmd->err_info->CommandStatus) {
 	case CMD_TARGET_STATUS:
-		rq->errors = evaluate_target_status(cmd);
+		rq->errors = evaluate_target_status(h, cmd, &retry_cmd);
 		break;
 	case CMD_DATA_UNDERRUN:
 		if (blk_fs_request(cmd->rq)) {

@@ -3008,6 +3028,62 @@ static irqreturn_t do_cciss_intr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int scan_thread(ctlr_info_t *h)
+{
+	int rc;
+	DECLARE_COMPLETION_ONSTACK(wait);
+	h->rescan_wait = &wait;
+
+	while (!kthread_should_stop()) {
+		rc = wait_for_completion_timeout(&wait, 300 * HZ);
+		if (!rc)
+			continue;
+		else
+			rebuild_lun_table(h, 0);
+	}
+	return 0;
+}
+
+static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c)
+{
+	if (c->err_info->SenseInfo[2] != UNIT_ATTENTION)
+		return 0;
+
+	switch (c->err_info->SenseInfo[12]) {
+	case STATE_CHANGED:
+		printk(KERN_WARNING "cciss%d: a state change "
+			"detected, command retried\n", h->ctlr);
+		return 1;
+	break;
+	case LUN_FAILED:
+		printk(KERN_WARNING "cciss%d: LUN failure "
+			"detected, action required\n", h->ctlr);
+		return 1;
+	break;
+	case REPORT_LUNS_CHANGED:
+		printk(KERN_WARNING "cciss%d: report LUN data "
+			"changed\n", h->ctlr);
+		if (h->rescan_wait)
+			complete(h->rescan_wait);
+		return 1;
+	break;
+	case POWER_OR_RESET:
+		printk(KERN_WARNING "cciss%d: a power on "
+			"or device reset detected\n", h->ctlr);
+		return 1;
+	break;
+	case UNIT_ATTENTION_CLEARED:
+		printk(KERN_WARNING "cciss%d: unit attention "
+		    "cleared by another initiator\n", h->ctlr);
+		return 1;
+	break;
+	default:
+		printk(KERN_WARNING "cciss%d: unknown "
+			"unit attention detected\n", h->ctlr);
+				return 1;
+	}
+}
+
 /*
  *  We cannot read the structure directly, for portability we must use
  *   the io functions.

@@ -3600,6 +3685,7 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 	int rc;
 	int dac, return_code;
 	InquiryData_struct *inq_buff = NULL;
+	struct task_struct *cciss_scan_thread;
 
 	if (reset_devices) {
 		/* Reset the controller with a PCI power-cycle */

@@ -3751,6 +3837,11 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 	hba[i]->busy_initializing = 0;
 
 	rebuild_lun_table(hba[i], 1);
+	cciss_scan_thread = kthread_run((void *)scan_thread, hba[i],
+				"scan_thread");
+	if (IS_ERR(cciss_scan_thread))
+		return PTR_ERR(cciss_scan_thread);
+
 	return 1;
 
 clean4:

drivers/block/cciss.h +1 -0

index 15e2b84..c85783f 100644
@@ -121,6 +121,7 @@ struct ctlr_info
 	struct sendcmd_reject_list scsi_rejects;
 #endif
 	unsigned char alive;
+	struct completion *rescan_wait;
 };
 
 /*  Defining the diffent access_menthods */

drivers/block/cciss_cmd.h +23 -0

index 24e22de..40b1b92 100644
@@ -25,6 +25,29 @@
 #define CMD_TIMEOUT             0x000B
 #define CMD_UNABORTABLE		0x000C
 
+/* Unit Attentions ASC's as defined for the MSA2012sa */
+#define POWER_OR_RESET			0x29
+#define STATE_CHANGED			0x2a
+#define UNIT_ATTENTION_CLEARED		0x2f
+#define LUN_FAILED			0x3e
+#define REPORT_LUNS_CHANGED		0x3f
+
+/* Unit Attentions ASCQ's as defined for the MSA2012sa */
+
+	/* These ASCQ's defined for ASC = POWER_OR_RESET */
+#define POWER_ON_RESET			0x00
+#define POWER_ON_REBOOT			0x01
+#define SCSI_BUS_RESET			0x02
+#define MSA_TARGET_RESET		0x03
+#define CONTROLLER_FAILOVER		0x04
+#define TRANSCEIVER_SE			0x05
+#define TRANSCEIVER_LVD			0x06
+
+	/* These ASCQ's defined for ASC = STATE_CHANGED */
+#define RESERVATION_PREEMPTED		0x03
+#define ASYM_ACCESS_CHANGED		0x06
+#define LUN_CAPACITY_CHANGED		0x09
+
 //transfer direction
 #define XFER_NONE               0x00
 #define XFER_WRITE              0x01

James Bottomley

maintainer

2009-03-06 18:24 UTC #1 lore raw

Quoted 15 lines · Mike Miller

Patch 1 of 2
This is a resubmission of yesterdays patch to detect changes on the MSA2012. I hope I've addressed all concerns. This patch rearranges some of the code so we also have coverage in the sg and the ioctl paths as well as the main data path.
The MSA2012 cannot inform the driver of configuration changes since all management is out of band. This is a departure from any storage we have supported in the past. We need some way to detect changes on the topology so we implement this kernel thread. In some instances there's nothing we can do from the driver (like LUN failure) so just print out a message. In the case where logical volumes are added or deleted we call rebuild_lun_table to refreash the driver's view of the world.

Quoted 1 line

Please consider this for inclusion.

I still don't quite see how the thread stops on module removal ... there needs to be an explicit kthread_stop() somewhere in the clean up path.

James

Miller, Mike (OS Dev)

author

2009-03-06 20:01 UTC #2 lore raw

Quoted 32 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Friday, March 06, 2009 12:24 PM
To: Miller, Mike (OS Dev)
Cc: Andrew Morton; Jens Axboe; LKML; LKML-scsi; [email protected]; [email protected]
Subject: Re: [PATCH 1/2] resubmit cciss: kernel thread to detect changes on MSA2012
Quoted 3 lines · Mike Miller
Patch 1 of 2
This is a resubmission of yesterdays patch to detect
changes on the MSA2012.
Quoted 1 line
I hope I've addressed all concerns. This patch rearranges
some of the
Quoted 1 line
code so we also have coverage in the sg and the ioctl paths
as well as
Quoted 4 lines
the main data path.
The MSA2012 cannot inform the driver of configuration changes since all management is out of band. This is a departure from any
storage we
Quoted 1 line
have supported in the past. We need some way to detect
changes on the
Quoted 1 line
topology so we implement this kernel thread. In some
instances there's
Quoted 1 line
nothing we can do from the driver (like LUN failure) so
just print out
Quoted 1 line
a message. In the case where logical volumes are added or

Quoted 9 lines

deleted we
Quoted 3 lines
call rebuild_lun_table to refreash the driver's view of the world.
Please consider this for inclusion.
I still don't quite see how the thread stops on module removal ... there needs to be an explicit kthread_stop() somewhere in the clean up path.

I thought that was probably needed, duh. Is there anything else I should address at the same time?

-- mikem

Mike Miller (OS Dev)

2009-03-06 23:29 UTC #3 lore raw

In reply to James Bottomley

Quoted 16 lines · James Bottomley

Quoted 15 lines · Mike Miller
Patch 1 of 2
This is a resubmission of yesterdays patch to detect changes on the MSA2012. I hope I've addressed all concerns. This patch rearranges some of the code so we also have coverage in the sg and the ioctl paths as well as the main data path.
The MSA2012 cannot inform the driver of configuration changes since all management is out of band. This is a departure from any storage we have supported in the past. We need some way to detect changes on the topology so we implement this kernel thread. In some instances there's nothing we can do from the driver (like LUN failure) so just print out a message. In the case where logical volumes are added or deleted we call rebuild_lun_table to refreash the driver's view of the world.

Quoted 8 lines

Quoted 1 line
Please consider this for inclusion.
I still don't quite see how the thread stops on module removal ... there needs to be an explicit kthread_stop() somewhere in the clean up path.
James

This time I make a call to kthread_stop in cciss_remove_one. The driver can be unloaded and the thread gets cleaned up.

KNOWN BUG: it seems the timeout must expire before kthread_stop actually stops the thread. This causes the driver to hang and wait during rmmod. I've played around with several things but haven't found the correct way to address the problem. Looking at other drivers hasn't been much help. Any advice is greatly appreciated.

Thanks,
mikem

Signed-off-by Mike Miller <[email protected]>

drivers/block/cciss.c +89 -2

index d2cb67b..5b5c61d 100644
@@ -51,6 +51,7 @@
 #include <scsi/scsi_ioctl.h>
 #include <linux/cdrom.h>
 #include <linux/scatterlist.h>
+#include <linux/kthread.h>
 
 #define CCISS_DRIVER_VERSION(maj,min,submin) ((maj<<16)|(min<<8)|(submin))
 #define DRIVER_NAME "HP CISS Driver (v 3.6.20)"

@@ -186,6 +187,8 @@ static int sendcmd_withirq(__u8 cmd, int ctlr, void *buff, size_t size,
 			   __u8 page_code, int cmd_type);
 
 static void fail_all_cmds(unsigned long ctlr);
+static int scan_thread(ctlr_info_t *h);
+static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c);
 
 #ifdef CONFIG_PROC_FS
 static void cciss_procinit(int i);

@@ -735,6 +738,12 @@ static int cciss_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
+static void check_ioctl_unit_attention(ctlr_info_t *host, CommandList_struct *c)
+{
+	if (c->err_info->CommandStatus == CMD_TARGET_STATUS &&
+			c->err_info->ScsiStatus != SAM_STAT_CHECK_CONDITION)
+		(void)check_for_unit_attention(host, c);
+}
 /*
  * ioctl
  */

@@ -1029,6 +1038,8 @@ static int cciss_ioctl(struct block_device *bdev, fmode_t mode,
 					 iocommand.buf_size,
 					 PCI_DMA_BIDIRECTIONAL);
 
+			check_ioctl_unit_attention(host, c);
+
 			/* Copy the error information out */
 			iocommand.error_info = *(c->err_info);
 			if (copy_to_user

@@ -1180,6 +1191,7 @@ static int cciss_ioctl(struct block_device *bdev, fmode_t mode,
 					(dma_addr_t) temp64.val, buff_size[i],
 					PCI_DMA_BIDIRECTIONAL);
 			}
+			check_ioctl_unit_attention(host, c);
 			/* Copy the error information out */
 			ioc->error_info = *(c->err_info);
 			if (copy_to_user(argp, ioc, sizeof(*ioc))) {

@@ -2585,12 +2597,14 @@ static inline unsigned int make_status_bytes(unsigned int scsi_status_byte,
 		((driver_byte & 0xff) << 24);
 }
 
-static inline int evaluate_target_status(CommandList_struct *cmd)
+static inline int evaluate_target_status(ctlr_info_t *h,
+			CommandList_struct *cmd, int *retry_cmd)
 {
 	unsigned char sense_key;
 	unsigned char status_byte, msg_byte, host_byte, driver_byte;
 	int error_value;
 
+	*retry_cmd = 0;
 	/* If we get in here, it means we got "target status", that is, scsi status */
 	status_byte = cmd->err_info->ScsiStatus;
 	driver_byte = DRIVER_OK;

@@ -2618,6 +2632,11 @@ static inline int evaluate_target_status(CommandList_struct *cmd)
 	if (((sense_key == 0x0) || (sense_key == 0x1)) && !blk_pc_request(cmd->rq))
 		error_value = 0;
 
+	if (check_for_unit_attention(h, cmd)) {
+		*retry_cmd = !blk_pc_request(cmd->rq);
+		return 0;
+	}
+
 	if (!blk_pc_request(cmd->rq)) { /* Not SG_IO or similar? */
 		if (error_value != 0)
 			printk(KERN_WARNING "cciss: cmd %p has CHECK CONDITION"

@@ -2657,7 +2676,7 @@ static inline void complete_command(ctlr_info_t *h, CommandList_struct *cmd,
 
 	switch (cmd->err_info->CommandStatus) {
 	case CMD_TARGET_STATUS:
-		rq->errors = evaluate_target_status(cmd);
+		rq->errors = evaluate_target_status(h, cmd, &retry_cmd);
 		break;
 	case CMD_DATA_UNDERRUN:
 		if (blk_fs_request(cmd->rq)) {

@@ -3008,6 +3027,62 @@ static irqreturn_t do_cciss_intr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int scan_thread(ctlr_info_t *h)
+{
+	int rc;
+	DECLARE_COMPLETION_ONSTACK(wait);
+	h->rescan_wait = &wait;
+
+	while (!kthread_should_stop()) {
+		rc = wait_for_completion_timeout(&wait, 300 * HZ);
+		if (!rc)
+			continue;
+		else
+			rebuild_lun_table(h, 0);
+	}
+	return 0;
+}
+
+static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c)
+{
+	if (c->err_info->SenseInfo[2] != UNIT_ATTENTION)
+		return 0;
+
+	switch (c->err_info->SenseInfo[12]) {
+	case STATE_CHANGED:
+		printk(KERN_WARNING "cciss%d: a state change "
+			"detected, command retried\n", h->ctlr);
+		return 1;
+	break;
+	case LUN_FAILED:
+		printk(KERN_WARNING "cciss%d: LUN failure "
+			"detected, action required\n", h->ctlr);
+		return 1;
+	break;
+	case REPORT_LUNS_CHANGED:
+		printk(KERN_WARNING "cciss%d: report LUN data "
+			"changed\n", h->ctlr);
+		if (h->rescan_wait)
+			complete(h->rescan_wait);
+		return 1;
+	break;
+	case POWER_OR_RESET:
+		printk(KERN_WARNING "cciss%d: a power on "
+			"or device reset detected\n", h->ctlr);
+		return 1;
+	break;
+	case UNIT_ATTENTION_CLEARED:
+		printk(KERN_WARNING "cciss%d: unit attention "
+		    "cleared by another initiator\n", h->ctlr);
+		return 1;
+	break;
+	default:
+		printk(KERN_WARNING "cciss%d: unknown "
+			"unit attention detected\n", h->ctlr);
+				return 1;
+	}
+}
+
 /*
  *  We cannot read the structure directly, for portability we must use
  *   the io functions.

@@ -3600,6 +3675,7 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 	int rc;
 	int dac, return_code;
 	InquiryData_struct *inq_buff = NULL;
+	char cciss_scan[14];
 
 	if (reset_devices) {
 		/* Reset the controller with a PCI power-cycle */

@@ -3751,6 +3827,13 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 	hba[i]->busy_initializing = 0;
 
 	rebuild_lun_table(hba[i], 1);
+	hba[i]->time_for_thread_to_die = 0;
+	snprintf(cciss_scan, 14, "cciss_scan%02d", i);
+	hba[i]->cciss_scan_thread = kthread_run((void *)scan_thread, hba[i],
+				cciss_scan);
+	if (IS_ERR(hba[i]->cciss_scan_thread))
+		return PTR_ERR(hba[i]->cciss_scan_thread);
+
 	return 1;
 
 clean4:

@@ -3826,6 +3909,7 @@ static void __devexit cciss_remove_one(struct pci_dev *pdev)
 		printk(KERN_ERR "cciss: Unable to remove device \n");
 		return;
 	}
+
 	tmp_ptr = pci_get_drvdata(pdev);
 	i = tmp_ptr->ctlr;
 	if (hba[i] == NULL) {

@@ -3834,6 +3918,9 @@ static void __devexit cciss_remove_one(struct pci_dev *pdev)
 		return;
 	}
 
+	kthread_stop(hba[i]->cciss_scan_thread);
+	complete(hba[i]->rescan_wait);
+
 	remove_proc_entry(hba[i]->devname, proc_cciss);
 	unregister_blkdev(hba[i]->major, hba[i]->devname);

drivers/block/cciss.h +2 -0

index 15e2b84..703e080 100644
@@ -121,6 +121,8 @@ struct ctlr_info
 	struct sendcmd_reject_list scsi_rejects;
 #endif
 	unsigned char alive;
+	struct completion *rescan_wait;
+	struct task_struct *cciss_scan_thread;
 };
 
 /*  Defining the diffent access_menthods */

drivers/block/cciss_cmd.h +23 -0

index 24e22de..40b1b92 100644
@@ -25,6 +25,29 @@
 #define CMD_TIMEOUT             0x000B
 #define CMD_UNABORTABLE		0x000C
 
+/* Unit Attentions ASC's as defined for the MSA2012sa */
+#define POWER_OR_RESET			0x29
+#define STATE_CHANGED			0x2a
+#define UNIT_ATTENTION_CLEARED		0x2f
+#define LUN_FAILED			0x3e
+#define REPORT_LUNS_CHANGED		0x3f
+
+/* Unit Attentions ASCQ's as defined for the MSA2012sa */
+
+	/* These ASCQ's defined for ASC = POWER_OR_RESET */
+#define POWER_ON_RESET			0x00
+#define POWER_ON_REBOOT			0x01
+#define SCSI_BUS_RESET			0x02
+#define MSA_TARGET_RESET		0x03
+#define CONTROLLER_FAILOVER		0x04
+#define TRANSCEIVER_SE			0x05
+#define TRANSCEIVER_LVD			0x06
+
+	/* These ASCQ's defined for ASC = STATE_CHANGED */
+#define RESERVATION_PREEMPTED		0x03
+#define ASYM_ACCESS_CHANGED		0x06
+#define LUN_CAPACITY_CHANGED		0x09
+
 //transfer direction
 #define XFER_NONE               0x00
 #define XFER_WRITE              0x01

Andrew Morton

2009-03-06 23:57 UTC #4 lore raw

Quoted 19 lines · On Fri, 6 Mar 2009 17:29:18 -0600 "Mike Miller (OS Dev)"…

Quoted 18 lines · James Bottomley
Quoted 16 lines · Mike Miller
Patch 1 of 2
This is a resubmission of yesterdays patch to detect changes on the MSA2012. I hope I've addressed all concerns. This patch rearranges some of the code so we also have coverage in the sg and the ioctl paths as well as the main data path.
The MSA2012 cannot inform the driver of configuration changes since all management is out of band. This is a departure from any storage we have supported in the past. We need some way to detect changes on the topology so we implement this kernel thread. In some instances there's nothing we can do from the driver (like LUN failure) so just print out a message. In the case where logical volumes are added or deleted we call rebuild_lun_table to refreash the driver's view of the world.
Please consider this for inclusion.

Quoted 8 lines

Quoted 6 lines
I still don't quite see how the thread stops on module removal ... there needs to be an explicit kthread_stop() somewhere in the clean up path.
James
This time I make a call to kthread_stop in cciss_remove_one. The driver can be unloaded and the thread gets cleaned up.

Please include a complete (and suitably updated) copy of the changelog with each iteration of a patch.

Quoted 5 lines

KNOWN BUG: it seems the timeout must expire before kthread_stop actually stops the thread. This causes the driver to hang and wait during rmmod. I've played around with several things but haven't found the correct way to address the problem. Looking at other drivers hasn't been much help. Any advice is greatly appreciated.

Well, wait_for_completion_timeout() is only going to return when the timeout timed out, or someone ran complete().

Quoted 5 lines

drivers/block/cciss.c +5 -0

+static int scan_thread(ctlr_info_t *h)
+{
+	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 3044

Quoted 10 lines

drivers/block/cciss.c +10 -0

+
+	while (!kthread_should_stop()) {
+		rc = wait_for_completion_timeout(&wait, 300 * HZ);
+		if (!rc)
+			continue;
+		else
+			rebuild_lun_table(h, 0);
+	}
+	return 0;
+}

So.. we shouldn't need the timeout here at all - just use wait_for_completion().

static int scan_thread(ctlr_info_t *h)

{
	DECLARE_COMPLETION_ONSTACK(wait);

	h->rescan_wait = &wait;
	for ( ; ; ) {
		wait_for_completion(&wait);
		if (kthread_should_stop())
			break;
		rebuild_lun_table(h, 0);
	}
	return 0;
}

And on the teardown path, do

	complete(...);
	kthread_stop(...);

James Bottomley

maintainer

2009-03-07 20:36 UTC #5 lore raw

Quoted 77 lines · Andrew Morton

Quoted 19 lines · On Fri, 6 Mar 2009 17:29:18 -0600 "Mike Miller (OS Dev)"…
Quoted 18 lines · James Bottomley
Quoted 16 lines · Mike Miller
Patch 1 of 2
This is a resubmission of yesterdays patch to detect changes on the MSA2012. I hope I've addressed all concerns. This patch rearranges some of the code so we also have coverage in the sg and the ioctl paths as well as the main data path.
The MSA2012 cannot inform the driver of configuration changes since all management is out of band. This is a departure from any storage we have supported in the past. We need some way to detect changes on the topology so we implement this kernel thread. In some instances there's nothing we can do from the driver (like LUN failure) so just print out a message. In the case where logical volumes are added or deleted we call rebuild_lun_table to refreash the driver's view of the world.
Please consider this for inclusion.
Quoted 8 lines
Quoted 6 lines
I still don't quite see how the thread stops on module removal ... there needs to be an explicit kthread_stop() somewhere in the clean up path.
James
This time I make a call to kthread_stop in cciss_remove_one. The driver can be unloaded and the thread gets cleaned up.
Please include a complete (and suitably updated) copy of the changelog with each iteration of a patch.
Quoted 5 lines
KNOWN BUG: it seems the timeout must expire before kthread_stop actually stops the thread. This causes the driver to hang and wait during rmmod. I've played around with several things but haven't found the correct way to address the problem. Looking at other drivers hasn't been much help. Any advice is greatly appreciated.
Well, wait_for_completion_timeout() is only going to return when the timeout timed out, or someone ran complete().
Quoted 5 lines
drivers/block/cciss.c +5 -0
+static int scan_thread(ctlr_info_t *h)
+{
+	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 3044
Quoted 10 lines
drivers/block/cciss.c +10 -0
+
+	while (!kthread_should_stop()) {
+		rc = wait_for_completion_timeout(&wait, 300 * HZ);
+		if (!rc)
+			continue;
+		else
+			rebuild_lun_table(h, 0);
+	}
+	return 0;
+}
So.. we shouldn't need the timeout here at all - just use wait_for_completion().
static int scan_thread(ctlr_info_t *h)
{
	DECLARE_COMPLETION_ONSTACK(wait);

	h->rescan_wait = &wait;
	for ( ; ; ) {
		wait_for_completion(&wait);
		if (kthread_should_stop())
			break;
		rebuild_lun_table(h, 0);
	}
	return 0;
}

Quoted 4 lines

And on the teardown path, do
	complete(...);
	kthread_stop(...);

This is racy ... although I think the race would only show in a pre-empt
kernel: complete causes the thread to run immediately pre-empting us. Now it runs around the loop, through kthread_should_stop() and back to wait_for_completion() before we get a chance to run kthread_stop().

The only way to avoid this seems to be to use wait queues and wake up (kthread_stop does an automatic wake_up of the process, which is ignored by completions).

James

Mike Miller (OS Dev)

2009-03-07 18:52 UTC #6 lore raw

In reply to Mike Miller

On Fri, Mar 06, 2009 at 12:16:03PM -0600, Miller, Mike (OS Dev)?? wrote:
Patch 1 of 2

This is yet another go at the patch to detect changes on the MSA2012. I hope I've addressed all concerns. This patch rearranges some of the code so we also have coverage in the sg and the ioctl paths as well as the main data path.

Changelog:
1. Do the completion(hba[i]->rescan_wait) before calling kthread_stop, this resolves the issue of waiting for the timeout on rmmod
2. Added a new function called check_ioctl_unit_attention to cover UA's from the ioctl patch
3. I preserved the wait_for_completion_timeout to avoid call traces caused by /proc/sys/kernel/hung_task_timeout_secs expiring
4. Moved the call to check_for_unit_attention to evaluate_target_status since it's already called from complete_command
5. Add retry_cmd as an argument to evaluate_target_status
6. Added *rescan_wait to the controller info struct

Please consider this for inclusion.

Signed-off-by Mike Miller <[email protected]>

drivers/block/cciss.c +88 -2

index d2cb67b..96e20e1 100644
@@ -51,6 +51,7 @@
 #include <scsi/scsi_ioctl.h>
 #include <linux/cdrom.h>
 #include <linux/scatterlist.h>
+#include <linux/kthread.h>
 
 #define CCISS_DRIVER_VERSION(maj,min,submin) ((maj<<16)|(min<<8)|(submin))
 #define DRIVER_NAME "HP CISS Driver (v 3.6.20)"

@@ -186,6 +187,8 @@ static int sendcmd_withirq(__u8 cmd, int ctlr, void *buff, size_t size,
 			   __u8 page_code, int cmd_type);
 
 static void fail_all_cmds(unsigned long ctlr);
+static int scan_thread(ctlr_info_t *h);
+static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c);
 
 #ifdef CONFIG_PROC_FS
 static void cciss_procinit(int i);

@@ -735,6 +738,12 @@ static int cciss_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
+static void check_ioctl_unit_attention(ctlr_info_t *host, CommandList_struct *c)
+{
+	if (c->err_info->CommandStatus == CMD_TARGET_STATUS &&
+			c->err_info->ScsiStatus != SAM_STAT_CHECK_CONDITION)
+		(void)check_for_unit_attention(host, c);
+}
 /*
  * ioctl
  */

@@ -1029,6 +1038,8 @@ static int cciss_ioctl(struct block_device *bdev, fmode_t mode,
 					 iocommand.buf_size,
 					 PCI_DMA_BIDIRECTIONAL);
 
+			check_ioctl_unit_attention(host, c);
+
 			/* Copy the error information out */
 			iocommand.error_info = *(c->err_info);
 			if (copy_to_user

@@ -1180,6 +1191,7 @@ static int cciss_ioctl(struct block_device *bdev, fmode_t mode,
 					(dma_addr_t) temp64.val, buff_size[i],
 					PCI_DMA_BIDIRECTIONAL);
 			}
+			check_ioctl_unit_attention(host, c);
 			/* Copy the error information out */
 			ioc->error_info = *(c->err_info);
 			if (copy_to_user(argp, ioc, sizeof(*ioc))) {

@@ -2585,12 +2597,14 @@ static inline unsigned int make_status_bytes(unsigned int scsi_status_byte,
 		((driver_byte & 0xff) << 24);
 }
 
-static inline int evaluate_target_status(CommandList_struct *cmd)
+static inline int evaluate_target_status(ctlr_info_t *h,
+			CommandList_struct *cmd, int *retry_cmd)
 {
 	unsigned char sense_key;
 	unsigned char status_byte, msg_byte, host_byte, driver_byte;
 	int error_value;
 
+	*retry_cmd = 0;
 	/* If we get in here, it means we got "target status", that is, scsi status */
 	status_byte = cmd->err_info->ScsiStatus;
 	driver_byte = DRIVER_OK;

@@ -2618,6 +2632,11 @@ static inline int evaluate_target_status(CommandList_struct *cmd)
 	if (((sense_key == 0x0) || (sense_key == 0x1)) && !blk_pc_request(cmd->rq))
 		error_value = 0;
 
+	if (check_for_unit_attention(h, cmd)) {
+		*retry_cmd = !blk_pc_request(cmd->rq);
+		return 0;
+	}
+
 	if (!blk_pc_request(cmd->rq)) { /* Not SG_IO or similar? */
 		if (error_value != 0)
 			printk(KERN_WARNING "cciss: cmd %p has CHECK CONDITION"

@@ -2657,7 +2676,7 @@ static inline void complete_command(ctlr_info_t *h, CommandList_struct *cmd,
 
 	switch (cmd->err_info->CommandStatus) {
 	case CMD_TARGET_STATUS:
-		rq->errors = evaluate_target_status(cmd);
+		rq->errors = evaluate_target_status(h, cmd, &retry_cmd);
 		break;
 	case CMD_DATA_UNDERRUN:
 		if (blk_fs_request(cmd->rq)) {

@@ -3008,6 +3027,62 @@ static irqreturn_t do_cciss_intr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int scan_thread(ctlr_info_t *h)
+{
+	int rc;
+	DECLARE_COMPLETION_ONSTACK(wait);
+	h->rescan_wait = &wait;
+
+	while (!kthread_should_stop()) {
+		rc = wait_for_completion_timeout(&wait, 300 * HZ);
+		if (!rc)
+			continue;
+		else
+			rebuild_lun_table(h, 0);
+	}
+	return 0;
+}
+
+static int check_for_unit_attention(ctlr_info_t *h, CommandList_struct *c)
+{
+	if (c->err_info->SenseInfo[2] != UNIT_ATTENTION)
+		return 0;
+
+	switch (c->err_info->SenseInfo[12]) {
+	case STATE_CHANGED:
+		printk(KERN_WARNING "cciss%d: a state change "
+			"detected, command retried\n", h->ctlr);
+		return 1;
+	break;
+	case LUN_FAILED:
+		printk(KERN_WARNING "cciss%d: LUN failure "
+			"detected, action required\n", h->ctlr);
+		return 1;
+	break;
+	case REPORT_LUNS_CHANGED:
+		printk(KERN_WARNING "cciss%d: report LUN data "
+			"changed\n", h->ctlr);
+		if (h->rescan_wait)
+			complete(h->rescan_wait);
+		return 1;
+	break;
+	case POWER_OR_RESET:
+		printk(KERN_WARNING "cciss%d: a power on "
+			"or device reset detected\n", h->ctlr);
+		return 1;
+	break;
+	case UNIT_ATTENTION_CLEARED:
+		printk(KERN_WARNING "cciss%d: unit attention "
+		    "cleared by another initiator\n", h->ctlr);
+		return 1;
+	break;
+	default:
+		printk(KERN_WARNING "cciss%d: unknown "
+			"unit attention detected\n", h->ctlr);
+				return 1;
+	}
+}
+
 /*
  *  We cannot read the structure directly, for portability we must use
  *   the io functions.

@@ -3600,6 +3675,7 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 	int rc;
 	int dac, return_code;
 	InquiryData_struct *inq_buff = NULL;
+	char cciss_scan[14];
 
 	if (reset_devices) {
 		/* Reset the controller with a PCI power-cycle */

@@ -3751,6 +3827,12 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 	hba[i]->busy_initializing = 0;
 
 	rebuild_lun_table(hba[i], 1);
+	snprintf(cciss_scan, 14, "cciss_scan%02d", i);
+	hba[i]->cciss_scan_thread = kthread_run((void *)scan_thread, hba[i],
+				cciss_scan);
+	if (IS_ERR(hba[i]->cciss_scan_thread))
+		return PTR_ERR(hba[i]->cciss_scan_thread);
+
 	return 1;
 
 clean4:

@@ -3826,6 +3908,7 @@ static void __devexit cciss_remove_one(struct pci_dev *pdev)
 		printk(KERN_ERR "cciss: Unable to remove device \n");
 		return;
 	}
+
 	tmp_ptr = pci_get_drvdata(pdev);
 	i = tmp_ptr->ctlr;
 	if (hba[i] == NULL) {

@@ -3834,6 +3917,9 @@ static void __devexit cciss_remove_one(struct pci_dev *pdev)
 		return;
 	}
 
+	complete(hba[i]->rescan_wait);
+	kthread_stop(hba[i]->cciss_scan_thread);
+
 	remove_proc_entry(hba[i]->devname, proc_cciss);
 	unregister_blkdev(hba[i]->major, hba[i]->devname);

drivers/block/cciss.h +2 -0

index 15e2b84..703e080 100644
@@ -121,6 +121,8 @@ struct ctlr_info
 	struct sendcmd_reject_list scsi_rejects;
 #endif
 	unsigned char alive;
+	struct completion *rescan_wait;
+	struct task_struct *cciss_scan_thread;
 };
 
 /*  Defining the diffent access_menthods */

drivers/block/cciss_cmd.h +23 -0

index 24e22de..40b1b92 100644
@@ -25,6 +25,29 @@
 #define CMD_TIMEOUT             0x000B
 #define CMD_UNABORTABLE		0x000C
 
+/* Unit Attentions ASC's as defined for the MSA2012sa */
+#define POWER_OR_RESET			0x29
+#define STATE_CHANGED			0x2a
+#define UNIT_ATTENTION_CLEARED		0x2f
+#define LUN_FAILED			0x3e
+#define REPORT_LUNS_CHANGED		0x3f
+
+/* Unit Attentions ASCQ's as defined for the MSA2012sa */
+
+	/* These ASCQ's defined for ASC = POWER_OR_RESET */
+#define POWER_ON_RESET			0x00
+#define POWER_ON_REBOOT			0x01
+#define SCSI_BUS_RESET			0x02
+#define MSA_TARGET_RESET		0x03
+#define CONTROLLER_FAILOVER		0x04
+#define TRANSCEIVER_SE			0x05
+#define TRANSCEIVER_LVD			0x06
+
+	/* These ASCQ's defined for ASC = STATE_CHANGED */
+#define RESERVATION_PREEMPTED		0x03
+#define ASYM_ACCESS_CHANGED		0x06
+#define LUN_CAPACITY_CHANGED		0x09
+
 //transfer direction
 #define XFER_NONE               0x00
 #define XFER_WRITE              0x01