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 <[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 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;
}
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 <[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 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.
On Sat, Jun 9, 2012 at 5:47 AM, Bart Van Assche <[email protected]> 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.
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.
On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <[email protected]> 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.
On Mon, Jun 11, 2012 at 10:33 AM, Muthu Kumar <[email protected]> wrote:
> On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <[email protected]> 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.
On Mon, Jun 11, 2012 at 02:23:23PM -0700, Muthu Kumar wrote:
> On Mon, Jun 11, 2012 at 10:33 AM, Muthu Kumar <[email protected]> wrote:
> > On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <[email protected]> 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
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 <[email protected]> wrote:
>>> On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <[email protected]> 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 <[email protected]>
Reported-by: Muthukumar Ratty <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Tejun Heo <[email protected]>
---
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
On Wed, Jun 20, 2012 at 11:53 AM, Bart Van Assche <[email protected]> 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 <[email protected]> wrote:
>>>> On Sun, Jun 10, 2012 at 10:40 AM, Bart Van Assche <[email protected]> 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 <[email protected]>
> Reported-by: Muthukumar Ratty <[email protected]>
Hmm... Shouldn't this be:
Signed-off-by: Muthukumar Ratty <[email protected]>
Tested-by: Bart Van Assche <[email protected]>
???
> Cc: Jens Axboe <[email protected]>
> Cc: Tejun Heo <[email protected]>
> ---
> ?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
>
>
Hello,
On Wed, Jun 20, 2012 at 5:53 PM, Muthu Kumar <[email protected]> wrote:
>> Signed-off-by: Bart Van Assche <[email protected]>
>> Reported-by: Muthukumar Ratty <[email protected]>
>
> Hmm... Shouldn't this be:
>
> Signed-off-by: Muthukumar Ratty <[email protected]>
> Tested-by: Bart Van Assche <[email protected]>
Yeah and From: tag at the beginning too. Bart?
Thanks.
--
tejun
On 06/21/12 00:53, Muthu Kumar wrote:
> Hmm... Shouldn't this be:
>
> Signed-off-by: Muthukumar Ratty <[email protected]>
> Tested-by: Bart Van Assche <[email protected]>
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.
On Wed, Jun 20, 2012 at 10:40 PM, Bart Van Assche <[email protected]> wrote:
> On 06/21/12 00:53, Muthu Kumar wrote:
>
>> Hmm... Shouldn't this be:
>>
>> Signed-off-by: Muthukumar Ratty <[email protected]>
>> Tested-by: Bart Van Assche <[email protected]>
>
>
> 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.
On 06/21/12 21:40, Muthu Kumar wrote:
> On Wed, Jun 20, 2012 at 10:40 PM, Bart Van Assche <[email protected]> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Bart Van Assche <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: James Bottomley <[email protected]>
---
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
On Fri, Jun 22, 2012 at 12:20 AM, Bart Van Assche <[email protected]> wrote:
> On 06/21/12 21:40, Muthu Kumar wrote:
>
>> On Wed, Jun 20, 2012 at 10:40 PM, Bart Van Assche <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Bart Van Assche <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: James Bottomley <[email protected]>
> ---
> ?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
>
>