[PATCH 0/5] Five blk-mq-debugfs patches

[PATCH 0/5] Five blk-mq-debugfs patches

[Bart Van Assche]

Hello Jens,

This series contains four patches that add new features to the blk-mq-debugfs
code and that are intended for kernel v4.13 and one bug fix that is intended
for kernel v4.12. The patches in this series are:

Bart Van Assche (5):
  blk-mq: Only register debugfs attributes for blk-mq queues
  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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++-
 block/blk-sysfs.c      |  6 ++---
 2 files changed, 73 insertions(+), 4 deletions(-)

-- 
2.12.2

[PATCH 1/5] blk-mq: Only register debugfs attributes for blk-mq queues

[Bart Van Assche]

The code in blk-mq-debugfs.c assumes that it is working on a blk-mq
queue and is not intended to work on a blk-sq queue. Hence only
register blk-mq debugfs attributes for blk-mq queues.

Fixes: commit 9c1051aacde8 ("blk-mq: untangle debugfs and sysfs")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Ming Lei <ming.lei@redhat.com>
---
 block/blk-sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 5e7b3924996c..0494baa743a3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -888,10 +888,10 @@ int blk_register_queue(struct gendisk *disk)
 		goto unlock;
 	}
 
-	if (q->mq_ops)
+	if (q->mq_ops) {
 		__blk_mq_register_dev(dev, q);
-
-	blk_mq_debugfs_register(q);
+		blk_mq_debugfs_register(q);
+	}
 
 	kobject_uevent(&q->kobj, KOBJ_ADD);
 
-- 
2.12.2

[PATCH 2/5] 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: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-debugfs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 803aed4d7221..d56ddd7a1285 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -267,6 +267,14 @@ static const char *const rqf_name[] = {
 };
 #undef RQF_NAME
 
+#define RQAF_NAME(name) [REQ_ATOM_##name] = #name
+static const char *const rqaf_name[] = {
+	RQAF_NAME(COMPLETE),
+	RQAF_NAME(STARTED),
+	RQAF_NAME(POLL_SLEPT),
+};
+#undef RQAF_NAME
+
 int __blk_mq_debugfs_rq_show(struct seq_file *m, struct request *rq)
 {
 	const struct blk_mq_ops *const mq_ops = rq->q->mq_ops;
@@ -283,6 +291,8 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, struct request *rq)
 	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 3/5] 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: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-debugfs.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index d56ddd7a1285..8b06a12c1461 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -308,6 +308,37 @@ int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
 }
 EXPORT_SYMBOL_GPL(blk_mq_debugfs_rq_show);
 
