Re: [PATCH RFC] Reduce amount of TURs sent in error recovery

[Mark Goodwin <mgoodwin@redhat.com>]

On 03/17/2011 04:43 AM, David Jeffery wrote:
> In error recovery, most scsi error recovery stages will send a TUR command
> for every bad command when a driver's error handler reports success.  When
> several bad commands to the same device, this results in a device
> being probed multiple times.
>
> This becomes very problematic if the device or connection is in a state
> where the device still doesn't respond to commands even after a recovery
> function returns success.  The error handler must wait for the test
> commands to time out.  The time waiting for the redundant commands can
> drastically lengthen error recovery.
>
> This patch alters the scsi mid-layer's error routines to send test commands
> once per device instead of once per bad command.  This can drastically
> lower error recovery time.
>

Looks good to me - it should also make recovery times far more predictable,
which is something a lot of sites currently struggle with because it depends
on the workload (number of queued commands at the time of the failure).
With this patch, those sites will need to re-tune their timeouts, right?

Has this been tested on real FC h/w yet, or just with scsi_debug?
We have a patch to make testing easier: it adds a new sd state 'zombie',
which stops a particular device/path from queueing any commands,
basically as follows (not the whole patch, just the gist of it) :

--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -554,7 +554,10 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
                 cmd->result = (DID_NO_CONNECT << 16);
                 scsi_done(cmd);
         } else {
-               rtn = host->hostt->queuecommand(cmd, scsi_done);
+               if (cmd->device->sdev_state == SDEV_ZOMBIE)
+                       rtn = 0; /* will timeout so err handler will run */
+               else
+                       rtn = host->hostt->queuecommand(cmd, scsi_done);
         }

Would we want to consider adding this to the patch set too? It will allow
affected sites to more easily tune their timeouts and redundancy config
(i.e. echo zombie > /sys/block/sdX/device/state to simulate a failure).

Cheers
-- Mark Goodwin