Re: [PATCH 0/2] blk request timeout handler: mv scsi timer code to block layer

Re: [PATCH 0/2] blk request timeout handler: mv scsi timer code to block layer

[Jens Axboe]

On Tue, Jul 25 2006, Mike Christie wrote:
> For the request based multipath I thought I would need to run some code
> when a command times out. I did not want to duplicate the scsi code, so
> I did the following patches which move the scsi timer code to the block
> layer then convert scsi.
> 
> I have tested the scsi_error.c and normal paths with iscsi. And, I have
> tested the normal IO paths with libata. Since libata uses the strategy
> handler it needs to be tested a lot more. Some of the drivers that were
> touching the timeout_per_command field need to be compile tested still
> too. I converted them, but I think some still need a "#include
> blkdev.h".
> 
> The patches only move the scsi timer code to the block layer and hook it
> in so others can use it. I have not started on the abort, reset and
> quiesce code since it is not really needed for multipath. I wanted to
> see if the timer code move was ok on its own without the rest of the
> scsi eh move because I do not want to manage the patches out of tree
> with the other request multipath patches. I also wanted to check if the
> scsi timer code was ok in general. Maybe scsi got it wrong and needed to
> be rewritten :)

Excellent, one item off my TODO list :-). I had pending code, but not
completed yet.

I had intended to make the timer addition/deletion implicit from the
activate/deactive rq paths, both to have it happen automatically and
from a cleanliness POV. That makes the timer only active when the
request is in the driver, and should also make the deletion implicit for
when the request gets requeued.

-- 
Jens Axboe

[PATCH 0/2] blk request timeout handler: mv scsi timer code to block layer

[Mike Christie]

For the request based multipath I thought I would need to run some code
when a command times out. I did not want to duplicate the scsi code, so
I did the following patches which move the scsi timer code to the block
layer then convert scsi.

I have tested the scsi_error.c and normal paths with iscsi. And, I have
tested the normal IO paths with libata. Since libata uses the strategy
handler it needs to be tested a lot more. Some of the drivers that were
touching the timeout_per_command field need to be compile tested still
too. I converted them, but I think some still need a "#include
blkdev.h".

The patches only move the scsi timer code to the block layer and hook it
in so others can use it. I have not started on the abort, reset and
quiesce code since it is not really needed for multipath. I wanted to
see if the timer code move was ok on its own without the rest of the
scsi eh move because I do not want to manage the patches out of tree
with the other request multipath patches. I also wanted to check if the
scsi timer code was ok in general. Maybe scsi got it wrong and needed to
be rewritten :)

Both patches were made over 2.6.18-rc2.

Re: [PATCH 0/2] blk request timeout handler: mv scsi timer code to block layer

[Mike Christie]

Jens Axboe wrote:
> On Tue, Jul 25 2006, Mike Christie wrote:
>> For the request based multipath I thought I would need to run some code
>> when a command times out. I did not want to duplicate the scsi code, so
>> I did the following patches which move the scsi timer code to the block
>> layer then convert scsi.
>>
>> I have tested the scsi_error.c and normal paths with iscsi. And, I have
>> tested the normal IO paths with libata. Since libata uses the strategy
>> handler it needs to be tested a lot more. Some of the drivers that were
>> touching the timeout_per_command field need to be compile tested still
>> too. I converted them, but I think some still need a "#include
>> blkdev.h".
>>
>> The patches only move the scsi timer code to the block layer and hook it
>> in so others can use it. I have not started on the abort, reset and
>> quiesce code since it is not really needed for multipath. I wanted to
>> see if the timer code move was ok on its own without the rest of the
>> scsi eh move because I do not want to manage the patches out of tree
>> with the other request multipath patches. I also wanted to check if the
>> scsi timer code was ok in general. Maybe scsi got it wrong and needed to
>> be rewritten :)
> 
> Excellent, one item off my TODO list :-). I had pending code, but not
> completed yet.
> 
> I had intended to make the timer addition/deletion implicit from the
> activate/deactive rq paths, both to have it happen automatically and
> from a cleanliness POV. That makes the timer only active when the
> request is in the driver, and should also make the deletion implicit for
> when the request gets requeued.
> 

Ok I did that, almost. For the normal request_fn/dequeue, requeue, and
blk softiriq completion paths the block layer handles all the timer
addition, deletion and restarting. There is one nasty path in the scsi,
where we need to requeue the command only if the timer has not expired
and for that I cheated and allowed scsi to do the blk_delete_timer() so
it could check the return value. I will work on fixing that case for the
next resend of the patches.

Re: [PATCH 0/2] blk request timeout handler: mv scsi timer code to block layer

[Mike Christie]

Mike Christie wrote:
> Jens Axboe wrote:
>> On Tue, Jul 25 2006, Mike Christie wrote:
>>> For the request based multipath I thought I would need to run some code
>>> when a command times out. I did not want to duplicate the scsi code, so
>>> I did the following patches which move the scsi timer code to the block
>>> layer then convert scsi.
>>>
>>> I have tested the scsi_error.c and normal paths with iscsi. And, I have
>>> tested the normal IO paths with libata. Since libata uses the strategy
>>> handler it needs to be tested a lot more. Some of the drivers that were
>>> touching the timeout_per_command field need to be compile tested still
>>> too. I converted them, but I think some still need a "#include
>>> blkdev.h".
>>>
>>> The patches only move the scsi timer code to the block layer and hook it
>>> in so others can use it. I have not started on the abort, reset and
>>> quiesce code since it is not really needed for multipath. I wanted to
>>> see if the timer code move was ok on its own without the rest of the
>>> scsi eh move because I do not want to manage the patches out of tree
>>> with the other request multipath patches. I also wanted to check if the
>>> scsi timer code was ok in general. Maybe scsi got it wrong and needed to
>>> be rewritten :)
>> Excellent, one item off my TODO list :-). I had pending code, but not
>> completed yet.
>>
>> I had intended to make the timer addition/deletion implicit from the
>> activate/deactive rq paths, both to have it happen automatically and
>> from a cleanliness POV. That makes the timer only active when the
>> request is in the driver, and should also make the deletion implicit for
>> when the request gets requeued.
>>
> 
> Ok I did that, almost. For the normal request_fn/dequeue, requeue, and
> blk softiriq completion paths the block layer handles all the timer
> addition, deletion and restarting. There is one nasty path in the scsi,

Oops, during testing and my own code review I noticed there was also
some places in the hotplug removal error paths that I used the wrong
block layer function and did not delete the timer. I will fix those up
as well.