| Message ID | 1534317915-5041-3-git-send-email-jianchao.w.wang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixes for the updating nr_hw_queues | expand |
On Wed, Aug 15, 2018 at 3:25 PM, Jianchao Wang <jianchao.w.wang@oracle.com> wrote: > For blk-mq, part_in_flight/rw will invoke blk_mq_in_flight/rw to > account the inflight requests. It will access the queue_hw_ctx and > nr_hw_queues w/o any protection. When updating nr_hw_queues and > blk_mq_in_flight/rw occur concurrently, panic comes up. When updating nr_hw_queues, all queues are frozen, and there shouldn't be any inflight requests, so there shouldn't be such issue. Thanks, Ming Lei
On 08/16/2018 12:50 PM, Ming Lei wrote: > On Wed, Aug 15, 2018 at 3:25 PM, Jianchao Wang > <jianchao.w.wang@oracle.com> wrote: >> For blk-mq, part_in_flight/rw will invoke blk_mq_in_flight/rw to >> account the inflight requests. It will access the queue_hw_ctx and >> nr_hw_queues w/o any protection. When updating nr_hw_queues and >> blk_mq_in_flight/rw occur concurrently, panic comes up. > > When updating nr_hw_queues, all queues are frozen, and there shouldn't > be any inflight requests, so there shouldn't be such issue. > I get following crash when do updating nr_hw_queues test. [ 112.643189] BUG: unable to handle kernel NULL pointer dereference at 0000000000000174 [ 112.643275] PGD 40baf8067 P4D 40baf8067 PUD 40bb38067 PMD 0 [ 112.643334] Oops: 0000 [#1] PREEMPT SMP [ 112.643372] CPU: 7 PID: 1526 Comm: fio Kdump: loaded Not tainted 4.18.0-rc6+ #250 [ 112.643434] Hardware name: LENOVO 10MLS0E339/3106, BIOS M1AKT22A 06/27/2017 [ 112.643499] RIP: 0010:blk_mq_queue_tag_busy_iter+0x4d/0x250 [ 112.643548] Code: 48 89 54 24 20 c7 44 24 0c 00 00 00 00 85 c9 0f 84 25 01 00 00 48 8b 7c 24 10 48 63 44 24 0c 48 8b 97 88 01 00 00 4c 8b 34 c2 <41> 8b 96 74 01 00 00 4d 8b a6 e8 01 00 00 85 d2 0f 84 e0 00 00 00 [ 112.643791] RSP: 0018:ffff95708284fc70 EFLAGS: 00010202 [ 112.643840] RAX: 0000000000000002 RBX: ffff895b49570e18 RCX: 00000000000000ff [ 112.643899] RDX: ffff895b4fc1d6c0 RSI: ffffffff8b6a94b0 RDI: ffff895b49570e18 [ 112.643961] RBP: 000000000000001f R08: 0000000000000000 R09: 0000000000000000 [ 112.644021] R10: ffff95708284fcd8 R11: ffffffff8b6b547d R12: ffff895b4fc30b40 [ 112.644081] R13: 0000000000000000 R14: 0000000000000000 R15: ffff895b48c96c40 [ 112.644144] FS: 00007fa79fd4c700(0000) GS:ffff895b62dc0000(0000) knlGS:0000000000000000 [ 112.644212] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 112.644270] CR2: 0000000000000174 CR3: 000000040b9ae001 CR4: 00000000003606e0 [ 112.644357] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 112.644459] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 112.644552] Call Trace: [ 112.644603] ? blk_mq_stop_hw_queues+0x50/0x50 [ 112.644675] ? blk_mq_stop_hw_queues+0x50/0x50 [ 112.644748] blk_mq_in_flight+0x2e/0x40 [ 112.644792] part_round_stats+0x158/0x160 [ 112.644835] part_stat_show+0x9c/0x530 [ 112.644873] ? lock_acquire+0xab/0x200 [ 112.644913] ? kernfs_seq_start+0x32/0x90 [ 112.644959] dev_attr_show+0x19/0x50 [ 112.644996] sysfs_kf_seq_show+0xad/0x100 [ 112.645039] seq_read+0xa5/0x410 [ 112.645075] ? __mutex_lock+0x20e/0x990 [ 112.645117] __vfs_read+0x23/0x160 [ 112.645158] vfs_read+0xa0/0x140 [ 112.645193] ksys_read+0x45/0xa0 [ 112.645230] do_syscall_64+0x5a/0x1a0 [ 112.645267] entry_SYSCALL_64_after_hwframe+0x49/0xbe The blk_mq_in_flight will access the queue_hw_ctx and nr_hw_queues when updating nr_hw_queues is ongoing. Thanks Jianchao > > Thanks, > Ming Lei >
On Thu, Aug 16, 2018 at 04:29:33PM +0800, jianchao.wang wrote: > > > On 08/16/2018 12:50 PM, Ming Lei wrote: > > On Wed, Aug 15, 2018 at 3:25 PM, Jianchao Wang > > <jianchao.w.wang@oracle.com> wrote: > >> For blk-mq, part_in_flight/rw will invoke blk_mq_in_flight/rw to > >> account the inflight requests. It will access the queue_hw_ctx and > >> nr_hw_queues w/o any protection. When updating nr_hw_queues and > >> blk_mq_in_flight/rw occur concurrently, panic comes up. > > > > When updating nr_hw_queues, all queues are frozen, and there shouldn't > > be any inflight requests, so there shouldn't be such issue. > > > > I get following crash when do updating nr_hw_queues test. > > [ 112.643189] BUG: unable to handle kernel NULL pointer dereference at 0000000000000174 > [ 112.643275] PGD 40baf8067 P4D 40baf8067 PUD 40bb38067 PMD 0 > [ 112.643334] Oops: 0000 [#1] PREEMPT SMP > [ 112.643372] CPU: 7 PID: 1526 Comm: fio Kdump: loaded Not tainted 4.18.0-rc6+ #250 > [ 112.643434] Hardware name: LENOVO 10MLS0E339/3106, BIOS M1AKT22A 06/27/2017 > [ 112.643499] RIP: 0010:blk_mq_queue_tag_busy_iter+0x4d/0x250 > [ 112.643548] Code: 48 89 54 24 20 c7 44 24 0c 00 00 00 00 85 c9 0f 84 25 01 00 00 48 8b 7c 24 10 48 63 44 24 0c 48 8b 97 88 01 00 00 4c 8b 34 c2 <41> 8b 96 74 01 00 00 4d 8b a6 e8 01 00 00 85 d2 0f 84 e0 00 00 00 > [ 112.643791] RSP: 0018:ffff95708284fc70 EFLAGS: 00010202 > [ 112.643840] RAX: 0000000000000002 RBX: ffff895b49570e18 RCX: 00000000000000ff > [ 112.643899] RDX: ffff895b4fc1d6c0 RSI: ffffffff8b6a94b0 RDI: ffff895b49570e18 > [ 112.643961] RBP: 000000000000001f R08: 0000000000000000 R09: 0000000000000000 > [ 112.644021] R10: ffff95708284fcd8 R11: ffffffff8b6b547d R12: ffff895b4fc30b40 > [ 112.644081] R13: 0000000000000000 R14: 0000000000000000 R15: ffff895b48c96c40 > [ 112.644144] FS: 00007fa79fd4c700(0000) GS:ffff895b62dc0000(0000) knlGS:0000000000000000 > [ 112.644212] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 112.644270] CR2: 0000000000000174 CR3: 000000040b9ae001 CR4: 00000000003606e0 > [ 112.644357] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 112.644459] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > [ 112.644552] Call Trace: > [ 112.644603] ? blk_mq_stop_hw_queues+0x50/0x50 > [ 112.644675] ? blk_mq_stop_hw_queues+0x50/0x50 > [ 112.644748] blk_mq_in_flight+0x2e/0x40 > [ 112.644792] part_round_stats+0x158/0x160 > [ 112.644835] part_stat_show+0x9c/0x530 > [ 112.644873] ? lock_acquire+0xab/0x200 > [ 112.644913] ? kernfs_seq_start+0x32/0x90 > [ 112.644959] dev_attr_show+0x19/0x50 > [ 112.644996] sysfs_kf_seq_show+0xad/0x100 > [ 112.645039] seq_read+0xa5/0x410 > [ 112.645075] ? __mutex_lock+0x20e/0x990 > [ 112.645117] __vfs_read+0x23/0x160 > [ 112.645158] vfs_read+0xa0/0x140 > [ 112.645193] ksys_read+0x45/0xa0 > [ 112.645230] do_syscall_64+0x5a/0x1a0 > [ 112.645267] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > The blk_mq_in_flight will access the queue_hw_ctx and nr_hw_queues when updating nr_hw_queues is ongoing. > Sorry for missing this use case, then what do you think of the following fix? diff --git a/block/blk-mq.c b/block/blk-mq.c index b42a2c9ba00e..fbc5534f8178 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -113,6 +113,10 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part, struct mq_inflight mi = { .part = part, .inflight = inflight, }; inflight[0] = inflight[1] = 0; + + if (percpu_ref_is_dying(&q->q_usage_counter)) + return; + blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi); } Thanks, Ming
Hi Ming On 08/16/2018 05:03 PM, Ming Lei wrote: > diff --git a/block/blk-mq.c b/block/blk-mq.c > index b42a2c9ba00e..fbc5534f8178 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -113,6 +113,10 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part, > struct mq_inflight mi = { .part = part, .inflight = inflight, }; > > inflight[0] = inflight[1] = 0; > + > + if (percpu_ref_is_dying(&q->q_usage_counter)) > + return; > + > blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi); > } That's a good idea to use q->q_usage_counter. But I think we could do following modification: 1. use percpu_ref_is_zero, then we will not miss any in-flight request here. 2. use rcu to ensure the user of blk_mq_in_flight has gone out of the critical section. Like following patch: diff --git a/block/blk-mq.c b/block/blk-mq.c index 89904cc..cd9878e 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -113,7 +113,12 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part, inflight[0] = inflight[1] = 0; + rcu_read_lock(); + if (percpu_ref_is_zero(&q->q_usage_counter)) + return; + blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi); + rcu_read_unlock(); } static void blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx, @@ -2907,6 +2912,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, list_for_each_entry(q, &set->tag_list, tag_set_list) blk_mq_freeze_queue(q); + synchronize_rcu(); /* * switch io scheduler to NULL to clean up the data in it. * will get it back after update mapping between cpu and hw queues. And also, some comment is needed to describe them. ；） Thanks Jianchao
On Thu, Aug 16, 2018 at 05:20:50PM +0800, jianchao.wang wrote: > Hi Ming > > On 08/16/2018 05:03 PM, Ming Lei wrote: > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > index b42a2c9ba00e..fbc5534f8178 100644 > > --- a/block/blk-mq.c > > +++ b/block/blk-mq.c > > @@ -113,6 +113,10 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part, > > struct mq_inflight mi = { .part = part, .inflight = inflight, }; > > > > inflight[0] = inflight[1] = 0; > > + > > + if (percpu_ref_is_dying(&q->q_usage_counter)) > > + return; > > + > > blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi); > > } > > That's a good idea to use q->q_usage_counter. > But I think we could do following modification: > 1. use percpu_ref_is_zero, then we will not miss any in-flight request here. > 2. use rcu to ensure the user of blk_mq_in_flight has gone out of the critical section. > Like following patch: > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 89904cc..cd9878e 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -113,7 +113,12 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part, > > inflight[0] = inflight[1] = 0; > > + rcu_read_lock(); > + if (percpu_ref_is_zero(&q->q_usage_counter)) > + return; > + > blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi); > + rcu_read_unlock(); > } > > static void blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx, > @@ -2907,6 +2912,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, > list_for_each_entry(q, &set->tag_list, tag_set_list) > blk_mq_freeze_queue(q); > > + synchronize_rcu(); > /* > * switch io scheduler to NULL to clean up the data in it. > * will get it back after update mapping between cpu and hw queues. > > And also, some comment is needed to describe them. ；） This patch looks fine for me. Thanks Ming
diff --git a/block/blk-mq.c b/block/blk-mq.c index 89904cc..ff50afc 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2517,6 +2517,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set, blk_mq_sysfs_unregister(q); + WRITE_ONCE(q->mq_realloc_hw_ctxs, true); + synchronize_rcu(); /* protect against switching io scheduler */ mutex_lock(&q->sysfs_lock); for (i = 0; i < set->nr_hw_queues; i++) { @@ -2564,6 +2566,7 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set, } q->nr_hw_queues = i; mutex_unlock(&q->sysfs_lock); + WRITE_ONCE(q->mq_realloc_hw_ctxs, false); blk_mq_sysfs_register(q); } diff --git a/block/genhd.c b/block/genhd.c index 8cc719a3..f9e46aa 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -69,7 +69,10 @@ void part_in_flight(struct request_queue *q, struct hd_struct *part, unsigned int inflight[2]) { if (q->mq_ops) { - blk_mq_in_flight(q, part, inflight); + rcu_read_lock(); + if (!q->mq_realloc_hw_ctxs) + blk_mq_in_flight(q, part, inflight); + rcu_read_unlock(); return; } @@ -86,7 +89,10 @@ void part_in_flight_rw(struct request_queue *q, struct hd_struct *part, unsigned int inflight[2]) { if (q->mq_ops) { - blk_mq_in_flight_rw(q, part, inflight); + rcu_read_lock(); + if (!q->mq_realloc_hw_ctxs) + blk_mq_in_flight_rw(q, part, inflight); + rcu_read_unlock(); return; } diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index ee930c4..5cb6662 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -662,6 +662,7 @@ struct request_queue { #endif bool mq_sysfs_init_done; + bool mq_realloc_hw_ctxs; size_t cmd_size; void *rq_alloc_data;
For blk-mq, part_in_flight/rw will invoke blk_mq_in_flight/rw to account the inflight requests. It will access the queue_hw_ctx and nr_hw_queues w/o any protection. When updating nr_hw_queues and blk_mq_in_flight/rw occur concurrently, panic comes up. To fix it, introduce mq_realloc_hw_ctxs into mark the updating nr_hw_queues process and use rcu to ensure the mq_realloc_hw_ctxs globally visible. Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> --- block/blk-mq.c | 3 +++ block/genhd.c | 10 ++++++++-- include/linux/blkdev.h | 1 + 3 files changed, 12 insertions(+), 2 deletions(-)