block: fix blk-mq debugfs vs. blktrace

PATCH In review Omar Sandoval 3 participants 15 messages Jan 31, 2017 – Feb 2, 2017

Subsystems (3)

Block layer Tracing Driver core

lore

Share & tools

lore

Omar Sandoval

author

2017-01-31 22:54 UTC #0 lore raw

cover block: fix blk-mq debugfs vs. blktrace

From: Omar Sandoval <[email protected]>

When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.

Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.

Patches 2 and 3 are cleanups.

Patch 4 is the first part of the fix, making blk-mq and blktrace use the same top-level "block" directory.

Patches 5 and 6 make blk-mq and blktrace play nicely with each other w.r.t. the debugfs "block/$dev" directories.

I tested this with multiple configurations, so hopefully I didn't mess that up this time.

Applies to for-4.11/block.

Thanks!

Cc Greg Kroah-Hartman <[email protected]>

Omar Sandoval (6):
debugfs: add debugfs_lookup()
block: fix debugfs config conditional in struct request_queue
blktrace: make do_blk_trace_setup() static
block: use same block debugfs directory for blk-mq and blktrace
blk-mq: move debugfs_remove() of disk dir to blk_release_queue()
blktrace: use existing disk debugfs directory

 block/blk-core.c             |  9 +++++++++
 block/blk-mq-debugfs.c       | 12 +++---------
 block/blk-mq-sysfs.c         |  2 +-
 block/blk-mq.c               |  2 --
 block/blk-mq.h               |  5 -----
 block/blk-sysfs.c            |  3 +++
 block/blk.h                  |  4 ++++
 fs/debugfs/inode.c           | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h       |  2 +-
 include/linux/blktrace_api.h |  4 ----
 include/linux/debugfs.h      |  8 ++++++++
 kernel/trace/blktrace.c      | 35 +++++++++++++++--------------------
 12 files changed, 80 insertions(+), 42 deletions(-)

Signature · 1 line

2.11.0

Omar Sandoval

author

2017-01-31 22:54 UTC #1 lore raw

PATCH 1/6 debugfs: add debugfs_lookup()

From: Omar Sandoval <[email protected]>

We don't always have easy access to the dentry of a file or directory we created in debugfs. Add a helper which allows us to get a dentry we previously created.

The motivation for this change is a problem with blktrace and the blk-mq debugfs entries introduced in 07e4fead45e6 ("blk-mq: create debugfs directory tree"). Namely, in some cases, the directory that blktrace needs to create may already exist, but in other cases, it may not. We _could_ rely on a bunch of implied knowledge to decide whether to create the directory or not, but it's much cleaner on our end to just look it up.

Cc Greg Kroah-Hartman <[email protected]>

Signed-off-by Omar Sandoval <[email protected]>

---
 fs/debugfs/inode.c      | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/debugfs.h |  8 ++++++++
 2 files changed, 44 insertions(+)

fs/debugfs/inode.c +36 -0

index f17fcf89e18e..7fb1732a3630 100644
@@ -248,6 +248,42 @@ static struct file_system_type debug_fs_type = {
 };
 MODULE_ALIAS_FS("debugfs");
 
