* [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached
@ 2017-04-20 21:13 Jens Axboe
2017-04-20 21:30 ` Omar Sandoval
0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2017-04-20 21:13 UTC (permalink / raw)
To: linux-block@vger.kernel.org; +Cc: Christoph Hellwig, Omar Sandoval
We must have dropped the ctx before we call
blk_mq_sched_insert_request() with can_block=true, otherwise we risk
that a flush request can block on insertion if we are currently out of
tags.
[ 47.667190] BUG: scheduling while atomic: jbd2/sda2-8/2089/0x00000002
[ 47.674493] Modules linked in: x86_pkg_temp_thermal btrfs xor zlib_deflate raid6_pq sr_mod cdre
[ 47.690572] Preemption disabled at:
[ 47.690584] [<ffffffff81326c7c>] blk_mq_sched_get_request+0x6c/0x280
[ 47.701764] CPU: 1 PID: 2089 Comm: jbd2/sda2-8 Not tainted 4.11.0-rc7+ #271
[ 47.709630] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016
[ 47.718081] Call Trace:
[ 47.720903] dump_stack+0x4f/0x73
[ 47.724694] ? blk_mq_sched_get_request+0x6c/0x280
[ 47.730137] __schedule_bug+0x6c/0xc0
[ 47.734314] __schedule+0x559/0x780
[ 47.738302] schedule+0x3b/0x90
[ 47.741899] io_schedule+0x11/0x40
[ 47.745788] blk_mq_get_tag+0x167/0x2a0
[ 47.750162] ? remove_wait_queue+0x70/0x70
[ 47.754901] blk_mq_get_driver_tag+0x92/0xf0
[ 47.759758] blk_mq_sched_insert_request+0x134/0x170
[ 47.765398] ? blk_account_io_start+0xd0/0x270
[ 47.770679] blk_mq_make_request+0x1b2/0x850
[ 47.775766] generic_make_request+0xf7/0x2d0
[ 47.780860] submit_bio+0x5f/0x120
[ 47.784979] ? submit_bio+0x5f/0x120
[ 47.789631] submit_bh_wbc.isra.46+0x10d/0x130
[ 47.794902] submit_bh+0xb/0x10
[ 47.798719] journal_submit_commit_record+0x190/0x210
[ 47.804686] ? _raw_spin_unlock+0x13/0x30
[ 47.809480] jbd2_journal_commit_transaction+0x180a/0x1d00
[ 47.815925] kjournald2+0xb6/0x250
[ 47.820022] ? kjournald2+0xb6/0x250
[ 47.824328] ? remove_wait_queue+0x70/0x70
[ 47.829223] kthread+0x10e/0x140
[ 47.833147] ? commit_timeout+0x10/0x10
[ 47.837742] ? kthread_create_on_node+0x40/0x40
[ 47.843122] ret_from_fork+0x29/0x40
Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request")
Signed-off-by: Jens Axboe <axboe@fb.com>
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9d7645f24b05..323eed50d615 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1634,8 +1634,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
blk_mq_try_issue_directly(data.hctx, rq, &cookie);
return cookie;
} else if (q->elevator) {
+ blk_mq_put_ctx(data.ctx);
blk_mq_bio_to_request(rq, bio);
blk_mq_sched_insert_request(rq, false, true, true, true);
+ return cookie;
} else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio))
blk_mq_run_hw_queue(data.hctx, true);
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached 2017-04-20 21:13 [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached Jens Axboe @ 2017-04-20 21:30 ` Omar Sandoval 2017-04-20 21:41 ` Jens Axboe 2017-04-20 22:39 ` Jens Axboe 0 siblings, 2 replies; 8+ messages in thread From: Omar Sandoval @ 2017-04-20 21:30 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block@vger.kernel.org, Christoph Hellwig On Thu, Apr 20, 2017 at 03:13:43PM -0600, Jens Axboe wrote: > We must have dropped the ctx before we call > blk_mq_sched_insert_request() with can_block=true, otherwise we risk > that a flush request can block on insertion if we are currently out of > tags. > > [ 47.667190] BUG: scheduling while atomic: jbd2/sda2-8/2089/0x00000002 > [ 47.674493] Modules linked in: x86_pkg_temp_thermal btrfs xor zlib_deflate raid6_pq sr_mod cdre > [ 47.690572] Preemption disabled at: > [ 47.690584] [<ffffffff81326c7c>] blk_mq_sched_get_request+0x6c/0x280 > [ 47.701764] CPU: 1 PID: 2089 Comm: jbd2/sda2-8 Not tainted 4.11.0-rc7+ #271 > [ 47.709630] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016 > [ 47.718081] Call Trace: > [ 47.720903] dump_stack+0x4f/0x73 > [ 47.724694] ? blk_mq_sched_get_request+0x6c/0x280 > [ 47.730137] __schedule_bug+0x6c/0xc0 > [ 47.734314] __schedule+0x559/0x780 > [ 47.738302] schedule+0x3b/0x90 > [ 47.741899] io_schedule+0x11/0x40 > [ 47.745788] blk_mq_get_tag+0x167/0x2a0 > [ 47.750162] ? remove_wait_queue+0x70/0x70 > [ 47.754901] blk_mq_get_driver_tag+0x92/0xf0 > [ 47.759758] blk_mq_sched_insert_request+0x134/0x170 > [ 47.765398] ? blk_account_io_start+0xd0/0x270 > [ 47.770679] blk_mq_make_request+0x1b2/0x850 > [ 47.775766] generic_make_request+0xf7/0x2d0 > [ 47.780860] submit_bio+0x5f/0x120 > [ 47.784979] ? submit_bio+0x5f/0x120 > [ 47.789631] submit_bh_wbc.isra.46+0x10d/0x130 > [ 47.794902] submit_bh+0xb/0x10 > [ 47.798719] journal_submit_commit_record+0x190/0x210 > [ 47.804686] ? _raw_spin_unlock+0x13/0x30 > [ 47.809480] jbd2_journal_commit_transaction+0x180a/0x1d00 > [ 47.815925] kjournald2+0xb6/0x250 > [ 47.820022] ? kjournald2+0xb6/0x250 > [ 47.824328] ? remove_wait_queue+0x70/0x70 > [ 47.829223] kthread+0x10e/0x140 > [ 47.833147] ? commit_timeout+0x10/0x10 > [ 47.837742] ? kthread_create_on_node+0x40/0x40 > [ 47.843122] ret_from_fork+0x29/0x40 > > Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request") > Signed-off-by: Jens Axboe <axboe@fb.com> > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 9d7645f24b05..323eed50d615 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -1634,8 +1634,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) > blk_mq_try_issue_directly(data.hctx, rq, &cookie); > return cookie; > } else if (q->elevator) { > + blk_mq_put_ctx(data.ctx); > blk_mq_bio_to_request(rq, bio); > blk_mq_sched_insert_request(rq, false, true, true, true); > + return cookie; > } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) > blk_mq_run_hw_queue(data.hctx, true); > > I'm confused, the first thing we check in make_request is: if (unlikely(is_flush_fua)) { blk_mq_bio_to_request(rq, bio); if (q->elevator) { blk_mq_sched_insert_request(rq, false, true, true, true); } else { blk_insert_flush(rq); blk_mq_run_hw_queue(data.hctx, true); } } and can_block doesn't do anything in the !flush case, so shouldn't it be changed in that one instead? Alternatively, blk_mq_get_tag() knows how to drop the ctx before it sleeps, but blk_mq_get_driver_tag() doesn't set data.ctx. I'm not sure if that'll do what we want, and this is making my head hurt. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached 2017-04-20 21:30 ` Omar Sandoval @ 2017-04-20 21:41 ` Jens Axboe 2017-04-20 21:47 ` Omar Sandoval 2017-04-20 22:39 ` Jens Axboe 1 sibling, 1 reply; 8+ messages in thread From: Jens Axboe @ 2017-04-20 21:41 UTC (permalink / raw) To: Omar Sandoval; +Cc: linux-block@vger.kernel.org, Christoph Hellwig On 04/20/2017 03:30 PM, Omar Sandoval wrote: > On Thu, Apr 20, 2017 at 03:13:43PM -0600, Jens Axboe wrote: >> We must have dropped the ctx before we call >> blk_mq_sched_insert_request() with can_block=true, otherwise we risk >> that a flush request can block on insertion if we are currently out of >> tags. >> >> [ 47.667190] BUG: scheduling while atomic: jbd2/sda2-8/2089/0x00000002 >> [ 47.674493] Modules linked in: x86_pkg_temp_thermal btrfs xor zlib_deflate raid6_pq sr_mod cdre >> [ 47.690572] Preemption disabled at: >> [ 47.690584] [<ffffffff81326c7c>] blk_mq_sched_get_request+0x6c/0x280 >> [ 47.701764] CPU: 1 PID: 2089 Comm: jbd2/sda2-8 Not tainted 4.11.0-rc7+ #271 >> [ 47.709630] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016 >> [ 47.718081] Call Trace: >> [ 47.720903] dump_stack+0x4f/0x73 >> [ 47.724694] ? blk_mq_sched_get_request+0x6c/0x280 >> [ 47.730137] __schedule_bug+0x6c/0xc0 >> [ 47.734314] __schedule+0x559/0x780 >> [ 47.738302] schedule+0x3b/0x90 >> [ 47.741899] io_schedule+0x11/0x40 >> [ 47.745788] blk_mq_get_tag+0x167/0x2a0 >> [ 47.750162] ? remove_wait_queue+0x70/0x70 >> [ 47.754901] blk_mq_get_driver_tag+0x92/0xf0 >> [ 47.759758] blk_mq_sched_insert_request+0x134/0x170 >> [ 47.765398] ? blk_account_io_start+0xd0/0x270 >> [ 47.770679] blk_mq_make_request+0x1b2/0x850 >> [ 47.775766] generic_make_request+0xf7/0x2d0 >> [ 47.780860] submit_bio+0x5f/0x120 >> [ 47.784979] ? submit_bio+0x5f/0x120 >> [ 47.789631] submit_bh_wbc.isra.46+0x10d/0x130 >> [ 47.794902] submit_bh+0xb/0x10 >> [ 47.798719] journal_submit_commit_record+0x190/0x210 >> [ 47.804686] ? _raw_spin_unlock+0x13/0x30 >> [ 47.809480] jbd2_journal_commit_transaction+0x180a/0x1d00 >> [ 47.815925] kjournald2+0xb6/0x250 >> [ 47.820022] ? kjournald2+0xb6/0x250 >> [ 47.824328] ? remove_wait_queue+0x70/0x70 >> [ 47.829223] kthread+0x10e/0x140 >> [ 47.833147] ? commit_timeout+0x10/0x10 >> [ 47.837742] ? kthread_create_on_node+0x40/0x40 >> [ 47.843122] ret_from_fork+0x29/0x40 >> >> Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request") >> Signed-off-by: Jens Axboe <axboe@fb.com> >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 9d7645f24b05..323eed50d615 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -1634,8 +1634,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) >> blk_mq_try_issue_directly(data.hctx, rq, &cookie); >> return cookie; >> } else if (q->elevator) { >> + blk_mq_put_ctx(data.ctx); >> blk_mq_bio_to_request(rq, bio); >> blk_mq_sched_insert_request(rq, false, true, true, true); >> + return cookie; >> } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) >> blk_mq_run_hw_queue(data.hctx, true); >> >> > > I'm confused, the first thing we check in make_request is: > > if (unlikely(is_flush_fua)) { > blk_mq_bio_to_request(rq, bio); > if (q->elevator) { > blk_mq_sched_insert_request(rq, false, true, true, > true); > } else { > blk_insert_flush(rq); > blk_mq_run_hw_queue(data.hctx, true); > } > } > > and can_block doesn't do anything in the !flush case, so shouldn't it be > changed in that one instead? It should be changed in both cases. But I do wonder why we're triggering the bottom one, and not the top one, since this is clearly a flush. > Alternatively, blk_mq_get_tag() knows how to drop the ctx before it > sleeps, but blk_mq_get_driver_tag() doesn't set data.ctx. I'm not sure > if that'll do what we want, and this is making my head hurt. How about the below? Drop the ctx in all the cases. We only really want to hold on to it for the case where we insert into the software queues. But as long as we guarantee that all cases in that massive if/else drops it, we should be fine, and we can skip the various places that do manual returns to avoid having to hit the put_ctx/return cookie case at the end. diff --git a/block/blk-mq.c b/block/blk-mq.c index dd9e8d6d471d..992f09772f8a 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1571,6 +1571,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) plug = current->plug; if (unlikely(is_flush_fua)) { + blk_mq_put_ctx(data.ctx); blk_mq_bio_to_request(rq, bio); if (q->elevator) { blk_mq_sched_insert_request(rq, false, true, true, @@ -1582,6 +1583,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) } else if (plug && q->nr_hw_queues == 1) { struct request *last = NULL; + blk_mq_put_ctx(data.ctx); blk_mq_bio_to_request(rq, bio); /* @@ -1626,20 +1628,19 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) if (same_queue_rq) blk_mq_try_issue_directly(data.hctx, same_queue_rq, &cookie); - - return cookie; } else if (q->nr_hw_queues > 1 && is_sync) { blk_mq_put_ctx(data.ctx); blk_mq_bio_to_request(rq, bio); blk_mq_try_issue_directly(data.hctx, rq, &cookie); - return cookie; } else if (q->elevator) { + blk_mq_put_ctx(data.ctx); blk_mq_bio_to_request(rq, bio); blk_mq_sched_insert_request(rq, false, true, true, true); - } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) + } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) { + blk_mq_put_ctx(data.ctx); blk_mq_run_hw_queue(data.hctx, true); + } - blk_mq_put_ctx(data.ctx); return cookie; } -- Jens Axboe ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached 2017-04-20 21:41 ` Jens Axboe @ 2017-04-20 21:47 ` Omar Sandoval 2017-04-20 21:50 ` Jens Axboe 0 siblings, 1 reply; 8+ messages in thread From: Omar Sandoval @ 2017-04-20 21:47 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block@vger.kernel.org, Christoph Hellwig On Thu, Apr 20, 2017 at 03:41:21PM -0600, Jens Axboe wrote: > On 04/20/2017 03:30 PM, Omar Sandoval wrote: > > On Thu, Apr 20, 2017 at 03:13:43PM -0600, Jens Axboe wrote: > >> We must have dropped the ctx before we call > >> blk_mq_sched_insert_request() with can_block=true, otherwise we risk > >> that a flush request can block on insertion if we are currently out of > >> tags. > >> > >> [ 47.667190] BUG: scheduling while atomic: jbd2/sda2-8/2089/0x00000002 > >> [ 47.674493] Modules linked in: x86_pkg_temp_thermal btrfs xor zlib_deflate raid6_pq sr_mod cdre > >> [ 47.690572] Preemption disabled at: > >> [ 47.690584] [<ffffffff81326c7c>] blk_mq_sched_get_request+0x6c/0x280 > >> [ 47.701764] CPU: 1 PID: 2089 Comm: jbd2/sda2-8 Not tainted 4.11.0-rc7+ #271 > >> [ 47.709630] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016 > >> [ 47.718081] Call Trace: > >> [ 47.720903] dump_stack+0x4f/0x73 > >> [ 47.724694] ? blk_mq_sched_get_request+0x6c/0x280 > >> [ 47.730137] __schedule_bug+0x6c/0xc0 > >> [ 47.734314] __schedule+0x559/0x780 > >> [ 47.738302] schedule+0x3b/0x90 > >> [ 47.741899] io_schedule+0x11/0x40 > >> [ 47.745788] blk_mq_get_tag+0x167/0x2a0 > >> [ 47.750162] ? remove_wait_queue+0x70/0x70 > >> [ 47.754901] blk_mq_get_driver_tag+0x92/0xf0 > >> [ 47.759758] blk_mq_sched_insert_request+0x134/0x170 > >> [ 47.765398] ? blk_account_io_start+0xd0/0x270 > >> [ 47.770679] blk_mq_make_request+0x1b2/0x850 > >> [ 47.775766] generic_make_request+0xf7/0x2d0 > >> [ 47.780860] submit_bio+0x5f/0x120 > >> [ 47.784979] ? submit_bio+0x5f/0x120 > >> [ 47.789631] submit_bh_wbc.isra.46+0x10d/0x130 > >> [ 47.794902] submit_bh+0xb/0x10 > >> [ 47.798719] journal_submit_commit_record+0x190/0x210 > >> [ 47.804686] ? _raw_spin_unlock+0x13/0x30 > >> [ 47.809480] jbd2_journal_commit_transaction+0x180a/0x1d00 > >> [ 47.815925] kjournald2+0xb6/0x250 > >> [ 47.820022] ? kjournald2+0xb6/0x250 > >> [ 47.824328] ? remove_wait_queue+0x70/0x70 > >> [ 47.829223] kthread+0x10e/0x140 > >> [ 47.833147] ? commit_timeout+0x10/0x10 > >> [ 47.837742] ? kthread_create_on_node+0x40/0x40 > >> [ 47.843122] ret_from_fork+0x29/0x40 > >> > >> Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request") > >> Signed-off-by: Jens Axboe <axboe@fb.com> > >> > >> diff --git a/block/blk-mq.c b/block/blk-mq.c > >> index 9d7645f24b05..323eed50d615 100644 > >> --- a/block/blk-mq.c > >> +++ b/block/blk-mq.c > >> @@ -1634,8 +1634,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) > >> blk_mq_try_issue_directly(data.hctx, rq, &cookie); > >> return cookie; > >> } else if (q->elevator) { > >> + blk_mq_put_ctx(data.ctx); > >> blk_mq_bio_to_request(rq, bio); > >> blk_mq_sched_insert_request(rq, false, true, true, true); > >> + return cookie; > >> } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) > >> blk_mq_run_hw_queue(data.hctx, true); > >> > >> > > > > I'm confused, the first thing we check in make_request is: > > > > if (unlikely(is_flush_fua)) { > > blk_mq_bio_to_request(rq, bio); > > if (q->elevator) { > > blk_mq_sched_insert_request(rq, false, true, true, > > true); > > } else { > > blk_insert_flush(rq); > > blk_mq_run_hw_queue(data.hctx, true); > > } > > } > > > > and can_block doesn't do anything in the !flush case, so shouldn't it be > > changed in that one instead? > > It should be changed in both cases. But I do wonder why we're triggering > the bottom one, and not the top one, since this is clearly a flush. > > > Alternatively, blk_mq_get_tag() knows how to drop the ctx before it > > sleeps, but blk_mq_get_driver_tag() doesn't set data.ctx. I'm not sure > > if that'll do what we want, and this is making my head hurt. > > How about the below? Drop the ctx in all the cases. We only really want > to hold on to it for the case where we insert into the software queues. So do we want to hold on to it or not when we insert into the software queues? Because this change looks like it drops it before we do the insert. > But as long as we guarantee that all cases in that massive if/else > drops it, we should be fine, and we can skip the various places that > do manual returns to avoid having to hit the put_ctx/return cookie > case at the end. > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index dd9e8d6d471d..992f09772f8a 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -1571,6 +1571,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) > > plug = current->plug; > if (unlikely(is_flush_fua)) { > + blk_mq_put_ctx(data.ctx); > blk_mq_bio_to_request(rq, bio); > if (q->elevator) { > blk_mq_sched_insert_request(rq, false, true, true, > @@ -1582,6 +1583,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) > } else if (plug && q->nr_hw_queues == 1) { > struct request *last = NULL; > > + blk_mq_put_ctx(data.ctx); > blk_mq_bio_to_request(rq, bio); > > /* > @@ -1626,20 +1628,19 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) > if (same_queue_rq) > blk_mq_try_issue_directly(data.hctx, same_queue_rq, > &cookie); > - > - return cookie; > } else if (q->nr_hw_queues > 1 && is_sync) { > blk_mq_put_ctx(data.ctx); > blk_mq_bio_to_request(rq, bio); > blk_mq_try_issue_directly(data.hctx, rq, &cookie); > - return cookie; > } else if (q->elevator) { > + blk_mq_put_ctx(data.ctx); > blk_mq_bio_to_request(rq, bio); > blk_mq_sched_insert_request(rq, false, true, true, true); If the scheduler doesn't have ->insert_request(), this'll insert into the software queue. > - } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) This also inserts into the software queue. > + } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) { > + blk_mq_put_ctx(data.ctx); > blk_mq_run_hw_queue(data.hctx, true); > + } > > - blk_mq_put_ctx(data.ctx); > return cookie; > } > > > -- > Jens Axboe > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached 2017-04-20 21:47 ` Omar Sandoval @ 2017-04-20 21:50 ` Jens Axboe 0 siblings, 0 replies; 8+ messages in thread From: Jens Axboe @ 2017-04-20 21:50 UTC (permalink / raw) To: Omar Sandoval; +Cc: linux-block@vger.kernel.org, Christoph Hellwig On 04/20/2017 03:47 PM, Omar Sandoval wrote: > On Thu, Apr 20, 2017 at 03:41:21PM -0600, Jens Axboe wrote: >> On 04/20/2017 03:30 PM, Omar Sandoval wrote: >>> On Thu, Apr 20, 2017 at 03:13:43PM -0600, Jens Axboe wrote: >>>> We must have dropped the ctx before we call >>>> blk_mq_sched_insert_request() with can_block=true, otherwise we risk >>>> that a flush request can block on insertion if we are currently out of >>>> tags. >>>> >>>> [ 47.667190] BUG: scheduling while atomic: jbd2/sda2-8/2089/0x00000002 >>>> [ 47.674493] Modules linked in: x86_pkg_temp_thermal btrfs xor zlib_deflate raid6_pq sr_mod cdre >>>> [ 47.690572] Preemption disabled at: >>>> [ 47.690584] [<ffffffff81326c7c>] blk_mq_sched_get_request+0x6c/0x280 >>>> [ 47.701764] CPU: 1 PID: 2089 Comm: jbd2/sda2-8 Not tainted 4.11.0-rc7+ #271 >>>> [ 47.709630] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016 >>>> [ 47.718081] Call Trace: >>>> [ 47.720903] dump_stack+0x4f/0x73 >>>> [ 47.724694] ? blk_mq_sched_get_request+0x6c/0x280 >>>> [ 47.730137] __schedule_bug+0x6c/0xc0 >>>> [ 47.734314] __schedule+0x559/0x780 >>>> [ 47.738302] schedule+0x3b/0x90 >>>> [ 47.741899] io_schedule+0x11/0x40 >>>> [ 47.745788] blk_mq_get_tag+0x167/0x2a0 >>>> [ 47.750162] ? remove_wait_queue+0x70/0x70 >>>> [ 47.754901] blk_mq_get_driver_tag+0x92/0xf0 >>>> [ 47.759758] blk_mq_sched_insert_request+0x134/0x170 >>>> [ 47.765398] ? blk_account_io_start+0xd0/0x270 >>>> [ 47.770679] blk_mq_make_request+0x1b2/0x850 >>>> [ 47.775766] generic_make_request+0xf7/0x2d0 >>>> [ 47.780860] submit_bio+0x5f/0x120 >>>> [ 47.784979] ? submit_bio+0x5f/0x120 >>>> [ 47.789631] submit_bh_wbc.isra.46+0x10d/0x130 >>>> [ 47.794902] submit_bh+0xb/0x10 >>>> [ 47.798719] journal_submit_commit_record+0x190/0x210 >>>> [ 47.804686] ? _raw_spin_unlock+0x13/0x30 >>>> [ 47.809480] jbd2_journal_commit_transaction+0x180a/0x1d00 >>>> [ 47.815925] kjournald2+0xb6/0x250 >>>> [ 47.820022] ? kjournald2+0xb6/0x250 >>>> [ 47.824328] ? remove_wait_queue+0x70/0x70 >>>> [ 47.829223] kthread+0x10e/0x140 >>>> [ 47.833147] ? commit_timeout+0x10/0x10 >>>> [ 47.837742] ? kthread_create_on_node+0x40/0x40 >>>> [ 47.843122] ret_from_fork+0x29/0x40 >>>> >>>> Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request") >>>> Signed-off-by: Jens Axboe <axboe@fb.com> >>>> >>>> diff --git a/block/blk-mq.c b/block/blk-mq.c >>>> index 9d7645f24b05..323eed50d615 100644 >>>> --- a/block/blk-mq.c >>>> +++ b/block/blk-mq.c >>>> @@ -1634,8 +1634,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) >>>> blk_mq_try_issue_directly(data.hctx, rq, &cookie); >>>> return cookie; >>>> } else if (q->elevator) { >>>> + blk_mq_put_ctx(data.ctx); >>>> blk_mq_bio_to_request(rq, bio); >>>> blk_mq_sched_insert_request(rq, false, true, true, true); >>>> + return cookie; >>>> } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) >>>> blk_mq_run_hw_queue(data.hctx, true); >>>> >>>> >>> >>> I'm confused, the first thing we check in make_request is: >>> >>> if (unlikely(is_flush_fua)) { >>> blk_mq_bio_to_request(rq, bio); >>> if (q->elevator) { >>> blk_mq_sched_insert_request(rq, false, true, true, >>> true); >>> } else { >>> blk_insert_flush(rq); >>> blk_mq_run_hw_queue(data.hctx, true); >>> } >>> } >>> >>> and can_block doesn't do anything in the !flush case, so shouldn't it be >>> changed in that one instead? >> >> It should be changed in both cases. But I do wonder why we're triggering >> the bottom one, and not the top one, since this is clearly a flush. >> >>> Alternatively, blk_mq_get_tag() knows how to drop the ctx before it >>> sleeps, but blk_mq_get_driver_tag() doesn't set data.ctx. I'm not sure >>> if that'll do what we want, and this is making my head hurt. >> >> How about the below? Drop the ctx in all the cases. We only really want >> to hold on to it for the case where we insert into the software queues. > > So do we want to hold on to it or not when we insert into the software > queues? Because this change looks like it drops it before we do the > insert. See below, just an optimization. >> But as long as we guarantee that all cases in that massive if/else >> drops it, we should be fine, and we can skip the various places that >> do manual returns to avoid having to hit the put_ctx/return cookie >> case at the end. >> >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index dd9e8d6d471d..992f09772f8a 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -1571,6 +1571,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) >> >> plug = current->plug; >> if (unlikely(is_flush_fua)) { >> + blk_mq_put_ctx(data.ctx); >> blk_mq_bio_to_request(rq, bio); >> if (q->elevator) { >> blk_mq_sched_insert_request(rq, false, true, true, >> @@ -1582,6 +1583,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) >> } else if (plug && q->nr_hw_queues == 1) { >> struct request *last = NULL; >> >> + blk_mq_put_ctx(data.ctx); >> blk_mq_bio_to_request(rq, bio); >> >> /* >> @@ -1626,20 +1628,19 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) >> if (same_queue_rq) >> blk_mq_try_issue_directly(data.hctx, same_queue_rq, >> &cookie); >> - >> - return cookie; >> } else if (q->nr_hw_queues > 1 && is_sync) { >> blk_mq_put_ctx(data.ctx); >> blk_mq_bio_to_request(rq, bio); >> blk_mq_try_issue_directly(data.hctx, rq, &cookie); >> - return cookie; >> } else if (q->elevator) { >> + blk_mq_put_ctx(data.ctx); >> blk_mq_bio_to_request(rq, bio); >> blk_mq_sched_insert_request(rq, false, true, true, true); > > If the scheduler doesn't have ->insert_request(), this'll insert into > the software queue. I forgot that Kyber uses the software queues. But we can't hold on to it here, we just have to hope that we don't preempt for that case, and insert on the "wrong" software queue. I'm guessing this should happen very rarely, if ever, so it's not a huge deal. >> - } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) > > This also inserts into the software queue. Right, and we hold on to it here. -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached 2017-04-20 21:30 ` Omar Sandoval 2017-04-20 21:41 ` Jens Axboe @ 2017-04-20 22:39 ` Jens Axboe 2017-04-20 22:41 ` Omar Sandoval 1 sibling, 1 reply; 8+ messages in thread From: Jens Axboe @ 2017-04-20 22:39 UTC (permalink / raw) To: Omar Sandoval; +Cc: linux-block@vger.kernel.org, Christoph Hellwig On 04/20/2017 03:30 PM, Omar Sandoval wrote: > On Thu, Apr 20, 2017 at 03:13:43PM -0600, Jens Axboe wrote: >> We must have dropped the ctx before we call >> blk_mq_sched_insert_request() with can_block=true, otherwise we risk >> that a flush request can block on insertion if we are currently out of >> tags. >> >> [ 47.667190] BUG: scheduling while atomic: jbd2/sda2-8/2089/0x00000002 >> [ 47.674493] Modules linked in: x86_pkg_temp_thermal btrfs xor zlib_deflate raid6_pq sr_mod cdre >> [ 47.690572] Preemption disabled at: >> [ 47.690584] [<ffffffff81326c7c>] blk_mq_sched_get_request+0x6c/0x280 >> [ 47.701764] CPU: 1 PID: 2089 Comm: jbd2/sda2-8 Not tainted 4.11.0-rc7+ #271 >> [ 47.709630] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016 >> [ 47.718081] Call Trace: >> [ 47.720903] dump_stack+0x4f/0x73 >> [ 47.724694] ? blk_mq_sched_get_request+0x6c/0x280 >> [ 47.730137] __schedule_bug+0x6c/0xc0 >> [ 47.734314] __schedule+0x559/0x780 >> [ 47.738302] schedule+0x3b/0x90 >> [ 47.741899] io_schedule+0x11/0x40 >> [ 47.745788] blk_mq_get_tag+0x167/0x2a0 >> [ 47.750162] ? remove_wait_queue+0x70/0x70 >> [ 47.754901] blk_mq_get_driver_tag+0x92/0xf0 >> [ 47.759758] blk_mq_sched_insert_request+0x134/0x170 >> [ 47.765398] ? blk_account_io_start+0xd0/0x270 >> [ 47.770679] blk_mq_make_request+0x1b2/0x850 >> [ 47.775766] generic_make_request+0xf7/0x2d0 >> [ 47.780860] submit_bio+0x5f/0x120 >> [ 47.784979] ? submit_bio+0x5f/0x120 >> [ 47.789631] submit_bh_wbc.isra.46+0x10d/0x130 >> [ 47.794902] submit_bh+0xb/0x10 >> [ 47.798719] journal_submit_commit_record+0x190/0x210 >> [ 47.804686] ? _raw_spin_unlock+0x13/0x30 >> [ 47.809480] jbd2_journal_commit_transaction+0x180a/0x1d00 >> [ 47.815925] kjournald2+0xb6/0x250 >> [ 47.820022] ? kjournald2+0xb6/0x250 >> [ 47.824328] ? remove_wait_queue+0x70/0x70 >> [ 47.829223] kthread+0x10e/0x140 >> [ 47.833147] ? commit_timeout+0x10/0x10 >> [ 47.837742] ? kthread_create_on_node+0x40/0x40 >> [ 47.843122] ret_from_fork+0x29/0x40 >> >> Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request") >> Signed-off-by: Jens Axboe <axboe@fb.com> >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 9d7645f24b05..323eed50d615 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -1634,8 +1634,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) >> blk_mq_try_issue_directly(data.hctx, rq, &cookie); >> return cookie; >> } else if (q->elevator) { >> + blk_mq_put_ctx(data.ctx); >> blk_mq_bio_to_request(rq, bio); >> blk_mq_sched_insert_request(rq, false, true, true, true); >> + return cookie; >> } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) >> blk_mq_run_hw_queue(data.hctx, true); >> >> > > I'm confused, the first thing we check in make_request is: > > if (unlikely(is_flush_fua)) { > blk_mq_bio_to_request(rq, bio); > if (q->elevator) { > blk_mq_sched_insert_request(rq, false, true, true, > true); > } else { > blk_insert_flush(rq); > blk_mq_run_hw_queue(data.hctx, true); > } > } > > and can_block doesn't do anything in the !flush case, so shouldn't it be > changed in that one instead? Just to get closure on this issue, the two cases ends up being folded into one. So we're really triggering the first case, but it's a jump to the 2nd one. Both cases should still be fixed up, the 2nd patch I sent out should be fine. -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached 2017-04-20 22:39 ` Jens Axboe @ 2017-04-20 22:41 ` Omar Sandoval 2017-04-20 22:42 ` Jens Axboe 0 siblings, 1 reply; 8+ messages in thread From: Omar Sandoval @ 2017-04-20 22:41 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block@vger.kernel.org, Christoph Hellwig On Thu, Apr 20, 2017 at 04:39:10PM -0600, Jens Axboe wrote: > On 04/20/2017 03:30 PM, Omar Sandoval wrote: > > On Thu, Apr 20, 2017 at 03:13:43PM -0600, Jens Axboe wrote: > >> We must have dropped the ctx before we call > >> blk_mq_sched_insert_request() with can_block=true, otherwise we risk > >> that a flush request can block on insertion if we are currently out of > >> tags. > >> > >> [ 47.667190] BUG: scheduling while atomic: jbd2/sda2-8/2089/0x00000002 > >> [ 47.674493] Modules linked in: x86_pkg_temp_thermal btrfs xor zlib_deflate raid6_pq sr_mod cdre > >> [ 47.690572] Preemption disabled at: > >> [ 47.690584] [<ffffffff81326c7c>] blk_mq_sched_get_request+0x6c/0x280 > >> [ 47.701764] CPU: 1 PID: 2089 Comm: jbd2/sda2-8 Not tainted 4.11.0-rc7+ #271 > >> [ 47.709630] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016 > >> [ 47.718081] Call Trace: > >> [ 47.720903] dump_stack+0x4f/0x73 > >> [ 47.724694] ? blk_mq_sched_get_request+0x6c/0x280 > >> [ 47.730137] __schedule_bug+0x6c/0xc0 > >> [ 47.734314] __schedule+0x559/0x780 > >> [ 47.738302] schedule+0x3b/0x90 > >> [ 47.741899] io_schedule+0x11/0x40 > >> [ 47.745788] blk_mq_get_tag+0x167/0x2a0 > >> [ 47.750162] ? remove_wait_queue+0x70/0x70 > >> [ 47.754901] blk_mq_get_driver_tag+0x92/0xf0 > >> [ 47.759758] blk_mq_sched_insert_request+0x134/0x170 > >> [ 47.765398] ? blk_account_io_start+0xd0/0x270 > >> [ 47.770679] blk_mq_make_request+0x1b2/0x850 > >> [ 47.775766] generic_make_request+0xf7/0x2d0 > >> [ 47.780860] submit_bio+0x5f/0x120 > >> [ 47.784979] ? submit_bio+0x5f/0x120 > >> [ 47.789631] submit_bh_wbc.isra.46+0x10d/0x130 > >> [ 47.794902] submit_bh+0xb/0x10 > >> [ 47.798719] journal_submit_commit_record+0x190/0x210 > >> [ 47.804686] ? _raw_spin_unlock+0x13/0x30 > >> [ 47.809480] jbd2_journal_commit_transaction+0x180a/0x1d00 > >> [ 47.815925] kjournald2+0xb6/0x250 > >> [ 47.820022] ? kjournald2+0xb6/0x250 > >> [ 47.824328] ? remove_wait_queue+0x70/0x70 > >> [ 47.829223] kthread+0x10e/0x140 > >> [ 47.833147] ? commit_timeout+0x10/0x10 > >> [ 47.837742] ? kthread_create_on_node+0x40/0x40 > >> [ 47.843122] ret_from_fork+0x29/0x40 > >> > >> Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request") > >> Signed-off-by: Jens Axboe <axboe@fb.com> > >> > >> diff --git a/block/blk-mq.c b/block/blk-mq.c > >> index 9d7645f24b05..323eed50d615 100644 > >> --- a/block/blk-mq.c > >> +++ b/block/blk-mq.c > >> @@ -1634,8 +1634,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) > >> blk_mq_try_issue_directly(data.hctx, rq, &cookie); > >> return cookie; > >> } else if (q->elevator) { > >> + blk_mq_put_ctx(data.ctx); > >> blk_mq_bio_to_request(rq, bio); > >> blk_mq_sched_insert_request(rq, false, true, true, true); > >> + return cookie; > >> } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) > >> blk_mq_run_hw_queue(data.hctx, true); > >> > >> > > > > I'm confused, the first thing we check in make_request is: > > > > if (unlikely(is_flush_fua)) { > > blk_mq_bio_to_request(rq, bio); > > if (q->elevator) { > > blk_mq_sched_insert_request(rq, false, true, true, > > true); > > } else { > > blk_insert_flush(rq); > > blk_mq_run_hw_queue(data.hctx, true); > > } > > } > > > > and can_block doesn't do anything in the !flush case, so shouldn't it be > > changed in that one instead? > > Just to get closure on this issue, the two cases ends up being folded > into one. So we're really triggering the first case, but it's a jump > to the 2nd one. > > Both cases should still be fixed up, the 2nd patch I sent out should be > fine. You can add Reviewed-by: Omar Sandoval <osandov@fb.com> for the 2nd patch. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached 2017-04-20 22:41 ` Omar Sandoval @ 2017-04-20 22:42 ` Jens Axboe 0 siblings, 0 replies; 8+ messages in thread From: Jens Axboe @ 2017-04-20 22:42 UTC (permalink / raw) To: Omar Sandoval; +Cc: linux-block@vger.kernel.org, Christoph Hellwig On 04/20/2017 04:41 PM, Omar Sandoval wrote: > On Thu, Apr 20, 2017 at 04:39:10PM -0600, Jens Axboe wrote: >> On 04/20/2017 03:30 PM, Omar Sandoval wrote: >>> On Thu, Apr 20, 2017 at 03:13:43PM -0600, Jens Axboe wrote: >>>> We must have dropped the ctx before we call >>>> blk_mq_sched_insert_request() with can_block=true, otherwise we risk >>>> that a flush request can block on insertion if we are currently out of >>>> tags. >>>> >>>> [ 47.667190] BUG: scheduling while atomic: jbd2/sda2-8/2089/0x00000002 >>>> [ 47.674493] Modules linked in: x86_pkg_temp_thermal btrfs xor zlib_deflate raid6_pq sr_mod cdre >>>> [ 47.690572] Preemption disabled at: >>>> [ 47.690584] [<ffffffff81326c7c>] blk_mq_sched_get_request+0x6c/0x280 >>>> [ 47.701764] CPU: 1 PID: 2089 Comm: jbd2/sda2-8 Not tainted 4.11.0-rc7+ #271 >>>> [ 47.709630] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016 >>>> [ 47.718081] Call Trace: >>>> [ 47.720903] dump_stack+0x4f/0x73 >>>> [ 47.724694] ? blk_mq_sched_get_request+0x6c/0x280 >>>> [ 47.730137] __schedule_bug+0x6c/0xc0 >>>> [ 47.734314] __schedule+0x559/0x780 >>>> [ 47.738302] schedule+0x3b/0x90 >>>> [ 47.741899] io_schedule+0x11/0x40 >>>> [ 47.745788] blk_mq_get_tag+0x167/0x2a0 >>>> [ 47.750162] ? remove_wait_queue+0x70/0x70 >>>> [ 47.754901] blk_mq_get_driver_tag+0x92/0xf0 >>>> [ 47.759758] blk_mq_sched_insert_request+0x134/0x170 >>>> [ 47.765398] ? blk_account_io_start+0xd0/0x270 >>>> [ 47.770679] blk_mq_make_request+0x1b2/0x850 >>>> [ 47.775766] generic_make_request+0xf7/0x2d0 >>>> [ 47.780860] submit_bio+0x5f/0x120 >>>> [ 47.784979] ? submit_bio+0x5f/0x120 >>>> [ 47.789631] submit_bh_wbc.isra.46+0x10d/0x130 >>>> [ 47.794902] submit_bh+0xb/0x10 >>>> [ 47.798719] journal_submit_commit_record+0x190/0x210 >>>> [ 47.804686] ? _raw_spin_unlock+0x13/0x30 >>>> [ 47.809480] jbd2_journal_commit_transaction+0x180a/0x1d00 >>>> [ 47.815925] kjournald2+0xb6/0x250 >>>> [ 47.820022] ? kjournald2+0xb6/0x250 >>>> [ 47.824328] ? remove_wait_queue+0x70/0x70 >>>> [ 47.829223] kthread+0x10e/0x140 >>>> [ 47.833147] ? commit_timeout+0x10/0x10 >>>> [ 47.837742] ? kthread_create_on_node+0x40/0x40 >>>> [ 47.843122] ret_from_fork+0x29/0x40 >>>> >>>> Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request") >>>> Signed-off-by: Jens Axboe <axboe@fb.com> >>>> >>>> diff --git a/block/blk-mq.c b/block/blk-mq.c >>>> index 9d7645f24b05..323eed50d615 100644 >>>> --- a/block/blk-mq.c >>>> +++ b/block/blk-mq.c >>>> @@ -1634,8 +1634,10 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) >>>> blk_mq_try_issue_directly(data.hctx, rq, &cookie); >>>> return cookie; >>>> } else if (q->elevator) { >>>> + blk_mq_put_ctx(data.ctx); >>>> blk_mq_bio_to_request(rq, bio); >>>> blk_mq_sched_insert_request(rq, false, true, true, true); >>>> + return cookie; >>>> } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) >>>> blk_mq_run_hw_queue(data.hctx, true); >>>> >>>> >>> >>> I'm confused, the first thing we check in make_request is: >>> >>> if (unlikely(is_flush_fua)) { >>> blk_mq_bio_to_request(rq, bio); >>> if (q->elevator) { >>> blk_mq_sched_insert_request(rq, false, true, true, >>> true); >>> } else { >>> blk_insert_flush(rq); >>> blk_mq_run_hw_queue(data.hctx, true); >>> } >>> } >>> >>> and can_block doesn't do anything in the !flush case, so shouldn't it be >>> changed in that one instead? >> >> Just to get closure on this issue, the two cases ends up being folded >> into one. So we're really triggering the first case, but it's a jump >> to the 2nd one. >> >> Both cases should still be fixed up, the 2nd patch I sent out should be >> fine. > > You can add > > Reviewed-by: Omar Sandoval <osandov@fb.com> > > for the 2nd patch. Thanks Omar, queued up for 4.12. -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-20 22:42 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-20 21:13 [PATCH] blk-mq: fix schedule-while-atomic with scheduler attached Jens Axboe 2017-04-20 21:30 ` Omar Sandoval 2017-04-20 21:41 ` Jens Axboe 2017-04-20 21:47 ` Omar Sandoval 2017-04-20 21:50 ` Jens Axboe 2017-04-20 22:39 ` Jens Axboe 2017-04-20 22:41 ` Omar Sandoval 2017-04-20 22:42 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox