Skip to content

Commit 360f92c

Browse files
committed
block: fix regression with block enabled tagging
Martin reported that his test system would not boot with current git, it oopsed with this: BUG: unable to handle kernel paging request at ffff88046c6c9e80 IP: [<ffffffff812971e0>] blk_queue_start_tag+0x90/0x150 PGD 1ddf067 PUD 1de2067 PMD 47fc7d067 PTE 800000046c6c9060 Oops: 0002 [alexander-zimmermann#1] SMP DEBUG_PAGEALLOC Modules linked in: sd_mod lpfc(+) scsi_transport_fc scsi_tgt oracleasm rpcsec_gss_krb5 ipv6 igb dca i2c_algo_bit i2c_core hwmon CPU: 3 PID: 87 Comm: kworker/u17:1 Not tainted 3.14.0+ torvalds#246 Hardware name: Supermicro X9DRX+-F/X9DRX+-F, BIOS 3.00 07/09/2013 Workqueue: events_unbound async_run_entry_fn task: ffff8802743c2150 ti: ffff880273d02000 task.ti: ffff880273d02000 RIP: 0010:[<ffffffff812971e0>] [<ffffffff812971e0>] blk_queue_start_tag+0x90/0x150 RSP: 0018:ffff880273d03a58 EFLAGS: 00010092 RAX: ffff88046c6c9e78 RBX: ffff880077208e78 RCX: 00000000fffc8da6 RDX: 00000000fffc186d RSI: 0000000000000009 RDI: 00000000fffc8d9d RBP: ffff880273d03a88 R08: 0000000000000001 R09: ffff8800021c2410 R10: 0000000000000005 R11: 0000000000015b30 R12: ffff88046c5bb8a0 R13: ffff88046c5c0890 R14: 000000000000001e R15: 000000000000001e FS: 0000000000000000(0000) GS:ffff880277b00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffff88046c6c9e80 CR3: 00000000018f6000 CR4: 00000000000407e0 Stack: ffff880273d03a98 ffff880474b18800 0000000000000000 ffff880474157000 ffff88046c5c0890 ffff880077208e78 ffff880273d03ae8 ffffffff813b9e62 ffff880200000010 ffff880474b18968 ffff880474b18848 ffff88046c5c0cd8 Call Trace: [<ffffffff813b9e62>] scsi_request_fn+0xf2/0x510 [<ffffffff81293167>] __blk_run_queue+0x37/0x50 [<ffffffff8129ac43>] blk_execute_rq_nowait+0xb3/0x130 [<ffffffff8129ad24>] blk_execute_rq+0x64/0xf0 [<ffffffff8108d2b0>] ? bit_waitqueue+0xd0/0xd0 [<ffffffff813bba35>] scsi_execute+0xe5/0x180 [<ffffffff813bbe4a>] scsi_execute_req_flags+0x9a/0x110 [<ffffffffa01b1304>] sd_spinup_disk+0x94/0x460 [sd_mod] [<ffffffff81160000>] ? __unmap_hugepage_range+0x200/0x2f0 [<ffffffffa01b2b9a>] sd_revalidate_disk+0xaa/0x3f0 [sd_mod] [<ffffffffa01b2fb8>] sd_probe_async+0xd8/0x200 [sd_mod] [<ffffffff8107703f>] async_run_entry_fn+0x3f/0x140 [<ffffffff8106a1c5>] process_one_work+0x175/0x410 [<ffffffff8106b373>] worker_thread+0x123/0x400 [<ffffffff8106b250>] ? manage_workers+0x160/0x160 [<ffffffff8107104e>] kthread+0xce/0xf0 [<ffffffff81070f80>] ? kthread_freezable_should_stop+0x70/0x70 [<ffffffff815f0bac>] ret_from_fork+0x7c/0xb0 [<ffffffff81070f80>] ? kthread_freezable_should_stop+0x70/0x70 Code: 48 0f ab 11 72 db 48 81 4b 40 00 00 10 00 89 83 08 01 00 00 48 89 df 49 8b 04 24 48 89 1c d0 e8 f7 a8 ff ff 49 8b 85 28 05 00 00 <48> 89 58 08 48 89 03 49 8d 85 28 05 00 00 48 89 43 08 49 89 9d RIP [<ffffffff812971e0>] blk_queue_start_tag+0x90/0x150 RSP <ffff880273d03a58> CR2: ffff88046c6c9e80 Martin bisected and found this to be the problem patch; commit 6d11339 Author: Jan Kara <[email protected]> Date: Mon Feb 24 16:39:54 2014 +0100 block: Stop abusing rq->csd.list in blk-softirq and the problem was immediately apparent. The patch states that it is safe to reuse queuelist at completion time, since it is no longer used. However, that is not true if a device is using block enabled tagging. If that is the case, then the queuelist is reused to keep track of busy tags. If a device also ended up using softirq completions, we'd reuse ->queuelist for the IPI handling while block tagging was still using it. Boom. Fix this by adding a new ipi_list list head, and share the memory used with the request hash table. The hash table is never used after the request is moved to the dispatch list, which happens long before any potential completion of the request. Add a new request bit for this, so we don't have cases that check rq->hash while it could potentially have been reused for the IPI completion. Reported-by: Martin K. Petersen <[email protected]> Tested-by: Benjamin Herrenschmidt <[email protected]> Signed-off-by: Jens Axboe <[email protected]>
1 parent 2bfad21 commit 360f92c

File tree

6 files changed

+24
-14
lines changed

6 files changed

+24
-14
lines changed

block/blk-core.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -1307,7 +1307,7 @@ void __blk_put_request(struct request_queue *q, struct request *req)
13071307
struct request_list *rl = blk_rq_rl(req);
13081308

13091309
BUG_ON(!list_empty(&req->queuelist));
1310-
BUG_ON(!hlist_unhashed(&req->hash));
1310+
BUG_ON(ELV_ON_HASH(req));
13111311

13121312
blk_free_request(rl, req);
13131313
freed_request(rl, flags);

block/blk-softirq.c

+6-11
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ static void blk_done_softirq(struct softirq_action *h)
3030
while (!list_empty(&local_list)) {
3131
struct request *rq;
3232

33-
rq = list_entry(local_list.next, struct request, queuelist);
34-
list_del_init(&rq->queuelist);
33+
rq = list_entry(local_list.next, struct request, ipi_list);
34+
list_del_init(&rq->ipi_list);
3535
rq->q->softirq_done_fn(rq);
3636
}
3737
}
@@ -45,14 +45,9 @@ static void trigger_softirq(void *data)
4545

