Requested changes for the SCSI error handler

Requested changes for the SCSI error handler

[Alan Stern]

Here are two things I'd like to see changed in the error handler, if 
possible.

	The recovery xxx_RESET_SETTLE_TIME delays in scsi_sleep() can not 
	be interrupted, even if the state changes to SDEV_CANCEL or 
	SHOST_CANCEL.  It would be nice if the delay could be cut short
	so that scsi_remove_host() doesn't have to wait.  Lengthy pauses
	during disconnect processing are bad for the USB hub driver.

	In scsi_eh_ready_devs(), it would be good if some of the resets
	could be skipped sometimes.  For example, if the low-level
	driver has just done its own bus-device reset (and called
	scsi_report_device_reset) then there's no point in doing 
	scsi_eh_bus_device_reset().  The same is true for bus resets.
	It just adds additional timeout delays to an already-lengthy
	recovery process.

Do these things seem feasible?

Alan Stern

Re: Requested changes for the SCSI error handler

[James Bottomley]

On Tue, 2004-06-01 at 13:50, Alan Stern wrote:
> Here are two things I'd like to see changed in the error handler, if 
> possible.
> 
> 	The recovery xxx_RESET_SETTLE_TIME delays in scsi_sleep() can not 
> 	be interrupted, even if the state changes to SDEV_CANCEL or 
> 	SHOST_CANCEL.  It would be nice if the delay could be cut short
> 	so that scsi_remove_host() doesn't have to wait.  Lengthy pauses
> 	during disconnect processing are bad for the USB hub driver.

Really, no.  The settle time is not transport independent, so it would
be very difficult to implement stomething like this in the mid-layer.

What about simply doing the settle in the eh_.. routines (you block the
host, schedule_timeout() for your settle time then unblock the host and
return to the eh thread)?

> 	In scsi_eh_ready_devs(), it would be good if some of the resets
> 	could be skipped sometimes.  For example, if the low-level
> 	driver has just done its own bus-device reset (and called
> 	scsi_report_device_reset) then there's no point in doing 
> 	scsi_eh_bus_device_reset().  The same is true for bus resets.
> 	It just adds additional timeout delays to an already-lengthy
> 	recovery process.

Well, tidying up the reports could be done.  Really we should treat them
as error conditions: mark all the in-progress commands for the devices
and probe with a TUR before resuming.

James

Re: Requested changes for the SCSI error handler

[Alan Stern]

On 1 Jun 2004, James Bottomley wrote:

> On Tue, 2004-06-01 at 13:50, Alan Stern wrote:
> > 
> > 	The recovery xxx_RESET_SETTLE_TIME delays in scsi_sleep() can not 
> > 	be interrupted, even if the state changes to SDEV_CANCEL or 
> > 	SHOST_CANCEL.  It would be nice if the delay could be cut short
> > 	so that scsi_remove_host() doesn't have to wait.  Lengthy pauses
> > 	during disconnect processing are bad for the USB hub driver.
> 
> Really, no.  The settle time is not transport independent, so it would
> be very difficult to implement stomething like this in the mid-layer.
> 
> What about simply doing the settle in the eh_.. routines (you block the
> host, schedule_timeout() for your settle time then unblock the host and
> return to the eh thread)?

You mean, in the hostt->eh_{bus|host}_reset_handler() routines?  That
would be fine with me.  Isn't it true that we don't even have to block the
host, since this all executes in the error-handler thread?

In addition, the settle-time delays would have to be removed from the
error handler -- which means adding it to all the low-level drivers.  Is 
that doable?


> > 	In scsi_eh_ready_devs(), it would be good if some of the resets
> > 	could be skipped sometimes.  For example, if the low-level
> > 	driver has just done its own bus-device reset (and called
> > 	scsi_report_device_reset) then there's no point in doing 
> > 	scsi_eh_bus_device_reset().  The same is true for bus resets.
> > 	It just adds additional timeout delays to an already-lengthy
> > 	recovery process.
> 
> Well, tidying up the reports could be done.  Really we should treat them
> as error conditions: mark all the in-progress commands for the devices
> and probe with a TUR before resuming.

In practice the LLD-initiated resets are likely to accompany command 
failures or errors anyway.  But this doesn't answer my question: Can the 
error-handler's redundant resets be skipped?

Alan Stern

Re: Requested changes for the SCSI error handler