+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_irq(&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_irq(&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 void *hctx_dispatch_start(struct seq_file *m, loff_t *pos)
 	__acquires(&hctx->lock)
 {
@@ -665,6 +696,7 @@ const struct file_operations blk_mq_debugfs_fops = {
 
 static const struct blk_mq_debugfs_attr blk_mq_debugfs_queue_attrs[] = {
 	{"poll_stat", 0400, queue_poll_stat_show},
+	{"requeue_list", 0400, .seq_ops = &queue_requeue_list_seq_ops},
 	{"state", 0600, queue_state_show, queue_state_write},
 	{},
 };
-- 
2.12.2

[PATCH 4/5] 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: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 8b06a12c1461..70a2b955afee 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
 	.show	= blk_mq_debugfs_rq_show,
 };
 
+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;
+
+	if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == ctx->hctx &&
+	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
+		blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
+}
+
+static int hctx_busy_show(void *data, struct seq_file *m)
+{
+	struct blk_mq_hw_ctx *hctx = data;
+	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_ctx_map_show(void *data, struct seq_file *m)
 {
 	struct blk_mq_hw_ctx *hctx = data;
@@ -705,6 +729,7 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_hctx_attrs[] = {
 	{"state", 0400, hctx_state_show},
 	{"flags", 0400, hctx_flags_show},
 	{"dispatch", 0400, .seq_ops = &hctx_dispatch_seq_ops},
+	{"busy", 0400, hctx_busy_show},
 	{"ctx_map", 0400, hctx_ctx_map_show},
 	{"tags", 0400, hctx_tags_show},
 	{"tags_bitmap", 0400, hctx_tags_bitmap_show},
-- 
2.12.2

[PATCH 5/5] 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: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Ming Lei <ming.lei@redhat.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 70a2b955afee..89277a888800 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -114,10 +114,12 @@ static ssize_t queue_state_write(void *data, const char __user *buf,
 		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'\n", __func__, op);
 inval:
-		pr_err("%s: use either 'run' or 'start'\n", __func__);
+		pr_err("%s: use 'run', 'start' or 'kick'\n", __func__);
 		return -EINVAL;
 	}
 	return count;
-- 
2.12.2

Re: [PATCH 1/5] blk-mq: Only register debugfs attributes for blk-mq queues

[Omar Sandoval]

On Thu, May 25, 2017 at 04:38:06PM -0700, Bart Van Assche wrote:
> The code in blk-mq-debugfs.c assumes that it is working on a blk-mq
> queue and is not intended to work on a blk-sq queue. Hence only
> register blk-mq debugfs attributes for blk-mq queues.

Oops. This is probably what David reported here:
https://github.com/ddiss/rapido/pull/13#event-1093484761. Thanks, Bart.

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

> Fixes: commit 9c1051aacde8 ("blk-mq: untangle debugfs and sysfs")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-sysfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 5e7b3924996c..0494baa743a3 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -888,10 +888,10 @@ int blk_register_queue(struct gendisk *disk)
>  		goto unlock;
>  	}
>  
> -	if (q->mq_ops)
> +	if (q->mq_ops) {
>  		__blk_mq_register_dev(dev, q);
> -
> -	blk_mq_debugfs_register(q);
> +		blk_mq_debugfs_register(q);
> +	}
>  
>  	kobject_uevent(&q->kobj, KOBJ_ADD);
>  
> -- 
> 2.12.2
> 

Re: [PATCH 1/5] blk-mq: Only register debugfs attributes for blk-mq queues

[Hannes Reinecke]

On 05/26/2017 01:38 AM, Bart Van Assche wrote:
> The code in blk-mq-debugfs.c assumes that it is working on a blk-mq
> queue and is not intended to work on a blk-sq queue. Hence only
> register blk-mq debugfs attributes for blk-mq queues.
> 
> Fixes: commit 9c1051aacde8 ("blk-mq: untangle debugfs and sysfs")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-sysfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 5e7b3924996c..0494baa743a3 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -888,10 +888,10 @@ int blk_register_queue(struct gendisk *disk)
>  		goto unlock;
>  	}
>  
> -	if (q->mq_ops)
> +	if (q->mq_ops) {
>  		__blk_mq_register_dev(dev, q);
> -
> -	blk_mq_debugfs_register(q);
> +		blk_mq_debugfs_register(q);
> +	}
>  
>  	kobject_uevent(&q->kobj, KOBJ_ADD);
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/5] blk-mq-debugfs: Show atomic request flags

[Hannes Reinecke]

On 05/26/2017 01:38 AM, 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.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-debugfs.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/5] blk-mq-debugfs: Show requeue list

[Hannes Reinecke]

On 05/26/2017 01:38 AM, 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.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-debugfs.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[Jens Axboe]

On 05/25/2017 05:38 PM, 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: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 8b06a12c1461..70a2b955afee 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
>  	.show	= blk_mq_debugfs_rq_show,
>  };
>  
> +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;

Let's not call that variable 'ctx', in blk-mq that's pretty much
reserved for the sw queues.

-- 
Jens Axboe

Re: [PATCH 1/5] blk-mq: Only register debugfs attributes for blk-mq queues

[Jens Axboe]

On 05/25/2017 05:38 PM, Bart Van Assche wrote:
> The code in blk-mq-debugfs.c assumes that it is working on a blk-mq
> queue and is not intended to work on a blk-sq queue. Hence only
> register blk-mq debugfs attributes for blk-mq queues.

Thanks Bart, I have queued this up for this series.

-- 
Jens Axboe

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

[Hannes Reinecke]

