[PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12

[PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12

[Bart Van Assche]

Hello Jens,

Please consider the ten patches in this series for kernel v4.12.
These patches improve blk-mq debugfs support.

Thanks,

Bart.

Changes compared to v4:
- Modified patch 4 such that debugfs registration failures no longer cause
  block device registration to fail.
- Modified patch 8 such that it no longer introduces new sparse warnings.
- Modified patch 9 such that .show_rq() is only defined if CONFIG_BLK_DEBUG_FS
  is enabled.
- Moved the definition of scsi_show_rq() to a new file in patch 10 and changed
  the implementation of scsi_show_rq() such that it now uses
  __scsi_format_command().

Changes compared to v3:
- Changed the mutex_lock_interruptible() calls back to mutex_lock()
  calls.
- Added a patch that renames the functions for registering and
  unregistering the "mq" directory in debugfs.
- Moved the changes that add checking of the blk_mq_debugfs_register()
  return value into a separate patch.
- Moved the unregistration of the "mq" directory into blk_cleanup_queue().
- Removed uninteresting information from scsi_show_rq().
- Added Reviewed-by tag to the patches that got a positive review.

Changes compared to v2:
- Changed the mutex_lock() calls in registration methods into
  mutex_lock_interruptible() since these functions can be called from
  the context of a user space process.
- Avoid that the blk_mq_register_dev() changes in patch 1/8 cause a
  deadlock.

Changes compared to v1:
- Added two patches and replaced patch 1/6 such that debugfs
  attributes are now unregistered before freeing of a blk-mq queue
  starts instead of checking the "dead" queue flag.
- Changed "rq->cmd_flags ^ op" into "rq->cmd_flags & ~REQ_OP_MASK" as
  proposed by Omar.
- A seq_file pointer is now passed to the new queue_rq callback function
  instead of a fixed-size char buffer.
Bart Van Assche (10):
  blk-mq: Register <dev>/queue/mq after having registered <dev>/queue
  blk-mq: Let blk_mq_debugfs_register() look up the queue name
  blk-mq-debugfs: Rename functions for registering and unregistering the
    mq directory
  blk-mq: Only unregister hctxs for which registration succeeded
  blk-mq: Unregister debugfs attributes earlier
  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-core.c            |   5 +++
 block/blk-mq-debugfs.c      | 102 ++++++++++++++++++++++++++++++++++++--------
 block/blk-mq-sysfs.c        |  61 +++++++++++++++++++-------
 block/blk-mq.h              |  14 +++---
 block/blk-sysfs.c           |   6 +--
 drivers/scsi/Makefile       |   1 +
 drivers/scsi/scsi_debugfs.c |  13 ++++++
 drivers/scsi/scsi_debugfs.h |   4 ++
 drivers/scsi/scsi_lib.c     |   4 ++
 include/linux/blk-mq.h      |   8 ++++
 10 files changed, 174 insertions(+), 44 deletions(-)
 create mode 100644 drivers/scsi/scsi_debugfs.c
 create mode 100644 drivers/scsi/scsi_debugfs.h

-- 
2.12.2

[PATCH v5 01/10] blk-mq: Register <dev>/queue/mq after having registered <dev>/queue

[Bart Van Assche]

A later patch in this series will modify blk_mq_debugfs_register()
such that it uses q->kobj.parent to determine the name of a
request queue. Hence make sure that that pointer is initialized
before blk_mq_debugfs_register() is called. To avoid lock inversion,
protect sysfs / debugfs registration with the queue sysfs_lock
instead of the global mutex all_q_mutex.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Omar Sandoval <osandov@fb.com>
---
 block/blk-mq-sysfs.c | 35 ++++++++++++++++++++++++++++-------
 block/blk-mq.h       |  1 +
 block/blk-sysfs.c    |  6 +++---
 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index d745ab81033a..a2dbb1a48e72 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -253,6 +253,8 @@ static void __blk_mq_unregister_dev(struct device *dev, struct request_queue *q)
 	struct blk_mq_hw_ctx *hctx;
 	int i;
 
+	lockdep_assert_held(&q->sysfs_lock);
+
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_unregister_hctx(hctx);
 
@@ -267,9 +269,9 @@ static void __blk_mq_unregister_dev(struct device *dev, struct request_queue *q)
 
 void blk_mq_unregister_dev(struct device *dev, struct request_queue *q)
 {
-	blk_mq_disable_hotplug();
+	mutex_lock(&q->sysfs_lock);
 	__blk_mq_unregister_dev(dev, q);
-	blk_mq_enable_hotplug();
+	mutex_unlock(&q->sysfs_lock);
 }
 
 void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx)
@@ -302,12 +304,13 @@ void blk_mq_sysfs_init(struct request_queue *q)
 	}
 }
 
-int blk_mq_register_dev(struct device *dev, struct request_queue *q)
+int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
 	int ret, i;
 
-	blk_mq_disable_hotplug();
+	WARN_ON_ONCE(!q->kobj.parent);
+	lockdep_assert_held(&q->sysfs_lock);
 
 	ret = kobject_add(&q->mq_kobj, kobject_get(&dev->kobj), "%s", "mq");
 	if (ret < 0)
@@ -327,8 +330,18 @@ int blk_mq_register_dev(struct device *dev, struct request_queue *q)
 		__blk_mq_unregister_dev(dev, q);
 	else
 		q->mq_sysfs_init_done = true;
+
 out:
-	blk_mq_enable_hotplug();
+	return ret;
+}
+
+int blk_mq_register_dev(struct device *dev, struct request_queue *q)
+{
+	int ret;
+
+	mutex_lock(&q->sysfs_lock);
+	ret = __blk_mq_register_dev(dev, q);
+	mutex_unlock(&q->sysfs_lock);
 
 	return ret;
 }
@@ -339,13 +352,17 @@ void blk_mq_sysfs_unregister(struct request_queue *q)
 	struct blk_mq_hw_ctx *hctx;
 	int i;
 
+	mutex_lock(&q->sysfs_lock);
 	if (!q->mq_sysfs_init_done)
-		return;
+		goto unlock;
 
 	blk_mq_debugfs_unregister_hctxs(q);
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_unregister_hctx(hctx);
+
+unlock:
+	mutex_unlock(&q->sysfs_lock);
 }
 
 int blk_mq_sysfs_register(struct request_queue *q)
@@ -353,8 +370,9 @@ int blk_mq_sysfs_register(struct request_queue *q)
 	struct blk_mq_hw_ctx *hctx;
 	int i, ret = 0;
 
+	mutex_lock(&q->sysfs_lock);
 	if (!q->mq_sysfs_init_done)
-		return ret;
+		goto unlock;
 
 	blk_mq_debugfs_register_hctxs(q);
 
@@ -364,5 +382,8 @@ int blk_mq_sysfs_register(struct request_queue *q)
 			break;
 	}
 
+unlock:
+	mutex_unlock(&q->sysfs_lock);
+
 	return ret;
 }
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 524f44742816..7d955c756810 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -78,6 +78,7 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q,
  */
 extern void blk_mq_sysfs_init(struct request_queue *q);
 extern void blk_mq_sysfs_deinit(struct request_queue *q);
+extern int __blk_mq_register_dev(struct device *dev, struct request_queue *q);
 extern int blk_mq_sysfs_register(struct request_queue *q);
 extern void blk_mq_sysfs_unregister(struct request_queue *q);
 extern void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index f85723332288..3f37813ccbaf 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -877,9 +877,6 @@ int blk_register_queue(struct gendisk *disk)
 	if (ret)
 		return ret;
 
-	if (q->mq_ops)
-		blk_mq_register_dev(dev, q);
-
 	/* Prevent changes through sysfs until registration is completed. */
 	mutex_lock(&q->sysfs_lock);
 
@@ -889,6 +886,9 @@ int blk_register_queue(struct gendisk *disk)
 		goto unlock;
 	}
 
+	if (q->mq_ops)
+		__blk_mq_register_dev(dev, q);
+
 	kobject_uevent(&q->kobj, KOBJ_ADD);
 
 	wbt_enable_default(q);
-- 
2.12.2

[PATCH v5 02/10] blk-mq: Let blk_mq_debugfs_register() look up the queue name

[Bart Van Assche]

A later patch will move the call of blk_mq_debugfs_register() to
a function to which the queue name is not passed as an argument.
To avoid having to add a 'name' argument to multiple callers, let
blk_mq_debugfs_register() look up the queue name.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 block/blk-mq-debugfs.c | 5 +++--
 block/blk-mq-sysfs.c   | 2 +-
 block/blk-mq.h         | 5 ++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 3057641d5d15..e9282b945f6b 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -785,12 +785,13 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_ctx_attrs[] = {
 	{},
 };
 
-int blk_mq_debugfs_register(struct request_queue *q, const char *name)
+int blk_mq_debugfs_register(struct request_queue *q)
 {
 	if (!blk_debugfs_root)
 		return -ENOENT;
 
-	q->debugfs_dir = debugfs_create_dir(name, blk_debugfs_root);
+	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
+					    blk_debugfs_root);
 	if (!q->debugfs_dir)
 		goto err;
 
diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index a2dbb1a48e72..afb3451cf8a5 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -318,7 +318,7 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
 
 	kobject_uevent(&q->mq_kobj, KOBJ_ADD);
 
-	blk_mq_debugfs_register(q, kobject_name(&dev->kobj));
+	blk_mq_debugfs_register(q);
 
 	queue_for_each_hw_ctx(q, hctx, i) {
 		ret = blk_mq_register_hctx(hctx);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 7d955c756810..9049c0f11505 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -87,13 +87,12 @@ extern void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx);
  * debugfs helpers
  */
 #ifdef CONFIG_BLK_DEBUG_FS
-int blk_mq_debugfs_register(struct request_queue *q, const char *name);
+int blk_mq_debugfs_register(struct request_queue *q);
 void blk_mq_debugfs_unregister(struct request_queue *q);
 int blk_mq_debugfs_register_hctxs(struct request_queue *q);
 void blk_mq_debugfs_unregister_hctxs(struct request_queue *q);
 #else
-static inline int blk_mq_debugfs_register(struct request_queue *q,
-					  const char *name)
+static inline int blk_mq_debugfs_register(struct request_queue *q)
 {
 	return 0;
 }
-- 
2.12.2

[PATCH v5 03/10] blk-mq-debugfs: Rename functions for registering and unregistering the mq directory

[Bart Van Assche]

Since the blk_mq_debugfs_*register_hctxs() functions register and
unregister all attributes under the "mq" directory, rename these
into blk_mq_debugfs_*register_mq().

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Omar Sandoval <osandov@fb.com>
---
 block/blk-mq-debugfs.c | 8 ++++----
 block/blk-mq-sysfs.c   | 6 +++---
 block/blk-mq.h         | 8 ++++----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index e9282b945f6b..d99064e9e76a 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -795,7 +795,7 @@ int blk_mq_debugfs_register(struct request_queue *q)
 	if (!q->debugfs_dir)
 		goto err;
 
