[PATCH 0/6] blk-mq debugfs patches for kernel v4.12

[PATCH 0/6] blk-mq debugfs patches for kernel v4.12

[Bart Van Assche]

Hello Jens,

Please consider the six patches in this series for kernel v4.12. The
first patch in this series is a bug fix for code that has already
been queued for kernel v4.12. The second patch implements a change
requested by Omar. Patches 3-6 are blk-mq debugfs enhancements.

Thanks,

Bart.

Bart Van Assche (6):
  blk-mq: Do not invoke queue operations on a dead queue
  blk-mq: Move the "state" debugfs attribute one level down
  blk-mq: Make blk_flags_show() callers append a newline character
  blk-mq: Show operation, cmd_flags and rq_flags names
  blk-mq: Add blk_mq_ops.show_rq()
  scsi: Implement blk_mq_ops.show_rq()

 block/blk-mq-debugfs.c  | 99 +++++++++++++++++++++++++++++++++++++++++++------
 drivers/scsi/scsi_lib.c | 27 ++++++++++++++
 include/linux/blk-mq.h  |  6 +++
 3 files changed, 120 insertions(+), 12 deletions(-)

-- 
2.12.0

[PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue

[Bart Van Assche]

The blk-mq debugfs attributes are removed after blk_cleanup_queue()
has finished. Since running a queue after a queue has entered the
"dead" state is not allowed, disallow this. This patch avoids that
an attempt to run a dead queue triggers a kernel crash.

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 df9b688b877c..a1ce823578c7 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -111,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
 	struct request_queue *q = file_inode(file)->i_private;
 	char op[16] = { }, *s;
 
+	/*
+	 * The debugfs attributes are removed after blk_cleanup_queue() has
+	 * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set
+	 * to avoid triggering a use-after-free.
+	 */
+	if (blk_queue_dead(q))
+		return -ENOENT;
+
 	len = min(len, sizeof(op) - 1);
 	if (copy_from_user(op, ubuf, len))
 		return -EFAULT;
-- 
2.12.0

[PATCH 2/6] blk-mq: Move the "state" debugfs attribute one level down

[Bart Van Assche]

Move the "state" attribute from the top level to the "mq" directory
as requested by Omar.

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 | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index a1ce823578c7..564470d4af52 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -149,11 +149,6 @@ static const struct file_operations blk_queue_flags_fops = {
 	.write		= blk_queue_flags_store,
 };
 
-static const struct blk_mq_debugfs_attr blk_queue_attrs[] = {
-	{"state", 0600, &blk_queue_flags_fops},
-	{},
-};
-
 static void print_stat(struct seq_file *m, struct blk_rq_stat *stat)
 {
 	if (stat->nr_samples) {
@@ -762,6 +757,7 @@ static const struct file_operations ctx_completed_fops = {
 
 static const struct blk_mq_debugfs_attr blk_mq_debugfs_queue_attrs[] = {
 	{"poll_stat", 0400, &queue_poll_stat_fops},
+	{"state", 0600, &blk_queue_flags_fops},
 	{},
 };
 
@@ -877,9 +873,6 @@ int blk_mq_debugfs_register_hctxs(struct request_queue *q)
 	if (!q->debugfs_dir)
 		return -ENOENT;
 
-	if (!debugfs_create_files(q->debugfs_dir, q, blk_queue_attrs))
-		goto err;
-
 	q->mq_debugfs_dir = debugfs_create_dir("mq", q->debugfs_dir);
 	if (!q->mq_debugfs_dir)
 		goto err;
-- 
2.12.0

[PATCH 3/6] blk-mq: Make blk_flags_show() callers append a newline character

[Bart Van Assche]

This patch does not change any functionality but makes it possible
to produce a single line of output with multiple flag-to-name
translations.

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 564470d4af52..aae4b7c7b7b0 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -60,7 +60,6 @@ static int blk_flags_show(struct seq_file *m, const unsigned long flags,
 		else
 			seq_printf(m, "%d", i);
 	}
-	seq_puts(m, "\n");
 	return 0;
 }
 
@@ -102,6 +101,7 @@ static int blk_queue_flags_show(struct seq_file *m, void *v)
 
 	blk_flags_show(m, q->queue_flags, blk_queue_flag_name,
 		       ARRAY_SIZE(blk_queue_flag_name));
+	seq_puts(m, "\n");
 	return 0;
 }
 
@@ -198,6 +198,7 @@ static int hctx_state_show(struct seq_file *m, void *v)
 
 	blk_flags_show(m, hctx->state, hctx_state_name,
 		       ARRAY_SIZE(hctx_state_name));
+	seq_puts(m, "\n");
 	return 0;
 }
 
@@ -241,6 +242,7 @@ static int hctx_flags_show(struct seq_file *m, void *v)
 	blk_flags_show(m,
 		       hctx->flags ^ BLK_ALLOC_POLICY_TO_MQ_FLAG(alloc_policy),
 		       hctx_flag_name, ARRAY_SIZE(hctx_flag_name));
+	seq_puts(m, "\n");
 	return 0;
 }
 
