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

Commit Message

Omar Sandoval Feb. 2, 2017, 5:01 p.m. UTC

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 <[e-mail]>
> > > > 
> > > > 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:



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
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Greg Kroah-Hartman Feb. 2, 2017, 5:18 p.m. UTC | #1

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 <[e-mail]>
> > > > > 
> > > > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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