RE: [PATCH]: Flexible timeout infrastructure

RE: [PATCH]: Flexible timeout infrastructure

[Smart, James]

Sorry, guess I wasn't clear.

reset - meant timer reset. And I completely follow that the command is not
being retried, etc, just the timeout restarted.

I'll try to restate. The patch proposed to allow only 1 restart of the timer
for a command. My comment was - why not 2, or 3?  I would think - the number
of restarts needed is a function of how long the timeout value is vs when
the LLDD event occurs and what the duration of the LLDD event is. I can see
capping the number of timer restarts, but am not sure 1 is the best choice. 

-- james

RE: [PATCH]: Flexible timeout infrastructure

[James Bottomley]

On Wed, 2004-06-16 at 12:33, Smart, James wrote:
> I'll try to restate. The patch proposed to allow only 1 restart of the timer
> for a command. My comment was - why not 2, or 3?  I would think - the number
> of restarts needed is a function of how long the timeout value is vs when
> the LLDD event occurs and what the duration of the LLDD event is. I can see
> capping the number of timer restarts, but am not sure 1 is the best choice. 

It does.  But the number of allowed restarts is linked to the command
retries (actually allowed retries + 1).

James

RE: [PATCH]: Flexible timeout infrastructure

[Smart, James]

So the jist isn't to get the command to survive the LLDD event, even if it
could. Rather, the user has set aside X amount of time (where X = #retries *
timeout length), and you want to return the command to the user as close to
X as possible. And there are 2 further expectations: you are willing to
suffer the failure of non-retryable commands that may be forced to fail; and
you expect the LLDDs to call the midlayer to block further commands if the
LLDD event persists. (Note: this is where I think more granularity could be
used - instead of host-global events, there may be target-specific events as
well).

Ok.  Just trying to get on the same page...

-- james

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> Sent: Wednesday, June 16, 2004 1:38 PM
> To: Smart, James
> Cc: Luben Tuikov; Mike Anderson; SCSI Mailing List
> Subject: RE: [PATCH]: Flexible timeout infrastructure
> 
> 
> On Wed, 2004-06-16 at 12:33, Smart, James wrote:
> > I'll try to restate. The patch proposed to allow only 1 
> restart of the timer
> > for a command. My comment was - why not 2, or 3?  I would 
> think - the number
> > of restarts needed is a function of how long the timeout 
> value is vs when
> > the LLDD event occurs and what the duration of the LLDD 
> event is. I can see
> > capping the number of timer restarts, but am not sure 1 is 
> the best choice. 
> 
> It does.  But the number of allowed restarts is linked to the command
> retries (actually allowed retries + 1).
> 
> James
> 
> 

RE: [PATCH]: Flexible timeout infrastructure

[Smart, James]

I can appreciate the protectionist point of view. My only contention is -
how do you really derive that 1 reset is any less valid than 2 ? it totally
depends on what the original timeout was relative to whatever the duration
is for the LLDD event that is requesting the reset. 

-- James

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> Sent: Wednesday, June 16, 2004 1:04 PM
> To: Smart, James
> Cc: Luben Tuikov; Mike Anderson; SCSI Mailing List
> Subject: RE: [PATCH]: Flexible timeout infrastructure
> 
> 
> On Wed, 2004-06-16 at 11:58, Smart, James wrote:
> > So why increment the retries counter at all ? 
> 
> So drivers can't return EH_RESET_TIMER forever. This really ties the
> handling into the behaviour the user has requested.  The only wrinkle
> was the no retry streaming commands.  The assumption is that a retry
> doesn't actually happen under eh_timed_out, but only an attempt to
> collect an existing command which may be delayed because of 
> transport or
> other problems the host knows about.
> 
> James
> 
> 

RE: [PATCH]: Flexible timeout infrastructure

[James Bottomley]

On Wed, 2004-06-16 at 12:10, Smart, James wrote:
> I can appreciate the protectionist point of view. My only contention is -
> how do you really derive that 1 reset is any less valid than 2 ? it totally
> depends on what the original timeout was relative to whatever the duration
> is for the LLDD event that is requesting the reset. 

Who said anything about reset?

This is just early notification of something going wrong.  If the
command needs to be retried (or an event that throws the command away is
to be triggered), that's done using the EH_NOT_HANDLED and activating
the eh thread.

I'm not wedded to the current method of handling retries.  It would be
feasible to use a more normal count to not allow non-retryable commands
to be delayed.

James

RE: [PATCH]: Flexible timeout infrastructure

[Smart, James]

> +		case EH_RESET_TIMER:
> +			/* This allows a single retry even of a command
> +			 * with allowed == 0 */
> +			if (scmd->retries++ > scmd->allowed)
> +				break;
> +			scsi_add_timer(scmd, scmd->timeout_per_command,
> +				       scsi_times_out);
> +			return;

So why increment the retries counter at all ? 

RE: [PATCH]: Flexible timeout infrastructure

[James Bottomley]

On Wed, 2004-06-16 at 11:58, Smart, James wrote:
> So why increment the retries counter at all ? 

So drivers can't return EH_RESET_TIMER forever. This really ties the
handling into the behaviour the user has requested.  The only wrinkle
was the no retry streaming commands.  The assumption is that a retry
doesn't actually happen under eh_timed_out, but only an attempt to
collect an existing command which may be delayed because of transport or
other problems the host knows about.

James

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

>  > So why increment the retries counter at all ?
> 
> So drivers can't return EH_RESET_TIMER forever. This really ties the

EH_RESET_TIMER means "This command had nothing to do with why
its timer timed out and is expected to complete shortly."

Do we want to increment its retry counter for this,
as it isn't really a retry.

> handling into the behaviour the user has requested.  The only wrinkle
> was the no retry streaming commands.  The assumption is that a retry

They also fall in the same category -- the application client cannot
retry them, but the LLDD knows best and it may perform some magic
for which SCSI Core shouldn't account for.

> doesn't actually happen under eh_timed_out, but only an attempt to
> collect an existing command which may be delayed because of transport or
> other problems the host knows about.

Can we get a finalised patch so we can test it?
James, is the one you posted what you'll have in?

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[James Bottomley]

On Wed, 2004-06-16 at 13:58, Luben Tuikov wrote:
> EH_RESET_TIMER means "This command had nothing to do with why
> its timer timed out and is expected to complete shortly."
> 
> Do we want to increment its retry counter for this,
> as it isn't really a retry.

It's a simple way to ensure the interface doesn't get abused.  As long
as the command is truly completed, there won't be a problem.

> > handling into the behaviour the user has requested.  The only wrinkle
> > was the no retry streaming commands.  The assumption is that a retry
> 
> They also fall in the same category -- the application client cannot
> retry them, but the LLDD knows best and it may perform some magic
> for which SCSI Core shouldn't account for.

Yes, that's why the proposal permits a single retry of an ostensibly
non-retryable command.

> > doesn't actually happen under eh_timed_out, but only an attempt to
> > collect an existing command which may be delayed because of transport or
> > other problems the host knows about.
> 
> Can we get a finalised patch so we can test it?
> James, is the one you posted what you'll have in?

I need to add comments and things, it can probably be in scsi-misc-2.6
in a day or so.

James

[PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

Hello,

This patch introduces a flexible command timeout infrastructure
accomodating completely current behaviour SCSI Core command timeout,
but also offering the ability to hand command timeout handling to
a LLDD, _yet_ still have it all go through *SCSI Core*.

This is somewhat similar to Christoph's proposal patch, but I wasn't
aware of his patch when I thought of this. Interestingly enough it can be
viewed as an extension to his patch.

The patch is very short (minimal chages indeed), but the text a bit
lenghty since it outlines the many different a usage of such an
infrastructure.

New method: 
  struct scsi_host_template :: int (*eh_cmd_timed_out)(struct scsi_cmnd *);
This method is marked as OPTIONAL status.

If the driver does not define eh_cmd_timed_out() method in its
host template, then current behaviour is assumed.

If the driver does define it, then SCSI Core will give the command
to LLDD without a timer running.  Then when the LLDD notifies the
transport and before it sends it to the interconnect it start
the timer *by calling SCSI Core*, scsi_add_timer(<cmd>,<timeout>,scsi_times_out).

Timeline:
---+-------------------------+-------------> t
   t0                        t1           
SCSI Core::queuecommand(), LLDD::scsi_add_timer(,,scsi_times_out)

NB: |t1-t0| is *always* bounded! (hint)

SUCCESS: If the command completes, LLDD calls
scsi_delete_timer(<cmd>), and then scsi_done().  This completes
the SCSI command transaction.

TO: If the command times out, then *SCSI Core* is called,
scsi_times_out(), which calls the defined, eh_cmd_timed_out()
method.

RETURNS: eh_cmd_timed_out() returns:
  EH_HANDLED:
          - LLDD has called scsi_done() having set the status/response
            codes appropriately,
       XOR
          - resent the command back to the LU.
    Either way, there's nothing to do at this point and the timer
    routine completes.

  EH_NONE:
          - SCSI Core should do error handling as usual.

Note that after eh_cmd_timed_out() returns, we are in _consistent_
state:
- either the command is back out to the LU: timer
has been rerun by the LLDD, the command belongs to LLDD,
SCSI Core should do nothing -- just the same as if it never fired,
XOR
- scsi_done() is called and SCSI Core will
be processing the command shortly, no timer is running
since it fired (since we're executing in this method).
Consistent as if the command completed.
Or we returned EH_NONE, and SCSI Core adds it to the
eh list for recovery.

Let me know your comments, USB, FC, SAS, iSCSI device driver
writers.  The main point of this patch is complicated
_recovery_ of protocols for which SCSI Core cannot have
internal knowlege, but will get notified *at every step*.
I.e. SAM like.

Note that the driver may also decide to do some processing
in the eh_cmd_timed_out() method (interrupt context) and still
return EH_NONE, to get back at that command in process context
from the eh tread (i.e. if TMF ABORT TASK takes some time to return).

Against scsi-misc-2.6:

===== drivers/scsi/scsi.c 1.143 vs edited =====
--- 1.143/drivers/scsi/scsi.c	2004-04-28 12:32:09 -04:00
+++ edited/drivers/scsi/scsi.c	2004-06-14 15:13:12 -04:00
@@ -549,7 +549,12 @@
 		host->resetting = 0;
 	}
 
-	scsi_add_timer(cmd, cmd->timeout_per_command, scsi_times_out);
+	/* If the LLDD claims to be able to handle its own command timeout,
+	 * don't start the timer.  It will start it before sending the command
+	 * to the LU.
+	 */
+	if (!host->hostt->eh_cmd_timed_out)
+		scsi_add_timer(cmd, cmd->timeout_per_command, scsi_times_out);
 
 	scsi_log_send(cmd);
 
@@ -699,8 +704,9 @@
 	 * that function could really be.  It might be on another processor,
 	 * etc, etc.
 	 */
-	if (!scsi_delete_timer(cmd))
-		return;
+	if (!cmd->device->host->hostt->eh_cmd_timed_out)
+		if (!scsi_delete_timer(cmd))
+			return;
 
 	/*
 	 * Set the serial numbers back to zero
===== drivers/scsi/scsi_error.c 1.76 vs edited =====
--- 1.76/drivers/scsi/scsi_error.c	2004-06-04 12:54:06 -04:00
+++ edited/drivers/scsi/scsi_error.c	2004-06-14 15:13:53 -04:00
@@ -155,8 +155,8 @@
 }
 
 /**
- * scsi_times_out - Timeout function for normal scsi commands.
- * @scmd:	Cmd that is timing out.
+ * scsi_times_out - Timeout function for normal SCSI commands.
+ * @scmd:	Command that is timing out.
  *
  * Notes:
  *     We do not need to lock this.  There is the potential for a race
@@ -166,7 +166,16 @@
  **/
 void scsi_times_out(struct scsi_cmnd *scmd)
 {
+	int ret = EH_NONE;
+	
 	scsi_log_completion(scmd, TIMEOUT_ERROR);
+
+	if (scmd->device->host->hostt->eh_cmd_timed_out)
+		ret = scmd->device->host->hostt->eh_cmd_timed_out(scmd);
+
+	if (ret == EH_HANDLED)
+		return;
+	
 	if (unlikely(!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
 		panic("Error handler thread not present at %p %p %s %d",
 		      scmd, scmd->device->host, __FILE__, __LINE__);
===== drivers/scsi/scsi_syms.c 1.47 vs edited =====
--- 1.47/drivers/scsi/scsi_syms.c	2004-05-19 13:46:14 -04:00
+++ edited/drivers/scsi/scsi_syms.c	2004-06-14 11:09:29 -04:00
@@ -107,3 +107,4 @@
  */
 EXPORT_SYMBOL(scsi_add_timer);
 EXPORT_SYMBOL(scsi_delete_timer);
+EXPORT_SYMBOL(scsi_times_out);
===== include/scsi/scsi.h 1.21 vs edited =====
--- 1.21/include/scsi/scsi.h	2004-04-21 12:54:43 -04:00
+++ edited/include/scsi/scsi.h	2004-06-14 14:33:15 -04:00
@@ -327,6 +327,12 @@
 #define SCSI_MLQUEUE_EH_RETRY    0x1057
 
 /*
+ * LLDD timeout handler return values
+ */
+#define EH_NONE        (0)
+#define EH_HANDLED     (1)
+
+/*
  *  Use these to separate status msg and our bytes
  *
  *  These are set by:
===== include/scsi/scsi_eh.h 1.2 vs edited =====
--- 1.2/include/scsi/scsi_eh.h	2004-06-04 07:45:01 -04:00
+++ edited/include/scsi/scsi_eh.h	2004-06-14 11:12:13 -04:00
@@ -7,10 +7,11 @@
 
 extern void scsi_add_timer(struct scsi_cmnd *, int,
 			   void (*)(struct scsi_cmnd *));
-extern int scsi_delete_timer(struct scsi_cmnd *);
+extern int  scsi_delete_timer(struct scsi_cmnd *);
+extern void scsi_times_out(struct scsi_cmnd *);
 extern void scsi_report_bus_reset(struct Scsi_Host *, int);
 extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
-extern int scsi_block_when_processing_errors(struct scsi_device *);
+extern int  scsi_block_when_processing_errors(struct scsi_device *);
 extern void scsi_sleep(int);
 
 /*
===== include/scsi/scsi_host.h 1.17 vs edited =====
--- 1.17/include/scsi/scsi_host.h	2004-06-04 12:51:31 -04:00
+++ edited/include/scsi/scsi_host.h	2004-06-14 15:12:23 -04:00
@@ -125,6 +125,40 @@
 	int (* eh_bus_reset_handler)(struct scsi_cmnd *);
 	int (* eh_host_reset_handler)(struct scsi_cmnd *);
 
+
+	/*
+	 * If defined, SCSI Core will leave _all_ command timeout
+	 * handling to the LLDD.  The LLDD should call
+	 * scsi_add_timer(<cmd>,<timeout>,scsi_times_out) when
+	 * appropriate, possibly before sending the command to the LU,
+	 * and scsi_delete_timer(<cmd>) before returning it to SCSI
+	 * Core. That is, a SCSI command is passed to the LLDD (via
+	 * queuecommand()) _without_ a timer running and is expected
+	 * to be returned to SCSI Core (via scsi_done()) just the
+	 * same, without one running.
+	 *
+	 * This method is called whenever a SCSI command times out, if
+	 * the driver had called
+	 * scsi_add_timer(<cmd>,<timeout>,scsi_times_out) of course.
+	 *
+	 * Returns: EH_HANDLED or EH_NONE.
+	 * EH_HANDLED -- means that the LLDD has taken all steps
+	 * to ensure proper command recovery handling and had
+	 * A) either called scsi_done() and set the response/result
+	 * properly in the SCSI command structure,
+	 * XOR
+	 * B) resent the command to the LU.
+	 * EH_NONE -- SCSI Core should try to recover the command as
+	 * usual (eh recovery thread, etc).
+	 *
+	 * If this method is not defined, current/old behaviour is assumed.
+	 *
+	 * NOTE: This runs in interrupt context, so it cannot sleep.
+	 * 
+	 * STATUS: OPTIONAL
+	 */
+	int (* eh_cmd_timed_out)(struct scsi_cmnd *);
+
 	/*
 	 * Old EH handlers, no longer used. Make them warn the user of old
 	 * drivers by using a wrong type


-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

On Tue, 2004-06-15 at 17:02, Luben Tuikov wrote:
> Hello,
> 
> This patch introduces a flexible command timeout infrastructure
> accomodating completely current behaviour SCSI Core command timeout,
> but also offering the ability to hand command timeout handling to
> a LLDD, _yet_ still have it all go through *SCSI Core*.

to me it somewhat sounds like the wrong approach.
I'm all for the LLDD to be able to influence the timeout value, but I
consider it a bad mistake to make every driver reinvent timeout
*handling*.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

> to me it somewhat sounds like the wrong approach.
> I'm all for the LLDD to be able to influence the timeout value, but I
> consider it a bad mistake to make every driver reinvent timeout
> *handling*.

This has very little to do with the timeout _value_, and it has
all to do with *recovery* which is a beast on its own as far as transport
protocols are concerned, especially over non-native interconnects (iSCSI, USB,
RDMA). I believe I did mention this at the end of the text.
(mea culpa it was kind of a long text) 

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Christoph Hellwig]

On Tue, Jun 15, 2004 at 11:40:02AM -0400, Luben Tuikov wrote:
> >to me it somewhat sounds like the wrong approach.
> >I'm all for the LLDD to be able to influence the timeout value, but I
> >consider it a bad mistake to make every driver reinvent timeout
> >*handling*.
> 
> This has very little to do with the timeout _value_, and it has
> all to do with *recovery* which is a beast on its own as far as transport
> protocols are concerned, especially over non-native interconnects (iSCSI, 
> USB,
> RDMA). I believe I did mention this at the end of the text.
> (mea culpa it was kind of a long text) 

That's why the plan is to move it into the transport class slowly.  Remeber
there are much more SPI drivers than just aic7xxx/aic79xx ;-)

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

> That's why the plan is to move it into the transport class slowly.  Remeber
> there are much more SPI drivers than just aic7xxx/aic79xx ;-)

Yes, in which case it would look like FreeBSD.  Then SCSI Core will
have to implement _all_ transports it wants to support (a la USB Storage).

But if we *do* allow for this patch, then _moving_ this functionality
into the transport class recovery would be really easy.  Just move
eh_cmd_timed_out() into the transport class.

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Christoph Hellwig]

On Tue, Jun 15, 2004 at 11:46:08AM -0400, Luben Tuikov wrote:
> >That's why the plan is to move it into the transport class slowly.  Remeber
> >there are much more SPI drivers than just aic7xxx/aic79xx ;-)
> 
> Yes, in which case it would look like FreeBSD.  Then SCSI Core will
> have to implement _all_ transports it wants to support (a la USB Storage).

Umm, no.  Look at a recent Linux 2.6 codebase.  We support modular transport
classes so it's _NOT_ a big mess like FreeBSD.  And we won't stop providing
sensible defaults abyway.

> But if we *do* allow for this patch, then _moving_ this functionality
> into the transport class recovery would be really easy.  Just move
> eh_cmd_timed_out() into the transport class.

Show us a patch with your SPI EH improvements.

Re: [PATCH]: Flexible timeout infrastructure

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Tue, Jun 15, 2004 at 11:40:02AM -0400, Luben Tuikov wrote:
> >to me it somewhat sounds like the wrong approach.
> >I'm all for the LLDD to be able to influence the timeout value, but I
> >consider it a bad mistake to make every driver reinvent timeout
> >*handling*.
> 
> This has very little to do with the timeout _value_, and it has
> all to do with *recovery* which is a beast on its own as far as transport
> protocols are concerned, especially over non-native interconnects (iSCSI, 
> USB,
> RDMA). I believe I did mention this at the end of the text.
> (mea culpa it was kind of a long text) 

so I can see the point of a new level of error handling function pointer,
which gets called on timeout expiry and which can return if the mid layer
needs to go into eh thread or not, or if the timeout deserves an extension.



[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

> so I can see the point of a new level of error handling function pointer,
> which gets called on timeout expiry and which can return if the mid layer
> needs to go into eh thread or not, or if the timeout deserves an extension.

Yes, thank you.

This really isn't anything new or paramount.  It's just a simple
extension, marked OPTIONAL, that only *capable* drivers should use.
(i.e. the aic7xxx drivers, maybe some FC and USB).

So it's not really that big of a deal, but does wonders to
SCSI Core in terms of recovery time and *uninterruptible*
IO.

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Christoph Hellwig]

On Tue, Jun 15, 2004 at 11:48:45AM -0400, Luben Tuikov wrote:
> This really isn't anything new or paramount.  It's just a simple
> extension, marked OPTIONAL, that only *capable* drivers should use.
> (i.e. the aic7xxx drivers, maybe some FC and USB).
> 
> So it's not really that big of a deal, but does wonders to
> SCSI Core in terms of recovery time and *uninterruptible*
> IO.

Well, the question is always what is so special about your driver that
it doesn't benefit from beeing in a common library.  I've pulled more crap
out of individual drivers than I'd ever want to imagine, and because of
that I'm pretty much allergic against giving driver writers more control
then nessecary.

Re: [PATCH]: Flexible timeout infrastructure

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

On Tue, Jun 15, 2004 at 04:57:16PM +0100, Christoph Hellwig wrote:
> On Tue, Jun 15, 2004 at 11:48:45AM -0400, Luben Tuikov wrote:
> > This really isn't anything new or paramount.  It's just a simple
> > extension, marked OPTIONAL, that only *capable* drivers should use.
> > (i.e. the aic7xxx drivers, maybe some FC and USB).
> > 
> > So it's not really that big of a deal, but does wonders to
> > SCSI Core in terms of recovery time and *uninterruptible*
> > IO.
> 
> Well, the question is always what is so special about your driver that
> it doesn't benefit from beeing in a common library.  I've pulled more crap
> out of individual drivers than I'd ever want to imagine, and because of
> that I'm pretty much allergic against giving driver writers more control
> then nessecary.

I agree with Christoph; if it makes sense then it makes sense in the common
layer, not so much in the hardware driver.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH]: Flexible timeout infrastructure

[Doug Ledford]

On Tue, 2004-06-15 at 11:57, Christoph Hellwig wrote:
> On Tue, Jun 15, 2004 at 11:48:45AM -0400, Luben Tuikov wrote:
> > This really isn't anything new or paramount.  It's just a simple
> > extension, marked OPTIONAL, that only *capable* drivers should use.
> > (i.e. the aic7xxx drivers, maybe some FC and USB).
> > 
> > So it's not really that big of a deal, but does wonders to
> > SCSI Core in terms of recovery time and *uninterruptible*
> > IO.
> 
> Well, the question is always what is so special about your driver that
> it doesn't benefit from beeing in a common library.

Well, pretty much the fact that the hardware determines what level of
recovery is possible, not the transport.  Even between different SPI
hardware, aka aic7xxx vs. sym53c8xx, you have different levels of
ability to perform recovery as dictated by how much of the adapter's
operation is handled by firmware you can control and how much isn't. 
Thinking that you can make a single SPI class recovery thread that's
actually elegant in relation to the particular hardware is wrong.  The
*only* place that will ever be able to do elegant error recovery is the
hardware driver.  It's the only code with the knowledge to be able to
discern a stuck device from a stuck bus from a missing transport from a
hung host, etc.  You can make a so-so generic layer if you want, like we
have now, but that shouldn't be a reason to stop a good driver from
doing the right thing if it wants to.  And that's what Luben's patch
does.  It doesn't force *any* driver to write any new eh code, it only
*allows* a driver to do so, and to fall back on the generic code if it
wants.  That would actually solve a lot of the reasons that drivers
currently do things like kill the timer on commands and set their own
timer on the command internally, stuff like that.  To me, the ability to
tell the eh thread to chill out, this command doesn't really have a
problem (and in my experience, the first command to timeout is not the
problem command 95% of the time) is a major benefit.

>   I've pulled more crap
> out of individual drivers than I'd ever want to imagine, and because of
> that I'm pretty much allergic against giving driver writers more control
> then nessecary.
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

Christoph Hellwig wrote:
> Well, the question is always what is so special about your driver that
> it doesn't benefit from beeing in a common library.  I've pulled more crap
> out of individual drivers than I'd ever want to imagine, and because of
> that I'm pretty much allergic against giving driver writers more control
> then nessecary.

We are *not* talking about programming here, recognizing patterns
and modularizing them.

We are talking about _recovery_.  And this method is OPTIONAL.

The mere _procedure_ which will get executed when eh_cmd_timed_out()
is called may *not* be always be readily "pulled out". It maybe a quirk
in the: transport; target (vendor, etc); LUN; HBA (vendor, etc), etc,
for which _only_ the LLDD would know.  It may also be a specific
interconnect _recovery_ (e.g. USB, iSCSI, RDMA).  It may also be
a specific _vendor_ recovery, as is possible for iSCSI.

This kind of thing you _cannot_ generalize and "pull out". Just sit
down with a vendor and write a LLDD and you will see that there's cases
where there's no way to pull this up into a nice generalized
method, because certain defects are not always known, or too complicated
or vendor specific or non-open, etc.  The possibilies are far too many
and touch far too many areas (hw bugs, fw bugs, protocol recovery,
vendor specific protocol recovery, etc).

This infrastructure is non-intrusive (minimal patch), optional,
and SCSI Core is still in control every step of the way.  It really
is not that big of a deal, but would make wonders to SCSI Core
in the hands of capable drivers.  Granted not all would be capable of
using this, but those that would be, would definitely _improve_ recovery.

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]


On Tue, Jun 15, 2004 at 12:27:45PM -0400, Luben Tuikov wrote:
> Christoph Hellwig wrote:
> >Well, the question is always what is so special about your driver that
> >it doesn't benefit from beeing in a common library.  I've pulled more crap
> >out of individual drivers than I'd ever want to imagine, and because of
> >that I'm pretty much allergic against giving driver writers more control
> >then nessecary.
> 
> We are *not* talking about programming here, recognizing patterns
> and modularizing them.
> 
> We are talking about _recovery_.  And this method is OPTIONAL.

so do explain why the recovery method requires the midlayer to hand over
timer handling to the driver ? Why does it matter for *recovery* who started
the timer ???

Yes I can see the use for the driver to tell the timer handler code "eh please
give me some more time, I'm configuring stuff for a bit". But that doesn't
explain why it shouldn't be the midlayer that keeps in charge of timer
handling and solicits the advice from the driver like this..

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

> so do explain why the recovery method requires the midlayer to hand over
> timer handling to the driver ?

This is explained in the rest of the message (the part you cut off),
and also very well explained by Doug, another LLDD writer.

> Why does it matter for *recovery* who started
> the timer ???

But that's _exactly_ the point!  It's the different scenarios
of recovery, (hw bugs, sw bugs, vendor recovery logic as is allowable
in specific protocols, etc, etc, etc)  I'm basically repeating the
previous email I posted.

You cannot have an arbitrary timer running alongside LLDD recovery
logic.
 
> Yes I can see the use for the driver to tell the timer handler code "eh please
> give me some more time, I'm configuring stuff for a bit". But that doesn't
> explain why it shouldn't be the midlayer that keeps in charge of timer
> handling and solicits the advice from the driver like this..

But that's exactly what Doug's and my emails explained!  I'm confused!?
Is this some kind of game of asking the same questions in different forms?
It's as if my email wasn't read at all? And just the same question was asked
in a different form?  It wasn't like this 4 years ago.

Again, this is just a non-intrusive _optional_ method, which would allow
capable LLDD (and non-native interconnects) to use if they decide to do so.
As I said, work with a vendor to write a LLDD (especially for newer protocols)
and the _option_ of such a method would be clear.

Any LLDD writers, FC, iSCSI, SAS people, SCSI architects are welcome
to chime in.  This patch was really intended for such a discussion.

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[James Bottomley]

On Tue, 2004-06-15 at 10:02, Luben Tuikov wrote:
> This patch introduces a flexible command timeout infrastructure
> accomodating completely current behaviour SCSI Core command timeout,
> but also offering the ability to hand command timeout handling to
> a LLDD, _yet_ still have it all go through *SCSI Core*.

But what this basically does is force any implementor of
eh_cmd_timed_out to handle all timers themselves.  Given that a large
number of driver writers who try to do this get it wrong (mostly around
del_timer() and del_timer_sync()), I don't think this is such a good
idea.

> This is somewhat similar to Christoph's proposal patch, but I wasn't
> aware of his patch when I thought of this. Interestingly enough it can be
> viewed as an extension to his patch.

This proposal was to allow LLD notification that the timer fired (rather
than first hearing about it when the eh activated).  I could see an
extension to this, like the return values you propose, where the LLD
tells the mid-layer that it corrected the command problem (in interrupt
context) and the command is ready for completion (otherwise proceed into
the eh).

Since we also already have the ability to modify the command times in
slave configure, is it really necessary to encourage the alteration of
SCSI timers in this way?

James

Re: [PATCH]: Flexible timeout infrastructure

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> On Tue, 2004-06-15 at 10:02, Luben Tuikov wrote:
> > This patch introduces a flexible command timeout infrastructure
> > accomodating completely current behaviour SCSI Core command timeout,
> > but also offering the ability to hand command timeout handling to
> > a LLDD, _yet_ still have it all go through *SCSI Core*.
> 
> But what this basically does is force any implementor of
> eh_cmd_timed_out to handle all timers themselves.  Given that a large
> number of driver writers who try to do this get it wrong (mostly around
> del_timer() and del_timer_sync()), I don't think this is such a good
> idea.
> 
> > This is somewhat similar to Christoph's proposal patch, but I wasn't
> > aware of his patch when I thought of this. Interestingly enough it can be
> > viewed as an extension to his patch.
> 
> This proposal was to allow LLD notification that the timer fired (rather
> than first hearing about it when the eh activated).  I could see an
> extension to this, like the return values you propose, where the LLD
> tells the mid-layer that it corrected the command problem (in interrupt
> context) and the command is ready for completion (otherwise proceed into
> the eh).
> 

Could we get the eh_times_out part of the patch broken out as the
addition of the return values look like a good idea along with some
method to call a modified version of scsi_done through a exported
function to the LLDD.

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

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

> Could we get the eh_times_out part of the patch broken out as the
> addition of the return values look like a good idea along with some
> method to call a modified version of scsi_done through a exported
> function to the LLDD.

Hm, yes, I saw the thread back in January I believe it was on this.

Basically I want to keep queuecommand() and scsi_done()
pretty much unchanged -- being antagonists of each other.

SAM 5.4 describes that interface (SCSI Core <--> LLDD) and
SAM 5.1 describes Application Client <--> SCSI Core.

We seem on track with 5.4 and I'd rather not overload
scsi_done().

Note that this patch would be a lot _less_ intrusive than the
one propsed in January, as the one proposed in January
discloses the internals of SCSI Core to LLDD, via
the scsi midlayer scsi_eh_scmd_add() method which
drivers would be supposed to call.
http://marc.theaimsgroup.com/?l=linux-scsi&m=107461451911771&w=2

I'd rather all this information be conveyed through
the Service Response and Status Code values to be
returned to SCSI Core from the LLDD via scsi_done(), in
the scsi command structure.

Then SCSI Core makes the decision, as to what is to be
done with the command.

This patch is very small and non-intrusive with 0 effects
to any LLDD or SCSI Core.

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Mike Anderson]

Luben Tuikov [luben_tuikov@adaptec.com] wrote:
> >Could we get the eh_times_out part of the patch broken out as the
> >addition of the return values look like a good idea along with some
> >method to call a modified version of scsi_done through a exported
> >function to the LLDD.
> 
> Hm, yes, I saw the thread back in January I believe it was on this.
> 
> Basically I want to keep queuecommand() and scsi_done()
> pretty much unchanged -- being antagonists of each other.
> 
> SAM 5.4 describes that interface (SCSI Core <--> LLDD) and
> SAM 5.1 describes Application Client <--> SCSI Core.
> 
> We seem on track with 5.4 and I'd rather not overload
> scsi_done().
> 
> Note that this patch would be a lot _less_ intrusive than the
> one propsed in January, as the one proposed in January
> discloses the internals of SCSI Core to LLDD, via
> the scsi midlayer scsi_eh_scmd_add() method which
> drivers would be supposed to call.
> http://marc.theaimsgroup.com/?l=linux-scsi&m=107461451911771&w=2
> 
> I'd rather all this information be conveyed through
> the Service Response and Status Code values to be
> returned to SCSI Core from the LLDD via scsi_done(), in
> the scsi command structure.
> 
> Then SCSI Core makes the decision, as to what is to be
> done with the command.
> 
> This patch is very small and non-intrusive with 0 effects
> to any LLDD or SCSI Core.

In the patch as is you overload the eh_cmd_timed_out template function
to mean the LLDD wants to try to handle timed out commands plus start /
restart / stop the timers. This would appear to limit the interface to
the LLDD doing both operations which should be separate decisions?

Also in the comments for the patch you mention that the LLDD may decide
to resubmit the IO which I assume is why you would want to have control
of the timers, but wouldn't the LLDD need to also consult if the IO
should be resubmitted which propagates these tests and could result in
inconsistent policy.

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

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

> In the patch as is you overload the eh_cmd_timed_out template function

But this method doesn't exist. How can I be overloading it?

> to mean the LLDD wants to try to handle timed out commands plus start /
> restart / stop the timers. This would appear to limit the interface to
> the LLDD doing both operations which should be separate decisions?

I don't understand what you mean here.
 
> Also in the comments for the patch you mention that the LLDD may decide
> to resubmit the IO which I assume is why you would want to have control
> of the timers, but wouldn't the LLDD need to also consult if the IO
> should be resubmitted which propagates these tests and could result in
> inconsistent policy.

But the LLDD *does* know if the IO is to resubmitted, it doesn't have to
consult anyone.  Hint: targets are *passive*, and thus the stipuations in SAM.

That is, if IO is NOT to be continued, then SCSI Core should call
a TMF, ABORT TASK or ABORT TASK SET, via eh_abort_handler(<cmd>).
(Yes, the eh interface doesn't do 1:1 TMF mapping.)

This is how SCSI Core communicates with LLDD, it's not the case that
LLDD asks SCSI Core each and every time for each and every decision.
First it's not by spec, and second it is unattainable both due to
the hardware reasons mentioned by Doug and I and due to the
recovery quirks.

I really don't see why this is such a big deal when SCSI Core
and LLDDs are unaffected.  It is just an _optional_ method
and will do good for SCSI Core. I promise. ;-)

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Mike Anderson]

Luben Tuikov [luben_tuikov@adaptec.com] wrote:
> >In the patch as is you overload the eh_cmd_timed_out template function
> 
> But this method doesn't exist. How can I be overloading it?
> 
> >to mean the LLDD wants to try to handle timed out commands plus start /
> >restart / stop the timers. This would appear to limit the interface to
> >the LLDD doing both operations which should be separate decisions?
> 
> I don't understand what you mean here.

The patch is adding an interface that adds two capabilities. A LLDD
callout to possibly handle timed out commands and another capability to
control when command timers start / stop. 

If a LLDD wanted to just get called to possibly handle timed out
commands it must also take care of starting and stopping command timers.
This is all I was saying in the previous comment.

> 
> >Also in the comments for the patch you mention that the LLDD may decide
> >to resubmit the IO which I assume is why you would want to have control
> >of the timers, but wouldn't the LLDD need to also consult if the IO
> >should be resubmitted which propagates these tests and could result in
> >inconsistent policy.
> 
> But the LLDD *does* know if the IO is to resubmitted, it doesn't have to
> consult anyone.  Hint: targets are *passive*, and thus the stipuations in 
> SAM.

Why does the LLDD not have to consult the same information that the SCSI
mid layer does on retries (i.e., scmd->allowed, blk_noretry_request). Is
there no idempotent issue to worry about? While SPI topologies may lead
to LLDD retries being successful in completing the command a major of
the time, there are other transport topologies that lead to the success
of the IO only through the re-driving of the IO through a disjoint path
(i.e., different LLDD instance, port, etc.) which would be done outside
the scope of the LLDD.

> 
> That is, if IO is NOT to be continued, then SCSI Core should call
> a TMF, ABORT TASK or ABORT TASK SET, via eh_abort_handler(<cmd>).
> (Yes, the eh interface doesn't do 1:1 TMF mapping.)
> 

The scsi mid layer would only call the eh_abort_handler post a timeout
event (there are other rare case in scsi_decide_disposition that wake
the error handler) which is being handled by the eh_cmd_timed_out
function of the LLDD so there would be no way for the scsi mid layer to
abort the IO.

> This is how SCSI Core communicates with LLDD, it's not the case that
> LLDD asks SCSI Core each and every time for each and every decision.
> First it's not by spec, and second it is unattainable both due to
> the hardware reasons mentioned by Doug and I and due to the
> recovery quirks.
> 
> I really don't see why this is such a big deal when SCSI Core
> and LLDDs are unaffected.  It is just an _optional_ method
> and will do good for SCSI Core. I promise. ;-)

I think the template callout in scsi_times_out is good. I believe there
is still debate on controlling the timers that is why I suggested maybe
the two capabilities could be separated.

I believe the callout as per my previous query last week may help in
some failover / failback case involing device mapper multipath. I
think it would be good for SCSI core to have this callout interface or
one like it.

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

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

> The patch is adding an interface that adds two capabilities. A LLDD
> callout to possibly handle timed out commands and another capability to
> control when command timers start / stop.

Yes, I know -- I wrote it. ;-)
 
> If a LLDD wanted to just get called to possibly handle timed out
> commands it must also take care of starting and stopping command timers.
> This is all I was saying in the previous comment.

Yes, it does take care of that -- see my comment before
int (* eh_cmd_timed_out)(struct scsi_cmnd *);
 
> Why does the LLDD not have to consult the same information that the SCSI

Because 
1. It's closed to the hardware/LU than SCSI Core is.
2. Because this is NOT the SAM interface. See SAM TMF.
   SCSI Core calls a TMF telling LU via LLDD what to do.
   LLDD doesn't consult each and every time SCSI Core what to do.
   It's a suble difference, but a difference nevertheless.

> mid layer does on retries (i.e., scmd->allowed, blk_noretry_request). Is
> there no idempotent issue to worry about? While SPI topologies may lead
> to LLDD retries being successful in completing the command a major of
> the time, there are other transport topologies that lead to the success
> of the IO only through the re-driving of the IO through a disjoint path
> (i.e., different LLDD instance, port, etc.) which would be done outside
> the scope of the LLDD.

And, _again_, the LLDD would *know* about and can return EH_NONE, to
have SCSI Core/Application client try another path.
 
>  >
>  > That is, if IO is NOT to be continued, then SCSI Core should call
>  > a TMF, ABORT TASK or ABORT TASK SET, via eh_abort_handler(<cmd>).
>  > (Yes, the eh interface doesn't do 1:1 TMF mapping.)
>  >
> 
> The scsi mid layer would only call the eh_abort_handler post a timeout
> event (there are other rare case in scsi_decide_disposition that wake

The reason for calling TMF ABORT TASK is beyond the point, whether it be
timeout of application client failure.  The importance is the mechanism.

> the error handler) which is being handled by the eh_cmd_timed_out
> function of the LLDD so there would be no way for the scsi mid layer to
> abort the IO.

There is: call the appropriate TMF.
We need to implement TMF ABORT TASK entry, et al, into SCSI Core
and into LLDD, as TMF are Application Client --> SCSI transport.
 
> I think the template callout in scsi_times_out is good. I believe there
> is still debate on controlling the timers that is why I suggested maybe
> the two capabilities could be separated.

I'm rather opposed to another separation, as this bloats up SCSI Core
and makes it more complicated, unecessarily so.  scsi_done() and
queuecommand() are quite simple an interface currently and I'd
rather keep it like that.
 
> I believe the callout as per my previous query last week may help in
> some failover / failback case involing device mapper multipath. I
> think it would be good for SCSI core to have this callout interface or
> one like it.
> 
> -andmike
> -- 
> Michael Anderson
> andmike@us.ibm.com
> 

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

Tuikov, Luben wrote:
> Because
> 1. It's closed to the hardware/LU than SCSI Core is.
   I meant "closer" here.
-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[Doug Ledford]

On Tue, 2004-06-15 at 16:57, Mike Anderson wrote:
> Luben Tuikov [luben_tuikov@adaptec.com] wrote:
> > >In the patch as is you overload the eh_cmd_timed_out template function
> > 
> > But this method doesn't exist. How can I be overloading it?
> > 
> > >to mean the LLDD wants to try to handle timed out commands plus start /
> > >restart / stop the timers. This would appear to limit the interface to
> > >the LLDD doing both operations which should be separate decisions?
> > 
> > I don't understand what you mean here.
> 
> The patch is adding an interface that adds two capabilities. A LLDD
> callout to possibly handle timed out commands and another capability to
> control when command timers start / stop. 
> 
> If a LLDD wanted to just get called to possibly handle timed out
> commands it must also take care of starting and stopping command timers.
> This is all I was saying in the previous comment.

I agree with this.  I can certianly see why I would want to be able to
control timers from the low level driver though.  The most common
problem I've seen is actual bus lockups (due to either a hung device, a
hardware or driver error, or due to cabling issues).  When that happens,
the command that hangs usually is not the oldest command on the bus, so
all the commands that time out prior to the actual suspect command
should not be penalized on retries (or at least should be more
intelligently penalized, for example if the problem is a hung device and
not a cabling problem, then you can be using up a retry for disk 1 when
tape 2 is the one locking the bus up and there isn't a thing wrong with
disk 1).  One of the easiest ways to tell the difference between a hung
device/bus and a starved command is to get a timeout, pause the
controller, find out what command is active and what exact position in
the command process it's in, then wait a short period of time (HZ/100 is
sufficient to be sure that progress *should* have happened if the device
isn't hung), if progress has been made then you likely have a starved
command so send an ordered tag command (assuming it's a tagged device,
if it's not then go to a separate error handler path) and wait for the
ordered tag to complete, if the lost command is still lost, then you
have a problem, if not then it completed properly.  If progress has not
been made, then try a bus device reset and reset timeout on the active
command to something like (HZ/20).  If the timeout fires, then your bus
device reset has failed, throw a bus reset.  If the bus device reset
succeeds it will do so in much less time than HZ/20 and all commands for
that device will have already been returned as DID_RESET and will be
getting retried.  Easy, elegant, but hardware dependant.

> > 
> > >Also in the comments for the patch you mention that the LLDD may decide
> > >to resubmit the IO which I assume is why you would want to have control
> > >of the timers, but wouldn't the LLDD need to also consult if the IO
> > >should be resubmitted which propagates these tests and could result in
> > >inconsistent policy.
> > 
> > But the LLDD *does* know if the IO is to resubmitted, it doesn't have to
> > consult anyone.  Hint: targets are *passive*, and thus the stipuations in 
> > SAM.
> 
> Why does the LLDD not have to consult the same information that the SCSI
> mid layer does on retries (i.e., scmd->allowed, blk_noretry_request). Is
> there no idempotent issue to worry about?

Following the scenario I just wrote above, this all works out properly. 
For starved commands, there is no check of retries because you don't
retry, you just force it to complete.  For a hung device, all of it's
commands will go through the retry stuff as a result of being sent back
with DID_RESET (although if we have to throw a complete bus reset then
commands for other devices will also go through this, which is correct
if the problem is a general cabling problem making all devices flake
out, not so correct if you have a specific command causing a single
device to lock up repeatedly, but this is something that can actually be
handled at the mid layer by doing intelligent requeuing after a bus
reset, things like slow start on each device and doing a round robin
style 1 command at a time to 1 device, wait for completion, mark device
working, go to next device, when you complete one loop through all the
devices, then start sending commands to multiple devices at a time
again, that sort of thing).

>  While SPI topologies may lead
> to LLDD retries being successful in completing the command a major of
> the time, there are other transport topologies that lead to the success
> of the IO only through the re-driving of the IO through a disjoint path
> (i.e., different LLDD instance, port, etc.) which would be done outside
> the scope of the LLDD.

Typically, a LLDD is very much aware of these things.  When the
transport goes away, the LLDD knows it.  Think fiber channel for
example.  If a driver knows the transport is gone, then it returns the
command with an appropriate error and the upper layers do the rest.

> > 
> > That is, if IO is NOT to be continued, then SCSI Core should call
> > a TMF, ABORT TASK or ABORT TASK SET, via eh_abort_handler(<cmd>).
> > (Yes, the eh interface doesn't do 1:1 TMF mapping.)
> > 
> 
> The scsi mid layer would only call the eh_abort_handler post a timeout
> event (there are other rare case in scsi_decide_disposition that wake
> the error handler) which is being handled by the eh_cmd_timed_out
> function of the LLDD so there would be no way for the scsi mid layer to
> abort the IO.
> 
> > This is how SCSI Core communicates with LLDD, it's not the case that
> > LLDD asks SCSI Core each and every time for each and every decision.
> > First it's not by spec, and second it is unattainable both due to
> > the hardware reasons mentioned by Doug and I and due to the
> > recovery quirks.
> > 
> > I really don't see why this is such a big deal when SCSI Core
> > and LLDDs are unaffected.  It is just an _optional_ method
> > and will do good for SCSI Core. I promise. ;-)
> 
> I think the template callout in scsi_times_out is good. I believe there
> is still debate on controlling the timers that is why I suggested maybe
> the two capabilities could be separated.

I agree the two should be separated, but I certainly think the lldd
should have the ability to deal with their own timers.

> I believe the callout as per my previous query last week may help in
> some failover / failback case involing device mapper multipath. I
> think it would be good for SCSI core to have this callout interface or
> one like it.
> 
> -andmike
> --
> Michael Anderson
> andmike@us.ibm.com
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

> But what this basically does is force any implementor of
> eh_cmd_timed_out to handle all timers themselves.  Given that a large
> number of driver writers who try to do this get it wrong (mostly around
> del_timer() and del_timer_sync()), I don't think this is such a good
> idea.

True, it is not a good idea for all LLDD to use this interface.
But a few capable LLDD exist who can make use of it (including
non-native interconnect subsystems).

Also we can include a comment in there that in order to use
this interface the driver has to <funny quote here>. ;-)
 
> This proposal was to allow LLD notification that the timer fired (rather
> than first hearing about it when the eh activated).  I could see an
> extension to this, like the return values you propose, where the LLD
> tells the mid-layer that it corrected the command problem (in interrupt
> context) and the command is ready for completion (otherwise proceed into
> the eh).

But to stay on track and in spec, the LLDD has to notify completion
of the command, via, the only one, antagonist of queuecommand(),
scsi_done().  This would keep the interface consistent.  I explained
this in my latter emails.
 
> Since we also already have the ability to modify the command times in
> slave configure, is it really necessary to encourage the alteration of
> SCSI timers in this way?

Keywords: optional, non-intrusive patch.  It merely adds an alternative
to capable only drivers.  This patch DOES NOT modify SCSI Core.

I'm not talking about an overhaul of SCSI Core here, just an optional
method which a capable driver could use.  It has no effect to the rest
of SCSI Core or LLDDs.

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[James Bottomley]

On Tue, 2004-06-15 at 14:12, Luben Tuikov wrote:
> > But what this basically does is force any implementor of
> > eh_cmd_timed_out to handle all timers themselves.  Given that a large
> > number of driver writers who try to do this get it wrong (mostly around
> > del_timer() and del_timer_sync()), I don't think this is such a good
> > idea.
> 
> True, it is not a good idea for all LLDD to use this interface.
> But a few capable LLDD exist who can make use of it (including
> non-native interconnect subsystems).
> 
> Also we can include a comment in there that in order to use
> this interface the driver has to <funny quote here>. ;-)

Really, no.  An "experts only" interface is asking for trouble.  A major
point about cleaning up the SCSI API is to encourage better driver
writing by making it difficult to user the API incorrectly.

> > Since we also already have the ability to modify the command times in
> > slave configure, is it really necessary to encourage the alteration of
> > SCSI timers in this way?
> 
> Keywords: optional, non-intrusive patch.  It merely adds an alternative
> to capable only drivers.  This patch DOES NOT modify SCSI Core.
> 
> I'm not talking about an overhaul of SCSI Core here, just an optional
> method which a capable driver could use.  It has no effect to the rest
> of SCSI Core or LLDDs.

I'm less interested in the amount of perturbation to the mid-layer than
I am in getting the API right.  I've really heard no arguments that
persuade me that turning over timer management to the LLDs is a good
thing to do.

What the argument has centered around is the fact that LLDs wish to do
operations to effect error recovery on their own.

The original proposal (by Christoph) was a simple notify that error
recovery was about to happen.

In the ensuing discussion there have been various changes to this
suggested, which seem to provide a framework for the solution:

1. Timer handling would still all be done in the mid-layer

2. Any driver supplying the notify function would have it called on
timer expiry.

3. The LLD communicates what action it wishes to be taken based on the
return value from the notify.  I suggest 3 possible return actions:

a. Do nothing and continue with error handling

b. I fixed the problem, complete the command immediately and proceed as
though nothing went wrong.

c. I need more time, reset the timer and notify me again when it fails.

For (c), I propose that we use the same timeout period, but increment
the retry count (and do this up to allowed retries plus one [so that
no-retry commands have one crack at being recovered by the LLD]) when
retries are exhausted, normal error handling would proceed on timer
expiry leading to certain failure of the command since it would be
ineligible to be retried.

what additional features do you need beyond this proposal?

James

Re: [PATCH]: Flexible timeout infrastructure

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> In the ensuing discussion there have been various changes to this
> suggested, which seem to provide a framework for the solution:
> 
> 1. Timer handling would still all be done in the mid-layer
> 
> 2. Any driver supplying the notify function would have it called on
> timer expiry.
> 
> 3. The LLD communicates what action it wishes to be taken based on the
> return value from the notify.  I suggest 3 possible return actions:
> 
> a. Do nothing and continue with error handling
> 
> b. I fixed the problem, complete the command immediately and proceed as
> though nothing went wrong.

Does this mean scsi_times_out will complete the command by calling a
SCSI mid layer internal form of the scsi_done function (less the
scsi_delete_timer call) or that the LLDD will call scsi_done and we will
need to modify scsi_done to accept these no timer running cases.

> 
> c. I need more time, reset the timer and notify me again when it fails.
> 
> For (c), I propose that we use the same timeout period, but increment
> the retry count (and do this up to allowed retries plus one [so that
> no-retry commands have one crack at being recovered by the LLD]) when
> retries are exhausted, normal error handling would proceed on timer
> expiry leading to certain failure of the command since it would be
> ineligible to be retried.

The comment on the no-retry commands appears counter to the intent of
FASTFAIL. On a multi-ported device if there really is a port / controller
issue we have increased the failover time 2x the timeout value which
IIRC was one case that FASTFAIL wished to address.

> 
> what additional features do you need beyond this proposal?
> 


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

Re: [PATCH]: Flexible timeout infrastructure

[James Bottomley]

On Wed, 2004-06-16 at 10:27, Mike Anderson wrote:
> Does this mean scsi_times_out will complete the command by calling a
> SCSI mid layer internal form of the scsi_done function (less the
> scsi_delete_timer call) or that the LLDD will call scsi_done and we will
> need to modify scsi_done to accept these no timer running cases.

Yes.  We'll just abstract all of scsi_done() bar the timer check into
__scsi_done, which will be private, and called in this instance.

> > 
> > c. I need more time, reset the timer and notify me again when it fails.
> > 
> > For (c), I propose that we use the same timeout period, but increment
> > the retry count (and do this up to allowed retries plus one [so that
> > no-retry commands have one crack at being recovered by the LLD]) when
> > retries are exhausted, normal error handling would proceed on timer
> > expiry leading to certain failure of the command since it would be
> > ineligible to be retried.
> 
> The comment on the no-retry commands appears counter to the intent of
> FASTFAIL. On a multi-ported device if there really is a port / controller
> issue we have increased the failover time 2x the timeout value which
> IIRC was one case that FASTFAIL wished to address.

Well ... perhaps the solution's to shorten the timers then for this
case?

James

Re: [PATCH]: Flexible timeout infrastructure

[Luben Tuikov]

James Bottomley wrote:
> On Wed, 2004-06-16 at 10:27, Mike Anderson wrote:
>  > Does this mean scsi_times_out will complete the command by calling a
>  > SCSI mid layer internal form of the scsi_done function (less the
>  > scsi_delete_timer call) or that the LLDD will call scsi_done and we will
>  > need to modify scsi_done to accept these no timer running cases.
> 
> Yes.  We'll just abstract all of scsi_done() bar the timer check into
> __scsi_done, which will be private, and called in this instance.

So, now, there will be a 2nd, "fuzzy" way of returning a command
back to SCSI Core:

a) LLDD calls scsi_done() when all went well, an antagonist to the 
   one and only queuecommand(),
XOR
b) command timed out, LLDD's eh_cmd_timed_out() was called and returned
   EH_HANDLED, and then _SCSI_Core_ calls __scsi_done().

I.e. in b) the LLDD _never_ gets to call scsi_done() (or a completion method)
on that command.

Anyway, do we have a patch for *this* solution?

>  > >
>  > > c. I need more time, reset the timer and notify me again when it 
> fails.
>  > >
>  > > For (c), I propose that we use the same timeout period, but increment
>  > > the retry count (and do this up to allowed retries plus one [so that
>  > > no-retry commands have one crack at being recovered by the LLD]) when
>  > > retries are exhausted, normal error handling would proceed on timer
>  > > expiry leading to certain failure of the command since it would be
>  > > ineligible to be retried.
>  >
>  > The comment on the no-retry commands appears counter to the intent of
>  > FASTFAIL. On a multi-ported device if there really is a port / 
> controller
>  > issue we have increased the failover time 2x the timeout value which
>  > IIRC was one case that FASTFAIL wished to address.
> 
> Well ... perhaps the solution's to shorten the timers then for this
> case?

-- 
Luben

Re: [PATCH]: Flexible timeout infrastructure

[James Bottomley]

On Wed, 2004-06-16 at 10:48, Luben Tuikov wrote:
> Anyway, do we have a patch for *this* solution?

Here's a sketch of the implementation (without all the comments etc that
would need to be done) so people can visualise it

James

===== drivers/scsi/scsi.c 1.143 vs edited =====
--- 1.143/drivers/scsi/scsi.c	2004-04-28 11:32:09 -05:00
+++ edited/drivers/scsi/scsi.c	2004-06-16 10:47:05 -05:00
@@ -689,8 +689,6 @@
  */
 void scsi_done(struct scsi_cmnd *cmd)
 {
-	unsigned long flags;
-
 	/*
 	 * We don't have to worry about this one timing out any more.
 	 * If we are unable to remove the timer, then the command
@@ -701,6 +699,14 @@
 	 */
 	if (!scsi_delete_timer(cmd))
 		return;
+	__scsi_done(cmd);
+}
+
+/* Private entry to scsi_done() to complete a command when the timer
+ * isn't running --- used by scsi_times_out */
+void __scsi_done(struct scsi_cmnd *cmd)
+{
+	unsigned long flags;
 
 	/*
 	 * Set the serial numbers back to zero
===== drivers/scsi/scsi_error.c 1.77 vs edited =====
--- 1.77/drivers/scsi/scsi_error.c	2004-06-06 06:19:15 -05:00
+++ edited/drivers/scsi/scsi_error.c	2004-06-16 10:53:02 -05:00
@@ -162,6 +162,24 @@
 void scsi_times_out(struct scsi_cmnd *scmd)
 {
 	scsi_log_completion(scmd, TIMEOUT_ERROR);
+
+	if (scmd->device->host->hostt->eh_timed_out)
+		switch (scmd->device->host->hostt->eh_timed_out(scmd)) {
+		case EH_HANDLED:
+			__scsi_done(scmd);
+			return;
+		case EH_RESET_TIMER:
+			/* This allows a single retry even of a command
+			 * with allowed == 0 */
+			if (scmd->retries++ > scmd->allowed)
+				break;
+			scsi_add_timer(scmd, scmd->timeout_per_command,
+				       scsi_times_out);
+			return;
+		case EH_NOT_HANDLED:
+			break;
+		}
+
 	if (unlikely(!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
 		panic("Error handler thread not present at %p %p %s %d",
 		      scmd, scmd->device->host, __FILE__, __LINE__);
===== drivers/scsi/scsi_priv.h 1.32 vs edited =====
--- 1.32/drivers/scsi/scsi_priv.h	2004-03-10 22:20:08 -06:00
+++ edited/drivers/scsi/scsi_priv.h	2004-06-16 10:45:44 -05:00
@@ -82,6 +82,7 @@
 extern void scsi_init_cmd_from_req(struct scsi_cmnd *cmd,
 		struct scsi_request *sreq);
 extern void __scsi_release_request(struct scsi_request *sreq);
+extern void __scsi_done(struct scsi_cmnd *cmd);
 #ifdef CONFIG_SCSI_LOGGING
 void scsi_log_send(struct scsi_cmnd *cmd);
 void scsi_log_completion(struct scsi_cmnd *cmd, int disposition);
===== include/scsi/scsi_host.h 1.17 vs edited =====
--- 1.17/include/scsi/scsi_host.h	2004-06-04 11:51:31 -05:00
+++ edited/include/scsi/scsi_host.h	2004-06-16 10:36:23 -05:00
@@ -30,6 +30,12 @@
 #define DISABLE_CLUSTERING 0
 #define ENABLE_CLUSTERING 1
 
+enum scsi_eh_timer_return {
+	EH_NOT_HANDLED,
+	EH_HANDLED,
+	EH_RESET_TIMER,
+};
+
 
 struct scsi_host_template {
 	struct module *module;
@@ -124,6 +130,8 @@
 	int (* eh_device_reset_handler)(struct scsi_cmnd *);
 	int (* eh_bus_reset_handler)(struct scsi_cmnd *);
 	int (* eh_host_reset_handler)(struct scsi_cmnd *);
+
+	enum scsi_eh_timer_return (* eh_timed_out)(struct scsi_cmnd *);
 
 	/*
 	 * Old EH handlers, no longer used. Make them warn the user of old