On 05/26/2017 01:38 AM, 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: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 8b06a12c1461..70a2b955afee 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
>  	.show	= blk_mq_debugfs_rq_show,
>  };
>  
> +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;
> +
> +	if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == ctx->hctx &&
> +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> +		blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
> +}
> +
> +static int hctx_busy_show(void *data, struct seq_file *m)
> +{
> +	struct blk_mq_hw_ctx *hctx = data;
> +	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_ctx_map_show(void *data, struct seq_file *m)
>  {
>  	struct blk_mq_hw_ctx *hctx = data;
> @@ -705,6 +729,7 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_hctx_attrs[] = {
>  	{"state", 0400, hctx_state_show},
>  	{"flags", 0400, hctx_flags_show},
>  	{"dispatch", 0400, .seq_ops = &hctx_dispatch_seq_ops},
> +	{"busy", 0400, hctx_busy_show},
>  	{"ctx_map", 0400, hctx_ctx_map_show},
>  	{"tags", 0400, hctx_tags_show},
>  	{"tags_bitmap", 0400, hctx_tags_bitmap_show},
> 
Hmm. I wonder if this is going to work as intended; 'busy' might be
changing rapidly, so the busy_iter might be giving us unintended results.
Have you checked?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[Bart Van Assche]

On Fri, 2017-05-26 at 07:26 -0600, Jens Axboe wrote:
> On 05/25/2017 05:38 PM, 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.
> >=20
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: Omar Sandoval <osandov@fb.com>
> > Cc: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >=20
> > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> > index 8b06a12c1461..70a2b955afee 100644
> > --- a/block/blk-mq-debugfs.c
> > +++ b/block/blk-mq-debugfs.c
> > @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_s=
eq_ops =3D {
> >  	.show	=3D blk_mq_debugfs_rq_show,
> >  };
> > =20
> > +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 reserv=
ed)
> > +{
> > +	const struct show_busy_ctx *ctx =3D data;
>=20
> Let's not call that variable 'ctx', in blk-mq that's pretty much
> reserved for the sw queues.

Hello Jens,

How about renaming show_busy_ctx into show_busy_params and ctx into params?

Bart.=

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

[Jens Axboe]

On 05/26/2017 03:17 PM, Bart Van Assche wrote:
> On Fri, 2017-05-26 at 07:26 -0600, Jens Axboe wrote:
>> On 05/25/2017 05:38 PM, 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: Christoph Hellwig <hch@lst.de>
>>> Cc: Hannes Reinecke <hare@suse.com>
>>> Cc: Omar Sandoval <osandov@fb.com>
>>> Cc: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>
>>> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
>>> index 8b06a12c1461..70a2b955afee 100644
>>> --- a/block/blk-mq-debugfs.c
>>> +++ b/block/blk-mq-debugfs.c
>>> @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
>>>  	.show	= blk_mq_debugfs_rq_show,
>>>  };
>>>  
>>> +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;
>>
>> Let's not call that variable 'ctx', in blk-mq that's pretty much
>> reserved for the sw queues.
> 
> Hello Jens,
> 
> How about renaming show_busy_ctx into show_busy_params and ctx into params?

I think that would be an improvement. Also, this:

	blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);

doesn't look safe at all, as you are passing in a node instead of a
head. I think you just want to use __blk_mq_debugfs_rq_show() here.

-- 
Jens Axboe

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

[Jens Axboe]

On 05/26/2017 03:20 PM, Jens Axboe wrote:
> On 05/26/2017 03:17 PM, Bart Van Assche wrote:
>> On Fri, 2017-05-26 at 07:26 -0600, Jens Axboe wrote:
>>> On 05/25/2017 05:38 PM, 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: Christoph Hellwig <hch@lst.de>
>>>> Cc: Hannes Reinecke <hare@suse.com>
>>>> Cc: Omar Sandoval <osandov@fb.com>
>>>> Cc: Ming Lei <ming.lei@redhat.com>
>>>> ---
>>>>  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
>>>>  1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
>>>> index 8b06a12c1461..70a2b955afee 100644
>>>> --- a/block/blk-mq-debugfs.c
>>>> +++ b/block/blk-mq-debugfs.c
>>>> @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
>>>>  	.show	= blk_mq_debugfs_rq_show,
>>>>  };
>>>>  
>>>> +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;
>>>
>>> Let's not call that variable 'ctx', in blk-mq that's pretty much
>>> reserved for the sw queues.
>>
>> Hello Jens,
>>
>> How about renaming show_busy_ctx into show_busy_params and ctx into params?
> 
> I think that would be an improvement. Also, this:
> 
> 	blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
> 
> doesn't look safe at all, as you are passing in a node instead of a
> head. I think you just want to use __blk_mq_debugfs_rq_show() here.

