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

[PATCH v4 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 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: Check blk_mq_debugfs_register() return value
  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    |  64 ++++++++++++++++++++++--------
 block/blk-mq.h          |  14 +++----
 block/blk-sysfs.c       |   6 +--
 drivers/scsi/scsi_lib.c |  11 ++++++
 include/linux/blk-mq.h  |   6 +++
 7 files changed, 164 insertions(+), 44 deletions(-)

-- 
2.12.2

[PATCH v4 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>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.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 v4 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>
Cc: Omar Sandoval <osandov@fb.com>
Cc: 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 v4 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>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.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 v4 04/10] blk-mq: Check blk_mq_debugfs_register() return value

[Bart Van Assche]

Make __blk_mq_register_dev() check the blk_mq_debugfs_register()
return value. 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 | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index c2cac20d981d..5f66a7798878 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -318,21 +318,32 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
 
 	kobject_uevent(&q->mq_kobj, KOBJ_ADD);
 
-	blk_mq_debugfs_register(q);
+	ret = blk_mq_debugfs_register(q);
+	if (ret)
+		goto remove;
 
 	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);
+
+remove:
+	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 v4 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 even can cause a kernel crash, unregister the debugfs attributes
before a queue reaches the "dead" state.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.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 v4 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>
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 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 v4 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>
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 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 v4 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>
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 ccc7b0f71230..3a99146ece39 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(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 & ~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 v4 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>
Reviewed-by: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
---
 block/blk-mq-debugfs.c | 6 +++++-
 include/linux/blk-mq.h | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 3a99146ece39..c5eca9245459 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..b7bf11c05568 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -121,6 +121,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 seq_file *m, struct request *rq);