[James Bottomley]

On Tue, 2004-06-01 at 15:29, Alan Stern wrote:
> You mean, in the hostt->eh_{bus|host}_reset_handler() routines?  That
> would be fine with me.  Isn't it true that we don't even have to block the
> host, since this all executes in the error-handler thread?

Actually yes.  The block would only have to be implemented in the
asynchronous report path.

> In addition, the settle-time delays would have to be removed from the
> error handler -- which means adding it to all the low-level drivers.  Is 
> that doable?

Well, for 2.6, I think that a simple flag indicating that the driver
will implement it's own timeout should suffice rather than altering
every LLD...

> > > 	In scsi_eh_ready_devs(), it would be good if some of the resets
> > > 	could be skipped sometimes.  For example, if the low-level
> > > 	driver has just done its own bus-device reset (and called
> > > 	scsi_report_device_reset) then there's no point in doing 
> > > 	scsi_eh_bus_device_reset().  The same is true for bus resets.
> > > 	It just adds additional timeout delays to an already-lengthy
> > > 	recovery process.
> > 
> > Well, tidying up the reports could be done.  Really we should treat them
> > as error conditions: mark all the in-progress commands for the devices
> > and probe with a TUR before resuming.
> 
> In practice the LLD-initiated resets are likely to accompany command 
> failures or errors anyway.  But this doesn't answer my question: Can the 
> error-handler's redundant resets be skipped?

Well, no.  The reason is that the report interfaces are reporting
asynchronous events (that the HBA may notice some while after they
actually occurred).  Even if the host reports a device reset, it is very
possible that a command went to the device *after* that event occurred,
so we'd still have to reset the device again to kill that command.

James

Re: Requested changes for the SCSI error handler

[Alan Stern]

On 1 Jun 2004, James Bottomley wrote:

> On Tue, 2004-06-01 at 15:29, Alan Stern wrote:
> > You mean, in the hostt->eh_{bus|host}_reset_handler() routines?  That
> > would be fine with me.  Isn't it true that we don't even have to block the
> > host, since this all executes in the error-handler thread?
> 
> Actually yes.  The block would only have to be implemented in the
> asynchronous report path.
> 
> > In addition, the settle-time delays would have to be removed from the
> > error handler -- which means adding it to all the low-level drivers.  Is 
> > that doable?
> 
> Well, for 2.6, I think that a simple flag indicating that the driver
> will implement it's own timeout should suffice rather than altering
> every LLD...

How does this look?

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

===== include/scsi/scsi_host.h 1.8 vs edited =====
--- 1.8/include/scsi/scsi_host.h	Fri Mar 12 03:16:47 2004
+++ edited/include/scsi/scsi_host.h	Fri Jun  4 16:51:31 2004
@@ -314,6 +314,11 @@
 	unsigned emulated:1;
 
 	/*
+	 * True if the low-level driver performs its own reset-settle delays.
+	 */
+	unsigned skip_settle_delay:1;
+
+	/*
 	 * Countdown for host blocking with no commands outstanding
 	 */
 	unsigned int max_host_blocked;
