[PATCH 0/2] fixes for the updating nr_hw_queues

[PATCH 0/2] fixes for the updating nr_hw_queues

[Jianchao Wang]

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

[PATCH 1/2] blk-mq: init hctx sched after update cpu & nr_hw_queues mapping

[Jianchao Wang]

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 <jianchao.w.wang@oracle.com>
---
 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

[PATCH 2/2] blk-mq: sync the update nr_hw_queues with part_in_flight

[Jianchao Wang]

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(-)

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

Re: [PATCH 1/2] blk-mq: init hctx sched after update cpu & nr_hw_queues mapping

[Ming Lei]

On Wed, Aug 15, 2018 at 3:25 PM, Jianchao Wang
<jianchao.w.wang@oracle.com> 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 <jianchao.w.wang@oracle.com>
> ---
>  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

Re: [PATCH 2/2] blk-mq: sync the update nr_hw_queues with part_in_flight

[Ming Lei]

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

Re: [PATCH 2/2] blk-mq: sync the update nr_hw_queues with part_in_flight

[jianchao.wang]

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
> 

Re: [PATCH 2/2] blk-mq: sync the update nr_hw_queues with part_in_flight

[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

Re: [PATCH 2/2] blk-mq: sync the update nr_hw_queues with part_in_flight

[jianchao.wang]

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

Re: [PATCH 2/2] blk-mq: sync the update nr_hw_queues with part_in_flight

[Ming Lei]

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

Re: [PATCH 1/2] blk-mq: init hctx sched after update cpu & nr_hw_queues mapping

[jianchao.wang]

On 08/15/2018 07:32 PM, Ming Lei wrote:
> On Wed, Aug 15, 2018 at 3:25 PM, Jianchao Wang
> <jianchao.w.wang@oracle.com> 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 <jianchao.w.wang@oracle.com>
>> ---
>>  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