-	if (blk_mq_debugfs_register_hctxs(q))
+	if (blk_mq_debugfs_register_mq(q))
 		goto err;
 
 	return 0;
@@ -865,7 +865,7 @@ static int blk_mq_debugfs_register_hctx(struct request_queue *q,
 	return 0;
 }
 
-int blk_mq_debugfs_register_hctxs(struct request_queue *q)
+int blk_mq_debugfs_register_mq(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
 	int i;
@@ -891,11 +891,11 @@ int blk_mq_debugfs_register_hctxs(struct request_queue *q)
 	return 0;
 
 err:
-	blk_mq_debugfs_unregister_hctxs(q);
+	blk_mq_debugfs_unregister_mq(q);
 	return -ENOMEM;
 }
 
-void blk_mq_debugfs_unregister_hctxs(struct request_queue *q)
+void blk_mq_debugfs_unregister_mq(struct request_queue *q)
 {
 	debugfs_remove_recursive(q->mq_debugfs_dir);
 	q->mq_debugfs_dir = NULL;
diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index afb3451cf8a5..c2cac20d981d 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -258,7 +258,7 @@ static void __blk_mq_unregister_dev(struct device *dev, struct request_queue *q)
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_unregister_hctx(hctx);
 
-	blk_mq_debugfs_unregister_hctxs(q);
+	blk_mq_debugfs_unregister_mq(q);
 
 	kobject_uevent(&q->mq_kobj, KOBJ_REMOVE);
 	kobject_del(&q->mq_kobj);
@@ -356,7 +356,7 @@ void blk_mq_sysfs_unregister(struct request_queue *q)
 	if (!q->mq_sysfs_init_done)
 		goto unlock;
 
-	blk_mq_debugfs_unregister_hctxs(q);
+	blk_mq_debugfs_unregister_mq(q);
 
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_unregister_hctx(hctx);
@@ -374,7 +374,7 @@ int blk_mq_sysfs_register(struct request_queue *q)
 	if (!q->mq_sysfs_init_done)
 		goto unlock;
 
-	blk_mq_debugfs_register_hctxs(q);
+	blk_mq_debugfs_register_mq(q);
 
 	queue_for_each_hw_ctx(q, hctx, i) {
 		ret = blk_mq_register_hctx(hctx);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 9049c0f11505..2814a14e529c 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -89,8 +89,8 @@ extern void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx);
 #ifdef CONFIG_BLK_DEBUG_FS
 int blk_mq_debugfs_register(struct request_queue *q);
 void blk_mq_debugfs_unregister(struct request_queue *q);
-int blk_mq_debugfs_register_hctxs(struct request_queue *q);
-void blk_mq_debugfs_unregister_hctxs(struct request_queue *q);
+int blk_mq_debugfs_register_mq(struct request_queue *q);
+void blk_mq_debugfs_unregister_mq(struct request_queue *q);
 #else
 static inline int blk_mq_debugfs_register(struct request_queue *q)
 {
@@ -101,12 +101,12 @@ static inline void blk_mq_debugfs_unregister(struct request_queue *q)
 {
 }
 
-static inline int blk_mq_debugfs_register_hctxs(struct request_queue *q)
+static inline int blk_mq_debugfs_register_mq(struct request_queue *q)
 {
 	return 0;
 }
 
-static inline void blk_mq_debugfs_unregister_hctxs(struct request_queue *q)
+static inline void blk_mq_debugfs_unregister_mq(struct request_queue *q)
 {
 }
 #endif
-- 
2.12.2

[PATCH v5 04/10] blk-mq: Only unregister hctxs for which registration succeeded

[Bart Van Assche]

Hctx unregistration involves calling kobject_del(). kobject_del()
must not be called if kobject_add() has not been called. Hence in
the error path only unregister hctxs for which registration succeeded.

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-sysfs.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index c2cac20d981d..053549a32778 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -323,16 +323,24 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
 	queue_for_each_hw_ctx(q, hctx, i) {
 		ret = blk_mq_register_hctx(hctx);
 		if (ret)
-			break;
+			goto unreg;
 	}
 
-	if (ret)
-		__blk_mq_unregister_dev(dev, q);
-	else
-		q->mq_sysfs_init_done = true;
+	q->mq_sysfs_init_done = true;
 
 out:
 	return ret;
+
+unreg:
+	while (--i >= 0)
+		blk_mq_unregister_hctx(hctx);
+
+	blk_mq_debugfs_unregister_mq(q);
+
+	kobject_uevent(&q->mq_kobj, KOBJ_REMOVE);
+	kobject_del(&q->mq_kobj);
+	kobject_put(&dev->kobj);
+	goto out;
 }
 
 int blk_mq_register_dev(struct device *dev, struct request_queue *q)
-- 
2.12.2

[PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Bart Van Assche]

One of the debugfs attributes allows to run a queue. Since running
a queue after a queue has entered the "dead" state is not allowed
and triggers a use-after-free, unregister the debugfs attributes
before a queue reaches the "dead" state.

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

diff --git a/block/blk-core.c b/block/blk-core.c
index a49b0830aaaf..33c91a4bee97 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -566,6 +566,11 @@ void blk_cleanup_queue(struct request_queue *q)
 	spin_lock_irq(lock);
 	if (!q->mq_ops)
 		__blk_drain_queue(q, true);
+	spin_unlock_irq(lock);
+
+	blk_mq_debugfs_unregister_mq(q);
+
+	spin_lock_irq(lock);
 	queue_flag_set(QUEUE_FLAG_DEAD, q);
 	spin_unlock_irq(lock);
 
-- 
2.12.2

[PATCH v5 06/10] 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>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: 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 d99064e9e76a..1132be4e9c1c 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -141,11 +141,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) {
@@ -757,6 +752,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},
 	{},
 };
 
