[PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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 <gregkh@linuxfoundation.org>

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

-- 
2.11.0

[PATCH 1/6] debugfs: add debugfs_lookup()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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 <gregkh@linuxfoundation.org>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/debugfs/inode.c      | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/debugfs.h |  8 ++++++++
 2 files changed, 44 insertions(+)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f17fcf89e18e..7fb1732a3630 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -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;
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 014cc564d1c4..c0befcf41b58 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -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)
-- 
2.11.0

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

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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 <osandov@fb.com>
---
 include/linux/blkdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 05675b1dfd20..0cd0066a3d48 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -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
-- 
2.11.0

[PATCH 3/6] blktrace: make do_blk_trace_setup() static

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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

Fixes: 62c2a7d969f3 ("block: push BKL into blktrace ioctls")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 include/linux/blktrace_api.h | 4 ----
 kernel/trace/blktrace.c      | 6 +++---
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index e417f080219a..75458bc19a16 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -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)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 95cecbf67f5c..83d3b5b58f53 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -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;
-- 
2.11.0

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

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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 <osandov@fb.com>
---
 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(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 02833ce5664e..16d3b06c743c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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;
 }
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 5cd2b435a9f5..0cd29b99068c 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -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);
-}
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 60dac10228fe..ff70219c5afd 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -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);
 
diff --git a/block/blk-mq.h b/block/blk-mq.h
index b52abd62b1b0..24b2256186f3 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -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)
 {
diff --git a/block/blk.h b/block/blk.h
index 9a716b5925a4..22283b05fe33 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -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;
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 83d3b5b58f53..38052f625a0f 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -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;
-- 
2.11.0

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

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

This needs to happen after we tear down blktrace.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 block/blk-mq-sysfs.c | 2 +-
 block/blk-sysfs.c    | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index 308b3f4fc310..295e69670c39 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -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);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 1dbce057592d..6cb637648936 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -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);
 
-- 
2.11.0

[PATCH 6/6] blktrace: use existing disk debugfs directory

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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 <osandov@fb.com>
---
 kernel/trace/blktrace.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 38052f625a0f..cc9d23692a35 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -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;
 }
 
-- 
2.11.0

Re: [PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Greg Kroah-Hartman]

On Tue, Jan 31, 2017 at 02:53:16PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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

Re: [PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Omar Sandoval]

On Wed, Feb 01, 2017 at 09:16:08AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 31, 2017 at 02:53:16PM -0800, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > 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

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.

Re: [PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Greg Kroah-Hartman]

On Wed, Feb 01, 2017 at 12:31:15AM -0800, Omar Sandoval wrote:
> On Wed, Feb 01, 2017 at 09:16:08AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 31, 2017 at 02:53:16PM -0800, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > > 
> > > 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
> 
> 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

Re: [PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Jens Axboe]

On 02/02/2017 03:58 AM, Greg Kroah-Hartman wrote:
> On Wed, Feb 01, 2017 at 12:31:15AM -0800, Omar Sandoval wrote:
>> On Wed, Feb 01, 2017 at 09:16:08AM +0100, Greg Kroah-Hartman wrote:
>>> On Tue, Jan 31, 2017 at 02:53:16PM -0800, Omar Sandoval wrote:
>>>> From: Omar Sandoval <osandov@fb.com>
>>>>
>>>> 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
>>
>> 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...

-- 
Jens Axboe

Re: [PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Omar Sandoval]

On Thu, Feb 02, 2017 at 11:58:53AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 01, 2017 at 12:31:15AM -0800, Omar Sandoval wrote:
> > On Wed, Feb 01, 2017 at 09:16:08AM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Jan 31, 2017 at 02:53:16PM -0800, Omar Sandoval wrote:
> > > > From: Omar Sandoval <osandov@fb.com>
> > > > 
> > > > 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
> > 
> > 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

Hey, Greg,

So here's the alternative to doing the lookup:

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 38052f625a0f..79ef6b9d1f96 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -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

Re: [PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Greg Kroah-Hartman]

On Thu, Feb 02, 2017 at 08:17:31AM -0700, Jens Axboe wrote:
> On 02/02/2017 03:58 AM, Greg Kroah-Hartman wrote:
> > On Wed, Feb 01, 2017 at 12:31:15AM -0800, Omar Sandoval wrote:
> >> On Wed, Feb 01, 2017 at 09:16:08AM +0100, Greg Kroah-Hartman wrote:
> >>> On Tue, Jan 31, 2017 at 02:53:16PM -0800, Omar Sandoval wrote:
> >>>> From: Omar Sandoval <osandov@fb.com>
> >>>>
> >>>> 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
> >>
> >> 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...

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 <gregkh@linuxfoundation.org>

Re: [PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Greg Kroah-Hartman]

On Thu, Feb 02, 2017 at 09:01:45AM -0800, Omar Sandoval wrote:
> On Thu, Feb 02, 2017 at 11:58:53AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Feb 01, 2017 at 12:31:15AM -0800, Omar Sandoval wrote:
> > > On Wed, Feb 01, 2017 at 09:16:08AM +0100, Greg Kroah-Hartman wrote:
> > > > On Tue, Jan 31, 2017 at 02:53:16PM -0800, Omar Sandoval wrote:
> > > > > From: Omar Sandoval <osandov@fb.com>
> > > > > 
> > > > > 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
> > > 
> > > 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
> 
> Hey, Greg,
> 
> So here's the alternative to doing the lookup:
> 
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 38052f625a0f..79ef6b9d1f96 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -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

Re: [PATCH 0/6] block: fix blk-mq debugfs vs. blktrace

[Jens Axboe]

On 01/31/2017 03:53 PM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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.

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

-- 
Jens Axboe