Skip to contentSkip to conversation

scsi: Add EH Start Unit retry

PATCH Douglas Gilbert 4 participants 6 messages Mar 29, 2007 – Apr 3, 2007 1 message not in the archive SCSI

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 11751999513070-patch-mail.ibm.com

Douglas Gilbert

author
#1 lore raw
Quoted 10 lines · Brian King

Currently, the scsi error handler will issue a START_UNIT command if the drive indicates it needs its motor started and the allow_restart flag is set in the scsi_device. If, after the scsi error handler invokes a host adapter reset due to error recovery, a device is in a unit attention state AND also needs a START_UNIT, that device will be placed offline. The disk array devices on an ipr RAID adapter will do exactly this when in a dual initiator configuration. This patch adds a single retry to the EH initiated START_UNIT.

I have no objection to this patch. Just seems a pity that SCSI devices go to the trouble of sending unit attentions while OSes just throw them away.

Perhaps the scsi_device sysfs directory could have entries like:
last_ua_asc
last_ua_ascq
last_ua_timestamp
where code could place the asc/ascq codes and a timestamp then continue doing a retry.
Could we get a log entry, hotplug event?

Logical units may queue unit attentions (sam4r10.pdf section 5.8.7) so it is possible that one retry may not be enough. With my suggestion above, only the last one would persist for a reasonable time.

Doug Gilbert

#2 lore raw
Quoted 12 lines · Douglas Gilbert
Quoted 10 lines · Brian King

Currently, the scsi error handler will issue a START_UNIT command if the drive indicates it needs its motor started and the allow_restart flag is set in the scsi_device. If, after the scsi error handler invokes a host adapter reset due to error recovery, a device is in a unit attention state AND also needs a START_UNIT, that device will be placed offline. The disk array devices on an ipr RAID adapter will do exactly this when in a dual initiator configuration. This patch adds a single retry to the EH initiated START_UNIT.

Quoted 3 lines

I have no objection to this patch. Just seems a pity that SCSI devices go to the trouble of sending unit attentions while OSes just throw them away.

I agree. The reason the ipr adapter firmware added this UA in this configuration was to support SCSI 1 reservations and communicate to the host that any reservation previously held to the disk array is now lost since the adapter was reset.

Quoted 8 lines

Perhaps the scsi_device sysfs directory could have entries like:
last_ua_asc
last_ua_ascq
last_ua_timestamp
where code could place the asc/ascq codes and a timestamp then continue doing a retry.
Could we get a log entry, hotplug event?

If we did have a way to communicate UA's to userspace like this it seems like it would allow usage of SCSI 1 reservations in this config and make it easier for an out of band tool to manage these reservations. I wonder if it would be cleaner if UAs could simply be sent up as netlink events / uevents, so they could contain all the information needed in one packet, rather than having to read sysfs attributes to figure out what happened.

Quoted 4 lines

Logical units may queue unit attentions (sam4r10.pdf section 5.8.7) so it is possible that one retry may not be enough. With my suggestion above, only the last one would persist for a reasonable time.

Yep. I've already ran into that with dual ported SAS devices. While one retry is sufficient for the ipr disk array devices I am trying to fix this for, I have no objection to increasing it. Maybe its just a case of increasing it later if it ends up being an issue.

Brian

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
#3 lore raw
Quoted 4 lines · Brian King

I agree. The reason the ipr adapter firmware added this UA in this configuration was to support SCSI 1 reservations and communicate to the host that any reservation previously held to the disk array is now lost since the adapter was reset.

For that particular case, scsi_report_bus_reset() was the original design concept ... it's asynchronous when the problem occurs ... if you have to wait until UA, you don't necessarily find there's a problem until much later.

James

#4 lore raw
Quoted 10 lines · James Bottomley
Quoted 4 lines · Brian King

I agree. The reason the ipr adapter firmware added this UA in this configuration was to support SCSI 1 reservations and communicate to the host that any reservation previously held to the disk array is now lost since the adapter was reset.

For that particular case, scsi_report_bus_reset() was the original design concept ... it's asynchronous when the problem occurs ... if you have to wait until UA, you don't necessarily find there's a problem until much later.

Right. One of the ipr patches in my queue is to call scsi_report_bus_reset for the logical disk array bus after the adapter is reset, so that scsi core knows about this reset and it can handle the UA, although this patch is obviously still needed even with that change...

Brian

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
#5 lore raw
In reply to a message not in the archive
Quoted 8 lines · Brian King
(quoted diff) +3 -2
-		int rtn;
+		int retry_cnt = 1, rtn;
 
+retry_stu:
 		rtn = scsi_send_eh_cmnd(scmd, stu_command, 6,
 					START_UNIT_TIMEOUT, 0);
-		if (rtn == SUCCESS)
+

Review

Quoted 10 lines
(quoted diff) +9 -0
+		switch (rtn) {
+		case SUCCESS:
 			return 0;
+		case NEEDS_RETRY:
+			if (retry_cnt--)
+				goto retry_stu;
+			/*FALLTHRU*/
+		default:
+			return 1;
+		}

This is pretty much an open coded for loop ... how about just doing a for loop as attached below?

The two advantages to this are:

1. it's cleaner and more readable
2. the compiler knows how to optimize it better

James

drivers/scsi/scsi_error.c +5 -3
index 7a1a1bb..28a266c 100644
@@ -932,10 +932,12 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
 	static unsigned char stu_command[6] = {START_STOP, 0, 0, 0, 1, 0};
 
 	if (scmd->device->allow_restart) {
-		int rtn;
+		int i, rtn = NEEDS_RETRY;
+
+		for (i = 0; rtn == NEEDS_RETRY && i < 2; i++)
+			rtn = scsi_send_eh_cmnd(scmd, stu_command, 6,
+						START_UNIT_TIMEOUT, 0);
 
-		rtn = scsi_send_eh_cmnd(scmd, stu_command, 6,
-					START_UNIT_TIMEOUT, 0);
 		if (rtn == SUCCESS)
 			return 0;
 	}
#6 lore raw
Quoted 3 lines · James Bottomley

On Thu, 2007-03-29 at 15:25 -0500, Brian King wrote:
This is pretty much an open coded for loop ... how about just doing a for loop as attached below?

Fine by me. I was simply coding it up to look like scsi_eh_tur.

Thanks,

Brian

Quoted 30 lines

The two advantages to this are:

1. it's cleaner and more readable
2. the compiler knows how to optimize it better

James

drivers/scsi/scsi_error.c +5 -3
index 7a1a1bb..28a266c 100644
@@ -932,10 +932,12 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
 	static unsigned char stu_command[6] = {START_STOP, 0, 0, 0, 1, 0};
 
 	if (scmd->device->allow_restart) {
-		int rtn;
+		int i, rtn = NEEDS_RETRY;
+
+		for (i = 0; rtn == NEEDS_RETRY && i < 2; i++)
+			rtn = scsi_send_eh_cmnd(scmd, stu_command, 6,
+						START_UNIT_TIMEOUT, 0);
 
-		rtn = scsi_send_eh_cmnd(scmd, stu_command, 6,
-					START_UNIT_TIMEOUT, 0);
 		if (rtn == SUCCESS)
 			return 0;
 	}
Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center