[PATCH] blk-exec-assign-endio-before-queue-dead-check

[Muthu Kumar]

On Wed, Jun 6, 2012 at 7:40 PM, Tejun Heo <tj@xxxxxxxxxx> 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 <muthur@xxxxxxxxx>
CC: Tejun Heo <tj@xxxxxxxxxx>
CC: Jens Axboe <axboe@xxxxxxxxx>
CC: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>

-----------------------
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/