-- 
2.12.0

[PATCH 4/6] blk-mq: Show operation, cmd_flags and rq_flags names

[Bart Van Assche]

Show the operation name, .cmd_flags and .rq_flags as names instead
of numbers.

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 | 72 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index aae4b7c7b7b0..161f30fc236f 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -258,13 +258,79 @@ static const struct file_operations hctx_flags_fops = {
 	.release	= single_release,
 };
 
+static const char *const op_name[] = {
+	[REQ_OP_READ]		= "READ",
+	[REQ_OP_WRITE]		= "WRITE",
+	[REQ_OP_FLUSH]		= "FLUSH",
+	[REQ_OP_DISCARD]	= "DISCARD",
+	[REQ_OP_ZONE_REPORT]	= "ZONE_REPORT",
+	[REQ_OP_SECURE_ERASE]	= "SECURE_ERASE",
+	[REQ_OP_ZONE_RESET]	= "ZONE_RESET",
+	[REQ_OP_WRITE_SAME]	= "WRITE_SAME",
+	[REQ_OP_WRITE_ZEROES]	= "WRITE_ZEROES",
+	[REQ_OP_SCSI_IN]	= "SCSI_IN",
+	[REQ_OP_SCSI_OUT]	= "SCSI_OUT",
+	[REQ_OP_DRV_IN]		= "DRV_IN",
+	[REQ_OP_DRV_OUT]	= "DRV_OUT",
+};
+
+static const char *const cmd_flag_name[] = {
+	[__REQ_FAILFAST_DEV]		= "FAILFAST_DEV",
+	[__REQ_FAILFAST_TRANSPORT]	= "FAILFAST_TRANSPORT",
+	[__REQ_FAILFAST_DRIVER]		= "FAILFAST_DRIVER",
+	[__REQ_SYNC]			= "SYNC",
+	[__REQ_META]			= "META",
+	[__REQ_PRIO]			= "PRIO",
+	[__REQ_NOMERGE]			= "NOMERGE",
+	[__REQ_IDLE]			= "IDLE",
+	[__REQ_INTEGRITY]		= "INTEGRITY",
+	[__REQ_FUA]			= "FUA",
+	[__REQ_PREFLUSH]		= "PREFLUSH",
+	[__REQ_RAHEAD]			= "RAHEAD",
+	[__REQ_BACKGROUND]		= "BACKGROUND",
+	[__REQ_NR_BITS]			= "NR_BITS",
+};
+
+static const char *const rqf_name[] = {
+	[ilog2(RQF_SORTED)]		= "SORTED",
+	[ilog2(RQF_STARTED)]		= "STARTED",
+	[ilog2(RQF_QUEUED)]		= "QUEUED",
+	[ilog2(RQF_SOFTBARRIER)]	= "SOFTBARRIER",
+	[ilog2(RQF_FLUSH_SEQ)]		= "FLUSH_SEQ",
+	[ilog2(RQF_MIXED_MERGE)]	= "MIXED_MERGE",
+	[ilog2(RQF_MQ_INFLIGHT)]	= "MQ_INFLIGHT",
+	[ilog2(RQF_DONTPREP)]		= "DONTPREP",
+	[ilog2(RQF_PREEMPT)]		= "PREEMPT",
+	[ilog2(RQF_COPY_USER)]		= "COPY_USER",
+	[ilog2(RQF_FAILED)]		= "FAILED",
+	[ilog2(RQF_QUIET)]		= "QUIET",
+	[ilog2(RQF_ELVPRIV)]		= "ELVPRIV",
+	[ilog2(RQF_IO_STAT)]		= "IO_STAT",
+	[ilog2(RQF_ALLOCED)]		= "ALLOCED",
+	[ilog2(RQF_PM)]			= "PM",
+	[ilog2(RQF_HASHED)]		= "HASHED",
+	[ilog2(RQF_STATS)]		= "STATS",
+	[ilog2(RQF_SPECIAL_PAYLOAD)]	= "SPECIAL_PAYLOAD",
+};
+
 static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
 {
 	struct request *rq = list_entry_rq(v);
+	const unsigned int op = rq->cmd_flags & REQ_OP_MASK;
 
-	seq_printf(m, "%p {.cmd_flags=0x%x, .rq_flags=0x%x, .tag=%d, .internal_tag=%d}\n",
-		   rq, rq->cmd_flags, (__force unsigned int)rq->rq_flags,
-		   rq->tag, rq->internal_tag);
+	seq_printf(m, "%p {.op=", rq);
+	if (op < ARRAY_SIZE(op_name) && op_name[op])
+		seq_printf(m, "%s", op_name[op]);
+	else
+		seq_printf(m, "%d", op);
+	seq_puts(m, ", .cmd_flags=");
+	blk_flags_show(m, rq->cmd_flags ^ op, cmd_flag_name,
+		       ARRAY_SIZE(cmd_flag_name));
+	seq_puts(m, ", .rq_flags=");
+	blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
+		       ARRAY_SIZE(rqf_name));
+	seq_printf(m, ", .tag=%d, .internal_tag=%d}\n", rq->tag,
+		   rq->internal_tag);
 	return 0;
 }
 
