Two fixes for updating nr_hw_queues.
The first patch fixes the following scenario:
Kyber depends on the mapping between cpu and nr_hw_queues. When
update nr_hw_queues, elevator_type->ops.mq.init_hctx will be
invoked before the mapping is adapted correctly, this would cause
panic in kyber.
The second patch fixes the following scenario:
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.
Jianchao Wang (2)
blk-mq: init hctx sched after update cpu & nr_hw_queues
blk-mq: sync the update nr_hw_queues with part_in_flight
block/blk-mq.c | 40 ++++++++++++++++++++++++++++++++--------
block/blk.h | 2 ++
block/elevator.c | 20 ++++++++++++--------
block/genhd.c | 10 ++++++++--
include/linux/blkdev.h | 4 ++++
5 files changed, 58 insertions(+), 18 deletions(-)
Thanks
Jianchao
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 <[email protected]>
---
block/blk-mq.c | 3 +++
block/genhd.c | 10 ++++++++--
include/linux/blkdev.h | 1 +
3 files changed, 12 insertions(+), 2 deletions(-)
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;
--
2.7.4
Kyber depends on the mapping between cpu and nr_hw_queues. When
update nr_hw_queues, elevator_type->ops.mq.init_hctx will be
invoked before the mapping is adapted correctly, this would cause
terrible result. A simply way to fix this is switch the io scheduler
to none before update the nr_hw_queues, and then get it back after
update nr_hw_queues. To achieve this, we add a new member elv_type
in request_queue to save the original elevator and adapt and export
elevator_switch_mq.
Signed-off-by: Jianchao Wang <[email protected]>
---
block/blk-mq.c | 37 +++++++++++++++++++++++++++++--------
block/blk.h | 2 ++
block/elevator.c | 20 ++++++++++++--------
include/linux/blkdev.h | 3 +++
4 files changed, 46 insertions(+), 16 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5efd789..89904cc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -112,6 +112,7 @@ 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;
+
blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi);
}
@@ -2147,8 +2148,6 @@ static void blk_mq_exit_hctx(struct request_queue *q,
if (set->ops->exit_request)
set->ops->exit_request(set, hctx->fq->flush_rq, hctx_idx);
- blk_mq_sched_exit_hctx(q, hctx, hctx_idx);
-
if (set->ops->exit_hctx)
set->ops->exit_hctx(hctx, hctx_idx);
@@ -2216,12 +2215,9 @@ static int blk_mq_init_hctx(struct request_queue *q,
set->ops->init_hctx(hctx, set->driver_data, hctx_idx))
goto free_bitmap;
- if (blk_mq_sched_init_hctx(q, hctx, hctx_idx))
- goto exit_hctx;
-
hctx->fq = blk_alloc_flush_queue(q, hctx->numa_node, set->cmd_size);
if (!hctx->fq)
- goto sched_exit_hctx;
+ goto exit_hctx;
if (blk_mq_init_request(set, hctx->fq->flush_rq, hctx_idx, node))
goto free_fq;
@@ -2235,8 +2231,6 @@ static int blk_mq_init_hctx(struct request_queue *q,
free_fq:
kfree(hctx->fq);
- sched_exit_hctx:
- blk_mq_sched_exit_hctx(q, hctx, hctx_idx);
exit_hctx:
if (set->ops->exit_hctx)
set->ops->exit_hctx(hctx, hctx_idx);
@@ -2913,6 +2907,25 @@ 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);
+ /*
+ * switch io scheduler to NULL to clean up the data in it.
+ * will get it back after update mapping between cpu and hw queues.
+ */
+ list_for_each_entry(q, &set->tag_list, tag_set_list) {
+ if (!q->elevator) {
+ q->elv_type = NULL;
+ continue;
+ }
+ q->elv_type = q->elevator->type;
+ mutex_lock(&q->sysfs_lock);
+ /*
+ * elevator_release will put it.
+ */
+ __module_get(q->elv_type->elevator_owner);
+ elevator_switch_mq(q, NULL);
+ mutex_unlock(&q->sysfs_lock);
+ }
+
set->nr_hw_queues = nr_hw_queues;
blk_mq_update_queue_map(set);
list_for_each_entry(q, &set->tag_list, tag_set_list) {
@@ -2920,6 +2933,14 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
blk_mq_queue_reinit(q);
}
+ list_for_each_entry(q, &set->tag_list, tag_set_list) {
+ if (!q->elv_type)
+ continue;
+
+ mutex_lock(&q->sysfs_lock);
+ elevator_switch_mq(q, q->elv_type);
+ mutex_unlock(&q->sysfs_lock);
+ }
list_for_each_entry(q, &set->tag_list, tag_set_list)
blk_mq_unfreeze_queue(q);
}
diff --git a/block/blk.h b/block/blk.h
index d4d67e9..0c9bc8d 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -234,6 +234,8 @@ static inline void elv_deactivate_rq(struct request_queue *q, struct request *rq
int elevator_init(struct request_queue *);
int elevator_init_mq(struct request_queue *q);
+int elevator_switch_mq(struct request_queue *q,
+ struct elevator_type *new_e);
void elevator_exit(struct request_queue *, struct elevator_queue *);
int elv_register_queue(struct request_queue *q);
void elv_unregister_queue(struct request_queue *q);
diff --git a/block/elevator.c b/block/elevator.c
index fa828b5..5ea6e7d 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -933,16 +933,13 @@ void elv_unregister(struct elevator_type *e)
}
EXPORT_SYMBOL_GPL(elv_unregister);
-static int elevator_switch_mq(struct request_queue *q,
+int elevator_switch_mq(struct request_queue *q,
struct elevator_type *new_e)
{
int ret;
lockdep_assert_held(&q->sysfs_lock);
- blk_mq_freeze_queue(q);
- blk_mq_quiesce_queue(q);
-
if (q->elevator) {
if (q->elevator->registered)
elv_unregister_queue(q);
@@ -968,8 +965,6 @@ static int elevator_switch_mq(struct request_queue *q,
blk_add_trace_msg(q, "elv switch: none");
out:
- blk_mq_unquiesce_queue(q);
- blk_mq_unfreeze_queue(q);
return ret;
}
@@ -1021,8 +1016,17 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
lockdep_assert_held(&q->sysfs_lock);
- if (q->mq_ops)
- return elevator_switch_mq(q, new_e);
+ if (q->mq_ops) {
+ blk_mq_freeze_queue(q);
+ blk_mq_quiesce_queue(q);
+
+ err = elevator_switch_mq(q, new_e);
+
+ blk_mq_unquiesce_queue(q);
+ blk_mq_unfreeze_queue(q);
+
+ return err;
+ }
/*
* Turn on BYPASS and drain all requests w/ elevator private data.
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index d6869e0..ee930c4 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -437,6 +437,9 @@ struct request_queue {
struct list_head queue_head;
struct request *last_merge;
struct elevator_queue *elevator;
+
+ /* used when update nr_hw_queues */
+ struct elevator_type *elv_type;
int nr_rqs[2]; /* # allocated [a]sync rqs */
int nr_rqs_elvpriv; /* # allocated rqs w/ elvpriv */
--
2.7.4
On Wed, Aug 15, 2018 at 3:25 PM, Jianchao Wang
<[email protected]> wrote:
> Kyber depends on the mapping between cpu and nr_hw_queues. When
> update nr_hw_queues, elevator_type->ops.mq.init_hctx will be
> invoked before the mapping is adapted correctly, this would cause
> terrible result. A simply way to fix this is switch the io scheduler
> to none before update the nr_hw_queues, and then get it back after
> update nr_hw_queues. To achieve this, we add a new member elv_type
> in request_queue to save the original elevator and adapt and export
> elevator_switch_mq.
>
> Signed-off-by: Jianchao Wang <[email protected]>
> ---
> block/blk-mq.c | 37 +++++++++++++++++++++++++++++--------
> block/blk.h | 2 ++
> block/elevator.c | 20 ++++++++++++--------
> include/linux/blkdev.h | 3 +++
> 4 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 5efd789..89904cc 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -112,6 +112,7 @@ 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;
> +
Not necessary to do that.
> blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi);
> }
>
> @@ -2147,8 +2148,6 @@ static void blk_mq_exit_hctx(struct request_queue *q,
> if (set->ops->exit_request)
> set->ops->exit_request(set, hctx->fq->flush_rq, hctx_idx);
>
> - blk_mq_sched_exit_hctx(q, hctx, hctx_idx);
> -
> if (set->ops->exit_hctx)
> set->ops->exit_hctx(hctx, hctx_idx);
>
> @@ -2216,12 +2215,9 @@ static int blk_mq_init_hctx(struct request_queue *q,
> set->ops->init_hctx(hctx, set->driver_data, hctx_idx))
> goto free_bitmap;
>
> - if (blk_mq_sched_init_hctx(q, hctx, hctx_idx))
> - goto exit_hctx;
> -
> hctx->fq = blk_alloc_flush_queue(q, hctx->numa_node, set->cmd_size);
> if (!hctx->fq)
> - goto sched_exit_hctx;
> + goto exit_hctx;
>
> if (blk_mq_init_request(set, hctx->fq->flush_rq, hctx_idx, node))
> goto free_fq;
> @@ -2235,8 +2231,6 @@ static int blk_mq_init_hctx(struct request_queue *q,
>
> free_fq:
> kfree(hctx->fq);
> - sched_exit_hctx:
> - blk_mq_sched_exit_hctx(q, hctx, hctx_idx);
Seems both blk_mq_sched_init_hctx() and blk_mq_sched_exit_hctx() may be
removed now.
> exit_hctx:
> if (set->ops->exit_hctx)
> set->ops->exit_hctx(hctx, hctx_idx);
> @@ -2913,6 +2907,25 @@ 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);
>
> + /*
> + * switch io scheduler to NULL to clean up the data in it.
> + * will get it back after update mapping between cpu and hw queues.
> + */
> + list_for_each_entry(q, &set->tag_list, tag_set_list) {
> + if (!q->elevator) {
> + q->elv_type = NULL;
> + continue;
> + }
> + q->elv_type = q->elevator->type;
> + mutex_lock(&q->sysfs_lock);
> + /*
> + * elevator_release will put it.
> + */
> + __module_get(q->elv_type->elevator_owner);
> + elevator_switch_mq(q, NULL);
> + mutex_unlock(&q->sysfs_lock);
> + }
> +
> set->nr_hw_queues = nr_hw_queues;
> blk_mq_update_queue_map(set);
> list_for_each_entry(q, &set->tag_list, tag_set_list) {
> @@ -2920,6 +2933,14 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
> blk_mq_queue_reinit(q);
> }
>
> + list_for_each_entry(q, &set->tag_list, tag_set_list) {
> + if (!q->elv_type)
> + continue;
> +
> + mutex_lock(&q->sysfs_lock);
> + elevator_switch_mq(q, q->elv_type);
> + mutex_unlock(&q->sysfs_lock);
> + }
BFQ defines .init_hctx() too, so seems this generic approach is correct way for
this issue.
thanks,
Ming Lei
On Wed, Aug 15, 2018 at 3:25 PM, Jianchao Wang
<[email protected]> 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
> <[email protected]> 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
> > <[email protected]> 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 08/15/2018 07:32 PM, Ming Lei wrote:
> On Wed, Aug 15, 2018 at 3:25 PM, Jianchao Wang
> <[email protected]> wrote:
>> Kyber depends on the mapping between cpu and nr_hw_queues. When
>> update nr_hw_queues, elevator_type->ops.mq.init_hctx will be
>> invoked before the mapping is adapted correctly, this would cause
>> terrible result. A simply way to fix this is switch the io scheduler
>> to none before update the nr_hw_queues, and then get it back after
>> update nr_hw_queues. To achieve this, we add a new member elv_type
>> in request_queue to save the original elevator and adapt and export
>> elevator_switch_mq.
>>
>> Signed-off-by: Jianchao Wang <[email protected]>
>> ---
>> block/blk-mq.c | 37 +++++++++++++++++++++++++++++--------
>> block/blk.h | 2 ++
>> block/elevator.c | 20 ++++++++++++--------
>> include/linux/blkdev.h | 3 +++
>> 4 files changed, 46 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 5efd789..89904cc 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -112,6 +112,7 @@ 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;
>> +
>
> Not necessary to do that.
Yes, I will discard this.
>
>> blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi);
>> }
>>
>> @@ -2147,8 +2148,6 @@ static void blk_mq_exit_hctx(struct request_queue *q,
>> if (set->ops->exit_request)
>> set->ops->exit_request(set, hctx->fq->flush_rq, hctx_idx);
>>
>> - blk_mq_sched_exit_hctx(q, hctx, hctx_idx);
>> -
>> if (set->ops->exit_hctx)
>> set->ops->exit_hctx(hctx, hctx_idx);
>>
>> @@ -2216,12 +2215,9 @@ static int blk_mq_init_hctx(struct request_queue *q,
>> set->ops->init_hctx(hctx, set->driver_data, hctx_idx))
>> goto free_bitmap;
>>
>> - if (blk_mq_sched_init_hctx(q, hctx, hctx_idx))
>> - goto exit_hctx;
>> -
>> hctx->fq = blk_alloc_flush_queue(q, hctx->numa_node, set->cmd_size);
>> if (!hctx->fq)
>> - goto sched_exit_hctx;
>> + goto exit_hctx;
>>
>> if (blk_mq_init_request(set, hctx->fq->flush_rq, hctx_idx, node))
>> goto free_fq;
>> @@ -2235,8 +2231,6 @@ static int blk_mq_init_hctx(struct request_queue *q,
>>
>> free_fq:
>> kfree(hctx->fq);
>> - sched_exit_hctx:
>> - blk_mq_sched_exit_hctx(q, hctx, hctx_idx);
>
> Seems both blk_mq_sched_init_hctx() and blk_mq_sched_exit_hctx() may be
> removed now.
Yes, I will remove them in V2.
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