@@ -873,9 +869,6 @@ int blk_mq_debugfs_register_mq(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.2

[PATCH v5 07/10] 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>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: 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 1132be4e9c1c..ccc7b0f71230 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;
 }
 
@@ -193,6 +193,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;
 }
 
@@ -236,6 +237,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.2

[PATCH v5 08/10] 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>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: 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 ccc7b0f71230..ac39093c4ef7 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -253,13 +253,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((__force u32)RQF_SORTED)]		= "SORTED",
+	[ilog2((__force u32)RQF_STARTED)]		= "STARTED",
+	[ilog2((__force u32)RQF_QUEUED)]		= "QUEUED",
+	[ilog2((__force u32)RQF_SOFTBARRIER)]		= "SOFTBARRIER",
+	[ilog2((__force u32)RQF_FLUSH_SEQ)]		= "FLUSH_SEQ",
+	[ilog2((__force u32)RQF_MIXED_MERGE)]		= "MIXED_MERGE",
+	[ilog2((__force u32)RQF_MQ_INFLIGHT)]		= "MQ_INFLIGHT",
+	[ilog2((__force u32)RQF_DONTPREP)]		= "DONTPREP",
+	[ilog2((__force u32)RQF_PREEMPT)]		= "PREEMPT",
+	[ilog2((__force u32)RQF_COPY_USER)]		= "COPY_USER",
+	[ilog2((__force u32)RQF_FAILED)]		= "FAILED",
+	[ilog2((__force u32)RQF_QUIET)]			= "QUIET",
+	[ilog2((__force u32)RQF_ELVPRIV)]		= "ELVPRIV",
+	[ilog2((__force u32)RQF_IO_STAT)]		= "IO_STAT",
+	[ilog2((__force u32)RQF_ALLOCED)]		= "ALLOCED",
+	[ilog2((__force u32)RQF_PM)]			= "PM",
+	[ilog2((__force u32)RQF_HASHED)]		= "HASHED",
+	[ilog2((__force u32)RQF_STATS)]			= "STATS",
+	[ilog2((__force u32)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 & ~REQ_OP_MASK, 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.2

[PATCH v5 09/10] 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 | 6 +++++-
 include/linux/blk-mq.h | 8 ++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index ac39093c4ef7..bcd2a7d4a3a5 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -311,6 +311,7 @@ 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;
 
 	seq_printf(m, "%p {.op=", rq);
@@ -324,8 +325,11 @@ 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,
+	seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
 		   rq->internal_tag);
+	if (mq_ops->show_rq)
+		mq_ops->show_rq(m, rq);
+	seq_puts(m, "}\n");
 	return 0;
 }
 
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 0c4dadb85f62..32bd8eb5ba67 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -142,6 +142,14 @@ struct blk_mq_ops {
 	reinit_request_fn	*reinit_request;
 
 	map_queues_fn		*map_queues;
+
+#ifdef CONFIG_BLK_DEBUG_FS
+	/*
+	 * Used by the debugfs implementation to show driver-specific
+	 * information about a request.
+	 */
+	void (*show_rq)(struct seq_file *m, struct request *rq);
+#endif
 };
 
 enum {
-- 
2.12.2

[PATCH v5 10/10] 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/Makefile       |  1 +
 drivers/scsi/scsi_debugfs.c | 13 +++++++++++++
 drivers/scsi/scsi_debugfs.h |  4 ++++
 drivers/scsi/scsi_lib.c     |  4 ++++
 4 files changed, 22 insertions(+)
 create mode 100644 drivers/scsi/scsi_debugfs.c
 create mode 100644 drivers/scsi/scsi_debugfs.h

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index fc2855565a51..93dbe58c47c8 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -166,6 +166,7 @@ scsi_mod-y			+= scsi_scan.o scsi_sysfs.o scsi_devinfo.o
 scsi_mod-$(CONFIG_SCSI_NETLINK)	+= scsi_netlink.o
 scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
 scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
+scsi_mod-$(CONFIG_BLK_DEBUG_FS)	+= scsi_debugfs.o
 scsi_mod-y			+= scsi_trace.o scsi_logging.o
 scsi_mod-$(CONFIG_PM)		+= scsi_pm.o
 scsi_mod-$(CONFIG_SCSI_DH)	+= scsi_dh.o
diff --git a/drivers/scsi/scsi_debugfs.c b/drivers/scsi/scsi_debugfs.c
new file mode 100644
index 000000000000..f831c23fdee3
--- /dev/null
+++ b/drivers/scsi/scsi_debugfs.c
@@ -0,0 +1,13 @@
+#include <linux/seq_file.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
+#include "scsi_debugfs.h"
+
+void scsi_show_rq(struct seq_file *m, struct request *rq)
+{
+	struct scsi_cmnd *cmd = container_of(scsi_req(rq), typeof(*cmd), req);
+	char buf[64];
+
+	__scsi_format_command(buf, sizeof(buf), cmd->cmnd, cmd->cmd_len);
+	seq_printf(m, ", .cmd=%s", buf);
+}
diff --git a/drivers/scsi/scsi_debugfs.h b/drivers/scsi/scsi_debugfs.h
new file mode 100644
index 000000000000..951b043e82d0
--- /dev/null
+++ b/drivers/scsi/scsi_debugfs.h
@@ -0,0 +1,4 @@
+struct request;
+struct seq_file;
+
+void scsi_show_rq(struct seq_file *m, struct request *rq);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index abc391e00f7d..1c3e87d6c48f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -34,6 +34,7 @@
 
 #include <trace/events/scsi.h>
 
+#include "scsi_debugfs.h"
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
@@ -2157,6 +2158,9 @@ static const struct blk_mq_ops scsi_mq_ops = {
 	.queue_rq	= scsi_queue_rq,
 	.complete	= scsi_softirq_done,
 	.timeout	= scsi_timeout,
+#ifdef CONFIG_BLK_DEBUG_FS
+	.show_rq	= scsi_show_rq,
+#endif
 	.init_request	= scsi_init_request,
 	.exit_request	= scsi_exit_request,
 	.map_queues	= scsi_map_queues,
-- 
2.12.2

[PATCH v5 10/10] 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/Makefile       |  1 +
 drivers/scsi/scsi_debugfs.c | 13 +++++++++++++
 drivers/scsi/scsi_debugfs.h |  4 ++++
 drivers/scsi/scsi_lib.c     |  4 ++++
 4 files changed, 22 insertions(+)
 create mode 100644 drivers/scsi/scsi_debugfs.c
 create mode 100644 drivers/scsi/scsi_debugfs.h

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index fc2855565a51..93dbe58c47c8 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -166,6 +166,7 @@ scsi_mod-y			+= scsi_scan.o scsi_sysfs.o scsi_devinfo.o
 scsi_mod-$(CONFIG_SCSI_NETLINK)	+= scsi_netlink.o
 scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
 scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
+scsi_mod-$(CONFIG_BLK_DEBUG_FS)	+= scsi_debugfs.o
 scsi_mod-y			+= scsi_trace.o scsi_logging.o
 scsi_mod-$(CONFIG_PM)		+= scsi_pm.o
 scsi_mod-$(CONFIG_SCSI_DH)	+= scsi_dh.o
diff --git a/drivers/scsi/scsi_debugfs.c b/drivers/scsi/scsi_debugfs.c
new file mode 100644
index 000000000000..f831c23fdee3
--- /dev/null
+++ b/drivers/scsi/scsi_debugfs.c
@@ -0,0 +1,13 @@
+#include <linux/seq_file.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
+#include "scsi_debugfs.h"
+
+void scsi_show_rq(struct seq_file *m, struct request *rq)
+{
+	struct scsi_cmnd *cmd = container_of(scsi_req(rq), typeof(*cmd), req);
+	char buf[64];
+
+	__scsi_format_command(buf, sizeof(buf), cmd->cmnd, cmd->cmd_len);
+	seq_printf(m, ", .cmd=%s", buf);
+}
diff --git a/drivers/scsi/scsi_debugfs.h b/drivers/scsi/scsi_debugfs.h
new file mode 100644
index 000000000000..951b043e82d0
--- /dev/null
+++ b/drivers/scsi/scsi_debugfs.h
@@ -0,0 +1,4 @@
+struct request;
+struct seq_file;
+
+void scsi_show_rq(struct seq_file *m, struct request *rq);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index abc391e00f7d..1c3e87d6c48f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -34,6 +34,7 @@
 
 #include <trace/events/scsi.h>
 
+#include "scsi_debugfs.h"
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
@@ -2157,6 +2158,9 @@ static const struct blk_mq_ops scsi_mq_ops = {
 	.queue_rq	= scsi_queue_rq,
 	.complete	= scsi_softirq_done,
 	.timeout	= scsi_timeout,
+#ifdef CONFIG_BLK_DEBUG_FS
+	.show_rq	= scsi_show_rq,
+#endif
 	.init_request	= scsi_init_request,
 	.exit_request	= scsi_exit_request,
 	.map_queues	= scsi_map_queues,
-- 
2.12.2

Re: [PATCH v5 04/10] blk-mq: Only unregister hctxs for which registration succeeded

[Omar Sandoval]

On Tue, Apr 25, 2017 at 01:37:39PM -0700, Bart Van Assche wrote:
> Hctx unregistration involves calling kobject_del(). kobject_del()
> must not be called if kobject_add() has not been called. Hence in
> the error path only unregister hctxs for which registration succeeded.
> 
> 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-sysfs.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
> index c2cac20d981d..053549a32778 100644
> --- a/block/blk-mq-sysfs.c
> +++ b/block/blk-mq-sysfs.c
> @@ -323,16 +323,24 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
>  	queue_for_each_hw_ctx(q, hctx, i) {
>  		ret = blk_mq_register_hctx(hctx);
>  		if (ret)
> -			break;
> +			goto unreg;
>  	}
>  
> -	if (ret)
> -		__blk_mq_unregister_dev(dev, q);
> -	else
> -		q->mq_sysfs_init_done = true;
> +	q->mq_sysfs_init_done = true;
>  
>  out:
>  	return ret;
> +
> +unreg:
> +	while (--i >= 0)
> +		blk_mq_unregister_hctx(hctx);

Missed this in your last submission, won't this unregister the same hctx
repeatedly?

> +	blk_mq_debugfs_unregister_mq(q);
> +
> +	kobject_uevent(&q->mq_kobj, KOBJ_REMOVE);
> +	kobject_del(&q->mq_kobj);
> +	kobject_put(&dev->kobj);
> +	goto out;

The out label doesn't do anything interesting, can we just return ret?

>  }
>  
>  int blk_mq_register_dev(struct device *dev, struct request_queue *q)
> -- 
> 2.12.2
> 

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Omar Sandoval]

On Tue, Apr 25, 2017 at 01:37:40PM -0700, Bart Van Assche wrote:
> One of the debugfs attributes allows to run a queue. Since running
> a queue after a queue has entered the "dead" state is not allowed
> and triggers a use-after-free, unregister the debugfs attributes
> before a queue reaches the "dead" state.

Still not happy with this commit message. I'd prefer:

We currently call blk_mq_free_queue() from blk_cleanup_queue() before we
unregister the debugfs attributes for that queue in blk_release_queue().
This leaves a window open during which accessing most of the mq debugfs
attributes would cause a use-after-free. Additionally, the "state"
attribute allows running the queue, which we should not do after the
queue has entered the "dead" state. Fix both of these cases by
unregistering the debugfs attributes before this.

Jens, could you ack that dropping the lock is okay?

> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> ---
>  block/blk-core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a49b0830aaaf..33c91a4bee97 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -566,6 +566,11 @@ void blk_cleanup_queue(struct request_queue *q)
>  	spin_lock_irq(lock);
>  	if (!q->mq_ops)
>  		__blk_drain_queue(q, true);
> +	spin_unlock_irq(lock);
> +
> +	blk_mq_debugfs_unregister_mq(q);
> +
> +	spin_lock_irq(lock);
>  	queue_flag_set(QUEUE_FLAG_DEAD, q);
>  	spin_unlock_irq(lock);
>  
> -- 
> 2.12.2
> 

Re: [PATCH v5 09/10] blk-mq: Add blk_mq_ops.show_rq()

[Omar Sandoval]

On Tue, Apr 25, 2017 at 01:37:44PM -0700, Bart Van Assche wrote:
> This new callback function will be used in the next patch to show
> more information about SCSI requests.

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 | 6 +++++-
>  include/linux/blk-mq.h | 8 ++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index ac39093c4ef7..bcd2a7d4a3a5 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -311,6 +311,7 @@ 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;
>  
>  	seq_printf(m, "%p {.op=", rq);
> @@ -324,8 +325,11 @@ 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,
> +	seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
>  		   rq->internal_tag);
> +	if (mq_ops->show_rq)
> +		mq_ops->show_rq(m, rq);
> +	seq_puts(m, "}\n");
>  	return 0;
>  }
>  
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 0c4dadb85f62..32bd8eb5ba67 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -142,6 +142,14 @@ struct blk_mq_ops {
>  	reinit_request_fn	*reinit_request;
>  
>  	map_queues_fn		*map_queues;
> +
> +#ifdef CONFIG_BLK_DEBUG_FS
> +	/*
> +	 * Used by the debugfs implementation to show driver-specific
> +	 * information about a request.
> +	 */
> +	void (*show_rq)(struct seq_file *m, struct request *rq);
> +#endif
>  };
>  
>  enum {
> -- 
> 2.12.2
> 

Re: [PATCH v5 10/10] scsi: Implement blk_mq_ops.show_rq()

[Omar Sandoval]

On Tue, Apr 25, 2017 at 01:37:45PM -0700, Bart Van Assche wrote:
> Show the SCSI CDB, .eh_eflags and .result for pending SCSI commands
> in /sys/kernel/debug/block/*/mq/*/dispatch and */rq_list.

Only thing I noticed was that the only other caller I see has buf[70].
No idea if that's a meaningful number. For the sake of this not getting
bike-shedded to death,

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

> 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/Makefile       |  1 +
>  drivers/scsi/scsi_debugfs.c | 13 +++++++++++++
>  drivers/scsi/scsi_debugfs.h |  4 ++++
>  drivers/scsi/scsi_lib.c     |  4 ++++
>  4 files changed, 22 insertions(+)
>  create mode 100644 drivers/scsi/scsi_debugfs.c
>  create mode 100644 drivers/scsi/scsi_debugfs.h
> 
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index fc2855565a51..93dbe58c47c8 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -166,6 +166,7 @@ scsi_mod-y			+= scsi_scan.o scsi_sysfs.o scsi_devinfo.o
>  scsi_mod-$(CONFIG_SCSI_NETLINK)	+= scsi_netlink.o
>  scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
>  scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
> +scsi_mod-$(CONFIG_BLK_DEBUG_FS)	+= scsi_debugfs.o
>  scsi_mod-y			+= scsi_trace.o scsi_logging.o
>  scsi_mod-$(CONFIG_PM)		+= scsi_pm.o
>  scsi_mod-$(CONFIG_SCSI_DH)	+= scsi_dh.o
> diff --git a/drivers/scsi/scsi_debugfs.c b/drivers/scsi/scsi_debugfs.c
> new file mode 100644
> index 000000000000..f831c23fdee3
> --- /dev/null
> +++ b/drivers/scsi/scsi_debugfs.c
> @@ -0,0 +1,13 @@
> +#include <linux/seq_file.h>
> +#include <scsi/scsi_cmnd.h>
> +#include <scsi/scsi_dbg.h>
> +#include "scsi_debugfs.h"
> +
> +void scsi_show_rq(struct seq_file *m, struct request *rq)
> +{
> +	struct scsi_cmnd *cmd = container_of(scsi_req(rq), typeof(*cmd), req);
> +	char buf[64];
> +
> +	__scsi_format_command(buf, sizeof(buf), cmd->cmnd, cmd->cmd_len);
> +	seq_printf(m, ", .cmd=%s", buf);
> +}
> diff --git a/drivers/scsi/scsi_debugfs.h b/drivers/scsi/scsi_debugfs.h
> new file mode 100644
> index 000000000000..951b043e82d0
> --- /dev/null
> +++ b/drivers/scsi/scsi_debugfs.h
> @@ -0,0 +1,4 @@
> +struct request;
> +struct seq_file;
> +
> +void scsi_show_rq(struct seq_file *m, struct request *rq);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index abc391e00f7d..1c3e87d6c48f 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -34,6 +34,7 @@
>  
>  #include <trace/events/scsi.h>
>  
> +#include "scsi_debugfs.h"
>  #include "scsi_priv.h"
>  #include "scsi_logging.h"
>  
> @@ -2157,6 +2158,9 @@ static const struct blk_mq_ops scsi_mq_ops = {
>  	.queue_rq	= scsi_queue_rq,
>  	.complete	= scsi_softirq_done,
>  	.timeout	= scsi_timeout,
> +#ifdef CONFIG_BLK_DEBUG_FS
> +	.show_rq	= scsi_show_rq,
> +#endif
>  	.init_request	= scsi_init_request,
>  	.exit_request	= scsi_exit_request,
>  	.map_queues	= scsi_map_queues,
> -- 
> 2.12.2
> 

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Jens Axboe]

On 04/25/2017 02:30 PM, Omar Sandoval wrote:
> On Tue, Apr 25, 2017 at 01:37:40PM -0700, Bart Van Assche wrote:
>> One of the debugfs attributes allows to run a queue. Since running
>> a queue after a queue has entered the "dead" state is not allowed
>> and triggers a use-after-free, unregister the debugfs attributes
>> before a queue reaches the "dead" state.
> 
> Still not happy with this commit message. I'd prefer:
> 
> We currently call blk_mq_free_queue() from blk_cleanup_queue() before we
> unregister the debugfs attributes for that queue in blk_release_queue().
> This leaves a window open during which accessing most of the mq debugfs
> attributes would cause a use-after-free. Additionally, the "state"
> attribute allows running the queue, which we should not do after the
> queue has entered the "dead" state. Fix both of these cases by
> unregistering the debugfs attributes before this.
> 
> Jens, could you ack that dropping the lock is okay?

Looks fine to me. However, I think there's room for improvement here.
Why don't we just make it:

	if (!q->mq_ops) {
		spin_lock_irq(lock);
		__blk_drain_queue(q, true);
	} else {
		blk_mq_debugfs_unregister_mq(q);
		spin_lock_irq(lock);
	}

	queue_flag_set(QUEUE_FLAG_DEAD, q);
	[...]

Would seem much more readable to me, and less dropping/acquiring for
cases where we don't need it.

-- 
Jens Axboe

Re: [PATCH v5 10/10] scsi: Implement blk_mq_ops.show_rq()

[Bart Van Assche]

On Tue, 2017-04-25 at 14:39 -0700, Omar Sandoval wrote:
> On Tue, Apr 25, 2017 at 01:37:45PM -0700, Bart Van Assche wrote:
> > Show the SCSI CDB, .eh_eflags and .result for pending SCSI commands
> > in /sys/kernel/debug/block/*/mq/*/dispatch and */rq_list.
>=20
> Only thing I noticed was that the only other caller I see has buf[70].
> No idea if that's a meaningful number. For the sake of this not getting
> bike-shedded to death,

Neither length is sufficient to avoid truncation of e.g. ATA pass-through
commands or commands with variable length CDBs. However, from the point of
view of debugging queue lockups the most useful information in a SCSI
command are the first two bytes of the CDB. The chosen buffer length is
definitely enough to make sure that these two bytes will be reported.

Bart.=

Re: [PATCH v5 10/10] scsi: Implement blk_mq_ops.show_rq()

[Bart Van Assche]

On Tue, 2017-04-25 at 14:39 -0700, Omar Sandoval wrote:
> On Tue, Apr 25, 2017 at 01:37:45PM -0700, Bart Van Assche wrote:
> > Show the SCSI CDB, .eh_eflags and .result for pending SCSI commands
> > in /sys/kernel/debug/block/*/mq/*/dispatch and */rq_list.
> 
> Only thing I noticed was that the only other caller I see has buf[70].
> No idea if that's a meaningful number. For the sake of this not getting
> bike-shedded to death,

Neither length is sufficient to avoid truncation of e.g. ATA pass-through
commands or commands with variable length CDBs. However, from the point of
view of debugging queue lockups the most useful information in a SCSI
command are the first two bytes of the CDB. The chosen buffer length is
definitely enough to make sure that these two bytes will be reported.

Bart.

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Bart Van Assche]

