: Flexible timeout infrastructure

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 =====

@@ -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 =====

@@ -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 =====

@@ -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 =====

@@ -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 =====

@@ -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 =====

@@ -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

===== drivers/scsi/scsi.c 1.143 vs edited =====

@@ -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 =====

@@ -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 =====

@@ -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 =====

@@ -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 =====

@@ -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 =====

@@ -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

Your patches have been brokenly linewrapped by your email system. Of course, this wouldn't matter if you weren't suffering from ia64-itis.

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

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

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

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.

Show us a patch with your SPI EH improvements.

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

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.

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.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606

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

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..

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

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.

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

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 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

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
[email protected]

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

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
[email protected]

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

I don't understand what you mean here.

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

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.

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.

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.

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
[email protected]

Yes, I know -- I wrote it. ;-)

Yes, it does take care of that -- see my comment before int (* eh_cmd_timed_out)(struct scsi_cmnd *);

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.

And, _again_, the LLDD would *know* about and can return EH_NONE, to have SCSI Core/Application client try another path.

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

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'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.

Luben

I meant "closer" here.

Luben

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.

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).

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.

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

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc.
         1801 Varsity Dr.
         Raleigh, NC 27606

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>. ;-)

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.

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

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.

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

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.

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.

-andmike
--
Michael Anderson
[email protected]

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.

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

James

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?

Luben

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 =====

@@ -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 =====

@@ -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 =====

@@ -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 =====

@@ -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