I guess it's safe as we don't iterate the list, but it's pointless
though. Just use __blk_mq_debugfs_rq_show().

-- 
Jens Axboe

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

[Bart Van Assche]

On Fri, 2017-05-26 at 15:20 -0600, Jens Axboe wrote:
> On 05/26/2017 03:17 PM, Bart Van Assche wrote:
> > On Fri, 2017-05-26 at 07:26 -0600, Jens Axboe wrote:
> > > On 05/25/2017 05:38 PM, 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.
> > > >=20
> > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Hannes Reinecke <hare@suse.com>
> > > > Cc: Omar Sandoval <osandov@fb.com>
> > > > Cc: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
> > > >  1 file changed, 25 insertions(+)
> > > >=20
> > > > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> > > > index 8b06a12c1461..70a2b955afee 100644
> > > > --- a/block/blk-mq-debugfs.c
> > > > +++ b/block/blk-mq-debugfs.c
> > > > @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispat=
ch_seq_ops =3D {
> > > >  	.show	=3D blk_mq_debugfs_rq_show,
> > > >  };
> > > > =20
> > > > +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 re=
served)
> > > > +{
> > > > +	const struct show_busy_ctx *ctx =3D data;
> > >=20
> > > Let's not call that variable 'ctx', in blk-mq that's pretty much
> > > reserved for the sw queues.
> >=20
> > Hello Jens,
> >=20
> > How about renaming show_busy_ctx into show_busy_params and ctx into par=
ams?
>=20
> I think that would be an improvement. Also, this:
>=20
> 	blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
>=20
> doesn't look safe at all, as you are passing in a node instead of a
> head. I think you just want to use __blk_mq_debugfs_rq_show() here.

Hello Jens,

That change will make the compiler perform type checking. I agree that's
an improvement. I will make that change.

Bart.=

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

[Bart Van Assche]

On Fri, 2017-05-26 at 16:38 +0200, Hannes Reinecke wrote:
> Hmm. I wonder if this is going to work as intended; 'busy' might be
> changing rapidly, so the busy_iter might be giving us unintended results.
> Have you checked?

Hello Hannes,

The intention is that this attribute is used by developers to figure out wh=
ich
requests got stuck if one or more requests got stuck. Querying this attribu=
te a
few times should help to see whether the requests shown are requests that
complete quickly or whether these are requests that got stuck. The output I
collected while testing this debugfs attribute looks fine to me:

# while true; do (cd /sys/kernel/debug/block && for f in */*/busy; do [ -e =
$f ] && grep -aH '' $f; done); done
[ ... ]
sde/hctx0/busy:ffff880267dcba00 {.op=3DWRITE, .cmd_flags=3DFAILFAST_TRANSPO=
RT|SYNC|META|PRIO|NOMERGE, .rq_flags=3DMQ_INFLIGHT|DONTPREP|IO_STAT|STATS, =
.atomic_flags=3DSTARTED, .tag=3D19, .internal_tag=3D-1,
.cmd=3DWrite(10) 2a 00 00 07 92 38 00 00 08 00}
sde/hctx0/busy:ffff880267dcd140 {.op=3DWRITE, .cmd_flags=3DFAILFAST_TRANSPO=
RT|SYNC|META|PRIO|NOMERGE, .rq_flags=3DMQ_INFLIGHT|DONTPREP|IO_STAT|STATS, =
.atomic_flags=3DSTARTED, .tag=3D20, .internal_tag=3D-1,
.cmd=3DWrite(10) 2a 00 00 07 92 80 00 00 08 00}
[ ... ]
sr0/hctx0/busy:ffff88046797d140 {.op=3DSCSI_IN, .cmd_flags=3D, .rq_flags=3D=
DONTPREP|PREEMPT|COPY_USER|QUIET|IO_STAT|STATS, .atomic_flags=3DSTARTED, .t=
ag=3D20, .internal_tag=3D-1, .cmd=3DGet event status
notification 4a 01 00 00 10 00 00 00 08 00}
[ ... ]