On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote:
> On Tue, Apr 25, 2017 at 01:37:40PM -0700, Bart Van Assche wrote:
> > One of the debugfs attributes allows to run a queue. Since running
> > a queue after a queue has entered the "dead" state is not allowed
> > and triggers a use-after-free, unregister the debugfs attributes
> > before a queue reaches the "dead" state.
>=20
> Still not happy with this commit message. I'd prefer:
>=20
> We currently call blk_mq_free_queue() from blk_cleanup_queue() before we
> unregister the debugfs attributes for that queue in blk_release_queue().
> This leaves a window open during which accessing most of the mq debugfs
> attributes would cause a use-after-free. Additionally, the "state"
> attribute allows running the queue, which we should not do after the
> queue has entered the "dead" state. Fix both of these cases by
> unregistering the debugfs attributes before this.

Hello Omar,

That's a very verbose description. How about this?

 =A0=A0=A0Unregister the debugfs attributes before freeing of request queue
=A0=A0=A0=A0resources starts to avoid that a use-after-free can be triggere=
d
=A0=A0=A0=A0through one of the debugfs attributes.

Bart.=

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Jens Axboe]

On 04/25/2017 03:24 PM, Bart Van Assche wrote:
> On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote:
>> On Tue, Apr 25, 2017 at 01:37:40PM -0700, Bart Van Assche wrote:
>>> One of the debugfs attributes allows to run a queue. Since running
>>> a queue after a queue has entered the "dead" state is not allowed
>>> and triggers a use-after-free, unregister the debugfs attributes
>>> before a queue reaches the "dead" state.
>>
>> Still not happy with this commit message. I'd prefer:
>>
>> We currently call blk_mq_free_queue() from blk_cleanup_queue() before we
>> unregister the debugfs attributes for that queue in blk_release_queue().
>> This leaves a window open during which accessing most of the mq debugfs
>> attributes would cause a use-after-free. Additionally, the "state"
>> attribute allows running the queue, which we should not do after the
>> queue has entered the "dead" state. Fix both of these cases by
>> unregistering the debugfs attributes before this.
> 
> Hello Omar,
> 
> That's a very verbose description. How about this?
> 
>     Unregister the debugfs attributes before freeing of request queue
>     resources starts to avoid that a use-after-free can be triggered
>     through one of the debugfs attributes.

