[PATCH 1/1] block: set req->timeout in blk_add_timer

[PATCH 1/1] block: set req->timeout in blk_add_timer

[michaelc]

From: Mike Christie <mchristi@redhat.com>

To prevent a reques from running forever, scsi-ml checks if
a command has been running req->timeout * cmd->allowed + 1 seconds.
If it has, scsi-ml will fail the request. From what I can tell
it looks like in Jen's tree that the block layer is not setting
this value, so a command is failed right away a lot of times because
wait_for in scsi_softirq_done is zero seconds. It is only set by ioctls
and passthrough.

This patch just copies the q timeout that is used, so any one
checking it will still be able to see it.

For the scsi-ml req->timeout usage, I think we can move the retries and
the infinite retry check from the scsi layer to the block layer. I was
not sure if we should do that in this patch or a separate one. I can
cook up another patch if people want. If it would be possible to do
that after MikeA's patches it would probably be best though since
he is working on that code too.

This patch was made over James's scsi post tree.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 block/blk-timeout.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index 779f439..6e7778d 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -132,9 +132,14 @@ void blk_add_timer(struct request *req)
 
 	if (req->timeout)
 		req->deadline = jiffies + req->timeout;
-	else
+	else {
 		req->deadline = jiffies + q->rq_timeout;
-
+		/*
+		 * Some LLDs, like scsi, peek at the timeout to prevent
+		 * a command from being retried forever.
+		 */
+		req->timeout = q->rq_timeout;
+	}
 	list_add_tail(&req->timeout_list, &q->timeout_list);
 
 	/*
-- 
1.5.5.1

Re: [PATCH 1/1] block: set req->timeout in blk_add_timer

[James Bottomley]

On Sat, 2008-09-06 at 00:50 -0500, michaelc@cs.wisc.edu wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> To prevent a reques from running forever, scsi-ml checks if
> a command has been running req->timeout * cmd->allowed + 1 seconds.
> If it has, scsi-ml will fail the request. From what I can tell
> it looks like in Jen's tree that the block layer is not setting
> this value, so a command is failed right away a lot of times because
> wait_for in scsi_softirq_done is zero seconds. It is only set by ioctls
> and passthrough.
> 
> This patch just copies the q timeout that is used, so any one
> checking it will still be able to see it.
> 
> For the scsi-ml req->timeout usage, I think we can move the retries and
> the infinite retry check from the scsi layer to the block layer. I was
> not sure if we should do that in this patch or a separate one. I can
> cook up another patch if people want. If it would be possible to do
> that after MikeA's patches it would probably be best though since
> he is working on that code too.

Sure, basically move all responsibility for retries to block.  That will
require a few other things like abstracting scsi_decide_disposition.  I
actually had an impression from one of the storage summits that someone
was working on this.

James

Re: [PATCH 1/1] block: set req->timeout in blk_add_timer

[Mike Christie]

James Bottomley wrote:
> On Sat, 2008-09-06 at 00:50 -0500, michaelc@cs.wisc.edu wrote:
>> From: Mike Christie <mchristi@redhat.com>
>>
>> To prevent a reques from running forever, scsi-ml checks if
>> a command has been running req->timeout * cmd->allowed + 1 seconds.
>> If it has, scsi-ml will fail the request. From what I can tell
>> it looks like in Jen's tree that the block layer is not setting
>> this value, so a command is failed right away a lot of times because
>> wait_for in scsi_softirq_done is zero seconds. It is only set by ioctls
>> and passthrough.
>>
>> This patch just copies the q timeout that is used, so any one
>> checking it will still be able to see it.
>>
>> For the scsi-ml req->timeout usage, I think we can move the retries and
>> the infinite retry check from the scsi layer to the block layer. I was
>> not sure if we should do that in this patch or a separate one. I can
>> cook up another patch if people want. If it would be possible to do
>> that after MikeA's patches it would probably be best though since
>> he is working on that code too.
> 
> Sure, basically move all responsibility for retries to block.  That will
> require a few other things like abstracting scsi_decide_disposition.  I
> actually had an impression from one of the storage summits that someone
> was working on this.
> 

I remember talking about at the first summit now. Maybe I was doing it 
as part of the move the command timer to the block layer patches and 
request based multipath stuff, or maybe someone else was as part of the 
request_queue groups stuff. I will look into it again. Thanks.

Re: [PATCH 1/1] block: set req->timeout in blk_add_timer

[Jens Axboe]

On Sat, Sep 06 2008, michaelc@cs.wisc.edu wrote:
> From: Mike Christie <mchristi@redhat.com>
> 
> To prevent a reques from running forever, scsi-ml checks if
> a command has been running req->timeout * cmd->allowed + 1 seconds.
> If it has, scsi-ml will fail the request. From what I can tell
> it looks like in Jen's tree that the block layer is not setting
> this value, so a command is failed right away a lot of times because
> wait_for in scsi_softirq_done is zero seconds. It is only set by ioctls
> and passthrough.
> 
> This patch just copies the q timeout that is used, so any one
> checking it will still be able to see it.
> 
> For the scsi-ml req->timeout usage, I think we can move the retries and
> the infinite retry check from the scsi layer to the block layer. I was
> not sure if we should do that in this patch or a separate one. I can
> cook up another patch if people want. If it would be possible to do
> that after MikeA's patches it would probably be best though since
> he is working on that code too.
> 
> This patch was made over James's scsi post tree.

Thanks Mike, I'll merge it into the original patch.

-- 
Jens Axboe