[PATCH 0/6] Six more patches for Linux kernel v4.12

[PATCH 0/6] Six more patches for Linux kernel v4.12

[Bart Van Assche]

Hello Jens,

Since the merge window will open soon, do you still accept patches for
Linux kernel v4.12? If so, please consider the six patches in this series.
This series contains one bug fix, one comment fix and four patches that
extend the blk-mq-debugfs functionality further.

Thanks,

Bart.

Bart Van Assche (6):
  blk-mq: Make blk_mq_quiesce_queue() wait for all .queue_rq() calls
  blk-mq: Fix the comment above blk_mq_quiesce_queue()
  blk-mq-debugfs: Show atomic request flags
  blk-mq-debugfs: Show requeue list
  blk-mq-debugfs: Show busy requests
  blk-mq-debugfs: Add 'kick' operation

 block/blk-mq-debugfs.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++-
 block/blk-mq.c         |  8 ++---
 2 files changed, 98 insertions(+), 5 deletions(-)

-- 
2.12.2

[PATCH 1/6] blk-mq: Make blk_mq_quiesce_queue() wait for all .queue_rq() calls

[Bart Van Assche]

blk_mq_quiesce_queue() callers, e.g. elevator_switch_mq(), assume
that no .queue_rq() calls occur while switching to another I/O
scheduler. This patch fixes the following kernel crash if another
I/O scheduler than "none" is the default scheduler:

general protection fault: 0000 [#1] SMP
RIP: 0010:__lock_acquire+0xfe/0x1280
Call Trace:
 lock_acquire+0xd5/0x1c0
 _raw_spin_lock+0x2a/0x40
 dd_dispatch_request+0x29/0x1e0
 blk_mq_sched_dispatch_requests+0x139/0x190
 __blk_mq_run_hw_queue+0x12d/0x1c0
 blk_mq_run_work_fn+0xd/0x10
 process_one_work+0x206/0x6a0
 worker_thread+0x49/0x4a0
 kthread+0x107/0x140
 ret_from_fork+0x2e/0x40

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: <stable@vger.kernel.org>
---
 block/blk-mq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index b75ef2392db7..3b3420f76b5a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1224,8 +1224,9 @@ EXPORT_SYMBOL(blk_mq_queue_stopped);
 
 void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx)
 {
-	cancel_work(&hctx->run_work);
-	cancel_delayed_work(&hctx->delay_work);
+	cancel_work_sync(&hctx->run_work);
+	cancel_delayed_work_sync(&hctx->delay_work);
+	cancel_delayed_work_sync(&hctx->delayed_run_work);
 	set_bit(BLK_MQ_S_STOPPED, &hctx->state);
 }
 EXPORT_SYMBOL(blk_mq_stop_hw_queue);
-- 
2.12.2

[PATCH 2/6] blk-mq: Fix the comment above blk_mq_quiesce_queue()

[Bart Van Assche]

The first version of blk_mq_quiesce_queue() that was posted on
the linux-block mailing list waited for ongoing .queue_rq()
calls but did not stop the request queue. The current version
however stops the request queue. Hence reflect this behavior in
the comment above that function.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3b3420f76b5a..113c09d8026c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -157,8 +157,7 @@ EXPORT_SYMBOL_GPL(blk_mq_unfreeze_queue);
  * @q: request queue.
  *
  * Note: this function does not prevent that the struct request end_io()
- * callback function is invoked. Additionally, it is not prevented that
- * new queue_rq() calls occur unless the queue has been stopped first.
+ * callback function is invoked.
  */
 void blk_mq_quiesce_queue(struct request_queue *q)
 {
-- 
2.12.2

[PATCH 3/6] blk-mq-debugfs: Show atomic request flags

[Bart Van Assche]

When analyzing e.g. queue lockups it is important to know whether
or not a request has already been started. Hence also show the
atomic request flags.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
---
 block/blk-mq-debugfs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index bcd2a7d4a3a5..9cb673bc7230 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -308,6 +308,12 @@ static const char *const rqf_name[] = {
 	[ilog2((__force u32)RQF_SPECIAL_PAYLOAD)]	= "SPECIAL_PAYLOAD",
 };
 
+static const char *const rqaf_name[] = {
+	[REQ_ATOM_COMPLETE]	= "COMPLETE",
+	[REQ_ATOM_STARTED]	= "STARTED",
+	[REQ_ATOM_POLL_SLEPT]	= "POLL_SLEPT",
+};
+
 static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
 {
 	struct request *rq = list_entry_rq(v);
@@ -325,6 +331,8 @@ static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
 	seq_puts(m, ", .rq_flags=");
 	blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
 		       ARRAY_SIZE(rqf_name));
+	seq_puts(m, ", .atomic_flags=");
+	blk_flags_show(m, rq->atomic_flags, rqaf_name, ARRAY_SIZE(rqaf_name));
 	seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
 		   rq->internal_tag);
 	if (mq_ops->show_rq)
