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

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

[Muthu Kumar]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

Since the queue->lock case is taken care of in the following patch:

(http://www.spinics.net/lists/linux-scsi/msg59845.html [PATCH 1/4]
block: Fix race on request_queue.end_io invocations)

Updated the patch with just the end_io assignment before dead queue 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.

Signed-off-by: Muthukumar Ratty <muthur@gmail.com>
CC: Tejun Heo <tj@kernel.org>
CC: Jens Axboe <axboe@kernel.dk>
CC: James Bottomley <James.Bottomley@hansenpartnership.com>

-------------
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 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;
@@ -61,8 +64,6 @@ void blk_execute_rq_nowait(struct request_queue *q,
struct gendisk *bd_disk,
                return;
        }

[-- Attachment #2: blk-exec-assign-endio-before-queue-dead-check.patch --]
[-- Type: application/octet-stream, Size: 1164 bytes --]


-------------
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 <muthur@gmail.com>
CC: Tejun Heo <tj@kernel.org>
CC: Jens Axboe <axboe@kernel.dk>
CC: James Bottomley <James.Bottomley@hansenpartnership.com>

-------------
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 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;
@@ -61,8 +64,6 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 		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 */
-------------

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

[Bart Van Assche]

On 06/09/12 00:10, Muthu Kumar wrote:

> Since the queue->lock case is taken care of in the following patch:
> 
> (http://www.spinics.net/lists/linux-scsi/msg59845.html [PATCH 1/4]
> block: Fix race on request_queue.end_io invocations)
> 
> Updated the patch with just the end_io assignment before dead queue 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.
> 
> Signed-off-by: Muthukumar Ratty <muthur@gmail.com>
> CC: Tejun Heo <tj@kernel.org>
> CC: Jens Axboe <axboe@kernel.dk>
> CC: James Bottomley <James.Bottomley@hansenpartnership.com>
> 
> -------------
> 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 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;
> @@ -61,8 +64,6 @@ void blk_execute_rq_nowait(struct request_queue *q,
> struct gendisk *bd_disk,
>                 return;
>         }


This patch does not apply since it's line-wrapped and a part is missing
in the second hunk. However, if I had overlooked your original patch
then I want to apologize.

If you can resend your original patch including the locking changes then
I'll test it further.

Bart.

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

[Muthu Kumar]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

On Sat, Jun 9, 2012 at 5:47 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On 06/09/12 00:10, Muthu Kumar wrote:
>
>> Since the queue->lock case is taken care of in the following patch:
>>
>> (http://www.spinics.net/lists/linux-scsi/msg59845.html [PATCH 1/4]
>> block: Fix race on request_queue.end_io invocations)
>>
>> Updated the patch with just the end_io assignment before dead queue check.
>>
>>
>
>
> This patch does not apply since it's line-wrapped and a part is missing
> in the second hunk. However, if I had overlooked your original patch
> then I want to apologize.
>

Please use the attached file since gmail messes with the line wraps.

> If you can resend your original patch including the locking changes then
> I'll test it further.
>

Locking change is the one you posted already (the link above). Anyway,
I have the attached patch *including* the locking change. Original
mail has attachment without locking change. Please use whatever you
need.
Thanks for testing.

Regards,
Muthu


> Bart.

[-- Attachment #2: blk-exec-patch-to-bart.patch --]
[-- Type: text/x-diff, Size: 682 bytes --]

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

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

[Bart Van Assche]

On 06/09/12 23:57, Muthu Kumar wrote:

> Locking change is the one you posted already (the link above). Anyway,
> I have the attached patch *including* the locking change. Original
> mail has attachment without locking change. Please use whatever you
> need.


While we are at it ... the rq->rq_disk and rq->end_io assignments can be
performed safely before the spinlock is taken, isn't it ?

Bart.

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

[Muthu Kumar]

On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On 06/09/12 23:57, Muthu Kumar wrote:
>
>> Locking change is the one you posted already (the link above). Anyway,
>> I have the attached patch *including* the locking change. Original
>> mail has attachment without locking change. Please use whatever you
>> need.
>
>
> While we are at it ... the rq->rq_disk and rq->end_io assignments can be
> performed safely before the spinlock is taken, isn't it ?
>

Yes.. that spinlock is to protect the q.


> Bart.

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

[Muthu Kumar]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On Mon, Jun 11, 2012 at 10:33 AM, Muthu Kumar <muthu.lkml@gmail.com> wrote:
> On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <bvanassche@acm.org> wrote:
>> On 06/09/12 23:57, Muthu Kumar wrote:
>>
>>> Locking change is the one you posted already (the link above). Anyway,
>>> I have the attached patch *including* the locking change. Original
>>> mail has attachment without locking change. Please use whatever you
>>> need.
>>
>>
>> While we are at it ... the rq->rq_disk and rq->end_io assignments can be
>> performed safely before the spinlock is taken, isn't it ?
>>
>
> Yes.. that spinlock is to protect the q.
>

Attached patch with assignment performed before taking the spinlock.




>
>> Bart.

[-- Attachment #2: blk-exec-assign-endio-to-bart-v2.patch --]
[-- Type: application/octet-stream, Size: 705 bytes --]

diff --git a/block/blk-exec.c b/block/blk-exec.c
index fb2cbd5..00a72da 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -51,6 +51,10 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
 
 	WARN_ON(irqs_disabled());
+
+	rq->rq_disk = bd_disk;
+	rq->end_io = done;
+
 	spin_lock_irq(q->queue_lock);
 
 	if (unlikely(blk_queue_dead(q))) {
@@ -61,8 +65,6 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 		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 */

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

[Tejun Heo]

On Mon, Jun 11, 2012 at 02:23:23PM -0700, Muthu Kumar wrote:
> On Mon, Jun 11, 2012 at 10:33 AM, Muthu Kumar <muthu.lkml@gmail.com> wrote:
> > On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> >> On 06/09/12 23:57, Muthu Kumar wrote:
> >>
> >>> Locking change is the one you posted already (the link above). Anyway,
> >>> I have the attached patch *including* the locking change. Original
> >>> mail has attachment without locking change. Please use whatever you
> >>> need.
> >>
> >>
> >> While we are at it ... the rq->rq_disk and rq->end_io assignments can be
> >> performed safely before the spinlock is taken, isn't it ?
> >>
> >
> > Yes.. that spinlock is to protect the q.
> >
> 
> Attached patch with assignment performed before taking the spinlock.

This looks correct to me.  Bart, can you please include this patch in
your series and repost the series?

Thank you.

-- 
tejun

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

[Bart Van Assche]

On 06/18/12 22:42, Tejun Heo wrote:

> On Mon, Jun 11, 2012 at 02:23:23PM -0700, Muthu Kumar wrote:
>> On Mon, Jun 11, 2012 at 10:33 AM, Muthu Kumar <muthu.lkml@gmail.com> wrote:
>>> On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <bvanassche@acm.org> wrote:
>>>> On 06/09/12 23:57, Muthu Kumar wrote:
>>>>
>>>>> Locking change is the one you posted already (the link above). Anyway,
>>>>> I have the attached patch *including* the locking change. Original
>>>>> mail has attachment without locking change. Please use whatever you
>>>>> need.
>>>>
>>>> While we are at it ... the rq->rq_disk and rq->end_io assignments can be
>>>> performed safely before the spinlock is taken, isn't it ?
>>>
>>> Yes.. that spinlock is to protect the q.
>>
>> Attached patch with assignment performed before taking the spinlock.
> 
> This looks correct to me.  Bart, can you please include this patch in
> your series and repost the series?


I'll start testing the patch below:

[PATCH] block: Fix blk_execute_rq_nowait() dead queue handling

Make sure that blk_execute_rq_nowait() invokes the proper end_io
function if the queue is dead and also that this call is performed
with the queue lock held. Found this through source code review.

Notes:
- Since blk_execute_rq_nowait() must not be invoked from interrupt
  context it is safe to invoke end_io directly from this function.
- blk_finish_request() already invokes request.end_io with the
  queue lock held.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reported-by: Muthukumar Ratty <muthur@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Tejun Heo <tj@kernel.org>
---
 block/blk-exec.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/blk-exec.c b/block/blk-exec.c
index fb2cbd5..c0fd83a 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -42,7 +42,8 @@ static void blk_end_sync_rq(struct request *rq, int error)
  *
  * Description:
  *    Insert a fully prepared request at the back of the I/O scheduler queue
- *    for execution.  Don't wait for completion.
+ *    for execution.  Don't wait for completion.  This function may invoke
+ *    rq->end_io directly.
  */
 void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 			   struct request *rq, int at_head,
@@ -51,18 +52,20 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
 
 	WARN_ON(irqs_disabled());
+
+	rq->rq_disk = bd_disk;
+	rq->end_io = done;
+
 	spin_lock_irq(q->queue_lock);
 
 	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 */
-- 
1.7.7

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

[Muthu Kumar]

On Wed, Jun 20, 2012 at 11:53 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On 06/18/12 22:42, Tejun Heo wrote:
>
>> On Mon, Jun 11, 2012 at 02:23:23PM -0700, Muthu Kumar wrote:
>>> On Mon, Jun 11, 2012 at 10:33 AM, Muthu Kumar <muthu.lkml@gmail.com> wrote:
>>>> On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <bvanassche@acm.org> wrote:
>>>>> On 06/09/12 23:57, Muthu Kumar wrote:
>>>>>
>>>>>> Locking change is the one you posted already (the link above). Anyway,
>>>>>> I have the attached patch *including* the locking change. Original
>>>>>> mail has attachment without locking change. Please use whatever you
>>>>>> need.
>>>>>
>>>>> While we are at it ... the rq->rq_disk and rq->end_io assignments can be
>>>>> performed safely before the spinlock is taken, isn't it ?
>>>>
>>>> Yes.. that spinlock is to protect the q.
>>>
>>> Attached patch with assignment performed before taking the spinlock.
>>
>> This looks correct to me.  Bart, can you please include this patch in
>> your series and repost the series?
>
>
> I'll start testing the patch below:
>
> [PATCH] block: Fix blk_execute_rq_nowait() dead queue handling
>
> Make sure that blk_execute_rq_nowait() invokes the proper end_io
> function if the queue is dead and also that this call is performed
> with the queue lock held. Found this through source code review.
>
> Notes:
> - Since blk_execute_rq_nowait() must not be invoked from interrupt
>  context it is safe to invoke end_io directly from this function.
> - blk_finish_request() already invokes request.end_io with the
>  queue lock held.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Reported-by: Muthukumar Ratty <muthur@gmail.com>

Hmm... Shouldn't this be:

Signed-off-by: Muthukumar Ratty <muthur@gmail.com>
Tested-by: Bart Van Assche <bvanassche@acm.org>

???

> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Tejun Heo <tj@kernel.org>
> ---
>  block/blk-exec.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/block/blk-exec.c b/block/blk-exec.c
> index fb2cbd5..c0fd83a 100644
> --- a/block/blk-exec.c
> +++ b/block/blk-exec.c
> @@ -42,7 +42,8 @@ static void blk_end_sync_rq(struct request *rq, int error)
>  *
>  * Description:
>  *    Insert a fully prepared request at the back of the I/O scheduler queue
> - *    for execution.  Don't wait for completion.
> + *    for execution.  Don't wait for completion.  This function may invoke
> + *    rq->end_io directly.
>  */
>  void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
>                           struct request *rq, int at_head,
> @@ -51,18 +52,20 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
>        int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
>
>        WARN_ON(irqs_disabled());
> +
> +       rq->rq_disk = bd_disk;
> +       rq->end_io = done;
> +
>        spin_lock_irq(q->queue_lock);
>
>        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);

This part already went in, its here only for testing - So in the final
patch, before applying this should be removed.

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

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

[Tejun Heo]

Hello,

On Wed, Jun 20, 2012 at 5:53 PM, Muthu Kumar <muthu.lkml@gmail.com> wrote:
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> Reported-by: Muthukumar Ratty <muthur@gmail.com>
>
> Hmm... Shouldn't this be:
>
> Signed-off-by: Muthukumar Ratty <muthur@gmail.com>
> Tested-by: Bart Van Assche <bvanassche@acm.org>

Yeah and From: tag at the beginning too. Bart?

Thanks.

-- 
tejun

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

[Bart Van Assche]

On 06/21/12 00:53, Muthu Kumar wrote:

> Hmm... Shouldn't this be:
> 
> Signed-off-by: Muthukumar Ratty <muthur@gmail.com>
> Tested-by: Bart Van Assche <bvanassche@acm.org>


I'll modify that part of the patch if you prefer to be mentioned as the
author. I hadn't done that because you hadn't posted the patch in its
entirety yet.

 
> This part already went in, its here only for testing - So in the final
> patch, before applying this should be removed.


When did it go in ? As far as I can see that patch is not yet present in
Linus' tree nor in James' tree.

Bart.

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

[Muthu Kumar]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

On Wed, Jun 20, 2012 at 10:40 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> On 06/21/12 00:53, Muthu Kumar wrote:
>
>> Hmm... Shouldn't this be:
>>
>> Signed-off-by: Muthukumar Ratty <muthur@gmail.com>
>> Tested-by: Bart Van Assche <bvanassche@acm.org>
>
>
> I'll modify that part of the patch if you prefer to be mentioned as the
> author. I hadn't done that because you hadn't posted the patch in its
> entirety yet.
>
>

Its been in the attachment in all my previous mails, since gmail and
patches in-line doesn't go well. Anyway, I have attached the patch
with this again (v3). Thanks.

>> This part already went in, its here only for testing - So in the final
>> patch, before applying this should be removed.
>
>
> When did it go in ? As far as I can see that patch is not yet present in
> Linus' tree nor in James' tree.
>

My bad. I thought Jens/James took it already... may be not.

Regards,
Muthu

> Bart.

[-- Attachment #2: blk-exec-assign-endio-to-bart-v3.patch --]
[-- Type: application/octet-stream, Size: 1291 bytes --]

--------
[PATCH] block: blk_execute_rq_nowait(): init rq->end_io before checking for dead queue.

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 <muthur@gmail.com>
Tested-by: Bart Van Assche <bvanassche@acm.org>
CC: Tejun Heo <tj@kernel.org>
CC: Jens Axboe <axboe@kernel.dk>
CC: James Bottomley <James.Bottomley@hansenpartnership.com>

--------
diff --git a/block/blk-exec.c b/block/blk-exec.c
index fb2cbd5..00a72da 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -51,6 +51,10 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
 
 	WARN_ON(irqs_disabled());
+
+	rq->rq_disk = bd_disk;
+	rq->end_io = done;
+
 	spin_lock_irq(q->queue_lock);
 
 	if (unlikely(blk_queue_dead(q))) {
@@ -61,8 +65,6 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 		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 */

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

[Bart Van Assche]

On 06/21/12 21:40, Muthu Kumar wrote:

> On Wed, Jun 20, 2012 at 10:40 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>> On 06/21/12 00:53, Muthu Kumar wrote:
>>> This part already went in, its here only for testing - So in the final
>>> patch, before applying this should be removed.
>>
>> When did it go in ? As far as I can see that patch is not yet present in
>> Linus' tree nor in James' tree.
> 
> My bad. I thought Jens/James took it already... may be not.


Do you agree with combining both patches into one (see below) ?

From: Muthukumar Ratty <muthur@gmail.com>
Subject: [PATCH] block: Fix blk_execute_rq_nowait() dead queue handling

If the queue is dead blk_execute_rq_nowait() doesn't invoke the done()
callback function. That will result in blk_execute_rq() being stuck
in wait_for_completion(). Avoid this by initializing rq->end_io to the
done() callback before we check the queue state. Also, make sure the
queue lock is held around the invocation of the done() callback. Found
this through source code review.

Signed-off-by: Muthukumar Ratty <muthur@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
---
 block/blk-exec.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/blk-exec.c b/block/blk-exec.c
index fb2cbd5..8b6dc5b 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -43,6 +43,9 @@ static void blk_end_sync_rq(struct request *rq, int error)
  * Description:
  *    Insert a fully prepared request at the back of the I/O scheduler queue
  *    for execution.  Don't wait for completion.
+ *
+ * Note:
+ *    This function will invoke @done directly if the queue is dead.
  */
 void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 			   struct request *rq, int at_head,
@@ -51,18 +54,20 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
 
 	WARN_ON(irqs_disabled());
+
+	rq->rq_disk = bd_disk;
+	rq->end_io = done;
+
 	spin_lock_irq(q->queue_lock);
 
 	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 */
-- 
1.7.7

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

[Muthu Kumar]

On Fri, Jun 22, 2012 at 12:20 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On 06/21/12 21:40, Muthu Kumar wrote:
>
>> On Wed, Jun 20, 2012 at 10:40 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>>> On 06/21/12 00:53, Muthu Kumar wrote:
>>>> This part already went in, its here only for testing - So in the final
>>>> patch, before applying this should be removed.
>>>
>>> When did it go in ? As far as I can see that patch is not yet present in
>>> Linus' tree nor in James' tree.
>>
>> My bad. I thought Jens/James took it already... may be not.
>
>
> Do you agree with combining both patches into one (see below) ?


Perfect. Thanks.

Regards,
Muthu.









>
> From: Muthukumar Ratty <muthur@gmail.com>
> Subject: [PATCH] block: Fix blk_execute_rq_nowait() dead queue handling
>
> If the queue is dead blk_execute_rq_nowait() doesn't invoke the done()
> callback function. That will result in blk_execute_rq() being stuck
> in wait_for_completion(). Avoid this by initializing rq->end_io to the
> done() callback before we check the queue state. Also, make sure the
> queue lock is held around the invocation of the done() callback. Found
> this through source code review.
>
> Signed-off-by: Muthukumar Ratty <muthur@gmail.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
> ---
>  block/blk-exec.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/block/blk-exec.c b/block/blk-exec.c
> index fb2cbd5..8b6dc5b 100644
> --- a/block/blk-exec.c
> +++ b/block/blk-exec.c
> @@ -43,6 +43,9 @@ static void blk_end_sync_rq(struct request *rq, int error)
>  * Description:
>  *    Insert a fully prepared request at the back of the I/O scheduler queue
>  *    for execution.  Don't wait for completion.
> + *
> + * Note:
> + *    This function will invoke @done directly if the queue is dead.
>  */
>  void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
>                           struct request *rq, int at_head,
> @@ -51,18 +54,20 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
>        int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
>
>        WARN_ON(irqs_disabled());
> +
> +       rq->rq_disk = bd_disk;
> +       rq->end_io = done;
> +
>        spin_lock_irq(q->queue_lock);
>
>        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 */
> --
> 1.7.7
>
>