Bart.=

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

[Jens Axboe]

On 05/26/2017 08:38 AM, Hannes Reinecke wrote:
> On 05/26/2017 01:38 AM, 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: Christoph Hellwig <hch@lst.de>
>> Cc: Hannes Reinecke <hare@suse.com>
>> Cc: Omar Sandoval <osandov@fb.com>
>> Cc: Ming Lei <ming.lei@redhat.com>
>> ---
>>  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
>> index 8b06a12c1461..70a2b955afee 100644
>> --- a/block/blk-mq-debugfs.c
>> +++ b/block/blk-mq-debugfs.c
>> @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
>>  	.show	= blk_mq_debugfs_rq_show,
>>  };
>>  
>> +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;
>> +
>> +	if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == ctx->hctx &&
>> +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
>> +		blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
>> +}
>> +
>> +static int hctx_busy_show(void *data, struct seq_file *m)
>> +{
>> +	struct blk_mq_hw_ctx *hctx = data;
>> +	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_ctx_map_show(void *data, struct seq_file *m)
>>  {
>>  	struct blk_mq_hw_ctx *hctx = data;
>> @@ -705,6 +729,7 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_hctx_attrs[] = {
>>  	{"state", 0400, hctx_state_show},
>>  	{"flags", 0400, hctx_flags_show},
>>  	{"dispatch", 0400, .seq_ops = &hctx_dispatch_seq_ops},
>> +	{"busy", 0400, hctx_busy_show},
>>  	{"ctx_map", 0400, hctx_ctx_map_show},
>>  	{"tags", 0400, hctx_tags_show},
>>  	{"tags_bitmap", 0400, hctx_tags_bitmap_show},
>>
> Hmm. I wonder if this is going to work as intended; 'busy' might be
> changing rapidly, so the busy_iter might be giving us unintended results.
> Have you checked?

That's true for _any_ of the debugfs exports for probing into internals
that hold/store requests. All of them are just a snapshot in time,
there's no intent (or posibility) for these to be stable in any way.

-- 
Jens Axboe

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

[Ming Lei]

On Thu, May 25, 2017 at 04:38:09PM -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.

The name of 'busy' isn't very explicit about this case, and I
guess you mean the requests are dispatched to hardware, so
'dispatched' or sort of name may be more accurate.

> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 8b06a12c1461..70a2b955afee 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
>  	.show	= blk_mq_debugfs_rq_show,
>  };
>  
> +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;
> +
> +	if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == ctx->hctx &&
> +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))

During this small window, the request can be freed and reallocated
in another I/O path, then use-after-free is caused.

> +		blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
> +}
> +
> +static int hctx_busy_show(void *data, struct seq_file *m)
> +{
> +	struct blk_mq_hw_ctx *hctx = data;
> +	struct show_busy_ctx ctx = { .m = m, .hctx = hctx };
> +
> +	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &ctx);

This way is easy to cause use-after-free, so as a debug function,
you can't affect the normal function.

But the new fixed blk_mq_quiesce_queue() can be used before calling
blk_mq_tagset_busy_iter() to avoid the race.

http://marc.info/?l=linux-block&m=149578610419654&w=2


Thanks,
Ming

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

[Ming Lei]

