block: fix oops with block tag queueing

PATCH In review James Bottomley 4 participants 7 messages May 20, 2009 – May 27, 2009

Subsystems (2)

SCSI Block layer

lore

Share & tools

lore

James Bottomley

author maintainer

2009-05-20 17:06 UTC #0 lore raw

PATCH 1/1 block: fix oops with block tag queueing

commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6

Author: Tejun Heo <[email protected]>
Date:   Fri May 8 11:54:16 2009 +0900

block: implement and enforce request peek/start/fetch

Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req(). Unfortunately, this checks whether req->queuelist is empty. This list is doing double duty both as the queue list and the tag list, so tagged requests come in here with this not empty and boom (the tag list is emptied by blk_queue_end_tag() lower down).

Fix this by moving the BUG_ON to below the end tag we also seem vulnerable to this in blk_requeue_request() as well. I think all uses of blk_queued_rq() need auditing because the check is clearly wrong in the tagged case.

Signed-off-by James Bottomley <[email protected]>

---
You might like to combine this with the above commit. Otherwise we'll have a bisect panic with adaptec and other drivers in the interval between the two commits.

block/blk-core.c +4 -4

index 05c2520..3ea7242 100644
@@ -956,8 +956,6 @@ EXPORT_SYMBOL(blk_make_request);
  */
 void blk_requeue_request(struct request_queue *q, struct request *rq)
 {
-	BUG_ON(blk_queued_rq(rq));
-
 	blk_delete_timer(rq);
 	blk_clear_rq_complete(rq);
 	trace_block_rq_requeue(q, rq);

@@ -965,6 +963,8 @@ void blk_requeue_request(struct request_queue *q, struct request *rq)
 	if (blk_rq_tagged(rq))
 		blk_queue_end_tag(q, rq);
 
+	BUG_ON(blk_queued_rq(rq));
+
 	elv_requeue_request(q, rq);
 }
 EXPORT_SYMBOL(blk_requeue_request);