Personally I find Omar's commit message much cleaner to read, and
more easily understandable. We really don't need to be laconic in
commit messages.

-- 
Jens Axboe

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Omar Sandoval]

On Tue, Apr 25, 2017 at 10:24:48PM +0000, Bart Van Assche wrote:
> On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote:
> > On Tue, Apr 25, 2017 at 01:37:40PM -0700, Bart Van Assche wrote:
> > > One of the debugfs attributes allows to run a queue. Since running
> > > a queue after a queue has entered the "dead" state is not allowed
> > > and triggers a use-after-free, unregister the debugfs attributes
> > > before a queue reaches the "dead" state.
> > 
> > Still not happy with this commit message. I'd prefer:
> > 
> > We currently call blk_mq_free_queue() from blk_cleanup_queue() before we
> > unregister the debugfs attributes for that queue in blk_release_queue().
> > This leaves a window open during which accessing most of the mq debugfs
> > attributes would cause a use-after-free. Additionally, the "state"
> > attribute allows running the queue, which we should not do after the
> > queue has entered the "dead" state. Fix both of these cases by
> > unregistering the debugfs attributes before this.
> 
> Hello Omar,
> 
> That's a very verbose description. How about this?
> 
>  ���Unregister the debugfs attributes before freeing of request queue
> ����resources starts to avoid that a use-after-free can be triggered
> ����through one of the debugfs attributes.
> 
> Bart.