+
+	/*
 	 * 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.2

[PATCH v4 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/scsi_lib.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4a20e6098f7c..90bb269042df 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2125,6 +2125,16 @@ static void scsi_exit_rq(struct request_queue *q, struct request *rq)
 	scsi_free_sense_buffer(shost, cmd->sense_buffer);
 }
 
+static void scsi_show_rq(struct seq_file *m, struct request *rq)
+{
+	struct scsi_cmnd *cmd = container_of(scsi_req(rq), typeof(*cmd), req);
+	unsigned int i;
+
+	seq_puts(m, ", .cmd =");
+	for (i = 0; i < cmd->cmd_len; i++)
+		seq_printf(m, " %02x", cmd->cmnd[i]);
+}
+
 struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 {
 	struct Scsi_Host *shost = sdev->host;
@@ -2157,6 +2167,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.2

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

[Hannes Reinecke]

On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> 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>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-sysfs.c | 35 ++++++++++++++++++++++++++++-------
>  block/blk-mq.h       |  1 +
>  block/blk-sysfs.c    |  6 +++---
>  3 files changed, 32 insertions(+), 10 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

[Hannes Reinecke]

On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> 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>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: 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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

[Hannes Reinecke]

On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> 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>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

Re: [PATCH v4 04/10] blk-mq: Check blk_mq_debugfs_register() return value

[Hannes Reinecke]

On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> Make __blk_mq_register_dev() check the blk_mq_debugfs_register()
> return value. 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 | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

[Hannes Reinecke]

On 04/22/2017 01:40 AM, 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 even can cause a kernel crash, unregister the debugfs attributes
> before a queue reaches the "dead" state.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

Re: [PATCH v4 06/10] blk-mq: Move the "state" debugfs attribute one level down

[Hannes Reinecke]

On 04/22/2017 01:40 AM, 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>
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

Re: [PATCH v4 07/10] blk-mq: Make blk_flags_show() callers append a newline character

[Hannes Reinecke]

On 04/22/2017 01:40 AM, 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>
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

Re: [PATCH v4 08/10] blk-mq: Show operation, cmd_flags and rq_flags names

[Hannes Reinecke]

On 04/22/2017 01:40 AM, 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>
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

[Hannes Reinecke]

On 04/22/2017 01:40 AM, 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>
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-mq-debugfs.c | 6 +++++-
>  include/linux/blk-mq.h | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 3a99146ece39..c5eca9245459 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..b7bf11c05568 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -121,6 +121,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 seq_file *m, struct request *rq);
> +
> +	/*
>  	 * 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.
> 
I don't really like this; what does happen if someone disabled
CONFIG_BLK_DEBUGFS?
Won't we end up with a stale callback?

Cheers,

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

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

[Hannes Reinecke]

On 04/22/2017 01:40 AM, 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.
> 
> 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 4a20e6098f7c..90bb269042df 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2125,6 +2125,16 @@ static void scsi_exit_rq(struct request_queue *q, struct request *rq)
>  	scsi_free_sense_buffer(shost, cmd->sense_buffer);
>  }
>  
> +static void scsi_show_rq(struct seq_file *m, struct request *rq)
> +{
> +	struct scsi_cmnd *cmd = container_of(scsi_req(rq), typeof(*cmd), req);
> +	unsigned int i;
> +
> +	seq_puts(m, ", .cmd =");
> +	for (i = 0; i < cmd->cmd_len; i++)
> +		seq_printf(m, " %02x", cmd->cmnd[i]);
> +}
> +
>  struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
>  {
>  	struct Scsi_Host *shost = sdev->host;
> @@ -2157,6 +2167,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,
> 
Hmm. Can't say I'm happy with this callback.
And I really would like to see a similar implementation for NVMe.

But if others agree:

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

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

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

[Omar Sandoval]

On Fri, Apr 21, 2017 at 04:40:19PM -0700, Bart Van Assche wrote:
> 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().

Thanks!

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

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

Re: [PATCH v4 04/10] blk-mq: Check blk_mq_debugfs_register() return value

[Omar Sandoval]

On Fri, Apr 21, 2017 at 04:40:20PM -0700, Bart Van Assche wrote:
> Make __blk_mq_register_dev() check the blk_mq_debugfs_register()
> return value.

I intentionally didn't check this. In my opinion, a missing debug
directory shouldn't be fatal.

> In the error path, only unregister hctxs for which
> registration succeeded.

This part of it seems reasonable.

> 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 | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
> index c2cac20d981d..5f66a7798878 100644
> --- a/block/blk-mq-sysfs.c
> +++ b/block/blk-mq-sysfs.c
> @@ -318,21 +318,32 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
>  
>  	kobject_uevent(&q->mq_kobj, KOBJ_ADD);
>  
> -	blk_mq_debugfs_register(q);
> +	ret = blk_mq_debugfs_register(q);
> +	if (ret)
> +		goto remove;
>  
>  	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);
> +
> +remove:
> +	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
> 

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

[Omar Sandoval]

On Fri, Apr 21, 2017 at 04:40:21PM -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 even can cause a kernel crash, unregister the debugfs attributes
> before a queue reaches the "dead" state.

More important than this case, I think, is that blk_cleanup_queue()
calls blk_mq_free_queue(q), so most of the debugfs entries would lead to
use-after-frees. If you add that to the commit message and address my
comment below,

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

Do we actually have to hold the queue lock when we set QUEUE_FLAG_DEAD?

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

[Omar Sandoval]

On Fri, Apr 21, 2017 at 04:40:18PM -0700, Bart Van Assche wrote:
> 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.

I think I already said this for v3, but

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 | 5 +++--
>  block/blk-mq-sysfs.c   | 2 +-
>  block/blk-mq.h         | 5 ++---
>  3 files changed, 6 insertions(+), 6 deletions(-)

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

[Omar Sandoval]

On Fri, Apr 21, 2017 at 04:40:17PM -0700, Bart Van Assche wrote:
> 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.

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-sysfs.c | 35 ++++++++++++++++++++++++++++-------
>  block/blk-mq.h       |  1 +
>  block/blk-sysfs.c    |  6 +++---
>  3 files changed, 32 insertions(+), 10 deletions(-)

Re: [PATCH v4 04/10] blk-mq: Check blk_mq_debugfs_register() return value

[Bart Van Assche]

On Mon, 2017-04-24 at 09:49 -0700, Omar Sandoval wrote:
> On Fri, Apr 21, 2017 at 04:40:20PM -0700, Bart Van Assche wrote:
> > Make __blk_mq_register_dev() check the blk_mq_debugfs_register()
> > return value.
>=20
> I intentionally didn't check this. In my opinion, a missing debug
> directory shouldn't be fatal.

OK, I will remove that return value check again.

Bart.=

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

[Bart Van Assche]

On Mon, 2017-04-24 at 09:55 -0700, Omar Sandoval wrote:
> On Fri, Apr 21, 2017 at 04:40:21PM -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 even can cause a kernel crash, unregister the debugfs attributes
> > before a queue reaches the "dead" state.
>=20
> More important than this case, I think, is that blk_cleanup_queue()
> calls blk_mq_free_queue(q), so most of the debugfs entries would lead to
> use-after-frees. If you add that to the commit message and address my
> comment below,
>=20
> Reviewed-by: Omar Sandoval <osandov@fb.com>

Thanks! I will update the commit message.

> > --- 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);
>=20
> Do we actually have to hold the queue lock when we set QUEUE_FLAG_DEAD?

It's way easier to keep that spin_lock()/spin_unlock() pair than to analyze
the block driver core and all block drivers to see whether or not any
concurrent queue flag changes could occur.

Bart.=

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

[Omar Sandoval]

On Mon, Apr 24, 2017 at 05:12:05PM +0000, Bart Van Assche wrote:
> On Mon, 2017-04-24 at 09:55 -0700, Omar Sandoval wrote:
> > On Fri, Apr 21, 2017 at 04:40:21PM -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 even can cause a kernel crash, unregister the debugfs attributes
> > > before a queue reaches the "dead" state.
> > 
> > More important than this case, I think, is that blk_cleanup_queue()
> > calls blk_mq_free_queue(q), so most of the debugfs entries would lead to
> > use-after-frees. If you add that to the commit message and address my
> > comment below,
> > 
> > Reviewed-by: Omar Sandoval <osandov@fb.com>
> 
> Thanks! I will update the commit message.
> 
> > > --- 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);
> > 
> > Do we actually have to hold the queue lock when we set QUEUE_FLAG_DEAD?
> 
> It's way easier to keep that spin_lock()/spin_unlock() pair than to analyze
> the block driver core and all block drivers to see whether or not any
> concurrent queue flag changes could occur.

Ah, I didn't realize that queue_flag_set() did a non-atomic set. I'm
wondering if anything bad could happen if something raced between when
we drop the lock and regrab it. Maybe just move the
blk_mq_debugfs_unregister_mq() before we grab the lock the first time
instead?

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

[Bart Van Assche]

On Mon, 2017-04-24 at 10:17 -0700, Omar Sandoval wrote:
> On Mon, Apr 24, 2017 at 05:12:05PM +0000, Bart Van Assche wrote:
> > On Mon, 2017-04-24 at 09:55 -0700, Omar Sandoval wrote:
> > > On Fri, Apr 21, 2017 at 04:40:21PM -0700, Bart Van Assche wrote:
> > > > @@ -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);
> > >=20
> > > Do we actually have to hold the queue lock when we set QUEUE_FLAG_DEA=
D?
> >=20
> > It's way easier to keep that spin_lock()/spin_unlock() pair than to ana=
lyze
> > the block driver core and all block drivers to see whether or not any
> > concurrent queue flag changes could occur.
>=20
> Ah, I didn't realize that queue_flag_set() did a non-atomic set. I'm
> wondering if anything bad could happen if something raced between when
> we drop the lock and regrab it. Maybe just move the
> blk_mq_debugfs_unregister_mq() before we grab the lock the first time
> instead?

That would have the disadvantage that debugfs attributes would be unregiste=
red
before __blk_drain_queue() is called and hence that these debugfs attribute=
s
would not be available to debug hangs in queue draining for traditional blo=
ck
layer queues ...

Bart.=

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

[Omar Sandoval]

On Mon, Apr 24, 2017 at 05:24:13PM +0000, Bart Van Assche wrote:
> On Mon, 2017-04-24 at 10:17 -0700, Omar Sandoval wrote:
> > On Mon, Apr 24, 2017 at 05:12:05PM +0000, Bart Van Assche wrote:
> > > On Mon, 2017-04-24 at 09:55 -0700, Omar Sandoval wrote:
> > > > On Fri, Apr 21, 2017 at 04:40:21PM -0700, Bart Van Assche wrote:
> > > > > @@ -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);
> > > > 
> > > > Do we actually have to hold the queue lock when we set QUEUE_FLAG_DEAD?
> > > 
> > > It's way easier to keep that spin_lock()/spin_unlock() pair than to analyze
> > > the block driver core and all block drivers to see whether or not any
> > > concurrent queue flag changes could occur.
> > 
> > Ah, I didn't realize that queue_flag_set() did a non-atomic set. I'm
> > wondering if anything bad could happen if something raced between when
> > we drop the lock and regrab it. Maybe just move the
> > blk_mq_debugfs_unregister_mq() before we grab the lock the first time
> > instead?
> 
> That would have the disadvantage that debugfs attributes would be unregistered
> before __blk_drain_queue() is called and hence that these debugfs attributes
> would not be available to debug hangs in queue draining for traditional block
> layer queues ...

True, this is probably fine, then.

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

[Omar Sandoval]

On Mon, Apr 24, 2017 at 10:26:15AM -0700, Omar Sandoval wrote:
> On Mon, Apr 24, 2017 at 05:24:13PM +0000, Bart Van Assche wrote:
> > On Mon, 2017-04-24 at 10:17 -0700, Omar Sandoval wrote:
> > > On Mon, Apr 24, 2017 at 05:12:05PM +0000, Bart Van Assche wrote:
> > > > On Mon, 2017-04-24 at 09:55 -0700, Omar Sandoval wrote:
> > > > > On Fri, Apr 21, 2017 at 04:40:21PM -0700, Bart Van Assche wrote:
> > > > > > @@ -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);
> > > > > 
> > > > > Do we actually have to hold the queue lock when we set QUEUE_FLAG_DEAD?
> > > > 
> > > > It's way easier to keep that spin_lock()/spin_unlock() pair than to analyze
> > > > the block driver core and all block drivers to see whether or not any
> > > > concurrent queue flag changes could occur.
> > > 
> > > Ah, I didn't realize that queue_flag_set() did a non-atomic set. I'm
> > > wondering if anything bad could happen if something raced between when
> > > we drop the lock and regrab it. Maybe just move the
> > > blk_mq_debugfs_unregister_mq() before we grab the lock the first time
> > > instead?
> > 
> > That would have the disadvantage that debugfs attributes would be unregistered
> > before __blk_drain_queue() is called and hence that these debugfs attributes
> > would not be available to debug hangs in queue draining for traditional block
> > layer queues ...
> 
> True, this is probably fine, then.

Actually, if we drop this lock, then for non-mq, can't we end up with
some I/O's sneaking in between when we drain the queue and mark it dead
while the lock is dropped?

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

[Bart Van Assche]

On Mon, 2017-04-24 at 10:29 -0700, Omar Sandoval wrote:
> On Mon, Apr 24, 2017 at 10:26:15AM -0700, Omar Sandoval wrote:
> > On Mon, Apr 24, 2017 at 05:24:13PM +0000, Bart Van Assche wrote:
> > > On Mon, 2017-04-24 at 10:17 -0700, Omar Sandoval wrote:
> > > > On Mon, Apr 24, 2017 at 05:12:05PM +0000, Bart Van Assche wrote:
> > > > > On Mon, 2017-04-24 at 09:55 -0700, Omar Sandoval wrote:
> > > > > > On Fri, Apr 21, 2017 at 04:40:21PM -0700, Bart Van Assche wrote=
:
> > > > > > > @@ -566,6 +566,11 @@ void blk_cleanup_queue(struct request_qu=
eue *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);
> > > > > >=20
> > > > > > Do we actually have to hold the queue lock when we set QUEUE_FL=
AG_DEAD?
> > > > >=20
> > > > > It's way easier to keep that spin_lock()/spin_unlock() pair than =
to analyze
> > > > > the block driver core and all block drivers to see whether or not=
 any
> > > > > concurrent queue flag changes could occur.
> > > >=20
> > > > Ah, I didn't realize that queue_flag_set() did a non-atomic set. I'=
m
> > > > wondering if anything bad could happen if something raced between w=
hen
> > > > we drop the lock and regrab it. Maybe just move the
> > > > blk_mq_debugfs_unregister_mq() before we grab the lock the first ti=
me
> > > > instead?
> > >=20
> > > That would have the disadvantage that debugfs attributes would be unr=
egistered
> > > before __blk_drain_queue() is called and hence that these debugfs att=
ributes
> > > would not be available to debug hangs in queue draining for tradition=
al block
> > > layer queues ...
> >=20
> > True, this is probably fine, then.
>=20
> Actually, if we drop this lock, then for non-mq, can't we end up with
> some I/O's sneaking in between when we drain the queue and mark it dead
> while the lock is dropped?

That's a good question but I don't think so. Queuing new I/O after a queue
has been marked as "dying" is not allowed. For both blk-sq and blk-mq queue=
s
blk_get_request() returns -ENODEV if that function is called after the "dyi=
ng"
flag has been set.

Bart.=

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

[Martin K. Petersen]

Bart,

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

Why not use SCSI tracing if you are interested in these?

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Bart Van Assche]

On Mon, 2017-04-24 at 17:35 -0400, Martin K. Petersen wrote:
> > Show the SCSI CDB, .eh_eflags and .result for pending SCSI commands
> > in /sys/kernel/debug/block/*/mq/*/dispatch and */rq_list.
>=20
> Why not use SCSI tracing if you are interested in these?