-- 
2.12.0

[PATCH 5/6] blk-mq: Add blk_mq_ops.show_rq()

[Bart Van Assche]

This new callback function will be used in the next patch to show
more information about SCSI requests.

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 | 10 ++++++++--
 include/linux/blk-mq.h |  6 ++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 161f30fc236f..6b28d92d4c0e 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -316,7 +316,9 @@ static const char *const rqf_name[] = {
 static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
 {
 	struct request *rq = list_entry_rq(v);
+	const struct blk_mq_ops *const mq_ops = rq->q->mq_ops;
 	const unsigned int op = rq->cmd_flags & REQ_OP_MASK;
+	char drv_info[200];
 
 	seq_printf(m, "%p {.op=", rq);
 	if (op < ARRAY_SIZE(op_name) && op_name[op])
@@ -329,8 +331,12 @@ 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_printf(m, ", .tag=%d, .internal_tag=%d}\n", rq->tag,
-		   rq->internal_tag);
+	if (mq_ops->show_rq)
+		mq_ops->show_rq(rq, drv_info, sizeof(drv_info));
+	else
+		drv_info[0] = '\0';
+	seq_printf(m, ", .tag=%d, .internal_tag=%d%s%s}\n", rq->tag,
+		   rq->internal_tag, drv_info[0] ? ", " : "", drv_info);
 	return 0;
 }
 
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index dea255dee359..de2a1c2ddd84 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -120,6 +120,12 @@ struct blk_mq_ops {
 	softirq_done_fn		*complete;
 
 	/*
+	 * Used by the debugfs implementation to show driver-specific
+	 * information about a request.
+	 */
+	void (*show_rq)(struct request *rq, char *info, unsigned int info_sz);
+
+	/*
 	 * Called when the block layer side of a hardware queue has been
 	 * set up, allowing the driver to allocate/init matching structures.
 	 * Ditto for exit/teardown.
-- 
2.12.0

[PATCH 6/6] scsi: Implement blk_mq_ops.show_rq()

[Bart Van Assche]

Show the SCSI CDB, .eh_eflags and .result for pending SCSI commands
in /sys/kernel/debug/block/*/mq/*/dispatch and */rq_list.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: <linux-scsi@vger.kernel.org>
---
 drivers/scsi/scsi_lib.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bc4513bf4e4..7d3efb8924ee 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2126,6 +2126,32 @@ static void scsi_exit_rq(struct request_queue *q, struct request *rq)
 	scsi_free_sense_buffer(shost, cmd->sense_buffer);
 }
 
+static const char *const ehflag_name[] = {
+	[ilog2(SCSI_EH_CANCEL_CMD)]	 = "CANCEL_CMD",
+	[ilog2(SCSI_EH_ABORT_SCHEDULED)] = "ABORT_SCHEDULED",
+};
+
+static void scsi_show_rq(struct request *rq, char *info, unsigned int info_sz)
+{
+	char *p = info, *const end = info + info_sz;
+	struct scsi_cmnd *cmd = container_of(scsi_req(rq), typeof(*cmd), req);
+	unsigned int i;
+
+	p += scnprintf(p, end - p, ".cmd =");
+	for (i = 0; i < cmd->cmd_len; i++)
+		p += scnprintf(p, end - p, " %02x", cmd->cmnd[i]);
+	p += scnprintf(p, end - p, ", .eh_eflags =");
+	for (i = 0; i < sizeof(cmd->eh_eflags) * BITS_PER_BYTE; i++) {
+		if (!(cmd->eh_eflags & BIT(i)))
+			continue;
+		if (i < ARRAY_SIZE(ehflag_name) && ehflag_name[i])
+			p += scnprintf(p, end - p, " %s", ehflag_name[i]);
+		else
+			p += scnprintf(p, end - p, " %d", i);
+	}
+	p += scnprintf(p, end - p, ", .result = %#06x", cmd->result);
+}
+
 struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 {
 	struct Scsi_Host *shost = sdev->host;
@@ -2158,6 +2184,7 @@ static const struct blk_mq_ops scsi_mq_ops = {
 	.queue_rq	= scsi_queue_rq,
 	.complete	= scsi_softirq_done,
 	.timeout	= scsi_timeout,
+	.show_rq	= scsi_show_rq,
 	.init_request	= scsi_init_request,
 	.exit_request	= scsi_exit_request,
 	.map_queues	= scsi_map_queues,
-- 
2.12.0

[PATCH 6/6] scsi: Implement blk_mq_ops.show_rq()

[Bart Van Assche]

Show the SCSI CDB, .eh_eflags and .result for pending SCSI commands
in /sys/kernel/debug/block/*/mq/*/dispatch and */rq_list.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: <linux-scsi@vger.kernel.org>
---
 drivers/scsi/scsi_lib.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bc4513bf4e4..7d3efb8924ee 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2126,6 +2126,32 @@ static void scsi_exit_rq(struct request_queue *q, struct request *rq)
 	scsi_free_sense_buffer(shost, cmd->sense_buffer);
 }
 
+static const char *const ehflag_name[] = {
+	[ilog2(SCSI_EH_CANCEL_CMD)]	 = "CANCEL_CMD",
+	[ilog2(SCSI_EH_ABORT_SCHEDULED)] = "ABORT_SCHEDULED",
+};
+
+static void scsi_show_rq(struct request *rq, char *info, unsigned int info_sz)
+{
+	char *p = info, *const end = info + info_sz;
+	struct scsi_cmnd *cmd = container_of(scsi_req(rq), typeof(*cmd), req);
+	unsigned int i;
+
+	p += scnprintf(p, end - p, ".cmd =");
+	for (i = 0; i < cmd->cmd_len; i++)
+		p += scnprintf(p, end - p, " %02x", cmd->cmnd[i]);
+	p += scnprintf(p, end - p, ", .eh_eflags =");
+	for (i = 0; i < sizeof(cmd->eh_eflags) * BITS_PER_BYTE; i++) {
+		if (!(cmd->eh_eflags & BIT(i)))
+			continue;
+		if (i < ARRAY_SIZE(ehflag_name) && ehflag_name[i])
+			p += scnprintf(p, end - p, " %s", ehflag_name[i]);
+		else
+			p += scnprintf(p, end - p, " %d", i);
+	}
+	p += scnprintf(p, end - p, ", .result = %#06x", cmd->result);
+}
+
 struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 {
 	struct Scsi_Host *shost = sdev->host;
@@ -2158,6 +2184,7 @@ static const struct blk_mq_ops scsi_mq_ops = {
 	.queue_rq	= scsi_queue_rq,
 	.complete	= scsi_softirq_done,
 	.timeout	= scsi_timeout,
+	.show_rq	= scsi_show_rq,
 	.init_request	= scsi_init_request,
 	.exit_request	= scsi_exit_request,
 	.map_queues	= scsi_map_queues,
-- 
2.12.0

Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue

[Omar Sandoval]

On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote:
> The blk-mq debugfs attributes are removed after blk_cleanup_queue()
> has finished. Since running a queue after a queue has entered the
> "dead" state is not allowed, disallow this. This patch avoids that
> an attempt to run a dead queue triggers a kernel crash.
> 
> 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 df9b688b877c..a1ce823578c7 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -111,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
>  	struct request_queue *q = file_inode(file)->i_private;
>  	char op[16] = { }, *s;
>  
> +	/*
> +	 * The debugfs attributes are removed after blk_cleanup_queue() has
> +	 * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set
> +	 * to avoid triggering a use-after-free.
> +	 */
> +	if (blk_queue_dead(q))
> +		return -ENOENT;
> +
>  	len = min(len, sizeof(op) - 1);
>  	if (copy_from_user(op, ubuf, len))
>  		return -EFAULT;
> -- 
> 2.12.0
> 

Hi, Bart,

Looking at this, I think we have similar issues with most of the other
debugfs files. Should we move the debugfs cleanup earlier?

Re: [PATCH 2/6] blk-mq: Move the "state" debugfs attribute one level down

[Omar Sandoval]

On Tue, Apr 11, 2017 at 01:58:38PM -0700, Bart Van Assche wrote:
> Move the "state" attribute from the top level to the "mq" directory
> as requested by Omar.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>

Thanks!

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

> ---
>  block/blk-mq-debugfs.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index a1ce823578c7..564470d4af52 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -149,11 +149,6 @@ static const struct file_operations blk_queue_flags_fops = {
>  	.write		= blk_queue_flags_store,
>  };
>  
> -static const struct blk_mq_debugfs_attr blk_queue_attrs[] = {
> -	{"state", 0600, &blk_queue_flags_fops},
> -	{},
> -};
> -
>  static void print_stat(struct seq_file *m, struct blk_rq_stat *stat)
>  {
>  	if (stat->nr_samples) {
> @@ -762,6 +757,7 @@ static const struct file_operations ctx_completed_fops = {
>  
>  static const struct blk_mq_debugfs_attr blk_mq_debugfs_queue_attrs[] = {
>  	{"poll_stat", 0400, &queue_poll_stat_fops},
> +	{"state", 0600, &blk_queue_flags_fops},
>  	{},
>  };
>  
> @@ -877,9 +873,6 @@ int blk_mq_debugfs_register_hctxs(struct request_queue *q)
>  	if (!q->debugfs_dir)
>  		return -ENOENT;
>  
> -	if (!debugfs_create_files(q->debugfs_dir, q, blk_queue_attrs))
> -		goto err;
> -
>  	q->mq_debugfs_dir = debugfs_create_dir("mq", q->debugfs_dir);
>  	if (!q->mq_debugfs_dir)
>  		goto err;
> -- 
> 2.12.0
> 

Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue

[Omar Sandoval]

On Thu, Apr 13, 2017 at 04:01:02PM -0700, Omar Sandoval wrote:
> On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote:
> > The blk-mq debugfs attributes are removed after blk_cleanup_queue()
> > has finished. Since running a queue after a queue has entered the
> > "dead" state is not allowed, disallow this. This patch avoids that
> > an attempt to run a dead queue triggers a kernel crash.
> > 
> > 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 df9b688b877c..a1ce823578c7 100644
> > --- a/block/blk-mq-debugfs.c
> > +++ b/block/blk-mq-debugfs.c
> > @@ -111,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
> >  	struct request_queue *q = file_inode(file)->i_private;
> >  	char op[16] = { }, *s;
> >  
> > +	/*
> > +	 * The debugfs attributes are removed after blk_cleanup_queue() has
> > +	 * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set
> > +	 * to avoid triggering a use-after-free.
> > +	 */
> > +	if (blk_queue_dead(q))
> > +		return -ENOENT;
> > +
> >  	len = min(len, sizeof(op) - 1);
> >  	if (copy_from_user(op, ubuf, len))
> >  		return -EFAULT;
> > -- 
> > 2.12.0
> > 
> 
> Hi, Bart,
> 
> Looking at this, I think we have similar issues with most of the other
> debugfs files. Should we move the debugfs cleanup earlier?

In particular, I think we can call blk_mq_debugfs_unregister_hctxs()
(which is somewhat poorly named, as it removes the whole mq directory)
before we call blk_mq_free_queue(). I was under the impression that
that's what it already did, but I think I was wrong.

Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue

[Bart Van Assche]

On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote:
> On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote:
> > The blk-mq debugfs attributes are removed after blk_cleanup_queue()
> > has finished. Since running a queue after a queue has entered the
> > "dead" state is not allowed, disallow this. This patch avoids that
> > an attempt to run a dead queue triggers a kernel crash.
> >=20
> > 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(+)
> >=20
> > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> > index df9b688b877c..a1ce823578c7 100644
> > --- a/block/blk-mq-debugfs.c
> > +++ b/block/blk-mq-debugfs.c
> > @@ -111,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *=
file, const char __user *ubuf,
> >  	struct request_queue *q =3D file_inode(file)->i_private;
> >  	char op[16] =3D { }, *s;
> > =20
> > +	/*
> > +	 * The debugfs attributes are removed after blk_cleanup_queue() has
> > +	 * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set
> > +	 * to avoid triggering a use-after-free.
> > +	 */
> > +	if (blk_queue_dead(q))
> > +		return -ENOENT;
> > +
> >  	len =3D min(len, sizeof(op) - 1);
> >  	if (copy_from_user(op, ubuf, len))
> >  		return -EFAULT;
>=20
> Looking at this, I think we have similar issues with most of the other
> debugfs files. Should we move the debugfs cleanup earlier?

Hello Omar,

That's a good question. However, while I was debugging it was very convenie=
nt
to be able to access the queue state after it had reached the "dead" state.
Performing the cleanup earlier would be an alternative solution but would
make debugging a bit harder ...

Bart.=

Re: [PATCH 3/6] blk-mq: Make blk_flags_show() callers append a newline character

[Omar Sandoval]

On Tue, Apr 11, 2017 at 01:58:39PM -0700, Bart Van Assche wrote:
> This patch does not change any functionality but makes it possible
> to produce a single line of output with multiple flag-to-name
> translations.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>

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

Re: [PATCH 4/6] blk-mq: Show operation, cmd_flags and rq_flags names

[Omar Sandoval]

On Tue, Apr 11, 2017 at 01:58:40PM -0700, Bart Van Assche wrote:
> Show the operation name, .cmd_flags and .rq_flags as names instead
> of numbers.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>

I like this :) one minor nit below.

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

> ---
>  block/blk-mq-debugfs.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index aae4b7c7b7b0..161f30fc236f 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
[snip]
>  static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
>  {
>  	struct request *rq = list_entry_rq(v);
> +	const unsigned int op = rq->cmd_flags & REQ_OP_MASK;
>  
> -	seq_printf(m, "%p {.cmd_flags=0x%x, .rq_flags=0x%x, .tag=%d, .internal_tag=%d}\n",
> -		   rq, rq->cmd_flags, (__force unsigned int)rq->rq_flags,
> -		   rq->tag, rq->internal_tag);
> +	seq_printf(m, "%p {.op=", rq);
> +	if (op < ARRAY_SIZE(op_name) && op_name[op])
> +		seq_printf(m, "%s", op_name[op]);
> +	else
> +		seq_printf(m, "%d", op);
> +	seq_puts(m, ", .cmd_flags=");
> +	blk_flags_show(m, rq->cmd_flags ^ op, cmd_flag_name,
                          ^^^^^^^^^^^^^^^^^^
I think rq->cmd_flags & ~REQ_OP_MASK is slightly clearer here, but I
don't feel that strongly about it, it's up to you.

> +		       ARRAY_SIZE(cmd_flag_name));
> +	seq_puts(m, ", .rq_flags=");
> +	blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
> +		       ARRAY_SIZE(rqf_name));
> +	seq_printf(m, ", .tag=%d, .internal_tag=%d}\n", rq->tag,
> +		   rq->internal_tag);
>  	return 0;
>  }
>  
> -- 
> 2.12.0
> 

Re: [PATCH 5/6] blk-mq: Add blk_mq_ops.show_rq()

[Omar Sandoval]

On Tue, Apr 11, 2017 at 01:58:41PM -0700, Bart Van Assche wrote:
> This new callback function will be used in the next patch to show
> more information about SCSI requests.
> 
> 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 | 10 ++++++++--
>  include/linux/blk-mq.h |  6 ++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 161f30fc236f..6b28d92d4c0e 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -316,7 +316,9 @@ static const char *const rqf_name[] = {
>  static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
>  {
>  	struct request *rq = list_entry_rq(v);
> +	const struct blk_mq_ops *const mq_ops = rq->q->mq_ops;
>  	const unsigned int op = rq->cmd_flags & REQ_OP_MASK;
> +	char drv_info[200];
>  
>  	seq_printf(m, "%p {.op=", rq);
>  	if (op < ARRAY_SIZE(op_name) && op_name[op])
> @@ -329,8 +331,12 @@ 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_printf(m, ", .tag=%d, .internal_tag=%d}\n", rq->tag,
> -		   rq->internal_tag);
> +	if (mq_ops->show_rq)
> +		mq_ops->show_rq(rq, drv_info, sizeof(drv_info));

How about passing the seq_file to the callback instead of this
arbitrarily-sized on-stack buffer?

Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue

[Omar Sandoval]

On Thu, Apr 13, 2017 at 11:05:32PM +0000, Bart Van Assche wrote:
> On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote:
> > On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote:
> > > The blk-mq debugfs attributes are removed after blk_cleanup_queue()
> > > has finished. Since running a queue after a queue has entered the
> > > "dead" state is not allowed, disallow this. This patch avoids that
> > > an attempt to run a dead queue triggers a kernel crash.
> > > 
> > > 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 df9b688b877c..a1ce823578c7 100644
> > > --- a/block/blk-mq-debugfs.c
> > > +++ b/block/blk-mq-debugfs.c
> > > @@ -111,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
> > >  	struct request_queue *q = file_inode(file)->i_private;
> > >  	char op[16] = { }, *s;
> > >  
> > > +	/*
> > > +	 * The debugfs attributes are removed after blk_cleanup_queue() has
> > > +	 * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set
> > > +	 * to avoid triggering a use-after-free.
> > > +	 */
> > > +	if (blk_queue_dead(q))
> > > +		return -ENOENT;
> > > +
> > >  	len = min(len, sizeof(op) - 1);
> > >  	if (copy_from_user(op, ubuf, len))
> > >  		return -EFAULT;
> > 
> > Looking at this, I think we have similar issues with most of the other
> > debugfs files. Should we move the debugfs cleanup earlier?
> 
> Hello Omar,
> 
> That's a good question. However, while I was debugging it was very convenient
> to be able to access the queue state after it had reached the "dead" state.
> Performing the cleanup earlier would be an alternative solution but would
> make debugging a bit harder ...
> 
> Bart.

What useful information were you getting out of debugfs once the queue
was already dead? Wasn't the interesting stuff freed at that point?

Re: [PATCH 5/6] blk-mq: Add blk_mq_ops.show_rq()

[Bart Van Assche]

On Thu, 2017-04-13 at 16:21 -0700, Omar Sandoval wrote:
> How about passing the seq_file to the callback instead of this
> arbitrarily-sized on-stack buffer?

Hello Omar,

That sounds like a good idea to me. I will make that change.

Bart.=

Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue

[Bart Van Assche]

On Fri, 2017-04-14 at 00:40 -0700, Omar Sandoval wrote:
> On Thu, Apr 13, 2017 at 11:05:32PM +0000, Bart Van Assche wrote:
> > On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote:
> > > Looking at this, I think we have similar issues with most of the othe=
r
> > > debugfs files. Should we move the debugfs cleanup earlier?
> >=20
> > That's a good question. However, while I was debugging it was very conv=
enient
> > to be able to access the queue state after it had reached the "dead" st=
ate.
> > Performing the cleanup earlier would be an alternative solution but wou=
ld
> > make debugging a bit harder ...
>=20
> What useful information were you getting out of debugfs once the queue
> was already dead? Wasn't the interesting stuff freed at that point?

Hello Omar,

I'm currently chasing a stall of dm-rq + dm-mpath that occurs after the
queues below it have reached the "dead" state. I will look for another
way to obtain the information I need such that we can remove the block
layer queue debugfs information before these queues reach the "dead"
state.

Bart.=

Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue

[Omar Sandoval]

On Fri, Apr 14, 2017 at 04:12:01PM +0000, Bart Van Assche wrote:
> On Fri, 2017-04-14 at 00:40 -0700, Omar Sandoval wrote:
> > On Thu, Apr 13, 2017 at 11:05:32PM +0000, Bart Van Assche wrote:
> > > On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote:
> > > > Looking at this, I think we have similar issues with most of the other
> > > > debugfs files. Should we move the debugfs cleanup earlier?
> > > 
> > > That's a good question. However, while I was debugging it was very convenient
> > > to be able to access the queue state after it had reached the "dead" state.
> > > Performing the cleanup earlier would be an alternative solution but would
> > > make debugging a bit harder ...
> > 
> > What useful information were you getting out of debugfs once the queue
> > was already dead? Wasn't the interesting stuff freed at that point?
> 
> Hello Omar,
> 
> I'm currently chasing a stall of dm-rq + dm-mpath that occurs after the
> queues below it have reached the "dead" state. I will look for another
> way to obtain the information I need such that we can remove the block
> layer queue debugfs information before these queues reach the "dead"
> state.
> 
> Bart.

Thanks, Bart. In this case, the absence of the "mq" directory should
tell you that the queue is dead. Will you move the cleanup in v2 or
should I submit a separate patch?

Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue

[Bart Van Assche]

On Fri, 2017-04-14 at 10:13 -0700, Omar Sandoval wrote:
> Thanks, Bart. In this case, the absence of the "mq" directory should
> tell you that the queue is dead. Will you move the cleanup in v2 or
> should I submit a separate patch?

Hello Omar,

I will include a patch in v2 of this patch series that unregisters the
debugfs attributes earlier.

Bart.=