On Sat, May 27, 2017 at 08:54:57AM +0800, Ming Lei wrote:
> On Thu, May 25, 2017 at 04:38:09PM -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.
> 
> The name of 'busy' isn't very explicit about this case, and I
> guess you mean the requests are dispatched to hardware, so
> 'dispatched' or sort of name may be more accurate.
> 
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: Omar Sandoval <osandov@fb.com>
> > Cc: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq-debugfs.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> > index 8b06a12c1461..70a2b955afee 100644
> > --- a/block/blk-mq-debugfs.c
> > +++ b/block/blk-mq-debugfs.c
> > @@ -370,6 +370,30 @@ static const struct seq_operations hctx_dispatch_seq_ops = {
> >  	.show	= blk_mq_debugfs_rq_show,
> >  };
> >  
> > +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;
> > +
> > +	if (blk_mq_map_queue(rq->q, rq->mq_ctx->cpu) == ctx->hctx &&
> > +	    test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> 
> During this small window, the request can be freed and reallocated
> in another I/O path, then use-after-free is caused.
> 
> > +		blk_mq_debugfs_rq_show(ctx->m, &rq->queuelist);
> > +}
> > +
> > +static int hctx_busy_show(void *data, struct seq_file *m)
> > +{
> > +	struct blk_mq_hw_ctx *hctx = data;
> > +	struct show_busy_ctx ctx = { .m = m, .hctx = hctx };
> > +
> > +	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy, &ctx);
> 
> This way is easy to cause use-after-free, so as a debug function,
> you can't affect the normal function.
> 
> But the new fixed blk_mq_quiesce_queue() can be used before calling
> blk_mq_tagset_busy_iter() to avoid the race.
> 
> http://marc.info/?l=linux-block&m=149578610419654&w=2

Actually blk_mq_quiesce_queue can make other cancel cases safe because
blk_mark_rq_complete() is used before canceling.

For this case, we can't use blk_mark_rq_complete(), so there can't
be a safe way to touch the request dispatched to hardware.

Given the dispatched request won't be touched by CPU,
and its state shouldn't be changed, I am just wondering what is
the real motivation for this debug interface, could Bart explain
a bit?

Thanks,
Ming

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

[Bart Van Assche]

On Sat, 2017-05-27 at 08:54 +0800, Ming Lei wrote:
> On Thu, May 25, 2017 at 04:38:09PM -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.
>=20
> The name of 'busy' isn't very explicit about this case, and I
> guess you mean the requests are dispatched to hardware, so
> 'dispatched' or sort of name may be more accurate.

Hello Ming,

There is already a debugfs attribute with the name "dispatch". I'm afraid
having one attribute with the name "dispatch" and another with the name
"dispatched" would be confusing.

> [ ... ]
> During this small window, the request can be freed and reallocated
> in another I/O path, then use-after-free is caused.

A similar remark applies to all request queue debugfs attributes: the queue
state can change immediately after having queried the state so that's not
unique to this attribute. Regarding the "use-after-free": the memory that i=
s
allocated for requests is only freed after the debugfs attributes have been
removed so the code that implements this attribute will read the contents o=
f
a struct request. It is up to the person who reads the contents of this
attribute to decide how to interpret the contents.

> But the new fixed blk_mq_quiesce_queue() can be used before calling
> blk_mq_tagset_busy_iter() to avoid the race.

That would be overkill. The "busy" attribute is intended as a debugging hel=
p.
The implementation of this function should trigger any crashes. But it was
not my intention to avoid data races when I implemented this function.

Bart.=

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

[Bart Van Assche]

On Sat, 2017-05-27 at 11:16 +0800, Ming Lei wrote:
> What is the real motivation for this debug interface, could Bart explain
> a bit?

As mentioned in the description of this patch, the purpose of this patch
is to allow anyone who is working on a block driver to figure out whether
or not a request got stuck in a block driver. This means that .queue_rq()
got called for a request but the block driver did not trigger a call to
.end_io().

Bart.=

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

[Jens Axboe]

On 05/30/2017 11:24 AM, Bart Van Assche wrote:
>> [ ... ]
>> During this small window, the request can be freed and reallocated
>> in another I/O path, then use-after-free is caused.
> 
> A similar remark applies to all request queue debugfs attributes: the
> queue state can change immediately after having queried the state so
> that's not unique to this attribute. Regarding the "use-after-free":
> the memory that is allocated for requests is only freed after the
> debugfs attributes have been removed so the code that implements this
> attribute will read the contents of a struct request. It is up to the
> person who reads the contents of this attribute to decide how to
> interpret the contents.

I think it's important to stress that the memory is not going away, so
it'll potentially just show a new state of the request. That's perfectly
fine, and will happen all the time for the various debugfs exports. The
useful aspect of them is when things have come to a halt, for whatever
reason. The states will tend to stay stable when that happens, and
provide a useful method of introspection to debug the issue.

The important part here is that the memory is perfectly valid, so we
won't run into issues with that.

-- 
Jens Axboe