+/**
+ * debugfs_lookup() - look up an existing debugfs file
+ * @name: a pointer to a string containing the name of the file to look up.
+ * @parent: a pointer to the parent dentry of the file.
+ *
+ * This function will return a pointer to a dentry if it succeeds.  If the file
+ * doesn't exist or an error occurs, %NULL will be returned.  The returned
+ * dentry must be passed to dput() when it is no longer needed.
+ *
+ * If debugfs is not enabled in the kernel, the value -%ENODEV will be
+ * returned.
+ */
+struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
+{
+	struct dentry *dentry;
+
+	if (IS_ERR(parent))
+		return NULL;
+
+	if (!parent)
+		parent = debugfs_mount->mnt_root;
+
+	inode_lock(d_inode(parent));
+	dentry = lookup_one_len(name, parent, strlen(name));
+	inode_unlock(d_inode(parent));
+
+	if (IS_ERR(dentry))
+		return NULL;
+	if (!d_really_is_positive(dentry)) {
+		dput(dentry);
+		return NULL;
+	}
+	return dentry;
+}
+EXPORT_SYMBOL_GPL(debugfs_lookup);
+
 static struct dentry *start_creating(const char *name, struct dentry *parent)
 {
 	struct dentry *dentry;

include/linux/debugfs.h +8 -0

index 014cc564d1c4..c0befcf41b58 100644
@@ -80,6 +80,8 @@ static const struct file_operations __fops = {				\
 
 #if defined(CONFIG_DEBUG_FS)
 
+struct dentry *debugfs_lookup(const char *name, struct dentry *parent);
+
 struct dentry *debugfs_create_file(const char *name, umode_t mode,
 				   struct dentry *parent, void *data,
 				   const struct file_operations *fops);

@@ -181,6 +183,12 @@ ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
  * want to duplicate the design decision mistakes of procfs and devfs again.
  */
 
+static inline struct dentry *debugfs_lookup(const char *name,
+					    struct dentry *parent)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline struct dentry *debugfs_create_file(const char *name, umode_t mode,
 					struct dentry *parent, void *data,
 					const struct file_operations *fops)

Signature · 1 line

2.11.0

Omar Sandoval

author

2017-01-31 22:54 UTC #2 lore raw

PATCH 2/6 block: fix debugfs config conditional in struct request_queue

In reply to Omar Sandoval

From: Omar Sandoval <[email protected]>

The debugfs dentries are only used for CONFIG_BLK_DEBUG_FS, so make them conditional on that instead of CONFIG_DEBUG_FS.

Signed-off-by Omar Sandoval <[email protected]>

---
 include/linux/blkdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/blkdev.h +1 -1

index 05675b1dfd20..0cd0066a3d48 100644
@@ -571,7 +571,7 @@ struct request_queue {
 	struct list_head	tag_set_list;
 	struct bio_set		*bio_split;
 
-#ifdef CONFIG_DEBUG_FS
+#ifdef CONFIG_BLK_DEBUG_FS
 	struct dentry		*debugfs_dir;
 	struct dentry		*mq_debugfs_dir;
 #endif

Signature · 1 line

2.11.0

Omar Sandoval

author

2017-01-31 23:01 UTC #3 lore raw

PATCH 3/6 blktrace: make do_blk_trace_setup() static

In reply to Omar Sandoval

From: Omar Sandoval <[email protected]>

This isn't used outside of blktrace.c anymore.

Fixes 62c2a7d969f3 ("block: push BKL into blktrace ioctls")

Signed-off-by Omar Sandoval <[email protected]>

---
 include/linux/blktrace_api.h | 4 ----
 kernel/trace/blktrace.c      | 6 +++---
 2 files changed, 3 insertions(+), 7 deletions(-)

include/linux/blktrace_api.h +0 -4

index e417f080219a..75458bc19a16 100644
@@ -30,9 +30,6 @@ struct blk_trace {
 
 extern int blk_trace_ioctl(struct block_device *, unsigned, char __user *);
 extern void blk_trace_shutdown(struct request_queue *);
-extern int do_blk_trace_setup(struct request_queue *q, char *name,
-			      dev_t dev, struct block_device *bdev,
-			      struct blk_user_trace_setup *buts);
 extern __printf(2, 3)
 void __trace_note_message(struct blk_trace *, const char *fmt, ...);

@@ -80,7 +77,6 @@ extern struct attribute_group blk_trace_attr_group;
 #else /* !CONFIG_BLK_DEV_IO_TRACE */
 # define blk_trace_ioctl(bdev, cmd, arg)		(-ENOTTY)
 # define blk_trace_shutdown(q)				do { } while (0)
-# define do_blk_trace_setup(q, name, dev, bdev, buts)	(-ENOTTY)
 # define blk_add_driver_data(q, rq, data, len)		do {} while (0)
 # define blk_trace_setup(q, name, dev, bdev, arg)	(-ENOTTY)
 # define blk_trace_startstop(q, start)			(-ENOTTY)

kernel/trace/blktrace.c +3 -3

index 95cecbf67f5c..83d3b5b58f53 100644
@@ -433,9 +433,9 @@ static void blk_trace_setup_lba(struct blk_trace *bt,
 /*
  * Setup everything required to start tracing
  */
-int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
-		       struct block_device *bdev,
-		       struct blk_user_trace_setup *buts)
+static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
+			      struct block_device *bdev,
+			      struct blk_user_trace_setup *buts)
 {
 	struct blk_trace *bt = NULL;
 	struct dentry *dir = NULL;

Signature · 1 line

2.11.0

Omar Sandoval

author

2017-01-31 22:54 UTC #4 lore raw

PATCH 4/6 block: use same block debugfs directory for blk-mq and blktrace

In reply to Omar Sandoval

From: Omar Sandoval <[email protected]>

When I added the blk-mq debugging information to debugfs, I didn't notice that blktrace also creates a "block" directory in debugfs. Make them use the same dentry, now created in the core block code. Based on a patch from Jens.

Signed-off-by Omar Sandoval <[email protected]>

---
 block/blk-core.c        |  9 +++++++++
 block/blk-mq-debugfs.c  | 12 +++---------
 block/blk-mq.c          |  2 --
 block/blk-mq.h          |  5 -----
 block/blk.h             |  4 ++++
 kernel/trace/blktrace.c | 18 +++++-------------
 6 files changed, 21 insertions(+), 29 deletions(-)

block/blk-core.c +9 -0

index 02833ce5664e..16d3b06c743c 100644
@@ -33,6 +33,7 @@
 #include <linux/ratelimit.h>
 #include <linux/pm_runtime.h>
 #include <linux/blk-cgroup.h>
+#include <linux/debugfs.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/block.h>

@@ -42,6 +43,10 @@
 #include "blk-mq-sched.h"
 #include "blk-wbt.h"
 
+#ifdef CONFIG_DEBUG_FS
+struct dentry *blk_debugfs_root;
+#endif
+
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_remap);
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_rq_remap);
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_complete);

@@ -3482,5 +3487,9 @@ int __init blk_dev_init(void)
 	blk_requestq_cachep = kmem_cache_create("request_queue",
 			sizeof(struct request_queue), 0, SLAB_PANIC, NULL);
 
+#ifdef CONFIG_DEBUG_FS
+	blk_debugfs_root = debugfs_create_dir("block", NULL);
+#endif
+
 	return 0;
 }