Are you aware that there is nothing wrong with a descriptive commit
message?

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Bart Van Assche]

On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote:
> Jens, could you ack that dropping the lock is okay?

Hello Omar,

If you have a look at the block layer history then you will see that the
current approach for shutting down block layer queues is an approach that
I had introduced myself. See also commits 3f3299d5c026, 807592a4fafb and
c246e80d8673.

Bart.=

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Jens Axboe]

On 04/26/2017 01:32 PM, Bart Van Assche wrote:
> On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote:
>> Jens, could you ack that dropping the lock is okay?
> 
> Hello Omar,
> 
> If you have a look at the block layer history then you will see that the
> current approach for shutting down block layer queues is an approach that
> I had introduced myself. See also commits 3f3299d5c026, 807592a4fafb and
> c246e80d8673.

Bart, did you see my reply on the topic?

-- 
Jens Axboe

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Omar Sandoval]

On Wed, Apr 26, 2017 at 08:32:31PM +0000, Bart Van Assche wrote:
> On Tue, 2017-04-25 at 14:30 -0700, Omar Sandoval wrote:
> > Jens, could you ack that dropping the lock is okay?
> 
> Hello Omar,
> 
> If you have a look at the block layer history then you will see that the
> current approach for shutting down block layer queues is an approach that
> I had introduced myself. See also commits 3f3299d5c026, 807592a4fafb and
> c246e80d8673.
> 
> Bart.

Thanks for the reference. Jens' suggestion looks the cleanest to me.

Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier

[Bart Van Assche]

On Tue, 2017-04-25 at 14:41 -0700, Jens Axboe wrote:
> Looks fine to me. However, I think there's room for improvement here.
> Why don't we just make it:
>=20
> 	if (!q->mq_ops) {
> 		spin_lock_irq(lock);
> 		__blk_drain_queue(q, true);
> 	} else {
> 		blk_mq_debugfs_unregister_mq(q);
> 		spin_lock_irq(lock);
> 	}
>=20
> 	queue_flag_set(QUEUE_FLAG_DEAD, q);
> 	[...]
>=20
> Would seem much more readable to me, and less dropping/acquiring for
> cases where we don't need it.

Hello Jens,

This looks fine to me. I will update the patch.

Bart.=