===== drivers/scsi/scsi_error.c 1.41 vs edited =====
--- 1.41/drivers/scsi/scsi_error.c	Tue Apr 20 18:22:11 2004
+++ edited/drivers/scsi/scsi_error.c	Fri Jun  4 16:54:06 2004
@@ -1026,7 +1026,8 @@
 	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	if (rtn == SUCCESS) {
-		scsi_sleep(BUS_RESET_SETTLE_TIME);
+		if (!scmd->device->host->hostt->skip_settle_delay)
+			scsi_sleep(BUS_RESET_SETTLE_TIME);
 		spin_lock_irqsave(scmd->device->host->host_lock, flags);
 		scsi_report_bus_reset(scmd->device->host, scmd->device->channel);
 		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
@@ -1057,7 +1058,8 @@
 	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	if (rtn == SUCCESS) {
-		scsi_sleep(HOST_RESET_SETTLE_TIME);
+		if (!scmd->device->host->hostt->skip_settle_delay)
+			scsi_sleep(HOST_RESET_SETTLE_TIME);
 		spin_lock_irqsave(scmd->device->host->host_lock, flags);
 		scsi_report_bus_reset(scmd->device->host, scmd->device->channel);
 		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
===== drivers/usb/storage/scsiglue.c 1.75 vs edited =====
--- 1.75/drivers/usb/storage/scsiglue.c	Sat May 15 11:48:19 2004
+++ edited/drivers/usb/storage/scsiglue.c	Fri Jun  4 16:52:56 2004
@@ -409,6 +409,9 @@
 	/* emulated HBA */
 	.emulated =			TRUE,
 
+	/* we do our own delay after a device or bus reset */
+	.skip_settle_delay =		1,
+
 	/* sysfs device attributes */
 	.sdev_attrs =			sysfs_device_attr_list,
 

Re: Requested changes for the SCSI error handler

[Mike Anderson]

Alan Stern [stern@rowland.harvard.edu] wrote:
> 
> How does this look?
> 

Looks ok. I know your patch is in line with James requests, but I had a
counter idea that allows for setting the settle delays up, down, or to
zero post scsi_host_alloc. If the LLDD does nothing behavior should not
change. This would allow increasing settle if some LLDD wanted it and
also set it through sysfs if we wanted that in the future.  You can
throw this patch in the bit bucket if you want to go the previously
stated direction.

-andmike
--
Michael Anderson
andmike@us.ibm.com

DESC
Make bus reset and host reset settle times adjustable.

Signed-off-by: Mike Anderson <andmike@us.ibm.com>
EDESC


 patched-2.6-andmike/drivers/scsi/hosts.c      |    8 ++++++++
 patched-2.6-andmike/drivers/scsi/scsi_error.c |    7 +++++--
 patched-2.6-andmike/include/scsi/scsi_host.h  |    4 ++++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff -puN include/scsi/scsi_host.h~host_settle include/scsi/scsi_host.h
--- patched-2.6/include/scsi/scsi_host.h~host_settle	Fri Jun  4 00:14:29 2004
+++ patched-2.6-andmike/include/scsi/scsi_host.h	Fri Jun  4 00:21:02 2004
@@ -485,6 +485,10 @@ struct Scsi_Host {
 	 */
 	struct list_head sht_legacy_list;
 
+	/* settle times */
+	int b_rst_settle_time;
+	int h_rst_settle_time;
+
 	/*
 	 * We should ensure that this is aligned, both for better performance
 	 * and also because some compilers (m68k) don't automatically force
diff -puN drivers/scsi/scsi_error.c~host_settle drivers/scsi/scsi_error.c
--- patched-2.6/drivers/scsi/scsi_error.c~host_settle	Fri Jun  4 00:17:53 2004
+++ patched-2.6-andmike/drivers/scsi/scsi_error.c	Fri Jun  4 00:20:37 2004
@@ -1026,7 +1026,7 @@ static int scsi_try_bus_reset(struct scs
 	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	if (rtn == SUCCESS) {
-		scsi_sleep(BUS_RESET_SETTLE_TIME);
+		scsi_sleep(scmd->device->host->b_rst_settle_time);
 		spin_lock_irqsave(scmd->device->host->host_lock, flags);
 		scsi_report_bus_reset(scmd->device->host, scmd->device->channel);
 		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
@@ -1057,7 +1057,7 @@ static int scsi_try_host_reset(struct sc
 	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	if (rtn == SUCCESS) {
-		scsi_sleep(HOST_RESET_SETTLE_TIME);
+		scsi_sleep(scmd->device->host->h_rst_settle_time);
 		spin_lock_irqsave(scmd->device->host->host_lock, flags);
 		scsi_report_bus_reset(scmd->device->host, scmd->device->channel);
 		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
@@ -1218,6 +1218,9 @@ void scsi_sleep(int timeout)
 {
 	DECLARE_MUTEX_LOCKED(sem);
 	struct timer_list timer;
+
+	if (!timeout)
+		return;
 
 	init_timer(&timer);
 	timer.data = (unsigned long)&sem;
diff -puN drivers/scsi/hosts.c~host_settle drivers/scsi/hosts.c
--- patched-2.6/drivers/scsi/hosts.c~host_settle	Fri Jun  4 00:21:33 2004
+++ patched-2.6-andmike/drivers/scsi/hosts.c	Fri Jun  4 14:36:07 2004
@@ -38,6 +38,12 @@
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
+/*
+ *  * These should *probably* be handled by the host itself.
+ *   * Since it is allowed to sleep, it probably should.
+ *    */
+#define BUS_RESET_SETTLE_TIME   10*HZ
+#define HOST_RESET_SETTLE_TIME  10*HZ
 
 static int scsi_host_next_hn;		/* host_no for next new host */
 
@@ -280,6 +286,8 @@ struct Scsi_Host *scsi_host_alloc(struct
 	snprintf(shost->shost_classdev.class_id, BUS_ID_SIZE, "host%d",
 		  shost->host_no);
 
+	shost->b_rst_settle_time = BUS_RESET_SETTLE_TIME;
+	shost->h_rst_settle_time = HOST_RESET_SETTLE_TIME;
 	shost->eh_notify = &complete;
 	rval = kernel_thread(scsi_error_handler, shost, 0);
 	if (rval < 0)

_

Re: Requested changes for the SCSI error handler

[Alan Stern]

On Fri, 4 Jun 2004, Mike Anderson wrote:

> Alan Stern [stern@rowland.harvard.edu] wrote:
> > 
> > How does this look?
> > 
> 
> Looks ok. I know your patch is in line with James requests, but I had a
> counter idea that allows for setting the settle delays up, down, or to
> zero post scsi_host_alloc. If the LLDD does nothing behavior should not
> change. This would allow increasing settle if some LLDD wanted it and
> also set it through sysfs if we wanted that in the future.  You can
> throw this patch in the bit bucket if you want to go the previously
> stated direction.

Either approach is fine with me.  James?

Alan Stern

Re: Requested changes for the SCSI error handler

[James Bottomley]

On Fri, 2004-06-04 at 18:23, Mike Anderson wrote:
> Looks ok. I know your patch is in line with James requests, but I had a
> counter idea that allows for setting the settle delays up, down, or to
> zero post scsi_host_alloc. If the LLDD does nothing behavior should not
> change. This would allow increasing settle if some LLDD wanted it and
> also set it through sysfs if we wanted that in the future.  You can
> throw this patch in the bit bucket if you want to go the previously
> stated direction.

Well, how about a compromise?  I like the ability to alter the host
settle time; however, I think turning these over to the driver to sort
out how it pleases gives the LLD more control in this situation,
certainly for the HOST reset.

I'm less sure that a bus reset timeout belongs in the LLD, but I suppose
that could be pulled into the SPI transport class eventually.

So, what about Alan's approach using a single flag to give the LLD
control, but give it control for both the host and bus resets?

James

Re: Requested changes for the SCSI error handler

[Mike Anderson]

James Bottomley [James.Bottomley@SteelEye.com] wrote:
> So, what about Alan's approach using a single flag to give the LLD
> control, but give it control for both the host and bus resets?
> 

This sounds good.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Requested changes for the SCSI error handler

[Alan Stern]

On 8 Jun 2004, James Bottomley wrote:

> Well, how about a compromise?  I like the ability to alter the host
> settle time; however, I think turning these over to the driver to sort
> out how it pleases gives the LLD more control in this situation,
> certainly for the HOST reset.
> 
> I'm less sure that a bus reset timeout belongs in the LLD, but I suppose
> that could be pulled into the SPI transport class eventually.
> 
> So, what about Alan's approach using a single flag to give the LLD
> control, but give it control for both the host and bus resets?

Sorry to appear slow... but isn't that exactly what my patch did?

Alan Stern

Re: Requested changes for the SCSI error handler

[Mike Anderson]

Alan Stern [stern@rowland.harvard.edu] wrote:
> On 8 Jun 2004, James Bottomley wrote:
> 
> > Well, how about a compromise?  I like the ability to alter the host
> > settle time; however, I think turning these over to the driver to sort
> > out how it pleases gives the LLD more control in this situation,
> > certainly for the HOST reset.
> > 
> > I'm less sure that a bus reset timeout belongs in the LLD, but I suppose
> > that could be pulled into the SPI transport class eventually.
> > 
> > So, what about Alan's approach using a single flag to give the LLD
> > control, but give it control for both the host and bus resets?
> 
> Sorry to appear slow... but isn't that exactly what my patch did?

Yes. That is what I thought you sent. 

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Requested changes for the SCSI error handler

[James Bottomley]

On Tue, 2004-06-08 at 14:31, Alan Stern wrote:
> Sorry to appear slow... but isn't that exactly what my patch did?

I'm sorry...travel is short circuiting my brain; I didn't notice the
host reset bits.

Anyway, the patch is in scsi-misc-2.6

James