block/blk-mq-debugfs.c +3 -9

index 5cd2b435a9f5..0cd29b99068c 100644
@@ -19,6 +19,7 @@
 #include <linux/debugfs.h>
 
 #include <linux/blk-mq.h>
+#include "blk.h"
 #include "blk-mq.h"
 #include "blk-mq-tag.h"

@@ -28,8 +29,6 @@ struct blk_mq_debugfs_attr {
 	const struct file_operations *fops;
 };
 
-static struct dentry *block_debugfs_root;
-
 static int blk_mq_debugfs_seq_open(struct inode *inode, struct file *file,
 				   const struct seq_operations *ops)
 {

@@ -641,10 +640,10 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_ctx_attrs[] = {
 
 int blk_mq_debugfs_register(struct request_queue *q, const char *name)
 {
-	if (!block_debugfs_root)
+	if (!blk_debugfs_root)
 		return -ENOENT;
 
-	q->debugfs_dir = debugfs_create_dir(name, block_debugfs_root);
+	q->debugfs_dir = debugfs_create_dir(name, blk_debugfs_root);
 	if (!q->debugfs_dir)
 		goto err;

@@ -749,8 +748,3 @@ void blk_mq_debugfs_unregister_hctxs(struct request_queue *q)
 	debugfs_remove_recursive(q->mq_debugfs_dir);
 	q->mq_debugfs_dir = NULL;
 }
-
-void blk_mq_debugfs_init(void)
-{
-	block_debugfs_root = debugfs_create_dir("block", NULL);
-}

block/blk-mq.c +0 -2

index 60dac10228fe..ff70219c5afd 100644
@@ -2827,8 +2827,6 @@ void blk_mq_enable_hotplug(void)
 
 static int __init blk_mq_init(void)
 {
-	blk_mq_debugfs_init();
-
 	cpuhp_setup_state_multi(CPUHP_BLK_MQ_DEAD, "block/mq:dead", NULL,
 				blk_mq_hctx_notify_dead);

block/blk-mq.h +0 -5

index b52abd62b1b0..24b2256186f3 100644
@@ -85,16 +85,11 @@ extern void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx);
  * debugfs helpers
  */
 #ifdef CONFIG_BLK_DEBUG_FS
-void blk_mq_debugfs_init(void);
 int blk_mq_debugfs_register(struct request_queue *q, const char *name);
 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 void blk_mq_debugfs_init(void)
-{
-}
-
 static inline int blk_mq_debugfs_register(struct request_queue *q,
 					  const char *name)
 {

block/blk.h +4 -0

index 9a716b5925a4..22283b05fe33 100644
@@ -14,6 +14,10 @@
 /* Max future timer expiry for timeouts */
 #define BLK_MAX_TIMEOUT		(5 * HZ)
 
+#ifdef CONFIG_DEBUG_FS
+extern struct dentry *blk_debugfs_root;
+#endif
+
 struct blk_flush_queue {
 	unsigned int		flush_queue_delayed:1;
 	unsigned int		flush_pending_idx:1;

kernel/trace/blktrace.c +5 ~~-13~~

index 83d3b5b58f53..38052f625a0f 100644
@@ -28,6 +28,8 @@
 #include <linux/uaccess.h>
 #include <linux/list.h>
 
+#include "../../block/blk.h"
+
 #include <trace/events/block.h>
 
 #include "trace_output.h"

@@ -292,9 +294,6 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 	local_irq_restore(flags);
 }
 
-static struct dentry *blk_tree_root;
-static DEFINE_MUTEX(blk_tree_mutex);
-
 static void blk_trace_free(struct blk_trace *bt)
 {
 	debugfs_remove(bt->msg_file);

@@ -468,17 +467,10 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 
 	ret = -ENOENT;
 
-	mutex_lock(&blk_tree_mutex);
-	if (!blk_tree_root) {
-		blk_tree_root = debugfs_create_dir("block", NULL);
-		if (!blk_tree_root) {
-			mutex_unlock(&blk_tree_mutex);
-			goto err;
-		}
-	}
-	mutex_unlock(&blk_tree_mutex);
+	if (!blk_debugfs_root)
+		goto err;
 
-	dir = debugfs_create_dir(buts->name, blk_tree_root);
+	dir = debugfs_create_dir(buts->name, blk_debugfs_root);
 
 	if (!dir)
 		goto err;

Signature · 1 line

2.11.0

Omar Sandoval

author

2017-01-31 23:01 UTC #5 lore raw

PATCH 5/6 blk-mq: move debugfs_remove() of disk dir to blk_release_queue()

In reply to Omar Sandoval

From: Omar Sandoval <[email protected]>

This needs to happen after we tear down blktrace.

Signed-off-by Omar Sandoval <[email protected]>

---
 block/blk-mq-sysfs.c | 2 +-
 block/blk-sysfs.c    | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

block/blk-mq-sysfs.c +1 -1

index 308b3f4fc310..295e69670c39 100644
@@ -254,7 +254,7 @@ static void __blk_mq_unregister_dev(struct device *dev, struct request_queue *q)
 		kobject_put(&hctx->kobj);
 	}
 
-	blk_mq_debugfs_unregister(q);
+	blk_mq_debugfs_unregister_hctxs(q);
 
 	kobject_uevent(&q->mq_kobj, KOBJ_REMOVE);
 	kobject_del(&q->mq_kobj);

block/blk-sysfs.c +3 -0

index 1dbce057592d..6cb637648936 100644
@@ -821,6 +821,9 @@ static void blk_release_queue(struct kobject *kobj)
 
 	blk_trace_shutdown(q);
 
+	if (q->mq_ops)
+		blk_mq_debugfs_unregister(q);
+
 	if (q->bio_split)
 		bioset_free(q->bio_split);

Signature · 1 line

2.11.0

Omar Sandoval

author

2017-01-31 23:01 UTC #6 lore raw

PATCH 6/6 blktrace: use existing disk debugfs directory

In reply to Omar Sandoval

From: Omar Sandoval <[email protected]>

We may already have a directory to put the blktrace stuff in if

1. The disk uses blk-mq
2. CONFIG_BLK_DEBUG_FS is enabled
3. We are tracing the whole disk and not a partition

Instead of hardcoding this very specific case, let's use the new debugfs_lookup(). If the directory exists, we use it, otherwise we create one and clean it up later.

Fixes 07e4fead45e6 ("blk-mq: create debugfs directory tree")

Signed-off-by Omar Sandoval <[email protected]>

---
 kernel/trace/blktrace.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

kernel/trace/blktrace.c +8 -5

index 38052f625a0f..cc9d23692a35 100644
@@ -470,12 +470,12 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (!blk_debugfs_root)
 		goto err;
 
-	dir = debugfs_create_dir(buts->name, blk_debugfs_root);
-
+	dir = debugfs_lookup(buts->name, blk_debugfs_root);
+	if (!dir)
+		bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
 	if (!dir)
 		goto err;
 
-	bt->dir = dir;
 	bt->dev = dev;
 	atomic_set(&bt->dropped, 0);
 	INIT_LIST_HEAD(&bt->running_list);

@@ -517,9 +517,12 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (atomic_inc_return(&blk_probes_ref) == 1)
 		blk_register_tracepoints();
 
-	return 0;
+	ret = 0;
 err:
-	blk_trace_free(bt);
+	if (dir && !bt->dir)
+		dput(dir);
+	if (ret)
+		blk_trace_free(bt);
 	return ret;
 }

Signature · 1 line

2.11.0

Greg Kroah-Hartman

maintainer

2017-02-01 08:16 UTC #7 lore raw

In reply to Omar Sandoval

Quoted 10 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>
When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.
Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.

I can't find patch 6, you only cc:ed me on the first patch :(

Care to bounce them all to me?

thanks,

greg k-h

Omar Sandoval

author

2017-02-01 08:31 UTC #8 lore raw

Quoted 12 lines · Greg Kroah-Hartman

Quoted 10 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.
Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.

Quoted 7 lines

I can't find patch 6, you only cc:ed me on the first patch :(
Care to bounce them all to me?
thanks,
greg k-h

Gah, I forgot --cc-cover to git-send-email. The series is all here:
http://marc.info/?l=linux-block&r=1&b=201701&w=2. I can also send the patches directly to you if you prefer that.

Greg Kroah-Hartman

maintainer

2017-02-02 10:58 UTC #9 lore raw

Quoted 17 lines · Omar Sandoval

Quoted 16 lines · Greg Kroah-Hartman
Quoted 10 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.
Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.
I can't find patch 6, you only cc:ed me on the first patch :(
Care to bounce them all to me?

Quoted 7 lines

Quoted 3 lines
thanks,
greg k-h
Gah, I forgot --cc-cover to git-send-email. The series is all here:
http://marc.info/?l=linux-block&r=1&b=201701&w=2. I can also send the patches directly to you if you prefer that.

I don't understand the problem here. How do you not know if you have created the debugfs file or not? You have the structure, with the correct name, how could it have been created? Can't you save the dentry to the debugfs file in the structure that has the name?

thanks,

greg k-h

Jens Axboe

2017-02-02 15:17 UTC #10 lore raw

Quoted 22 lines · Greg Kroah-Hartman

Quoted 21 lines · Omar Sandoval
Quoted 19 lines · Greg Kroah-Hartman
Quoted 10 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.
Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.
I can't find patch 6, you only cc:ed me on the first patch :(
Care to bounce them all to me?
thanks,
greg k-h

Quoted 8 lines

Quoted 3 lines
Gah, I forgot --cc-cover to git-send-email. The series is all here:
http://marc.info/?l=linux-block&r=1&b=201701&w=2. I can also send the patches directly to you if you prefer that.
I don't understand the problem here. How do you not know if you have created the debugfs file or not? You have the structure, with the correct name, how could it have been created? Can't you save the dentry to the debugfs file in the structure that has the name?

The problem is that blktrace registers a trace name directory, which can be either whole device or partition, depending on what you trace. For the blk-mq debug parts, we always just register the whole device name. There's no way to save the partition dentry, and imho, why even would you when you can just look it up. It's a file system...

Signature · 1 line

Jens Axboe

Greg Kroah-Hartman

maintainer Acked-by

2017-02-02 17:18 UTC #11 lore raw

Quoted 32 lines · Jens Axboe

Quoted 22 lines · Greg Kroah-Hartman
Quoted 21 lines · Omar Sandoval
Quoted 19 lines · Greg Kroah-Hartman
Quoted 10 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.
Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.
I can't find patch 6, you only cc:ed me on the first patch :(
Care to bounce them all to me?
thanks,
greg k-h
Quoted 8 lines
Quoted 3 lines
Gah, I forgot --cc-cover to git-send-email. The series is all here:
http://marc.info/?l=linux-block&r=1&b=201701&w=2. I can also send the patches directly to you if you prefer that.
I don't understand the problem here. How do you not know if you have created the debugfs file or not? You have the structure, with the correct name, how could it have been created? Can't you save the dentry to the debugfs file in the structure that has the name?

Quoted 5 lines

The problem is that blktrace registers a trace name directory, which can be either whole device or partition, depending on what you trace. For the blk-mq debug parts, we always just register the whole device name. There's no way to save the partition dentry, and imho, why even would you when you can just look it up. It's a file system...

I agree, it is a file system, but usually that debugfs file is associated with some sort of data you want to keep track of outside of a filesystem :)

Anyway, if it's such a big pain, then it's fine, add the function, no objection from me anymore.

Acked-by Greg Kroah-Hartman <[email protected]>

Omar Sandoval

author

2017-02-02 17:02 UTC #12 lore raw

In reply to Greg Kroah-Hartman

Quoted 26 lines · Greg Kroah-Hartman

Quoted 17 lines · Omar Sandoval
Quoted 16 lines · Greg Kroah-Hartman
Quoted 10 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.
Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.
I can't find patch 6, you only cc:ed me on the first patch :(
Care to bounce them all to me?
Quoted 7 lines
Quoted 3 lines
thanks,
greg k-h
Gah, I forgot --cc-cover to git-send-email. The series is all here:
http://marc.info/?l=linux-block&r=1&b=201701&w=2. I can also send the patches directly to you if you prefer that.

Quoted 8 lines

I don't understand the problem here. How do you not know if you have created the debugfs file or not? You have the structure, with the correct name, how could it have been created? Can't you save the dentry to the debugfs file in the structure that has the name?
thanks,
greg k-h

Hey, Greg,

So here's the alternative to doing the lookup:

kernel/trace/blktrace.c +6 -3

index 38052f625a0f..79ef6b9d1f96 100644
@@ -470,12 +470,15 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (!blk_debugfs_root)
 		goto err;
 
-	dir = debugfs_create_dir(buts->name, blk_debugfs_root);
-
+#ifdef CONFIG_BLK_DEBUG_FS
+	if (q->mq_ops && !bdev->bd_part.partno)
+		dir = q->debugfs_dir;
+#endif
+	if (!dir)
+		bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
 	if (!dir)
 		goto err;
 
-	bt->dir = dir;
 	bt->dev = dev;
 	atomic_set(&bt->dropped, 0);
 	INIT_LIST_HEAD(&bt->running_list);

So we could figure out if it exists, but it's very special-cased and fragile. And then there's this further up:

	/*
	 * some device names have larger paths - convert the slashes
	 * to underscores for this to work as expected
	 */
	strreplace(buts->name, '/', '_');

which I'm not sure applies anymore but would also break this. Doing the lookup is just more foolproof.

Thanks,
Omar

Greg Kroah-Hartman

maintainer

2017-02-02 17:18 UTC #13 lore raw

Quoted 45 lines · Omar Sandoval

Quoted 26 lines · Greg Kroah-Hartman
Quoted 17 lines · Omar Sandoval
Quoted 16 lines · Greg Kroah-Hartman
Quoted 10 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.
Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.
I can't find patch 6, you only cc:ed me on the first patch :(
Care to bounce them all to me?
Quoted 7 lines
Quoted 3 lines
thanks,
greg k-h
Gah, I forgot --cc-cover to git-send-email. The series is all here:
http://marc.info/?l=linux-block&r=1&b=201701&w=2. I can also send the patches directly to you if you prefer that.
Quoted 8 lines
I don't understand the problem here. How do you not know if you have created the debugfs file or not? You have the structure, with the correct name, how could it have been created? Can't you save the dentry to the debugfs file in the structure that has the name?
thanks,
greg k-h
Hey, Greg,
So here's the alternative to doing the lookup:
kernel/trace/blktrace.c +0 -0
index 38052f625a0f..79ef6b9d1f96 100644

kernel/trace/blktrace.c static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, · line 476

Quoted 9 lines

kernel/trace/blktrace.c +4 -2

@@ -470,12 +470,15 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (!blk_debugfs_root)
 		goto err;
 
-	dir = debugfs_create_dir(buts->name, blk_debugfs_root);
-
+#ifdef CONFIG_BLK_DEBUG_FS
+	if (q->mq_ops && !bdev->bd_part.partno)
+		dir = q->debugfs_dir;
+#endif

Eeek, no #ifdefs please :)

The lookup patch is fine, please take it.

thanks,

greg k-h

Jens Axboe

2017-02-02 17:20 UTC #14 lore raw

In reply to Omar Sandoval

Quoted 16 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>
When I moved the blk-mq debugging information to debugfs, I didn't realize that blktrace also created directories in debugfs that conflicted with the blk-mq directories. This series fixes that.
Patch 1 adds a new debugfs helper needed for patch 6. Greg, could I get an ack on that if it makes sense? Jens and I went back and forth on this for a little while, but patch 6 has more of the rationale on why we decided that this approach was the cleanest.
Patches 2 and 3 are cleanups.
Patch 4 is the first part of the fix, making blk-mq and blktrace use the same top-level "block" directory.

Quoted 7 lines

Patches 5 and 6 make blk-mq and blktrace play nicely with each other w.r.t. the debugfs "block/$dev" directories.
I tested this with multiple configurations, so hopefully I didn't mess that up this time.
Applies to for-4.11/block.

Applied with Greg's ack on the first patch, thanks Omar.

Signature · 1 line

Jens Axboe