4646
local_irq_save(flags);
4747
list = this_cpu_ptr(&blk_cpu_done);
48-
/*
49-
* We reuse queuelist for a list of requests to process. Since the
50-
* queuelist is used by the block layer only for requests waiting to be
51-
* submitted to the device it is unused now.
52-
*/
53-
list_add_tail(&rq->queuelist, list);
48+
list_add_tail(&rq->ipi_list, list);
5449

55-
if (list->next == &rq->queuelist)
50+
if (list->next == &rq->ipi_list)
5651
raise_softirq_irqoff(BLOCK_SOFTIRQ);
5752

5853
local_irq_restore(flags);
@@ -141,15 +136,15 @@ void __blk_complete_request(struct request *req)
141136
struct list_head *list;
142137
do_local:
143138
list = this_cpu_ptr(&blk_cpu_done);
144-
list_add_tail(&req->queuelist, list);
139+
list_add_tail(&req->ipi_list, list);
145140

146141
/*
147142
* if the list only contains our just added request,
148143
* signal a raise of the softirq. If there are already
149144
* entries there, someone already raised the irq but it
150145
* hasn't run yet.
151146
*/
152-
if (list->next == &req->queuelist)
147+
if (list->next == &req->ipi_list)
153148
raise_softirq_irqoff(BLOCK_SOFTIRQ);
154149
} else if (raise_blk_irq(ccpu, req))
155150
goto do_local;

block/blk.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ static inline void blk_clear_rq_complete(struct request *rq)
7878
/*
7979
* Internal elevator interface
8080
*/
81-
#define ELV_ON_HASH(rq) hash_hashed(&(rq)->hash)
81+
#define ELV_ON_HASH(rq) ((rq)->cmd_flags & REQ_HASHED)
8282

8383
void blk_insert_flush(struct request *rq);
8484
void blk_abort_flushes(struct request_queue *q);

block/elevator.c

+2
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,7 @@ EXPORT_SYMBOL(elevator_exit);
247247
static inline void __elv_rqhash_del(struct request *rq)
248248
{
249249
hash_del(&rq->hash);
250+
rq->cmd_flags &= ~REQ_HASHED;
250251
}
251252

252253
static void elv_rqhash_del(struct request_queue *q, struct request *rq)
@@ -261,6 +262,7 @@ static void elv_rqhash_add(struct request_queue *q, struct request *rq)
261262

262263
BUG_ON(ELV_ON_HASH(rq));
263264
hash_add(e->hash, &rq->hash, rq_hash_key(rq));
265+
rq->cmd_flags |= REQ_HASHED;
264266
}
265267

266268
static void elv_rqhash_reposition(struct request_queue *q, struct request *rq)

include/linux/blk_types.h

+2
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,7 @@ enum rq_flag_bits {
189189
__REQ_KERNEL, /* direct IO to kernel pages */
190190
__REQ_PM, /* runtime pm request */
191191
__REQ_END, /* last of chain of requests */
192+
__REQ_HASHED, /* on IO scheduler merge hash */
192193
__REQ_NR_BITS, /* stops here */
193194
};
194195

@@ -241,5 +242,6 @@ enum rq_flag_bits {
241242
#define REQ_KERNEL (1ULL << __REQ_KERNEL)
242243
#define REQ_PM (1ULL << __REQ_PM)
243244
#define REQ_END (1ULL << __REQ_END)
245+
#define REQ_HASHED (1ULL << __REQ_HASHED)
244246

245247
#endif /* __LINUX_BLK_TYPES_H */

include/linux/blkdev.h

+12-1
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,18 @@ struct request {
118118
struct bio *bio;
119119
struct bio *biotail;
120120

121-
struct hlist_node hash; /* merge hash */
121+
/*
122+
* The hash is used inside the scheduler, and killed once the
123+
* request reaches the dispatch list. The ipi_list is only used
124+
* to queue the request for softirq completion, which is long
125+
* after the request has been unhashed (and even removed from
126+
* the dispatch list).
127+
*/
128+
union {
129+
struct hlist_node hash; /* merge hash */
130+
struct list_head ipi_list;
131+
};
132+
122133
/*
123134
* The rb_node is only used inside the io scheduler, requests
124135
* are pruned when moved to the dispatch queue. So let the

0 commit comments

Comments
 (0)