-- 
2.12.2

[PATCH 4/6] blk-mq-debugfs: Show requeue list

[Bart Van Assche]

When verifying whether or not a blk-mq driver forgot to kick the
requeue list after having requeued a request it is important to
be able to verify the contents of the requeue list. Hence export
that list through debugfs.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
---
 block/blk-mq-debugfs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 9cb673bc7230..5092d90e37f6 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -341,6 +341,50 @@ static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
 	return 0;
 }
 
+static void *queue_requeue_list_start(struct seq_file *m, loff_t *pos)
+	__acquires(&q->requeue_lock)
+{
+	struct request_queue *q = m->private;
+
+	spin_lock(&q->requeue_lock);
+	return seq_list_start(&q->requeue_list, *pos);
+}
+
+static void *queue_requeue_list_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	struct request_queue *q = m->private;
+
+	return seq_list_next(v, &q->requeue_list, pos);
+}
+
+static void queue_requeue_list_stop(struct seq_file *m, void *v)
+	__releases(&q->requeue_lock)
+{
+	struct request_queue *q = m->private;
+
+	spin_unlock(&q->requeue_lock);
+}
+
+static const struct seq_operations queue_requeue_list_seq_ops = {
+	.start	= queue_requeue_list_start,
+	.next	= queue_requeue_list_next,
+	.stop	= queue_requeue_list_stop,
+	.show	= blk_mq_debugfs_rq_show,
+};
+
+static int queue_requeue_list_open(struct inode *inode, struct file *file)
+{
+	return blk_mq_debugfs_seq_open(inode, file,
+				       &queue_requeue_list_seq_ops);
+}
+
+static const struct file_operations queue_requeue_list_fops = {
+	.open		= queue_requeue_list_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
+};
+
 static void *hctx_dispatch_start(struct seq_file *m, loff_t *pos)
 	__acquires(&hctx->lock)
 {
@@ -831,6 +875,7 @@ static const struct file_operations ctx_completed_fops = {
 };
 
 static const struct blk_mq_debugfs_attr blk_mq_debugfs_queue_attrs[] = {
+	{"requeue_list", 0400, &queue_requeue_list_fops},
 	{"poll_stat", 0400, &queue_poll_stat_fops},
 	{"state", 0600, &blk_queue_flags_fops},
 	{},
-- 
2.12.2

[PATCH 5/6] blk-mq-debugfs: Show busy requests

[Bart Van Assche]

Requests that got stuck in a block driver are neither on
blk_mq_ctx.rq_list nor on any hw dispatch queue. Make these
visible in debugfs through the "busy" attribute.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
---
 block/blk-mq-debugfs.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 5092d90e37f6..a5e286e04569 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -428,6 +428,43 @@ static const struct file_operations hctx_dispatch_fops = {
 	.release	= seq_release,
 };
 
+struct show_busy_ctx {
+	struct seq_file		*m;
+	struct blk_mq_hw_ctx	*hctx;
+};
+
+static void hctx_show_busy(struct request *rq, void *data, bool reserved)
+{
+	const struct show_busy_ctx *ctx = data;
+	struct request_queue *q = rq->q;
+
+	if (q->queue_hw_ctx[rq->mq_ctx->index_hw] == ctx->hctx &&
+	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
+		blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
+}
+
+static int hctx_busy_show(struct seq_file *m, void *v)
+{
+	struct blk_mq_hw_ctx *hctx = m->private;
+	struct show_busy_ctx ctx = { .m = m, .hctx = hctx };
+
+	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &ctx);
+
+	return 0;
+}
+
+static int hctx_busy_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, hctx_busy_show, inode->i_private);
+}
+
+static const struct file_operations hctx_busy_fops = {
+	.open		= hctx_busy_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static int hctx_ctx_map_show(struct seq_file *m, void *v)
 {
 	struct blk_mq_hw_ctx *hctx = m->private;
@@ -885,6 +922,7 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_hctx_attrs[] = {
 	{"state", 0400, &hctx_state_fops},
 	{"flags", 0400, &hctx_flags_fops},
 	{"dispatch", 0400, &hctx_dispatch_fops},
+	{"busy", 0400, &hctx_busy_fops},
 	{"ctx_map", 0400, &hctx_ctx_map_fops},
 	{"tags", 0400, &hctx_tags_fops},
 	{"tags_bitmap", 0400, &hctx_tags_bitmap_fops},
-- 
2.12.2

[PATCH 6/6] blk-mq-debugfs: Add 'kick' operation

[Bart Van Assche]

Running a queue causes the block layer to examine the per-CPU and
hw queues but not the requeue list. Hence add a 'kick' operation
that also examines the requeue list.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
---
 block/blk-mq-debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index a5e286e04569..aeca26c739d1 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -120,8 +120,10 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
 		blk_mq_run_hw_queues(q, true);
 	} else if (strcmp(op, "start") == 0) {
 		blk_mq_start_stopped_hw_queues(q, true);
+	} else if (strcmp(op, "kick") == 0) {
+		blk_mq_kick_requeue_list(q);
 	} else {
-		pr_err("%s: unsupported operation %s. Use either 'run' or 'start'\n",
+		pr_err("%s: unsupported operation %s. Use 'run', 'start' or 'kick'\n",
 		       __func__, op);
 		return -EINVAL;
 	}
-- 
2.12.2

Re: [PATCH 2/6] blk-mq: Fix the comment above blk_mq_quiesce_queue()

[Omar Sandoval]

On Thu, Apr 27, 2017 at 08:54:33AM -0700, Bart Van Assche wrote:
> The first version of blk_mq_quiesce_queue() that was posted on
> the linux-block mailing list waited for ongoing .queue_rq()
> calls but did not stop the request queue. The current version
> however stops the request queue. Hence reflect this behavior in
> the comment above that function.

Can you just combine this with patch 1?

> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3b3420f76b5a..113c09d8026c 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -157,8 +157,7 @@ EXPORT_SYMBOL_GPL(blk_mq_unfreeze_queue);
>   * @q: request queue.
>   *
>   * Note: this function does not prevent that the struct request end_io()
> - * callback function is invoked. Additionally, it is not prevented that
> - * new queue_rq() calls occur unless the queue has been stopped first.
> + * callback function is invoked.
>   */
>  void blk_mq_quiesce_queue(struct request_queue *q)
>  {
> -- 
> 2.12.2
> 

Re: [PATCH 3/6] blk-mq-debugfs: Show atomic request flags

[Omar Sandoval]

On Thu, Apr 27, 2017 at 08:54:34AM -0700, Bart Van Assche wrote:
> When analyzing e.g. queue lockups it is important to know whether
> or not a request has already been started. Hence also show the
> atomic request flags.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index bcd2a7d4a3a5..9cb673bc7230 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -308,6 +308,12 @@ static const char *const rqf_name[] = {
>  	[ilog2((__force u32)RQF_SPECIAL_PAYLOAD)]	= "SPECIAL_PAYLOAD",
>  };
>  
> +static const char *const rqaf_name[] = {
> +	[REQ_ATOM_COMPLETE]	= "COMPLETE",
> +	[REQ_ATOM_STARTED]	= "STARTED",
> +	[REQ_ATOM_POLL_SLEPT]	= "POLL_SLEPT",
> +};
> +
>  static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
>  {
>  	struct request *rq = list_entry_rq(v);
> @@ -325,6 +331,8 @@ static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
>  	seq_puts(m, ", .rq_flags=");
>  	blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
>  		       ARRAY_SIZE(rqf_name));
> +	seq_puts(m, ", .atomic_flags=");
> +	blk_flags_show(m, rq->atomic_flags, rqaf_name, ARRAY_SIZE(rqaf_name));
>  	seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
>  		   rq->internal_tag);
>  	if (mq_ops->show_rq)
> -- 
> 2.12.2
> 

Re: [PATCH 2/6] blk-mq: Fix the comment above blk_mq_quiesce_queue()

[Bart Van Assche]

On Thu, 2017-04-27 at 15:37 -0700, Omar Sandoval wrote:
> On Thu, Apr 27, 2017 at 08:54:33AM -0700, Bart Van Assche wrote:
> > The first version of blk_mq_quiesce_queue() that was posted on
> > the linux-block mailing list waited for ongoing .queue_rq()
> > calls but did not stop the request queue. The current version
> > however stops the request queue. Hence reflect this behavior in
> > the comment above that function.
>=20
> Can you just combine this with patch 1?

Hello Omar,

Patch 1/6 has a "Cc: stable" tag but this patch not. The comment change
is a separate patch because I don't think this comment change needs to
be integrated in the stable kernel series.

Bart.=

Re: [PATCH 4/6] blk-mq-debugfs: Show requeue list

[Omar Sandoval]

On Thu, Apr 27, 2017 at 08:54:35AM -0700, Bart Van Assche wrote:
> When verifying whether or not a blk-mq driver forgot to kick the
> requeue list after having requeued a request it is important to
> be able to verify the contents of the requeue list. Hence export
> that list through debugfs.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 9cb673bc7230..5092d90e37f6 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -341,6 +341,50 @@ static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
>  	return 0;
>  }
>  
> +static void *queue_requeue_list_start(struct seq_file *m, loff_t *pos)
> +	__acquires(&q->requeue_lock)
> +{
> +	struct request_queue *q = m->private;
> +
> +	spin_lock(&q->requeue_lock);
> +	return seq_list_start(&q->requeue_list, *pos);
> +}
> +
> +static void *queue_requeue_list_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> +	struct request_queue *q = m->private;
> +
> +	return seq_list_next(v, &q->requeue_list, pos);
> +}
> +
> +static void queue_requeue_list_stop(struct seq_file *m, void *v)
> +	__releases(&q->requeue_lock)
> +{
> +	struct request_queue *q = m->private;
> +
> +	spin_unlock(&q->requeue_lock);
> +}
> +
> +static const struct seq_operations queue_requeue_list_seq_ops = {
> +	.start	= queue_requeue_list_start,
> +	.next	= queue_requeue_list_next,
> +	.stop	= queue_requeue_list_stop,
> +	.show	= blk_mq_debugfs_rq_show,
> +};
> +
> +static int queue_requeue_list_open(struct inode *inode, struct file *file)
> +{
> +	return blk_mq_debugfs_seq_open(inode, file,
> +				       &queue_requeue_list_seq_ops);
> +}
> +
> +static const struct file_operations queue_requeue_list_fops = {
> +	.open		= queue_requeue_list_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= seq_release,
> +};
> +
>  static void *hctx_dispatch_start(struct seq_file *m, loff_t *pos)
>  	__acquires(&hctx->lock)
>  {
> @@ -831,6 +875,7 @@ static const struct file_operations ctx_completed_fops = {
>  };
>  
>  static const struct blk_mq_debugfs_attr blk_mq_debugfs_queue_attrs[] = {
> +	{"requeue_list", 0400, &queue_requeue_list_fops},
>  	{"poll_stat", 0400, &queue_poll_stat_fops},
>  	{"state", 0600, &blk_queue_flags_fops},
>  	{},
> -- 
> 2.12.2
> 

Re: [PATCH 1/6] blk-mq: Make blk_mq_quiesce_queue() wait for all .queue_rq() calls

[Ming Lei]

Hi Bart,

On Thu, Apr 27, 2017 at 08:54:32AM -0700, Bart Van Assche wrote:
> blk_mq_quiesce_queue() callers, e.g. elevator_switch_mq(), assume
> that no .queue_rq() calls occur while switching to another I/O

I think we should call blk_mq_freeze_queue_wait() instead
of blk_quiesce_queue() in elevator_switch_mq(), otherwise
it is easy to cause use-after-free.

> scheduler. This patch fixes the following kernel crash if another
> I/O scheduler than "none" is the default scheduler:
> 
> general protection fault: 0000 [#1] SMP
> RIP: 0010:__lock_acquire+0xfe/0x1280
> Call Trace:
>  lock_acquire+0xd5/0x1c0
>  _raw_spin_lock+0x2a/0x40
>  dd_dispatch_request+0x29/0x1e0
>  blk_mq_sched_dispatch_requests+0x139/0x190
>  __blk_mq_run_hw_queue+0x12d/0x1c0
>  blk_mq_run_work_fn+0xd/0x10
>  process_one_work+0x206/0x6a0
>  worker_thread+0x49/0x4a0
>  kthread+0x107/0x140
>  ret_from_fork+0x2e/0x40
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: <stable@vger.kernel.org>
> ---
>  block/blk-mq.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index b75ef2392db7..3b3420f76b5a 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1224,8 +1224,9 @@ EXPORT_SYMBOL(blk_mq_queue_stopped);
>  
>  void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx)
>  {
> -	cancel_work(&hctx->run_work);
> -	cancel_delayed_work(&hctx->delay_work);
> +	cancel_work_sync(&hctx->run_work);
> +	cancel_delayed_work_sync(&hctx->delay_work);

Could you explain it a bit why we need the sync version?

> +	cancel_delayed_work_sync(&hctx->delayed_run_work);

More introduced, more bugs may come, :-)

So I suggest to unity both .run_work and .dealyed_run_work
into one work, just as what Jens did in the following link:

	http://marc.info/?t=149183989800010&r=1&w=2


Thanks,
Ming

Re: [PATCH 2/6] blk-mq: Fix the comment above blk_mq_quiesce_queue()

[Ming Lei]

On Thu, Apr 27, 2017 at 08:54:33AM -0700, Bart Van Assche wrote:
> The first version of blk_mq_quiesce_queue() that was posted on
> the linux-block mailing list waited for ongoing .queue_rq()
> calls but did not stop the request queue. The current version
> however stops the request queue. Hence reflect this behavior in
> the comment above that function.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3b3420f76b5a..113c09d8026c 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -157,8 +157,7 @@ EXPORT_SYMBOL_GPL(blk_mq_unfreeze_queue);
>   * @q: request queue.
>   *
>   * Note: this function does not prevent that the struct request end_io()
> - * callback function is invoked. Additionally, it is not prevented that
> - * new queue_rq() calls occur unless the queue has been stopped first.
> + * callback function is invoked.
>   */
>  void blk_mq_quiesce_queue(struct request_queue *q)
>  {

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

Re: [PATCH 1/6] blk-mq: Make blk_mq_quiesce_queue() wait for all .queue_rq() calls

[Ming Lei]

Hi,

On Fri, Apr 28, 2017 at 10:00:44AM +0800, Ming Lei wrote:
> Hi Bart,
> 
> On Thu, Apr 27, 2017 at 08:54:32AM -0700, Bart Van Assche wrote:
> > blk_mq_quiesce_queue() callers, e.g. elevator_switch_mq(), assume
> > that no .queue_rq() calls occur while switching to another I/O
> 
> I think we should call blk_mq_freeze_queue_wait() instead
> of blk_quiesce_queue() in elevator_switch_mq(), otherwise
> it is easy to cause use-after-free.

Sorry, my fault, blk_mq_freeze_queue_wait() has been done in
blk_mq_freeze_queue() already, so no new IO can enter queue
any more, and all old pending IOs will be fninished when 
blk_mq_freeze_queue() returns.

> 
> > scheduler. This patch fixes the following kernel crash if another
> > I/O scheduler than "none" is the default scheduler:
> > 
> > general protection fault: 0000 [#1] SMP
> > RIP: 0010:__lock_acquire+0xfe/0x1280
> > Call Trace:
> >  lock_acquire+0xd5/0x1c0
> >  _raw_spin_lock+0x2a/0x40
> >  dd_dispatch_request+0x29/0x1e0
> >  blk_mq_sched_dispatch_requests+0x139/0x190
> >  __blk_mq_run_hw_queue+0x12d/0x1c0
> >  blk_mq_run_work_fn+0xd/0x10
> >  process_one_work+0x206/0x6a0
> >  worker_thread+0x49/0x4a0
> >  kthread+0x107/0x140
> >  ret_from_fork+0x2e/0x40

So I am just wondering where the I/O in the above issue is
from? and feels there might be other bug behind the issue. 

Also blk_mq_quiesce_queue() in elevator_switch_mq()
shouldn't be necessary.

Thanks,
Ming

Re: [PATCH 1/6] blk-mq: Make blk_mq_quiesce_queue() wait for all .queue_rq() calls

[Bart Van Assche]

On Fri, 2017-04-28 at 10:00 +0800, Ming Lei wrote:
> On Thu, Apr 27, 2017 at 08:54:32AM -0700, Bart Van Assche wrote:
> >  void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx)
> >  {
> > -	cancel_work(&hctx->run_work);
> > -	cancel_delayed_work(&hctx->delay_work);
> > +	cancel_work_sync(&hctx->run_work);
> > +	cancel_delayed_work_sync(&hctx->delay_work);
>=20
> Could you explain it a bit why we need the sync version?

Because the purpose of this patch is to make blk_mq_quiesce_queue() wait fo=
r
all .queue_rq() calls.

> So I suggest to unity both .run_work and .dealyed_run_work
> into one work, just as what Jens did in the following link:
>=20
> 	http://marc.info/?t=3D149183989800010&r=3D1&w=3D2

That should be done after this patch is upstream otherwise this
patch won't apply to the stable trees.

Bart.=

Re: [PATCH 1/6] blk-mq: Make blk_mq_quiesce_queue() wait for all .queue_rq() calls

[Bart Van Assche]

On Fri, 2017-04-28 at 10:00 +0800, Ming Lei wrote:
> On Thu, Apr 27, 2017 at 08:54:32AM -0700, Bart Van Assche wrote:
> >  void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx)
> >  {
> > -	cancel_work(&hctx->run_work);
> > -	cancel_delayed_work(&hctx->delay_work);
> > +	cancel_work_sync(&hctx->run_work);
> > +	cancel_delayed_work_sync(&hctx->delay_work);
> 
> Could you explain it a bit why we need the sync version?

Because the purpose of this patch is to make blk_mq_quiesce_queue() wait for
all .queue_rq() calls.

> So I suggest to unity both .run_work and .dealyed_run_work
> into one work, just as what Jens did in the following link:
> 
> 	http://marc.info/?t=149183989800010&r=1&w=2

That should be done after this patch is upstream otherwise this
patch won't apply to the stable trees.

Bart.

Re: [PATCH 1/6] blk-mq: Make blk_mq_quiesce_queue() wait for all .queue_rq() calls

[Ming Lei]

On Fri, Apr 28, 2017 at 03:35:59PM +0000, Bart Van Assche wrote:
> On Fri, 2017-04-28 at 10:00 +0800, Ming Lei wrote:
> > On Thu, Apr 27, 2017 at 08:54:32AM -0700, Bart Van Assche wrote:
> > >  void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx)
> > >  {
> > > -	cancel_work(&hctx->run_work);
> > > -	cancel_delayed_work(&hctx->delay_work);
> > > +	cancel_work_sync(&hctx->run_work);
> > > +	cancel_delayed_work_sync(&hctx->delay_work);
> > 
> > Could you explain it a bit why we need the sync version?
> 
> Because the purpose of this patch is to make blk_mq_quiesce_queue() wait for
> all .queue_rq() calls.

If only .queue_rq() from this queue is waited, blk_mq_freeze_queue() is
enough. If you want to wait .queue_rq() from all queues,
blk_mq_quiesce_queue() can't do that too.

Firstly there shouldn't be a 'struct request_queue' parameter passed to this
function, because you want to wait for .queue_rq() from all queues.

Secondaly your current implementation can't guarantee that point:
	- only hw queues of this queue are stopped in this function, and
	.queue_rq() from other queues still can be run once
	synchronize_rcu() returns.

	- for BLOCKING case, the SRCU is per hctx.

Thirdly I am still not sure why we want to wait for all .queue_rq(),
could you explain it a bit?

At least for mq scheduler switch, looks all schedule data is per request
queue, and we shouldn't have needed to wait all .queue_rq().

> 
> > So I suggest to unity both .run_work and .dealyed_run_work
> > into one work, just as what Jens did in the following link:
> > 
> > 	http://marc.info/?t=149183989800010&r=1&w=2
> 
> That should be done after this patch is upstream otherwise this
> patch won't apply to the stable trees.

OK.

Thanks,
Ming

Re: [PATCH 5/6] blk-mq-debugfs: Show busy requests

[Omar Sandoval]

On Thu, Apr 27, 2017 at 08:54:36AM -0700, Bart Van Assche wrote:
> Requests that got stuck in a block driver are neither on
> blk_mq_ctx.rq_list nor on any hw dispatch queue. Make these
> visible in debugfs through the "busy" attribute.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 5092d90e37f6..a5e286e04569 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -428,6 +428,43 @@ static const struct file_operations hctx_dispatch_fops = {
>  	.release	= seq_release,
>  };
>  
> +struct show_busy_ctx {
> +	struct seq_file		*m;
> +	struct blk_mq_hw_ctx	*hctx;
> +};
> +
> +static void hctx_show_busy(struct request *rq, void *data, bool reserved)
> +{
> +	const struct show_busy_ctx *ctx = data;
> +	struct request_queue *q = rq->q;
> +
> +	if (q->queue_hw_ctx[rq->mq_ctx->index_hw] == ctx->hctx &&

This doesn't look right. ctx->index_hw is the index into hctx->ctxs for
this ctx. I think you mean blk_mq_map_queue(q, rq->mq_ctx->cpu).

> +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> +		blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
> +}
> +
> +static int hctx_busy_show(struct seq_file *m, void *v)
> +{
> +	struct blk_mq_hw_ctx *hctx = m->private;
> +	struct show_busy_ctx ctx = { .m = m, .hctx = hctx };
> +
> +	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &ctx);
> +
> +	return 0;
> +}
> +
> +static int hctx_busy_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, hctx_busy_show, inode->i_private);
> +}
> +
> +static const struct file_operations hctx_busy_fops = {
> +	.open		= hctx_busy_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};
> +
>  static int hctx_ctx_map_show(struct seq_file *m, void *v)
>  {
>  	struct blk_mq_hw_ctx *hctx = m->private;
> @@ -885,6 +922,7 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_hctx_attrs[] = {
>  	{"state", 0400, &hctx_state_fops},
>  	{"flags", 0400, &hctx_flags_fops},
>  	{"dispatch", 0400, &hctx_dispatch_fops},
> +	{"busy", 0400, &hctx_busy_fops},
>  	{"ctx_map", 0400, &hctx_ctx_map_fops},
>  	{"tags", 0400, &hctx_tags_fops},
>  	{"tags_bitmap", 0400, &hctx_tags_bitmap_fops},
> -- 
> 2.12.2
> 

Re: [PATCH 6/6] blk-mq-debugfs: Add 'kick' operation

[Omar Sandoval]

On Thu, Apr 27, 2017 at 08:54:37AM -0700, Bart Van Assche wrote:
> Running a queue causes the block layer to examine the per-CPU and
> hw queues but not the requeue list. Hence add a 'kick' operation
> that also examines the requeue list.

The naming of these operations isn't super intuitive, but it makes
enough sense if you know the code, I guess.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index a5e286e04569..aeca26c739d1 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -120,8 +120,10 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
>  		blk_mq_run_hw_queues(q, true);
>  	} else if (strcmp(op, "start") == 0) {
>  		blk_mq_start_stopped_hw_queues(q, true);
> +	} else if (strcmp(op, "kick") == 0) {
> +		blk_mq_kick_requeue_list(q);
>  	} else {
> -		pr_err("%s: unsupported operation %s. Use either 'run' or 'start'\n",
> +		pr_err("%s: unsupported operation %s. Use 'run', 'start' or 'kick'\n",
>  		       __func__, op);
>  		return -EINVAL;
>  	}
> -- 
> 2.12.2
> 

Re: [PATCH 6/6] blk-mq-debugfs: Add 'kick' operation

[Jens Axboe]

On 05/01/2017 06:19 PM, Omar Sandoval wrote:
> On Thu, Apr 27, 2017 at 08:54:37AM -0700, Bart Van Assche wrote:
>> Running a queue causes the block layer to examine the per-CPU and
>> hw queues but not the requeue list. Hence add a 'kick' operation
>> that also examines the requeue list.
> 
> The naming of these operations isn't super intuitive, but it makes
> enough sense if you know the code, I guess.

I don't worry about that too much, but I do think it's important
that we have some way of knowing WHAT commands are available
for a given kernel, without having to consult the source. It's
no big deal you're running the latest and greatest debug kernels,
but it's a bigger issue if you're debugging kernel x.y.z for
a customer and you have to consult the source to find them.

Can we include it in the show output?

-- 
Jens Axboe

Re: [PATCH 5/6] blk-mq-debugfs: Show busy requests

[Bart Van Assche]

On Mon, 2017-05-01 at 17:17 -0700, Omar Sandoval wrote:
> On Thu, Apr 27, 2017 at 08:54:36AM -0700, Bart Van Assche wrote:
> > +	if (q->queue_hw_ctx[rq->mq_ctx->index_hw] =3D=3D ctx->hctx &&
>=20
> This doesn't look right. ctx->index_hw is the index into hctx->ctxs for
> this ctx. I think you mean blk_mq_map_queue(q, rq->mq_ctx->cpu).

Hello Omar,

Thanks for the feedback. I will integrate this fix when I repost this patch=
 series
(after the merge window has closed).

Bart.=

Re: [PATCH 6/6] blk-mq-debugfs: Add 'kick' operation

[Bart Van Assche]

On Mon, 2017-05-01 at 18:24 -0600, Jens Axboe wrote:
> On 05/01/2017 06:19 PM, Omar Sandoval wrote:
> > On Thu, Apr 27, 2017 at 08:54:37AM -0700, Bart Van Assche wrote:
> > > Running a queue causes the block layer to examine the per-CPU and
> > > hw queues but not the requeue list. Hence add a 'kick' operation
> > > that also examines the requeue list.
> >=20
> > The naming of these operations isn't super intuitive, but it makes
> > enough sense if you know the code, I guess.
>=20
> I don't worry about that too much, but I do think it's important
> that we have some way of knowing WHAT commands are available
> for a given kernel, without having to consult the source. It's
> no big deal you're running the latest and greatest debug kernels,
> but it's a bigger issue if you're debugging kernel x.y.z for
> a customer and you have to consult the source to find them.
>=20
> Can we include it in the show output?

Hello Jens,

Sorry but I'm afraid that including the list of supported commands in the
'show' output would make that output harder to read. Figuring out what the
supported commands are is not that hard as the output below shows:

# dmesg -c >/dev/null; for d in /sys/kernel/debug/block/*/mq/state; do \
  if [ -e "$d" ]; then echo help >$d 2>/dev/null; break; fi; done; dmesg
blk_queue_flags_store: unsupported operation help. Use 'run', 'start' or 'k=
ick'

Bart.=

Re: [PATCH 6/6] blk-mq-debugfs: Add 'kick' operation

[Jens Axboe]

On 05/02/2017 10:23 AM, Bart Van Assche wrote:
> On Mon, 2017-05-01 at 18:24 -0600, Jens Axboe wrote:
>> On 05/01/2017 06:19 PM, Omar Sandoval wrote:
>>> On Thu, Apr 27, 2017 at 08:54:37AM -0700, Bart Van Assche wrote:
>>>> Running a queue causes the block layer to examine the per-CPU and
>>>> hw queues but not the requeue list. Hence add a 'kick' operation
>>>> that also examines the requeue list.
>>>
>>> The naming of these operations isn't super intuitive, but it makes
>>> enough sense if you know the code, I guess.
>>
>> I don't worry about that too much, but I do think it's important
>> that we have some way of knowing WHAT commands are available
>> for a given kernel, without having to consult the source. It's
>> no big deal you're running the latest and greatest debug kernels,
>> but it's a bigger issue if you're debugging kernel x.y.z for
>> a customer and you have to consult the source to find them.
>>
>> Can we include it in the show output?
> 
> Hello Jens,
> 
> Sorry but I'm afraid that including the list of supported commands in the
> 'show' output would make that output harder to read. Figuring out what the
> supported commands are is not that hard as the output below shows:
> 
> # dmesg -c >/dev/null; for d in /sys/kernel/debug/block/*/mq/state; do \
>   if [ -e "$d" ]; then echo help >$d 2>/dev/null; break; fi; done; dmesg
> blk_queue_flags_store: unsupported operation help. Use 'run', 'start' or 'kick'

Ah perfect, I missed that. That's perfectly fine, all I care about is
that there is some way to tell what the valid commands are, without
having to find the source.

-- 
Jens Axboe