* [PATCH v5 01/10] blk-mq: Register <dev>/queue/mq after having registered <dev>/queue
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 02/10] blk-mq: Let blk_mq_debugfs_register() look up the queue name Bart Van Assche
` (8 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 02/10] blk-mq: Let blk_mq_debugfs_register() look up the queue name
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 01/10] blk-mq: Register <dev>/queue/mq after having registered <dev>/queue Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 03/10] blk-mq-debugfs: Rename functions for registering and unregistering the mq directory Bart Van Assche
` (7 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 03/10] blk-mq-debugfs: Rename functions for registering and unregistering the mq directory
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 01/10] blk-mq: Register <dev>/queue/mq after having registered <dev>/queue Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 02/10] blk-mq: Let blk_mq_debugfs_register() look up the queue name Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 04/10] blk-mq: Only unregister hctxs for which registration succeeded Bart Van Assche
` (6 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 04/10] blk-mq: Only unregister hctxs for which registration succeeded
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
` (2 preceding siblings ...)
2017-04-25 20:37 ` [PATCH v5 03/10] blk-mq-debugfs: Rename functions for registering and unregistering the mq directory Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 21:18 ` Omar Sandoval
2017-04-25 20:37 ` [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier Bart Van Assche
` (5 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Bart Van Assche, Omar Sandoval, Hannes Reinecke
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 04/10] blk-mq: Only unregister hctxs for which registration succeeded
2017-04-25 20:37 ` [PATCH v5 04/10] blk-mq: Only unregister hctxs for which registration succeeded Bart Van Assche
@ 2017-04-25 21:18 ` Omar Sandoval
0 siblings, 0 replies; 26+ messages in thread
From: Omar Sandoval @ 2017-04-25 21:18 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Omar Sandoval, Hannes Reinecke
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
` (3 preceding siblings ...)
2017-04-25 20:37 ` [PATCH v5 04/10] blk-mq: Only unregister hctxs for which registration succeeded Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 21:30 ` Omar Sandoval
2017-04-25 20:37 ` [PATCH v5 06/10] blk-mq: Move the "state" debugfs attribute one level down Bart Van Assche
` (4 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-25 20:37 ` [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier Bart Van Assche
@ 2017-04-25 21:30 ` Omar Sandoval
2017-04-25 21:41 ` Jens Axboe
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Omar Sandoval @ 2017-04-25 21:30 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-25 21:30 ` Omar Sandoval
@ 2017-04-25 21:41 ` Jens Axboe
2017-04-26 20:38 ` Bart Van Assche
2017-04-25 22:24 ` Bart Van Assche
2017-04-26 20:32 ` Bart Van Assche
2 siblings, 1 reply; 26+ messages in thread
From: Jens Axboe @ 2017-04-25 21:41 UTC (permalink / raw)
To: Omar Sandoval, Bart Van Assche; +Cc: linux-block
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-25 21:41 ` Jens Axboe
@ 2017-04-26 20:38 ` Bart Van Assche
0 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-26 20:38 UTC (permalink / raw)
To: osandov, axboe; +Cc: linux-block
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.=
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-25 21:30 ` Omar Sandoval
2017-04-25 21:41 ` Jens Axboe
@ 2017-04-25 22:24 ` Bart Van Assche
2017-04-25 22:29 ` Jens Axboe
2017-04-25 22:30 ` Omar Sandoval
2017-04-26 20:32 ` Bart Van Assche
2 siblings, 2 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 22:24 UTC (permalink / raw)
To: osandov; +Cc: linux-block, axboe
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.=
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-25 22:24 ` Bart Van Assche
@ 2017-04-25 22:29 ` Jens Axboe
2017-04-25 22:30 ` Omar Sandoval
1 sibling, 0 replies; 26+ messages in thread
From: Jens Axboe @ 2017-04-25 22:29 UTC (permalink / raw)
To: Bart Van Assche, osandov; +Cc: linux-block
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-25 22:24 ` Bart Van Assche
2017-04-25 22:29 ` Jens Axboe
@ 2017-04-25 22:30 ` Omar Sandoval
1 sibling, 0 replies; 26+ messages in thread
From: Omar Sandoval @ 2017-04-25 22:30 UTC (permalink / raw)
To: Bart Van Assche; +Cc: linux-block, axboe
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?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-25 21:30 ` Omar Sandoval
2017-04-25 21:41 ` Jens Axboe
2017-04-25 22:24 ` Bart Van Assche
@ 2017-04-26 20:32 ` Bart Van Assche
2017-04-26 20:37 ` Jens Axboe
2017-04-26 20:37 ` Omar Sandoval
2 siblings, 2 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-26 20:32 UTC (permalink / raw)
To: osandov; +Cc: linux-block, axboe
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.=
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-26 20:32 ` Bart Van Assche
@ 2017-04-26 20:37 ` Jens Axboe
2017-04-26 20:37 ` Omar Sandoval
1 sibling, 0 replies; 26+ messages in thread
From: Jens Axboe @ 2017-04-26 20:37 UTC (permalink / raw)
To: Bart Van Assche, osandov; +Cc: linux-block
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier
2017-04-26 20:32 ` Bart Van Assche
2017-04-26 20:37 ` Jens Axboe
@ 2017-04-26 20:37 ` Omar Sandoval
1 sibling, 0 replies; 26+ messages in thread
From: Omar Sandoval @ 2017-04-26 20:37 UTC (permalink / raw)
To: Bart Van Assche; +Cc: linux-block, axboe
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 06/10] blk-mq: Move the "state" debugfs attribute one level down
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
` (4 preceding siblings ...)
2017-04-25 20:37 ` [PATCH v5 05/10] blk-mq: Unregister debugfs attributes earlier Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 07/10] blk-mq: Make blk_flags_show() callers append a newline character Bart Van Assche
` (3 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 07/10] blk-mq: Make blk_flags_show() callers append a newline character
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
` (5 preceding siblings ...)
2017-04-25 20:37 ` [PATCH v5 06/10] blk-mq: Move the "state" debugfs attribute one level down Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 08/10] blk-mq: Show operation, cmd_flags and rq_flags names Bart Van Assche
` (2 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 08/10] blk-mq: Show operation, cmd_flags and rq_flags names
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
` (6 preceding siblings ...)
2017-04-25 20:37 ` [PATCH v5 07/10] blk-mq: Make blk_flags_show() callers append a newline character Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 09/10] blk-mq: Add blk_mq_ops.show_rq() Bart Van Assche
2017-04-25 20:37 ` Bart Van Assche
9 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 09/10] blk-mq: Add blk_mq_ops.show_rq()
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
` (7 preceding siblings ...)
2017-04-25 20:37 ` [PATCH v5 08/10] blk-mq: Show operation, cmd_flags and rq_flags names Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 21:35 ` Omar Sandoval
2017-04-25 20:37 ` Bart Van Assche
9 siblings, 1 reply; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Bart Van Assche, Omar Sandoval, Hannes Reinecke
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 09/10] blk-mq: Add blk_mq_ops.show_rq()
2017-04-25 20:37 ` [PATCH v5 09/10] blk-mq: Add blk_mq_ops.show_rq() Bart Van Assche
@ 2017-04-25 21:35 ` Omar Sandoval
0 siblings, 0 replies; 26+ messages in thread
From: Omar Sandoval @ 2017-04-25 21:35 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Omar Sandoval, Hannes Reinecke
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 10/10] scsi: Implement blk_mq_ops.show_rq()
2017-04-25 20:37 [PATCH v5 00/10] blk-mq debugfs patches for kernel v4.12 Bart Van Assche
@ 2017-04-25 20:37 ` Bart Van Assche
2017-04-25 20:37 ` [PATCH v5 02/10] blk-mq: Let blk_mq_debugfs_register() look up the queue name Bart Van Assche
` (8 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Bart Van Assche, Martin K . Petersen,
James Bottomley, Omar Sandoval, Hannes Reinecke, linux-scsi
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 10/10] scsi: Implement blk_mq_ops.show_rq()
@ 2017-04-25 20:37 ` Bart Van Assche
0 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 20:37 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Bart Van Assche, Martin K . Petersen,
James Bottomley, Omar Sandoval, Hannes Reinecke, linux-scsi
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 10/10] scsi: Implement blk_mq_ops.show_rq()
2017-04-25 20:37 ` Bart Van Assche
(?)
@ 2017-04-25 21:39 ` Omar Sandoval
2017-04-25 22:06 ` Bart Van Assche
-1 siblings, 1 reply; 26+ messages in thread
From: Omar Sandoval @ 2017-04-25 21:39 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
Omar Sandoval, Hannes Reinecke, linux-scsi
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 10/10] scsi: Implement blk_mq_ops.show_rq()
2017-04-25 21:39 ` Omar Sandoval
@ 2017-04-25 22:06 ` Bart Van Assche
0 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 22:06 UTC (permalink / raw)
To: osandov
Cc: linux-scsi, James.Bottomley, hare, linux-block, osandov,
martin.petersen, axboe
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.=
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 10/10] scsi: Implement blk_mq_ops.show_rq()
@ 2017-04-25 22:06 ` Bart Van Assche
0 siblings, 0 replies; 26+ messages in thread
From: Bart Van Assche @ 2017-04-25 22:06 UTC (permalink / raw)
To: osandov
Cc: linux-scsi, James.Bottomley, hare, linux-block, osandov,
martin.petersen, axboe
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.
^ permalink raw reply [flat|nested] 26+ messages in thread