Make scsi error recovery play nice with devices blocked by transport

As no one has commented, I'm reposting this rfc as a patch. I've been using it for all my testing during development of the LSI MPT Fusion fc transport attribute patch and it has shown no ill effect.

--

Error recovery doesn't interact very well with fc targets which have been blocked by the fc transport. Error recovery continues to attempt to recover the target and ends up marking the fc target offline. Once offline, if the target returns before the remote port is removed, commands which could have been successfully reissued instead are completed with an error status due to the offline status of the target.

This patch makes a couple of hopefully minor tweaks to the error recovery logic to work better with targets which have been blocked by the transport.

First, if the target is blocked and error recovery gives up, don't put the device offline. Either the transport will delete the target thus disposing of any queued requests or it will unblock the target and requests will be reissued.

Second, if a device is blocked, queue up commands being flushed from the done queue for retry instead of completing them with an error.

Thanks,
Mike Reed

------------------------------------------------------------------------

diff -ru linux-2.6.15-rc5-git6-patched/drivers/scsi/scsi_error.c linux-2.6.15-rc5-git6-patched-mdr/drivers/scsi/scsi_error.c

drivers/scsi/scsi_error.c +11 -6

@@ -1130,10 +1130,14 @@
 	struct scsi_cmnd *scmd, *next;
 
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		sdev_printk(KERN_INFO, scmd->device,
-			    "scsi: Device offlined - not"
-			    " ready after error recovery\n");
-		scsi_device_set_state(scmd->device, SDEV_OFFLINE);
+		/* if blocked, transport will provide final device disposition */
+		if (!scsi_device_blocked(scmd->device)) {
+			sdev_printk(KERN_INFO, scmd->device,
+				    "scsi: Device offlined - not"
+				    " ready after error recovery\n");
+			scsi_device_set_state(scmd->device, SDEV_OFFLINE);
+		}
+
 		if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
 			/*
 			 * FIXME: Handle lost cmds.

@@ -1460,9 +1464,10 @@
 
 	list_for_each_entry_safe(scmd, next, done_q, eh_entry) {
 		list_del_init(&scmd->eh_entry);
-		if (scsi_device_online(scmd->device) &&
+		if (scsi_device_blocked(scmd->device) ||
+		    (scsi_device_online(scmd->device) &&
 		    !blk_noretry_request(scmd->request) &&
-		    (++scmd->retries < scmd->allowed)) {
+		    (++scmd->retries < scmd->allowed))) {
 			SCSI_LOG_ERROR_RECOVERY(3, printk("%s: flush"
 							  " retry cmd: %p\n",
 							  current->comm,

diff -ru linux-2.6.15-rc5-git6-patched/include/scsi/scsi_device.h linux-2.6.15-rc5-git6-patched-mdr/include/scsi/scsi_device.h

include/scsi/scsi_device.h +5 -0

@@ -275,6 +275,11 @@
 			    int data_direction, void *buffer, unsigned bufflen,
 			    struct scsi_sense_hdr *, int timeout, int retries);
 
+static inline int scsi_device_blocked(struct scsi_device *sdev)
+{
+	return sdev->sdev_state == SDEV_BLOCK;
+}
+
 static inline unsigned int sdev_channel(struct scsi_device *sdev)
 {
 	return sdev->channel;

This patch looks ok. It's certainly an issue that we (Emulex) have been fighting, with the answer being : always ensure that the device i/o timeout is 2x (+ some fudge?) the devloss_tmo. As we recommend setting the i/o timeout to 60 seconds for enterprise arrays, and as devloss_tmo defaults to 30, this generally worked.

One other point though that should be addressed. This doesn't affect the successive "bigger hammer" recovery steps in scsi_eh_ready_devs(). We should have the function recognize that the scsi_eh_stu(), scsi_eh_bus_device_reset(), scsi_eh_bus_reset() functions can fail due to a cable pull (e.g. devices being blocked), which is ok, and does not require the next level of reset. Causing a host reset because of a temporary cable pull, or a bus reset (all targets on a bus to be reset) because an unrelated one temporarily went away - are not good things.

-- james s

Quoted 83 lines · Michael Reed

As no one has commented, I'm reposting this rfc as a patch. I've been using it for all my testing during development of the LSI MPT Fusion fc transport attribute patch and it has shown no ill effect.

--

Error recovery doesn't interact very well with fc targets which have been blocked by the fc transport. Error recovery continues to attempt to recover the target and ends up marking the fc target offline. Once offline, if the target returns before the remote port is removed, commands which could have been successfully reissued instead are completed with an error status due to the offline status of the target.

This patch makes a couple of hopefully minor tweaks to the error recovery logic to work better with targets which have been blocked by the transport.

First, if the target is blocked and error recovery gives up, don't put the device offline. Either the transport will delete the target thus disposing of any queued requests or it will unblock the target and requests will be reissued.

Second, if a device is blocked, queue up commands being flushed from the done queue for retry instead of completing them with an error.

Thanks,
Mike Reed

------------------------------------------------------------------------

diff -ru linux-2.6.15-rc5-git6-patched/drivers/scsi/scsi_error.c linux-2.6.15-rc5-git6-patched-mdr/drivers/scsi/scsi_error.c

drivers/scsi/scsi_error.c +11 -6

@@ -1130,10 +1130,14 @@
 	struct scsi_cmnd *scmd, *next;
 
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		sdev_printk(KERN_INFO, scmd->device,
-			    "scsi: Device offlined - not"
-			    " ready after error recovery\n");
-		scsi_device_set_state(scmd->device, SDEV_OFFLINE);
+		/* if blocked, transport will provide final device disposition */
+		if (!scsi_device_blocked(scmd->device)) {
+			sdev_printk(KERN_INFO, scmd->device,
+				    "scsi: Device offlined - not"
+				    " ready after error recovery\n");
+			scsi_device_set_state(scmd->device, SDEV_OFFLINE);
+		}
+
 		if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
 			/*
 			 * FIXME: Handle lost cmds.

@@ -1460,9 +1464,10 @@
 
 	list_for_each_entry_safe(scmd, next, done_q, eh_entry) {
 		list_del_init(&scmd->eh_entry);
-		if (scsi_device_online(scmd->device) &&
+		if (scsi_device_blocked(scmd->device) ||
+		    (scsi_device_online(scmd->device) &&
 		    !blk_noretry_request(scmd->request) &&
-		    (++scmd->retries < scmd->allowed)) {
+		    (++scmd->retries < scmd->allowed))) {
 			SCSI_LOG_ERROR_RECOVERY(3, printk("%s: flush"
 							  " retry cmd: %p\n",
 							  current->comm,

diff -ru linux-2.6.15-rc5-git6-patched/include/scsi/scsi_device.h linux-2.6.15-rc5-git6-patched-mdr/include/scsi/scsi_device.h

include/scsi/scsi_device.h +5 -0

@@ -275,6 +275,11 @@
 			    int data_direction, void *buffer, unsigned bufflen,
 			    struct scsi_sense_hdr *, int timeout, int retries);
 
+static inline int scsi_device_blocked(struct scsi_device *sdev)
+{
+	return sdev->sdev_state == SDEV_BLOCK;
+}
+
 static inline unsigned int sdev_channel(struct scsi_device *sdev)
 {
 	return sdev->channel;

Quoted 6 lines · James Smart

This patch looks ok. It's certainly an issue that we (Emulex) have been fighting, with the answer being : always ensure that the device i/o timeout is 2x (+ some fudge?) the devloss_tmo. As we recommend setting the i/o timeout to 60 seconds for enterprise arrays, and as devloss_tmo defaults to 30, this generally worked.

Quoted 8 lines

One other point though that should be addressed. This doesn't affect the successive "bigger hammer" recovery steps in scsi_eh_ready_devs(). We should have the function recognize that the scsi_eh_stu(), scsi_eh_bus_device_reset(), scsi_eh_bus_reset() functions can fail due to a cable pull (e.g. devices being blocked), which is ok, and does not require the next level of reset. Causing a host reset because of a temporary cable pull, or a bus reset (all targets on a bus to be reset) because an unrelated one temporarily went away - are not good things.

Well, a device can go away for reasons other than a cable pull. Sometimes (in my experience with SGI's driver for qlogic fc cards) harder resets can bring it back whereas waiting won't. Think firmware problems....

I think letting the harder resets happen is a good thing (or at least not a bad thing) as long as recovery waits for the driver to report that the drive is gone (offline).

Mike

Quoted 87 lines

-- james s

Quoted 83 lines · Michael Reed

As no one has commented, I'm reposting this rfc as a patch. I've been using it for all my testing during development of the LSI MPT Fusion fc transport attribute patch and it has shown no ill effect.

--

Error recovery doesn't interact very well with fc targets which have been blocked by the fc transport. Error recovery continues to attempt to recover the target and ends up marking the fc target offline. Once offline, if the target returns before the remote port is removed, commands which could have been successfully reissued instead are completed with an error status due to the offline status of the target.

This patch makes a couple of hopefully minor tweaks to the error recovery logic to work better with targets which have been blocked by the transport.

First, if the target is blocked and error recovery gives up, don't put the device offline. Either the transport will delete the target thus disposing of any queued requests or it will unblock the target and requests will be reissued.

Second, if a device is blocked, queue up commands being flushed from the done queue for retry instead of completing them with an error.

Thanks,
Mike Reed

------------------------------------------------------------------------

diff -ru linux-2.6.15-rc5-git6-patched/drivers/scsi/scsi_error.c linux-2.6.15-rc5-git6-patched-mdr/drivers/scsi/scsi_error.c

drivers/scsi/scsi_error.c +11 -6

@@ -1130,10 +1130,14 @@
 	struct scsi_cmnd *scmd, *next;
 
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		sdev_printk(KERN_INFO, scmd->device,
-			    "scsi: Device offlined - not"
-			    " ready after error recovery\n");
-		scsi_device_set_state(scmd->device, SDEV_OFFLINE);
+		/* if blocked, transport will provide final device disposition */
+		if (!scsi_device_blocked(scmd->device)) {
+			sdev_printk(KERN_INFO, scmd->device,
+				    "scsi: Device offlined - not"
+				    " ready after error recovery\n");
+			scsi_device_set_state(scmd->device, SDEV_OFFLINE);
+		}
+
 		if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
 			/*
 			 * FIXME: Handle lost cmds.

@@ -1460,9 +1464,10 @@
 
 	list_for_each_entry_safe(scmd, next, done_q, eh_entry) {
 		list_del_init(&scmd->eh_entry);
-		if (scsi_device_online(scmd->device) &&
+		if (scsi_device_blocked(scmd->device) ||
+		    (scsi_device_online(scmd->device) &&
 		    !blk_noretry_request(scmd->request) &&
-		    (++scmd->retries < scmd->allowed)) {
+		    (++scmd->retries < scmd->allowed))) {
 			SCSI_LOG_ERROR_RECOVERY(3, printk("%s: flush"
 							  " retry cmd: %p\n",
 							  current->comm,

diff -ru linux-2.6.15-rc5-git6-patched/include/scsi/scsi_device.h linux-2.6.15-rc5-git6-patched-mdr/include/scsi/scsi_device.h

include/scsi/scsi_device.h +5 -0

@@ -275,6 +275,11 @@
 			    int data_direction, void *buffer, unsigned bufflen,
 			    struct scsi_sense_hdr *, int timeout, int retries);
 
+static inline int scsi_device_blocked(struct scsi_device *sdev)
+{
+	return sdev->sdev_state == SDEV_BLOCK;
+}
+
 static inline unsigned int sdev_channel(struct scsi_device *sdev)
 {
 	return sdev->channel;

Quoted 3 lines

I think letting the harder resets happen is a good thing (or at least not a bad thing) as long as recovery waits for the driver to report that the drive is gone (offline).

Well, in thinking through this further after my initial reply...

I think we really do want to leave scsi_eh_ready_devs() logic with the bigger hammer steps alone. Ultimately, they are trying to regain the resources for an i/o that is trying to be killed but the LLDD (or device) isn't cooperating. I still believe in not resetting everyone just because a device is temporarily blocked. However, we need to intercept it at a earlier point... Ultimately, to reach this path, it starts with an i/o timing out, and the eh_abort handler failing. In Emulex's case, we are planning on never failing the eh_abort handler if we're in this temporarily blocked state, even at the expense of a long wait. This is actually too much to ask of an LLDD - and is hokey. The logic really should be to intercept the timeout handler, note that the device is blocked, and delay the abort request until the device has been given a chance to return (e.g. just restart the i/o abort timer for the amount of devloss_tmo that remains). Otherwise, we're always guaranteeing a failure from the abort handlers (for i/o and device) as there's no device to talk to.

This should remove the need for your if-blocked test in scsi_error.c, replacing it with the logic in the i/o timeout handler.

Quoted 20 lines · James Smart

Quoted 3 lines

I think letting the harder resets happen is a good thing (or at least not a bad thing) as long as recovery waits for the driver to report that the drive is gone (offline).

Well, in thinking through this further after my initial reply...

I think we really do want to leave scsi_eh_ready_devs() logic with the bigger hammer steps alone. Ultimately, they are trying to regain the resources for an i/o that is trying to be killed but the LLDD (or device) isn't cooperating. I still believe in not resetting everyone just because a device is temporarily blocked. However, we need to intercept it at a earlier point... Ultimately, to reach this path, it starts with an i/o timing out, and the eh_abort handler failing. In Emulex's case, we are planning on never failing the eh_abort handler if we're in this temporarily blocked state, even at the expense of a long wait. This is actually too much to ask of an LLDD - and is hokey. The logic really should be to intercept the timeout handler, note that the device is blocked, and delay the abort request until the device has been given a chance to return (e.g. just restart the i/o abort timer for the amount of devloss_tmo that remains). Otherwise, we're always guaranteeing a failure from the abort handlers (for i/o and device) as there's no device to talk to.

Quoted 2 lines

This should remove the need for your if-blocked test in scsi_error.c, replacing it with the logic in the i/o timeout handler.

Actually, there is another thing you can do even earlier: implement scsi_eh_timer_return() in the host template (probably with a generic routine from the fc class). This would allow you to hold off the timeout at least for the length of the user specified timeout and all the retries. Probably the routine would simply check to see if the device is in a devloss timeout and if it is return EH_RESET_TIMER; otherwise return EH_NOT_HANDLED.

James

I'm planning on working on this once the fusion fc_transport code finally makes it in. What do you see as a deadline for code addressing this problem?

Perhaps someone else already has something to address this in progress? It's a significant issue with our fibre channel customers.

Quoted 5 lines · Michael Reed

I'm planning on working on this once the fusion fc_transport code finally makes it in. What do you see as a deadline for code addressing this problem?

Perhaps someone else already has something to address this in progress? It's a significant issue with our fibre channel customers.

There's not at all. Sunday is feature deadline for 2.6.16, but we'll happily accept everything good for 2.6.17. OTOH this probably falls into fixes that can go in after -rc1 anyway.