[PATCH] scsi/block: increase flush/sync timeout

[PATCH] scsi/block: increase flush/sync timeout

[michaelc]

From: Mike Christie <mchristi@redhat.com>

We have been seeing the flush request timeout with a wide
range of hardware from tgt+iser to FC targets from a major vendor.

I did this patch which added a flush timeout:
http://marc.info/?l=linux-scsi&m=127957359200466&w=2

A problem with that patch is how to determine what value
should be set and when you need to set it. You will
not know that you need to increase until it times out
and fails, then to figure out what to set it to you
need to set the timeout and try it out a couple times.

So this patch just increases the flush/sync cache timeout
to 2 minutes. In testing, it has taken at most around 60
seconds to complete the operation, so I thought 120 would
be safe and not add that long of delay if the device was
really jammed and needed the scsi eh.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/sd.c |    4 ++--
 drivers/scsi/sd.h |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 108daea..3ef08bd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -477,7 +477,7 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 
 static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
 {
-	rq->timeout = SD_TIMEOUT;
+	rq->timeout = SD_FLUSH_TIMEOUT;
 	rq->retries = SD_MAX_RETRIES;
 	rq->cmd[0] = SYNCHRONIZE_CACHE;
 	rq->cmd_len = 10;
@@ -1064,7 +1064,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 		 * flush everything.
 		 */
 		res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
-				       SD_TIMEOUT, SD_MAX_RETRIES, NULL);
+				       SD_FLUSH_TIMEOUT, SD_MAX_RETRIES, NULL);
 		if (res == 0)
 			break;
 	}
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index f81a930..86e6dc3 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -19,6 +19,7 @@
  */
 #define SD_TIMEOUT		(30 * HZ)
 #define SD_MOD_TIMEOUT		(75 * HZ)
+#define SD_FLUSH_TIMEOUT	(120 * HZ)
 
 /*
  * Number of allowed retries
-- 
1.6.6.1

Re: [PATCH] scsi/block: increase flush/sync timeout

[Boaz Harrosh]

On 08/03/2010 10:26 PM, michaelc@cs.wisc.edu wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> We have been seeing the flush request timeout with a wide
> range of hardware from tgt+iser to FC targets from a major vendor.
> 
> I did this patch which added a flush timeout:
> http://marc.info/?l=linux-scsi&m=127957359200466&w=2
> 
> A problem with that patch is how to determine what value
> should be set and when you need to set it. You will
> not know that you need to increase until it times out
> and fails, then to figure out what to set it to you
> need to set the timeout and try it out a couple times.
> 
> So this patch just increases the flush/sync cache timeout
> to 2 minutes. In testing, it has taken at most around 60
> seconds to complete the operation, so I thought 120 would
> be safe and not add that long of delay if the device was
> really jammed and needed the scsi eh.
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

I'm not sure either way, but you might also use the
request_queue's timeout which is settable from user-mode,
times a factor like "request_queue->timeout * 4"

A slow device might be slower on flush as well.

Or not, just an option ;-)
(Or be really smart like can_queue * single_io_timeout / 2)

Boaz

> ---
>  drivers/scsi/sd.c |    4 ++--
>  drivers/scsi/sd.h |    1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 108daea..3ef08bd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -477,7 +477,7 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
>  
>  static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
>  {
> -	rq->timeout = SD_TIMEOUT;
> +	rq->timeout = SD_FLUSH_TIMEOUT;
>  	rq->retries = SD_MAX_RETRIES;
>  	rq->cmd[0] = SYNCHRONIZE_CACHE;
>  	rq->cmd_len = 10;
> @@ -1064,7 +1064,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>  		 * flush everything.
>  		 */
>  		res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
> -				       SD_TIMEOUT, SD_MAX_RETRIES, NULL);
> +				       SD_FLUSH_TIMEOUT, SD_MAX_RETRIES, NULL);
>  		if (res == 0)
>  			break;
>  	}
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index f81a930..86e6dc3 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -19,6 +19,7 @@
>   */
>  #define SD_TIMEOUT		(30 * HZ)
>  #define SD_MOD_TIMEOUT		(75 * HZ)
> +#define SD_FLUSH_TIMEOUT	(120 * HZ)
>  
>  /*
>   * Number of allowed retries

Re: [PATCH] scsi/block: increase flush/sync timeout

[Bernd Schubert]

On Tuesday, August 03, 2010, michaelc@cs.wisc.edu wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> We have been seeing the flush request timeout with a wide
> range of hardware from tgt+iser to FC targets from a major vendor.
> 
> I did this patch which added a flush timeout:
> http://marc.info/?l=linux-scsi&m=127957359200466&w=2
> 
> A problem with that patch is how to determine what value
> should be set and when you need to set it. You will
> not know that you need to increase until it times out
> and fails, then to figure out what to set it to you
> need to set the timeout and try it out a couple times.
> 
> So this patch just increases the flush/sync cache timeout
> to 2 minutes. In testing, it has taken at most around 60
> seconds to complete the operation, so I thought 120 would
> be safe and not add that long of delay if the device was
> really jammed and needed the scsi eh.

Sorry for my late reply. Doesn't it depend on the device cache how long it 
takes? 120s is should be sufficient for everything I worked with, but then 
there are units out there that that have far more cache. Although I don't know 
if those honor the SYNC_CACHE command. Personally I would prefer the previous 
patch as it allows it to tune it with udev (*). So shouldn't just the previous 
patch be updated to 120s default timeout?


Thanks,
Bernd

PS: I just think it is a bit confusing to have different timeouts in differnt 
directories (/sys/block/sdX/device/timeout and 
/sys/block/sdX/queue/flush_timeout). The patch I have on my disk, which I 
never managed to test and therefore never sent to the list, added it to 
/sys/block/sdX/device.




-- 
Bernd Schubert
DataDirect Networks