@@ -2042,11 +2042,11 @@ static bool blk_update_bidi_request(struct request *rq, int error,
  */
 static void blk_finish_request(struct request *req, int error)
 {
-	BUG_ON(blk_queued_rq(req));
-
 	if (blk_rq_tagged(req))
 		blk_queue_end_tag(req->q, req);
 
+	BUG_ON(blk_queued_rq(req));
+
 	if (unlikely(laptop_mode) && blk_fs_request(req))
 		laptop_io_completion();

Tejun Heo

Acked-by on v1 1/1

2009-05-21 01:55 UTC #1 lore raw

Quoted 12 lines · James Bottomley

commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
Author: Tejun Heo <[email protected]>
Date:   Fri May 8 11:54:16 2009 +0900
block: implement and enforce request peek/start/fetch
Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req(). Unfortunately, this checks whether req->queuelist is empty. This list is doing double duty both as the queue list and the tag list, so tagged requests come in here with this not empty and boom (the tag list is emptied by blk_queue_end_tag() lower down).

Quoted 6 lines

Fix this by moving the BUG_ON to below the end tag we also seem vulnerable to this in blk_requeue_request() as well. I think all uses of blk_queued_rq() need auditing because the check is clearly wrong in the tagged case.
Signed-off-by James Bottomley <[email protected]>

Oops,

Acked-by Tejun Heo <[email protected]>

There are also some drivers which use queuelist for internal purposes after dequeueing, which also screws up blk_queued_rq() test in addition to being questionable practice to begin with. Maybe we would be better off with a flag?

Thanks.

Signature · 1 line

tejun

Boaz Harrosh

2009-05-21 16:47 UTC #2 lore raw

Quoted 20 lines · Tejun Heo

Quoted 12 lines · James Bottomley
commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
Author: Tejun Heo <[email protected]>
Date:   Fri May 8 11:54:16 2009 +0900
block: implement and enforce request peek/start/fetch
Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req(). Unfortunately, this checks whether req->queuelist is empty. This list is doing double duty both as the queue list and the tag list, so tagged requests come in here with this not empty and boom (the tag list is emptied by blk_queue_end_tag() lower down).
Quoted 6 lines
Fix this by moving the BUG_ON to below the end tag we also seem vulnerable to this in blk_requeue_request() as well. I think all uses of blk_queued_rq() need auditing because the check is clearly wrong in the tagged case.
Signed-off-by James Bottomley <[email protected]>

Quoted 9 lines

Oops,
Acked-by Tejun Heo <[email protected]>
There are also some drivers which use queuelist for internal purposes after dequeueing, which also screws up blk_queued_rq() test in addition to being questionable practice to begin with. Maybe we would be better off with a flag?

What is the REQ_STARTED flag for?

Quoted 2 lines

Thanks.

Boaz

Tejun Heo

2009-05-21 23:21 UTC #3 lore raw

Quoted 9 lines · Boaz Harrosh

Quoted 6 lines · Tejun Heo
James Bottomley wrote:
There are also some drivers which use queuelist for internal purposes after dequeueing, which also screws up blk_queued_rq() test in addition to being questionable practice to begin with. Maybe we would be better off with a flag?
What is the REQ_STARTED flag for?

That is used to pin a request which has been peeked by low level driver at the top of the queue so that the next peek or fetch returns the same request. I'm not sure whether that is necessary at this point tho.

Thanks.

Signature · 1 line

tejun

James Bottomley

author maintainer

2009-05-26 15:58 UTC #4 lore raw

In reply to Tejun Heo

Quoted 20 lines · Tejun Heo

Quoted 12 lines · James Bottomley
commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
Author: Tejun Heo <[email protected]>
Date:   Fri May 8 11:54:16 2009 +0900
block: implement and enforce request peek/start/fetch
Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req(). Unfortunately, this checks whether req->queuelist is empty. This list is doing double duty both as the queue list and the tag list, so tagged requests come in here with this not empty and boom (the tag list is emptied by blk_queue_end_tag() lower down).
Quoted 6 lines
Fix this by moving the BUG_ON to below the end tag we also seem vulnerable to this in blk_requeue_request() as well. I think all uses of blk_queued_rq() need auditing because the check is clearly wrong in the tagged case.
Signed-off-by James Bottomley <[email protected]>

Quoted 8 lines

Oops,
Acked-by Tejun Heo <[email protected]>
There are also some drivers which use queuelist for internal purposes after dequeueing, which also screws up blk_queued_rq() test in addition to being questionable practice to begin with. Maybe we would be better off with a flag?

Either is fine by me ... could we get some fix in, please? I'm currently carrying this below the merge-base on the SCSI postmerge tree to prevent my main build server oopsing under SCSI testing ... I'm a bit surprised we haven't had more reports from linux-oops ... but you can bet that if Jens moves libata to generic tag use, that will change ...

James

Tejun Heo

2009-05-26 22:52 UTC #5 lore raw

Quoted 29 lines · James Bottomley

Quoted 19 lines · Tejun Heo
Quoted 12 lines · James Bottomley
commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
Author: Tejun Heo <[email protected]>
Date:   Fri May 8 11:54:16 2009 +0900
block: implement and enforce request peek/start/fetch
Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req(). Unfortunately, this checks whether req->queuelist is empty. This list is doing double duty both as the queue list and the tag list, so tagged requests come in here with this not empty and boom (the tag list is emptied by blk_queue_end_tag() lower down).
Quoted 6 lines
Fix this by moving the BUG_ON to below the end tag we also seem vulnerable to this in blk_requeue_request() as well. I think all uses of blk_queued_rq() need auditing because the check is clearly wrong in the tagged case.
Signed-off-by James Bottomley <[email protected]>
Quoted 8 lines
Oops,
Acked-by Tejun Heo <[email protected]>
There are also some drivers which use queuelist for internal purposes after dequeueing, which also screws up blk_queued_rq() test in addition to being questionable practice to begin with. Maybe we would be better off with a flag?

Quoted 5 lines

Either is fine by me ... could we get some fix in, please? I'm currently carrying this below the merge-base on the SCSI postmerge tree to prevent my main build server oopsing under SCSI testing ... I'm a bit surprised we haven't had more reports from linux-oops ... but you can bet that if Jens moves libata to generic tag use, that will change ...

For now, I think your patch is fine and thus the Acked-by. Jens?

Thanks.

Signature · 1 line

tejun

Jens Axboe

2009-05-27 04:27 UTC #6 lore raw

Quoted 30 lines · Tejun Heo

Quoted 29 lines · James Bottomley
Quoted 27 lines · Tejun Heo
Quoted 12 lines · James Bottomley
commit e8939a50466fd963eb1ba9118c34b9ffb7ff6aa6
Author: Tejun Heo <[email protected]>
Date:   Fri May 8 11:54:16 2009 +0900
block: implement and enforce request peek/start/fetch
Added a BUG_ON(blk_queued_rq(req)) to the top of blk_finish_req(). Unfortunately, this checks whether req->queuelist is empty. This list is doing double duty both as the queue list and the tag list, so tagged requests come in here with this not empty and boom (the tag list is emptied by blk_queue_end_tag() lower down).
Quoted 6 lines
Fix this by moving the BUG_ON to below the end tag we also seem vulnerable to this in blk_requeue_request() as well. I think all uses of blk_queued_rq() need auditing because the check is clearly wrong in the tagged case.
Signed-off-by James Bottomley <[email protected]>
Oops,
Acked-by Tejun Heo <[email protected]>
There are also some drivers which use queuelist for internal purposes after dequeueing, which also screws up blk_queued_rq() test in addition to being questionable practice to begin with. Maybe we would be better off with a flag?

Quoted 7 lines

Quoted 5 lines
Either is fine by me ... could we get some fix in, please? I'm currently carrying this below the merge-base on the SCSI postmerge tree to prevent my main build server oopsing under SCSI testing ... I'm a bit surprised we haven't had more reports from linux-oops ... but you can bet that if Jens moves libata to generic tag use, that will change ...
For now, I think your patch is fine and thus the Acked-by. Jens?

Yes indeed, sorry for the delay. I'll commit it.

Signature · 1 line

Jens Axboe