[PATCH] Do not requeue requests if FAILFAST is set

[PATCH] Do not requeue requests if FAILFAST is set

[Hannes Reinecke]

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

Hi James,

the FAILFAST flag is currently not honored in scsi_request_fn().
So if scsi_dispatch_cmd() returns an error or the queue is not
ready, the request will always be requeued regardless of the
FAILFAST setting.
This can result in a delayed fallback for multipathing, as
these requests will only be catched once the timeout expires.

This patch avoids requeuing if the FAILFAST flag is set, and
terminates the request immediately.

Comments et al welcome.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: scsi-kill-failfast-requests --]
[-- Type: text/plain, Size: 2016 bytes --]

Do not requeue requests if REQ_FAILFAST is set

Any requests with the REQ_FAILFAST flag set should not be requeued
to the requeust queue, but rather terminated directly.
Otherwise the multipath failover will stall until the command
timeout triggers.

Signed-off-by: Hannes Reinecke <hare@suse.de>

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 277f1b6..a12d2f1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1200,6 +1200,11 @@ static int scsi_prep_fn(struct request_queue *q, struct request *req)
 			 */
 			if (!(req->cmd_flags & REQ_PREEMPT))
 				ret = BLKPREP_DEFER;
+			/*
+			 * Return failfast requests immediately
+			 */
+			if (req->cmd_flags & REQ_FAILFAST)
+				ret = BLKPREP_KILL;
 			break;
 		default:
 			/*
@@ -1327,6 +1332,17 @@ static inline int scsi_host_queue_ready(struct request_queue *q,
 	return 1;
 }
 
+static void __scsi_kill_request(struct request *req)
+{
+	struct scsi_cmnd *cmd = req->special;
+	struct scsi_device *sdev = cmd->device;
+
+	cmd->result = DID_NO_CONNECT << 16;
+	atomic_inc(&cmd->device->iorequest_cnt);
+	sdev->device_busy--;
+	__scsi_done(cmd);
+}
+
 /*
  * Kill a request for a dead device
  */
@@ -1440,8 +1456,16 @@ static void scsi_request_fn(struct request_queue *q)
 		 * accept it.
 		 */
 		req = elv_next_request(q);
-		if (!req || !scsi_dev_queue_ready(q, sdev))
+		if (!req)
+			break;
+
+		if (!scsi_dev_queue_ready(q, sdev)) {
+			if (req->cmd_flags & REQ_FAILFAST) {
+				scsi_kill_request(req, q);
+				continue;
+			}
 			break;
+		}
 
 		if (unlikely(!scsi_device_online(sdev))) {
 			sdev_printk(KERN_ERR, sdev,
@@ -1522,8 +1546,12 @@ static void scsi_request_fn(struct request_queue *q)
 	 * later time.
 	 */
 	spin_lock_irq(q->queue_lock);
-	blk_requeue_request(q, req);
-	sdev->device_busy--;
+	if (unlikely(req->cmd_flags & REQ_FAILFAST))
+		__scsi_kill_request(req);
+	else {
+		blk_requeue_request(q, req);
+		sdev->device_busy--;
+	}
 	if(sdev->device_busy == 0)
 		blk_plug_device(q);
  out:

Re: [PATCH] Do not requeue requests if FAILFAST is set

[Mike Christie]

Hannes Reinecke wrote:
> Hi James,
> 
> the FAILFAST flag is currently not honored in scsi_request_fn().
> So if scsi_dispatch_cmd() returns an error or the queue is not
> ready, the request will always be requeued regardless of the
> FAILFAST setting.
> This can result in a delayed fallback for multipathing, as
> these requests will only be catched once the timeout expires.
> 
> This patch avoids requeuing if the FAILFAST flag is set, and
> terminates the request immediately.
> 
> Comments et al welcome.
> 

I think that might be better than my transport errors patch. If 
multipath is being used then we told people to set the recovery timeout, 
to a very low value. So that way commands would not sit in a blocked 
queue very long.

With your patch, if the queue was blocked because we are doing recovery, 
then the command is returned upwards as soon as the driver/class returns 
the command to the scsi layer. And that is nice since it fixes that 
issue where the fibre channel class/driver fails commands in the driver 
when fail fast tmo fires, but commands in the blocked queue sit around 
until dev loss tmo fires or the rport comes back.

Is there any case though where fail fast is set on the request, but the 
driver really does want to retry on the driver? Was there some other 
user of something like DID_IMM_RETRY that could be used with multipath 
or raid and when they return DID_IMM_RETRY they wanted to bypass the 
fail fast checks? If so that would be my only reason for splitting up 
the error values and relying on the transport class timers (fc fail fast 
tmo and iscsi recovery tmo) to decide when to fail commands (assuming 
that the fc class is also changed so when fail fast tmo fires blocked 
commands are failed).

Oh yeah since we are now doing dm-multipath with SAS, what is that class 
doing as far as failing commands when there is a transport issue? Does 
it have something like fc's fail fast tmo? Does it fail running commands 
right away so the scsi layer can requeue or does it try to do some lower 
level recovery and try to continue the command until some timeout fires?