Hello Martin,

SCSI tracing has to be enabled before a test is started, produces a huge am=
ount
of data, and deriving state information from a huge trace is far from easy.=
 The
information in debugfs provides an easy to read overview of the current sta=
te
without having to analyze megabytes of traces, without introducing any slow=
down
and without having to enable any tracing mechanism from beforehand.

Bart.=

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

[Bart Van Assche]

On Mon, 2017-04-24 at 09:32 +0200, Hannes Reinecke wrote:
> On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> > --- a/include/linux/blk-mq.h
> > +++ b/include/linux/blk-mq.h
> > @@ -121,6 +121,12 @@ struct blk_mq_ops {
> >  	softirq_done_fn		*complete;
> > =20
> >  	/*
> > +	 * Used by the debugfs implementation to show driver-specific
> > +	 * information about a request.
> > +	 */
> > +	void (*show_rq)(struct seq_file *m, struct request *rq);
> > +
> > +	/*
> >  	 * 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.
> >=20
>=20
> I don't really like this; what does happen if someone disabled
> CONFIG_BLK_DEBUGFS? Won't we end up with a stale callback?

Hello Hannes,

How about surrounding (*show_rq)() function pointer with #ifdef CONFIG_BLK_=
DEBUGFS /
#endif?

Bart.=

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

[Martin K. Petersen]

Bart,

> SCSI tracing has to be enabled before a test is started, produces a
> huge amount of data, and deriving state information from a huge trace
> is far from easy. The information in debugfs provides an easy to read
> overview of the current state without having to analyze megabytes of
> traces, without introducing any slowdown and without having to enable
> any tracing mechanism from beforehand.

Fair enough. Just seems like there's an obvious overlap in plumbing.
Don't know if that can be leveraged instead of introducing something
completely new?

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Omar Sandoval]

On Mon, Apr 24, 2017 at 07:19:50PM -0400, Martin K. Petersen wrote:
> 
> Bart,
> 
> > SCSI tracing has to be enabled before a test is started, produces a
> > huge amount of data, and deriving state information from a huge trace
> > is far from easy. The information in debugfs provides an easy to read
> > overview of the current state without having to analyze megabytes of
> > traces, without introducing any slowdown and without having to enable
> > any tracing mechanism from beforehand.
> 
> Fair enough. Just seems like there's an obvious overlap in plumbing.
> Don't know if that can be leveraged instead of introducing something
> completely new?

The debugfs infrastructure is already there, and it already supports
showing requests. Bart is just exposing the SCSI information.

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

[Martin K. Petersen]

Omar,

> The debugfs infrastructure is already there, and it already supports
> showing requests. Bart is just exposing the SCSI information.

That's fine.

I was merely objecting to the fact that we already have umpteen existing
interfaces for displaying SCSI command information.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Bart Van Assche]

On Mon, 2017-04-24 at 19:33 -0400, Martin K. Petersen wrote:
> > The debugfs infrastructure is already there, and it already supports
> > showing requests. Bart is just exposing the SCSI information.
>=20
> That's fine.
>=20
> I was merely objecting to the fact that we already have umpteen existing
> interfaces for displaying SCSI command information.

Hello Martin,

Do you perhaps want me to change the for-loop into a call to
__scsi_format_command()?

Thanks,

Bart.=

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

[Hannes Reinecke]

On 04/24/2017 11:51 PM, Bart Van Assche wrote:
> On Mon, 2017-04-24 at 09:32 +0200, Hannes Reinecke wrote:
>> On 04/22/2017 01:40 AM, Bart Van Assche wrote:
>>> --- a/include/linux/blk-mq.h
>>> +++ b/include/linux/blk-mq.h
>>> @@ -121,6 +121,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 seq_file *m, struct request *rq);
>>> +
>>> +	/*
>>>  	 * 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.
>>>
>>
>> I don't really like this; what does happen if someone disabled
>> CONFIG_BLK_DEBUGFS? Won't we end up with a stale callback?
> 
> Hello Hannes,
> 
> How about surrounding (*show_rq)() function pointer with #ifdef CONFIG_BLK_DEBUGFS /
> #endif?
> 
Nope.

Then you'll end up with different offsets in the structures, depending
on how the kernel is compiled. Making debugging a nightmare.

Not sure what would be the best way here ...

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 v4 09/10] blk-mq: Add blk_mq_ops.show_rq()

[Bart Van Assche]

On Tue, 2017-04-25 at 17:16 +0200, Hannes Reinecke wrote:
> On 04/24/2017 11:51 PM, Bart Van Assche wrote:
> > On Mon, 2017-04-24 at 09:32 +0200, Hannes Reinecke wrote:
> > > On 04/22/2017 01:40 AM, Bart Van Assche wrote:
> > > > --- a/include/linux/blk-mq.h
> > > > +++ b/include/linux/blk-mq.h
> > > > @@ -121,6 +121,12 @@ struct blk_mq_ops {
> > > >  	softirq_done_fn		*complete;
> > > > =20
> > > >  	/*
> > > > +	 * Used by the debugfs implementation to show driver-specific
> > > > +	 * information about a request.
> > > > +	 */
> > > > +	void (*show_rq)(struct seq_file *m, struct request *rq);
> > > > +
> > > > +	/*
> > > >  	 * Called when the block layer side of a hardware queue has been
> > > >  	 * set up, allowing the driver to allocate/init matching structur=
es.
> > > >  	 * Ditto for exit/teardown.
> > > >=20
> > >=20
> > > I don't really like this; what does happen if someone disabled
> > > CONFIG_BLK_DEBUGFS? Won't we end up with a stale callback?
> >=20
> > How about surrounding (*show_rq)() function pointer with #ifdef CONFIG_=
BLK_DEBUGFS /
> > #endif?
>=20
> Then you'll end up with different offsets in the structures, depending
> on how the kernel is compiled. Making debugging a nightmare.

Hello Hannes,

How about moving the .show_rq function pointer to the end such that the
offset of other members of struct blk_mq_ops does not depend on whether or
not CONFIG_BLK_DEBUGFS has been defined?

Bart.=

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

[Jens Axboe]

On 04/25/2017 08:16 AM, Hannes Reinecke wrote:
> On 04/24/2017 11:51 PM, Bart Van Assche wrote:
>> On Mon, 2017-04-24 at 09:32 +0200, Hannes Reinecke wrote:
>>> On 04/22/2017 01:40 AM, Bart Van Assche wrote:
>>>> --- a/include/linux/blk-mq.h
>>>> +++ b/include/linux/blk-mq.h
>>>> @@ -121,6 +121,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 seq_file *m, struct request *rq);
>>>> +
>>>> +	/*
>>>>  	 * 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.
>>>>
>>>
>>> I don't really like this; what does happen if someone disabled
>>> CONFIG_BLK_DEBUGFS? Won't we end up with a stale callback?
>>
>> Hello Hannes,
>>
>> How about surrounding (*show_rq)() function pointer with #ifdef CONFIG_BLK_DEBUGFS /
>> #endif?
>>
> Nope.
> 
> Then you'll end up with different offsets in the structures, depending
> on how the kernel is compiled. Making debugging a nightmare.

That's nonsense, we deal with this all the time already.

-- 
Jens Axboe

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

[Martin K. Petersen]

Bart,

>> I was merely objecting to the fact that we already have umpteen existing
>> interfaces for displaying SCSI command information.

> Do you perhaps want me to change the for-loop into a call to
> __scsi_format_command()?

If possible, I would love to see some commonality in the per-command
information regardless of whether it is displayed due to SCSI logging,
SCSI tracing or an error condition.

-- 
Martin K. Petersen	Oracle Linux Engineering