On Wed, Jun 6, 2012 at 7:40 PM, Tejun Heo <[email protected]> wrote:
> On Wed, Jun 06, 2012 at 02:24:35PM -0700, Muthu Kumar wrote:
>> How about this change?
>>
>> diff --git a/block/blk-exec.c b/block/blk-exec.c
>> index fb2cbd5..6bf5c0b 100644
>> --- a/block/blk-exec.c
>> +++ b/block/blk-exec.c
>> @@ -56,8 +56,10 @@ void blk_execute_rq_nowait(struct request_queue *q, struct ge
>> if (unlikely(blk_queue_dead(q))) {
>> spin_unlock_irq(q->queue_lock);
>> rq->errors = -ENXIO;
>> - if (rq->end_io)
>> - rq->end_io(rq, rq->errors);
>> + if (done)
>> + done(rq, rq->errors);
>> + else if (rq->end_io) //XXX Not sure if this check and end_io
>> + rq->end_io(rq, rq->errors);
>> return;
>> }
>>
>> Only one driver - sx8.c, doesn't set done() function and every one
>> else expects done() to be called with error.
>
> Looks like the bug there is rq->rq_disk and rq->end_io assignments
> happening after the queue_dead check. Just move the two lines before
> queue_head check?
>
> Thanks.
Thought about that. But the problem is, original rq->end_io is not
saved before the new assignment. But exploring further, I guess its ok
in this use case.
One more thing to consider is, the completion function is called from
the same calling context here. As far as my check, it looks ok. Let me
know if you think otherwise.
Anyway, patch attached (as well as inline).
Regards,
Muthu
-----------------------
blk-exec.c: In blk_execute_rq_nowait(), assign rq->endio,rq_disk
before queue dead check.
Signed-off-by: Muthukumar Ratty <[email protected]>
CC: Tejun Heo <[email protected]>
CC: Jens Axboe <[email protected]>
CC: James Bottomley <[email protected]>
-----------------------
diff --git a/block/blk-exec.c b/block/blk-exec.c
index fb2cbd5..f8b00c7 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -53,6 +53,9 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gen
WARN_ON(irqs_disabled());
spin_lock_irq(q->queue_lock);
+ rq->rq_disk = bd_disk;
+ rq->end_io = done;
+
if (unlikely(blk_queue_dead(q))) {
spin_unlock_irq(q->queue_lock);
rq->errors = -ENXIO;
@@ -61,8 +64,6 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gen
return;
}
- rq->rq_disk = bd_disk;
- rq->end_io = done;
__elv_add_request(q, rq, where);
__blk_run_queue(q);
/* the queue is stopped so it won't be run */
-------------------------------------------------
>
> --
> tejun
>
> Thanks.
>
> --
> tejun
Hello,
On Thu, Jun 7, 2012 at 2:35 PM, Muthu Kumar <[email protected]> wrote:
>> Looks like the bug there is rq->rq_disk and rq->end_io assignments
>> happening after the queue_dead check. ?Just move the two lines before
>> queue_head check?
>
> Thought about that. But the problem is, original rq->end_io is not
> saved before the new assignment. But exploring further, I guess its ok
> in this use case.
It's supposed to be overridden, so I don't think that matters.
> One more thing to consider is, the completion function is called from
> the same calling context here. As far as my check, it looks ok. Let me
> know if you think otherwise.
Not sure what you mean.
> Anyway, patch attached (as well as inline).
>
> Regards,
> Muthu
>
> -----------------------
> blk-exec.c: In blk_execute_rq_nowait(), assign rq->endio,rq_disk
> before queue dead check.
Needs way more description.
Thanks.
--
tejun
On Wed, Jun 6, 2012 at 10:37 PM, Tejun Heo <[email protected]> wrote:
> Hello,
>
> On Thu, Jun 7, 2012 at 2:35 PM, Muthu Kumar <[email protected]> wrote:
>>> Looks like the bug there is rq->rq_disk and rq->end_io assignments
>>> happening after the queue_dead check. ?Just move the two lines before
>>> queue_head check?
>>
>> Thought about that. But the problem is, original rq->end_io is not
>> saved before the new assignment. But exploring further, I guess its ok
>> in this use case.
>
> It's supposed to be overridden, so I don't think that matters.
Agree.
>
>> One more thing to consider is, the completion function is called from
>> the same calling context here. As far as my check, it looks ok. Let me
>> know if you think otherwise.
>
> Not sure what you mean.
If there is no error, then the completion routine is called in a
different context - rq completion context. But here, we call the
completion routine in the same context of the caller.
>
>> Anyway, patch attached (as well as inline).
>>
>> Regards,
>> Muthu
>>
>> -----------------------
>> blk-exec.c: In blk_execute_rq_nowait(), assign rq->endio,rq_disk
>> before queue dead check.
>
> Needs way more description.
>
Let me try.
blk-exec.c: In blk_execute_rq_nowait(), if the queue is dead, call to
done() routine is not made. That will result in blk_execute_rq() stuck
in wait_for_completion(). Avoid this by initializing rq->end_io to
done() routine before we check for dead queue.
> Thanks.
>
> --
> tejun
Hello,
On Thu, Jun 7, 2012 at 2:42 PM, Muthu Kumar <[email protected]> wrote:
>>> One more thing to consider is, the completion function is called from
>>> the same calling context here. As far as my check, it looks ok. Let me
>>> know if you think otherwise.
>>
>> Not sure what you mean.
>
> If there is no error, then the completion routine is called in a
> different context - rq completion context. But here, we call the
> completion routine in the same context of the caller.
Ah, okay. I think the only problem there would be that the end_io
callback is being called outside queue lock. Can you please take care
of that too?
> blk-exec.c: In blk_execute_rq_nowait(), if the queue is dead, call to
> done() routine is not made. That will result in blk_execute_rq() stuck
> in wait_for_completion(). Avoid this by initializing rq->end_io to
> done() routine before we check for dead queue.
Yeah, sounds about right.
Thanks.
--
tejun
On Wed, Jun 6, 2012 at 10:53 PM, Tejun Heo <[email protected]> wrote:
> Hello,
>
> On Thu, Jun 7, 2012 at 2:42 PM, Muthu Kumar <[email protected]> wrote:
>>>> One more thing to consider is, the completion function is called from
>>>> the same calling context here. As far as my check, it looks ok. Let me
>>>> know if you think otherwise.
>>>
>>> Not sure what you mean.
>>
>> If there is no error, then the completion routine is called in a
>> different context - rq completion context. But here, we call the
>> completion routine in the same context of the caller.
>
> Ah, okay. I think the only problem there would be that the end_io
> callback is being called outside queue lock. Can you please take care
> of that too?
>
Revised patch (below as well as attached)
>> blk-exec.c: In blk_execute_rq_nowait(), if the queue is dead, call to
>> done() routine is not made. That will result in blk_execute_rq() stuck
>> in wait_for_completion(). Avoid this by initializing rq->end_io to
>> done() routine before we check for dead queue.
>
> Yeah, sounds about right.
>
> Thanks.
>
> --
> tejun
-----------------------
blk-exec.c: In blk_execute_rq_nowait(), if the queue is dead, call to
done() routine is not made. That will result in blk_execute_rq() stuck
in wait_for_completion(). Avoid this by initializing rq->end_io to
done() routine before we check for dead queue.
Signed-off-by: Muthukumar Ratty <[email protected]>
CC: Tejun Heo <[email protected]>
CC: Jens Axboe <[email protected]>
CC: James Bottomley <[email protected]>
-----------------------
diff --git a/block/blk-exec.c b/block/blk-exec.c
index fb2cbd5..284bf56 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -53,16 +53,17 @@ void blk_execute_rq_nowait(struct request_queue
*q, struct gendisk *bd_disk,
WARN_ON(irqs_disabled());
spin_lock_irq(q->queue_lock);
+ rq->rq_disk = bd_disk;
+ rq->end_io = done;
+
if (unlikely(blk_queue_dead(q))) {
- spin_unlock_irq(q->queue_lock);
rq->errors = -ENXIO;
if (rq->end_io)
rq->end_io(rq, rq->errors);
+ spin_unlock_irq(q->queue_lock);
return;
}
- rq->rq_disk = bd_disk;
- rq->end_io = done;
__elv_add_request(q, rq, where);
__blk_run_queue(q);
/* the queue is stopped so it won't be run */
Hello, again.
On Thu, Jun 7, 2012 at 3:13 PM, Muthu Kumar <[email protected]> wrote:
> blk-exec.c: In blk_execute_rq_nowait(), if the queue is dead, call to
> done() routine is not made. That will result in blk_execute_rq() stuck
> in wait_for_completion(). Avoid this by initializing rq->end_io to
> done() routine before we check for dead queue.
Sorry to be nit-picky but Jens would appreciate proper subject and it
would also be nice to mention the locking change (or even separate
that into a separate patch).
Thanks.
--
tejun