2012-06-07 06:27:45

by Muthu Kumar

[permalink] [raw]

Subject: [PATCH] block: In blk_execute_rq_nowait, init rq->end_io before checking for dead queue.

---

On Wed, Jun 6, 2012 at 11:16 PM, Tejun Heo <[email protected]> wrote:
> 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).

No problem. Changed the subject and also added the info about queue
lock in the description. Please check.

-----------------------
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.

Also, in case of dead queue, call the completion routine with queue->lock held.


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 */


------------------------------------







>
> Thanks.
>
> --
> tejun

---

Attachments:

blk-exec-assign-endio-before-queue-dead-check.patch (1.22 kB)

2012-06-08 15:37:44

by Muthu Kumar

[permalink] [raw]

Subject: Re: [PATCH] block: In blk_execute_rq_nowait, init rq->end_io before checking for dead queue.

---

Tejun, Jens,
This is a real problem, right? Should we fix it differently?

Regards,
Muthu

On Wed, Jun 6, 2012 at 11:27 PM, Muthu Kumar <[email protected]> wrote:
> On Wed, Jun 6, 2012 at 11:16 PM, Tejun Heo <[email protected]> wrote:
>> 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).
>
> No problem. Changed the subject and also added the info about queue
> lock in the description. Please check.
>
> -----------------------
> 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.
>
> Also, in case of dead queue, call the completion routine with queue->lock held.
>
>
> 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 */
>
>
> ------------------------------------
>
>
>
>
>
>
>
>>
>> Thanks.
>>
>> --
>> tejun