[PATCH V2 0/2] block: fix "Directory XXXXX with parent 'block' already present!"

[PATCH V2 0/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

Hello,

The 1st patch fixes declaration of debugfs_rename for avoiding warning
caused by the 2nd patch.

The 2nd patch fixes warning of "Directory XXXXX with parent 'block'
already present!"

Ming Lei (2):
  debugfs: fix declaration of debugfs_rename
  block: fix "Directory XXXXX with parent 'block' already present!"

 block/blk-core.c        | 4 ++++
 block/blk-sysfs.c       | 4 ++--
 block/genhd.c           | 8 ++++++++
 include/linux/debugfs.h | 2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH V2 1/2] debugfs: fix declaration of debugfs_rename

[Ming Lei]

debugfs_rename() declaration isn't same between CONFIG_DEBUG_FS
and !CONFIG_DEBUG_FS, which causes the following warning in case
of !CONFIG_DEBUG_FS:

   include/linux/debugfs.h:252:47: note: expected 'char *' but argument is of type 'const char *'
     252 |                 struct dentry *new_dir, char *new_name)
         |                                         ~~~~~~^~~~~~~~

So fix declaration of debugfs_rename().

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/debugfs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index c869f1e73d75..bba0f514d97e 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -249,7 +249,7 @@ static inline ssize_t debugfs_attr_write(struct file *file,
 }
 
 static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
-                struct dentry *new_dir, char *new_name)
+                struct dentry *new_dir, const char *new_name)
 {
 	return ERR_PTR(-ENODEV);
 }
-- 
2.31.1

[PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
created when adding disk, and removed when releasing request queue.

There is small window between releasing disk and releasing request
queue, and during the period, one disk with same name may be created
and added, so debugfs_create_dir() may complain with "Directory XXXXX
with parent 'block' already present!"

Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
and the dir name is named with q->id from beginning, and switched to
disk name when adding disk, and finally changed to q->id in disk_release().

Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reported-by: Dan Williams <dan.j.williams@intel.com>
Cc: yukuai (C) <yukuai3@huawei.com>
Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c  | 4 ++++
 block/blk-sysfs.c | 4 ++--
 block/genhd.c     | 8 ++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index f305cb66c72a..245ec664753d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -438,6 +438,7 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
 {
 	struct request_queue *q;
 	int ret;
+	char q_name[16];
 
 	q = kmem_cache_alloc_node(blk_get_queue_kmem_cache(alloc_srcu),
 			GFP_KERNEL | __GFP_ZERO, node_id);
@@ -495,6 +496,9 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
 	blk_set_default_limits(&q->limits);
 	q->nr_requests = BLKDEV_DEFAULT_RQ;
 
+	sprintf(q_name, "%d", q->id);
+	q->debugfs_dir = debugfs_create_dir(q_name, blk_debugfs_root);
+
 	return q;
 
 fail_stats:
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 88bd41d4cb59..1f986c20a07b 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -837,8 +837,8 @@ int blk_register_queue(struct gendisk *disk)
 	}
 
 	mutex_lock(&q->debugfs_mutex);
-	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
-					    blk_debugfs_root);
+	q->debugfs_dir = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
+			blk_debugfs_root, kobject_name(q->kobj.parent));
 	mutex_unlock(&q->debugfs_mutex);
 
 	if (queue_is_mq(q)) {
diff --git a/block/genhd.c b/block/genhd.c
index 36532b931841..08895f9f7087 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -25,6 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/badblocks.h>
 #include <linux/part_stat.h>
+#include <linux/debugfs.h>
 #include "blk-throttle.h"
 
 #include "blk.h"
@@ -1160,6 +1161,7 @@ static void disk_release_mq(struct request_queue *q)
 static void disk_release(struct device *dev)
 {
 	struct gendisk *disk = dev_to_disk(dev);
+	char q_name[16];
 
 	might_sleep();
 	WARN_ON_ONCE(disk_live(disk));
@@ -1173,6 +1175,12 @@ static void disk_release(struct device *dev)
 	kfree(disk->random);
 	xa_destroy(&disk->part_tbl);
 
+	mutex_lock(&disk->queue->debugfs_mutex);
+	sprintf(q_name, "%d", disk->queue->id);
+	disk->queue->debugfs_dir = debugfs_rename(blk_debugfs_root,
+			disk->queue->debugfs_dir, blk_debugfs_root, q_name);
+	mutex_unlock(&disk->queue->debugfs_mutex);
+
 	disk->queue->disk = NULL;
 	blk_put_queue(disk->queue);
 
-- 
2.31.1

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Christoph Hellwig]

On Sat, Apr 23, 2022 at 10:39:52PM +0800, Ming Lei wrote:
> q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> created when adding disk, and removed when releasing request queue.
> 
> There is small window between releasing disk and releasing request
> queue, and during the period, one disk with same name may be created
> and added, so debugfs_create_dir() may complain with "Directory XXXXX
> with parent 'block' already present!"
> 
> Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> and the dir name is named with q->id from beginning, and switched to
> disk name when adding disk, and finally changed to q->id in disk_release().

As said before I very much think this is going in the wrong direction.

As the debugfs directory use the name of the gendisk, the lifetime rules
should simply match those of the gendisk.  If anyone wants to trace
SCSI commands sent before probing the gendisk or after removing it
they can use blktrace on the /dev/sg node.

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

On Sat, Apr 23, 2022 at 06:29:37PM +0200, Christoph Hellwig wrote:
> On Sat, Apr 23, 2022 at 10:39:52PM +0800, Ming Lei wrote:
> > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > created when adding disk, and removed when releasing request queue.
> > 
> > There is small window between releasing disk and releasing request
> > queue, and during the period, one disk with same name may be created
> > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > with parent 'block' already present!"
> > 
> > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > and the dir name is named with q->id from beginning, and switched to
> > disk name when adding disk, and finally changed to q->id in disk_release().
> 
> As said before I very much think this is going in the wrong direction.

So far I'd suggest to keep q->debugfs_dir inside request queue, another
goodness is that we can use it for exposing non-blk qeueue's debug info,
and there is request queue without gendisk attached.

> 
> As the debugfs directory use the name of the gendisk, the lifetime rules
> should simply match those of the gendisk.  If anyone wants to trace
> SCSI commands sent before probing the gendisk or after removing it
> they can use blktrace on the /dev/sg node.

Not sure blktrace can trace on /dev/sg since blktrace works on
block_device.


Thanks, 
Ming

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Christoph Hellwig]

On Sun, Apr 24, 2022 at 05:24:12PM +0800, Ming Lei wrote:
> > As the debugfs directory use the name of the gendisk, the lifetime rules
> > should simply match those of the gendisk.  If anyone wants to trace
> > SCSI commands sent before probing the gendisk or after removing it
> > they can use blktrace on the /dev/sg node.
> 
> Not sure blktrace can trace on /dev/sg since blktrace works on
> block_device.

Unless someone broke it recently it does.  Take a look at all the mess
it causes in the blktrace code.

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

On Mon, Apr 25, 2022 at 09:49:18AM +0200, Christoph Hellwig wrote:
> On Sun, Apr 24, 2022 at 05:24:12PM +0800, Ming Lei wrote:
> > > As the debugfs directory use the name of the gendisk, the lifetime rules
> > > should simply match those of the gendisk.  If anyone wants to trace
> > > SCSI commands sent before probing the gendisk or after removing it
> > > they can use blktrace on the /dev/sg node.
> > 
> > Not sure blktrace can trace on /dev/sg since blktrace works on
> > block_device.
> 
> Unless someone broke it recently it does.  Take a look at all the mess
> it causes in the blktrace code.

But /dev/sg and /dev/sdX can't be opened by blktrace at the same
time.

Thank,
Ming

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Hannes Reinecke]

On 4/23/22 16:39, Ming Lei wrote:
> q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> created when adding disk, and removed when releasing request queue.
> 
> There is small window between releasing disk and releasing request
> queue, and during the period, one disk with same name may be created
> and added, so debugfs_create_dir() may complain with "Directory XXXXX
> with parent 'block' already present!"
> 
> Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> and the dir name is named with q->id from beginning, and switched to
> disk name when adding disk, and finally changed to q->id in disk_release().
> 
> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Reported-by: Dan Williams <dan.j.williams@intel.com>
> Cc: yukuai (C) <yukuai3@huawei.com>
> Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-core.c  | 4 ++++
>   block/blk-sysfs.c | 4 ++--
>   block/genhd.c     | 8 ++++++++
>   3 files changed, 14 insertions(+), 2 deletions(-)
> 
Errm.

Isn't this superfluous now that Jens merged Yu Kuais patch?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
> On 4/23/22 16:39, Ming Lei wrote:
> > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > created when adding disk, and removed when releasing request queue.
> > 
> > There is small window between releasing disk and releasing request
> > queue, and during the period, one disk with same name may be created
> > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > with parent 'block' already present!"
> > 
> > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > and the dir name is named with q->id from beginning, and switched to
> > disk name when adding disk, and finally changed to q->id in disk_release().
> > 
> > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > Cc: yukuai (C) <yukuai3@huawei.com>
> > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   block/blk-core.c  | 4 ++++
> >   block/blk-sysfs.c | 4 ++--
> >   block/genhd.c     | 8 ++++++++
> >   3 files changed, 14 insertions(+), 2 deletions(-)
> > 
> Errm.
> 
> Isn't this superfluous now that Jens merged Yu Kuais patch?

Jens has dropped Yu Kuai's patch which caused kernel panic.


Thanks,
Ming

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Hannes Reinecke]

On 4/24/22 11:28, Ming Lei wrote:
> On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
>> On 4/23/22 16:39, Ming Lei wrote:
>>> q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
>>> created when adding disk, and removed when releasing request queue.
>>>
>>> There is small window between releasing disk and releasing request
>>> queue, and during the period, one disk with same name may be created
>>> and added, so debugfs_create_dir() may complain with "Directory XXXXX
>>> with parent 'block' already present!"
>>>
>>> Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
>>> and the dir name is named with q->id from beginning, and switched to
>>> disk name when adding disk, and finally changed to q->id in disk_release().
>>>
>>> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>> Reported-by: Dan Williams <dan.j.williams@intel.com>
>>> Cc: yukuai (C) <yukuai3@huawei.com>
>>> Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>    block/blk-core.c  | 4 ++++
>>>    block/blk-sysfs.c | 4 ++--
>>>    block/genhd.c     | 8 ++++++++
>>>    3 files changed, 14 insertions(+), 2 deletions(-)
>>>
>> Errm.
>>
>> Isn't this superfluous now that Jens merged Yu Kuais patch?
> 
> Jens has dropped Yu Kuai's patch which caused kernel panic.
> 
Right.
But still, this patch looks really odd.
How is userspace supposed to use the directories prior to the renaming?

And as you already have identified the places where we can safely create 
(and remove) the debugfs directories, why can't we move the call to 
create and remove the debugfs directories to those locations and do away 
with the renaming?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

On Sun, Apr 24, 2022 at 01:51:45PM +0200, Hannes Reinecke wrote:
> On 4/24/22 11:28, Ming Lei wrote:
> > On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
> > > On 4/23/22 16:39, Ming Lei wrote:
> > > > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > > > created when adding disk, and removed when releasing request queue.
> > > > 
> > > > There is small window between releasing disk and releasing request
> > > > queue, and during the period, one disk with same name may be created
> > > > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > > > with parent 'block' already present!"
> > > > 
> > > > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > > > and the dir name is named with q->id from beginning, and switched to
> > > > disk name when adding disk, and finally changed to q->id in disk_release().
> > > > 
> > > > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > > Cc: yukuai (C) <yukuai3@huawei.com>
> > > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >    block/blk-core.c  | 4 ++++
> > > >    block/blk-sysfs.c | 4 ++--
> > > >    block/genhd.c     | 8 ++++++++
> > > >    3 files changed, 14 insertions(+), 2 deletions(-)
> > > > 
> > > Errm.
> > > 
> > > Isn't this superfluous now that Jens merged Yu Kuais patch?
> > 
> > Jens has dropped Yu Kuai's patch which caused kernel panic.
> > 
> Right.
> But still, this patch looks really odd.
> How is userspace supposed to use the directories prior to the renaming?

That doesn't make any difference for current uses, but we may extend it
to support debugfs for non-blk request queue in future by exporting q->id
somewhere. Even though now the interested q->id can be figured out
easily by very simple ebpf trace prog.

> 
> And as you already have identified the places where we can safely create
> (and remove) the debugfs directories, why can't we move the call to create
> and remove the debugfs directories to those locations and do away with the
> renaming?

First it needs more change to fix the kernel panic.

Second removing debugfs dir in del_gendisk will break blktests block/002.

Finally this patch's approach is more flexible, since it provides chance to
export debugfs for request queue without disk attached, such as scsi queue
without disk attached, nvme admin queue, ...

Thanks,
Ming

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Greg Kroah-Hartman]

On Sun, Apr 24, 2022 at 08:04:59PM +0800, Ming Lei wrote:
> On Sun, Apr 24, 2022 at 01:51:45PM +0200, Hannes Reinecke wrote:
> > On 4/24/22 11:28, Ming Lei wrote:
> > > On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
> > > > On 4/23/22 16:39, Ming Lei wrote:
> > > > > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > > > > created when adding disk, and removed when releasing request queue.
> > > > > 
> > > > > There is small window between releasing disk and releasing request
> > > > > queue, and during the period, one disk with same name may be created
> > > > > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > > > > with parent 'block' already present!"
> > > > > 
> > > > > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > > > > and the dir name is named with q->id from beginning, and switched to
> > > > > disk name when adding disk, and finally changed to q->id in disk_release().
> > > > > 
> > > > > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > > > Cc: yukuai (C) <yukuai3@huawei.com>
> > > > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > > ---
> > > > >    block/blk-core.c  | 4 ++++
> > > > >    block/blk-sysfs.c | 4 ++--
> > > > >    block/genhd.c     | 8 ++++++++
> > > > >    3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > 
> > > > Errm.
> > > > 
> > > > Isn't this superfluous now that Jens merged Yu Kuais patch?
> > > 
> > > Jens has dropped Yu Kuai's patch which caused kernel panic.
> > > 
> > Right.
> > But still, this patch looks really odd.
> > How is userspace supposed to use the directories prior to the renaming?
> 
> That doesn't make any difference for current uses, but we may extend it
> to support debugfs for non-blk request queue in future by exporting q->id
> somewhere. Even though now the interested q->id can be figured out
> easily by very simple ebpf trace prog.
> 
> > 
> > And as you already have identified the places where we can safely create
> > (and remove) the debugfs directories, why can't we move the call to create
> > and remove the debugfs directories to those locations and do away with the
> > renaming?
> 
> First it needs more change to fix the kernel panic.
> 
> Second removing debugfs dir in del_gendisk will break blktests block/002.

Then fix the test?  debugfs interactions that cause kernel bugs should
be ok to change the functionality of.  Remember, this is for
debugging...

thanks,

greg k-h

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

On Sun, Apr 24, 2022 at 03:45:59PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Apr 24, 2022 at 08:04:59PM +0800, Ming Lei wrote:
> > On Sun, Apr 24, 2022 at 01:51:45PM +0200, Hannes Reinecke wrote:
> > > On 4/24/22 11:28, Ming Lei wrote:
> > > > On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
> > > > > On 4/23/22 16:39, Ming Lei wrote:
> > > > > > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > > > > > created when adding disk, and removed when releasing request queue.
> > > > > > 
> > > > > > There is small window between releasing disk and releasing request
> > > > > > queue, and during the period, one disk with same name may be created
> > > > > > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > > > > > with parent 'block' already present!"
> > > > > > 
> > > > > > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > > > > > and the dir name is named with q->id from beginning, and switched to
> > > > > > disk name when adding disk, and finally changed to q->id in disk_release().
> > > > > > 
> > > > > > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > Cc: yukuai (C) <yukuai3@huawei.com>
> > > > > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > > > ---
> > > > > >    block/blk-core.c  | 4 ++++
> > > > > >    block/blk-sysfs.c | 4 ++--
> > > > > >    block/genhd.c     | 8 ++++++++
> > > > > >    3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > Errm.
> > > > > 
> > > > > Isn't this superfluous now that Jens merged Yu Kuais patch?
> > > > 
> > > > Jens has dropped Yu Kuai's patch which caused kernel panic.
> > > > 
> > > Right.
> > > But still, this patch looks really odd.
> > > How is userspace supposed to use the directories prior to the renaming?
> > 
> > That doesn't make any difference for current uses, but we may extend it
> > to support debugfs for non-blk request queue in future by exporting q->id
> > somewhere. Even though now the interested q->id can be figured out
> > easily by very simple ebpf trace prog.
> > 
> > > 
> > > And as you already have identified the places where we can safely create
> > > (and remove) the debugfs directories, why can't we move the call to create
> > > and remove the debugfs directories to those locations and do away with the
> > > renaming?
> > 
> > First it needs more change to fix the kernel panic.
> > 
> > Second removing debugfs dir in del_gendisk will break blktests block/002.
> 
> Then fix the test?  debugfs interactions that cause kernel bugs should
> be ok to change the functionality of.  Remember, this is for
> debugging...

But what is wrong with the test? Isn't it reasonable to keep debugfs dir
when blktrace is collecting log? After debugfs dir is removed, blktrace
may not collect intact log, and people may complain it is one kernel
regression.


Thanks, 
Ming

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Greg Kroah-Hartman]

On Mon, Apr 25, 2022 at 09:28:27AM +0800, Ming Lei wrote:
> On Sun, Apr 24, 2022 at 03:45:59PM +0200, Greg Kroah-Hartman wrote:
> > On Sun, Apr 24, 2022 at 08:04:59PM +0800, Ming Lei wrote:
> > > On Sun, Apr 24, 2022 at 01:51:45PM +0200, Hannes Reinecke wrote:
> > > > On 4/24/22 11:28, Ming Lei wrote:
> > > > > On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
> > > > > > On 4/23/22 16:39, Ming Lei wrote:
> > > > > > > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > > > > > > created when adding disk, and removed when releasing request queue.
> > > > > > > 
> > > > > > > There is small window between releasing disk and releasing request
> > > > > > > queue, and during the period, one disk with same name may be created
> > > > > > > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > > > > > > with parent 'block' already present!"
> > > > > > > 
> > > > > > > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > > > > > > and the dir name is named with q->id from beginning, and switched to
> > > > > > > disk name when adding disk, and finally changed to q->id in disk_release().
> > > > > > > 
> > > > > > > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > > Cc: yukuai (C) <yukuai3@huawei.com>
> > > > > > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > > > > ---
> > > > > > >    block/blk-core.c  | 4 ++++
> > > > > > >    block/blk-sysfs.c | 4 ++--
> > > > > > >    block/genhd.c     | 8 ++++++++
> > > > > > >    3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > Errm.
> > > > > > 
> > > > > > Isn't this superfluous now that Jens merged Yu Kuais patch?
> > > > > 
> > > > > Jens has dropped Yu Kuai's patch which caused kernel panic.
> > > > > 
> > > > Right.
> > > > But still, this patch looks really odd.
> > > > How is userspace supposed to use the directories prior to the renaming?
> > > 
> > > That doesn't make any difference for current uses, but we may extend it
> > > to support debugfs for non-blk request queue in future by exporting q->id
> > > somewhere. Even though now the interested q->id can be figured out
> > > easily by very simple ebpf trace prog.
> > > 
> > > > 
> > > > And as you already have identified the places where we can safely create
> > > > (and remove) the debugfs directories, why can't we move the call to create
> > > > and remove the debugfs directories to those locations and do away with the
> > > > renaming?
> > > 
> > > First it needs more change to fix the kernel panic.
> > > 
> > > Second removing debugfs dir in del_gendisk will break blktests block/002.
> > 
> > Then fix the test?  debugfs interactions that cause kernel bugs should
> > be ok to change the functionality of.  Remember, this is for
> > debugging...
> 
> But what is wrong with the test? Isn't it reasonable to keep debugfs dir
> when blktrace is collecting log?

How can you collect something from a device that is gone?

> After debugfs dir is removed, blktrace may not collect intact log, and
> people may complain it is one kernel regression.

What exactly breaks?  The device is removed, why should a trace continue
to give you data?

thanks,

greg k-h

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Christoph Hellwig]

On Mon, Apr 25, 2022 at 07:10:46AM +0200, Greg Kroah-Hartman wrote:
> > But what is wrong with the test? Isn't it reasonable to keep debugfs dir
> > when blktrace is collecting log?
> 
> How can you collect something from a device that is gone?
> 
> > After debugfs dir is removed, blktrace may not collect intact log, and
> > people may complain it is one kernel regression.
> 
> What exactly breaks?  The device is removed, why should a trace continue
> to give you data?

This is a good question.  All but one of the block device drivers
really only have a concept of a block "queue" that is attached to a
live block device.  In that case the awnser is simple and obvious.

But SCSI allocates these queues before the block device, and they can
outlive it, because SCSI is a layered architecture where the "upper level"
drivers like sd and st are only bound to the queue based on information
returned from it, and the queue can outlive unbinding these drivers
(which is a bit pointless but possible due to full device model
integration).

So there might be some uses cases to keep on tracing.  I don't think they
are very valid, though, because if you really want to trace that raw
queue you can do it using the /dev/sg node.

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Hannes Reinecke]

On 4/25/22 09:48, Christoph Hellwig wrote:
> On Mon, Apr 25, 2022 at 07:10:46AM +0200, Greg Kroah-Hartman wrote:
>>> But what is wrong with the test? Isn't it reasonable to keep debugfs dir
>>> when blktrace is collecting log?
>>
>> How can you collect something from a device that is gone?
>>
>>> After debugfs dir is removed, blktrace may not collect intact log, and
>>> people may complain it is one kernel regression.
>>
>> What exactly breaks?  The device is removed, why should a trace continue
>> to give you data?
> 
> This is a good question.  All but one of the block device drivers
> really only have a concept of a block "queue" that is attached to a
> live block device.  In that case the awnser is simple and obvious.
> 
> But SCSI allocates these queues before the block device, and they can
> outlive it, because SCSI is a layered architecture where the "upper level"
> drivers like sd and st are only bound to the queue based on information
> returned from it, and the queue can outlive unbinding these drivers
> (which is a bit pointless but possible due to full device model
> integration).
> 
> So there might be some uses cases to keep on tracing.  I don't think they
> are very valid, though, because if you really want to trace that raw
> queue you can do it using the /dev/sg node.
> 
Which is thinking, too.
While it might be that some I/O can arrive during shutdown, it is 
_quite_ questionable whether one may want to trace it.
And if so whether blktrace/debugfs is the correct way to do it, as it's 
certainly not performance critical, and there are other things at play 
during shutdown having a much larger impact on the overall timing (rcu 
grace periods, lock contention, you name it).

So I'd say we should go for least complexity here, and allow tracing 
only if the device is in a sane state.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

On Mon, Apr 25, 2022 at 07:10:46AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 25, 2022 at 09:28:27AM +0800, Ming Lei wrote:
> > On Sun, Apr 24, 2022 at 03:45:59PM +0200, Greg Kroah-Hartman wrote:
> > > On Sun, Apr 24, 2022 at 08:04:59PM +0800, Ming Lei wrote:
> > > > On Sun, Apr 24, 2022 at 01:51:45PM +0200, Hannes Reinecke wrote:
> > > > > On 4/24/22 11:28, Ming Lei wrote:
> > > > > > On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
> > > > > > > On 4/23/22 16:39, Ming Lei wrote:
> > > > > > > > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > > > > > > > created when adding disk, and removed when releasing request queue.
> > > > > > > > 
> > > > > > > > There is small window between releasing disk and releasing request
> > > > > > > > queue, and during the period, one disk with same name may be created
> > > > > > > > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > > > > > > > with parent 'block' already present!"
> > > > > > > > 
> > > > > > > > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > > > > > > > and the dir name is named with q->id from beginning, and switched to
> > > > > > > > disk name when adding disk, and finally changed to q->id in disk_release().
> > > > > > > > 
> > > > > > > > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > > > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > > > Cc: yukuai (C) <yukuai3@huawei.com>
> > > > > > > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > > > > > ---
> > > > > > > >    block/blk-core.c  | 4 ++++
> > > > > > > >    block/blk-sysfs.c | 4 ++--
> > > > > > > >    block/genhd.c     | 8 ++++++++
> > > > > > > >    3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > Errm.
> > > > > > > 
> > > > > > > Isn't this superfluous now that Jens merged Yu Kuais patch?
> > > > > > 
> > > > > > Jens has dropped Yu Kuai's patch which caused kernel panic.
> > > > > > 
> > > > > Right.
> > > > > But still, this patch looks really odd.
> > > > > How is userspace supposed to use the directories prior to the renaming?
> > > > 
> > > > That doesn't make any difference for current uses, but we may extend it
> > > > to support debugfs for non-blk request queue in future by exporting q->id
> > > > somewhere. Even though now the interested q->id can be figured out
> > > > easily by very simple ebpf trace prog.
> > > > 
> > > > > 
> > > > > And as you already have identified the places where we can safely create
> > > > > (and remove) the debugfs directories, why can't we move the call to create
> > > > > and remove the debugfs directories to those locations and do away with the
> > > > > renaming?
> > > > 
> > > > First it needs more change to fix the kernel panic.
> > > > 
> > > > Second removing debugfs dir in del_gendisk will break blktests block/002.
> > > 
> > > Then fix the test?  debugfs interactions that cause kernel bugs should
> > > be ok to change the functionality of.  Remember, this is for
> > > debugging...
> > 
> > But what is wrong with the test? Isn't it reasonable to keep debugfs dir
> > when blktrace is collecting log?
> 
> How can you collect something from a device that is gone?

Here the 'gone' may be just in logical/soft viewpoint, such as, one disk
is removed by sysfs, and the driver still may send sync cache command
to make sure the cache inside drive is flushed, such as scsi's
SYNCHRONIZE_CACHE.

> 
> > After debugfs dir is removed, blktrace may not collect intact log, and
> > people may complain it is one kernel regression.
> 
> What exactly breaks?  The device is removed, why should a trace continue
> to give you data?

Such as the above example, the command of SYNCHRONIZE_CACHE can't be
observed in blktrace any more.


Thanks,
Ming

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Hannes Reinecke]

On 4/25/22 11:07, Ming Lei wrote:
> On Mon, Apr 25, 2022 at 07:10:46AM +0200, Greg Kroah-Hartman wrote:
>> On Mon, Apr 25, 2022 at 09:28:27AM +0800, Ming Lei wrote:
>>> On Sun, Apr 24, 2022 at 03:45:59PM +0200, Greg Kroah-Hartman wrote:
>>>> On Sun, Apr 24, 2022 at 08:04:59PM +0800, Ming Lei wrote:
>>>>> On Sun, Apr 24, 2022 at 01:51:45PM +0200, Hannes Reinecke wrote:
>>>>>> On 4/24/22 11:28, Ming Lei wrote:
>>>>>>> On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
>>>>>>>> On 4/23/22 16:39, Ming Lei wrote:
>>>>>>>>> q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
>>>>>>>>> created when adding disk, and removed when releasing request queue.
>>>>>>>>>
>>>>>>>>> There is small window between releasing disk and releasing request
>>>>>>>>> queue, and during the period, one disk with same name may be created
>>>>>>>>> and added, so debugfs_create_dir() may complain with "Directory XXXXX
>>>>>>>>> with parent 'block' already present!"
>>>>>>>>>
>>>>>>>>> Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
>>>>>>>>> and the dir name is named with q->id from beginning, and switched to
>>>>>>>>> disk name when adding disk, and finally changed to q->id in disk_release().
>>>>>>>>>
>>>>>>>>> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>>>>>>>> Reported-by: Dan Williams <dan.j.williams@intel.com>
>>>>>>>>> Cc: yukuai (C) <yukuai3@huawei.com>
>>>>>>>>> Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>>>>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>>>>>> ---
>>>>>>>>>     block/blk-core.c  | 4 ++++
>>>>>>>>>     block/blk-sysfs.c | 4 ++--
>>>>>>>>>     block/genhd.c     | 8 ++++++++
>>>>>>>>>     3 files changed, 14 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>> Errm.
>>>>>>>>
>>>>>>>> Isn't this superfluous now that Jens merged Yu Kuais patch?
>>>>>>>
>>>>>>> Jens has dropped Yu Kuai's patch which caused kernel panic.
>>>>>>>
>>>>>> Right.
>>>>>> But still, this patch looks really odd.
>>>>>> How is userspace supposed to use the directories prior to the renaming?
>>>>>
>>>>> That doesn't make any difference for current uses, but we may extend it
>>>>> to support debugfs for non-blk request queue in future by exporting q->id
>>>>> somewhere. Even though now the interested q->id can be figured out
>>>>> easily by very simple ebpf trace prog.
>>>>>
>>>>>>
>>>>>> And as you already have identified the places where we can safely create
>>>>>> (and remove) the debugfs directories, why can't we move the call to create
>>>>>> and remove the debugfs directories to those locations and do away with the
>>>>>> renaming?
>>>>>
>>>>> First it needs more change to fix the kernel panic.
>>>>>
>>>>> Second removing debugfs dir in del_gendisk will break blktests block/002.
>>>>
>>>> Then fix the test?  debugfs interactions that cause kernel bugs should
>>>> be ok to change the functionality of.  Remember, this is for
>>>> debugging...
>>>
>>> But what is wrong with the test? Isn't it reasonable to keep debugfs dir
>>> when blktrace is collecting log?
>>
>> How can you collect something from a device that is gone?
> 
> Here the 'gone' may be just in logical/soft viewpoint, such as, one disk
> is removed by sysfs, and the driver still may send sync cache command
> to make sure the cache inside drive is flushed, such as scsi's
> SYNCHRONIZE_CACHE.
> 
And that is my argument: what does this buy us?
Is is relevant (for blktrace) to have the SYNCHRONIZE_CACHE to be 
present in the logs?
 From my POV, blktrace is there to analyze I/O flow; device shutdown is 
not really relevant for that as the results of that operation depend on 
other factors which won't show up in blktrace at all.

So we're not losing much by (maybe) missing shutdown commands in 
blktrace; if needs be device shutdown can be traced by other means.

I'd rather keep the code simple, and not having an operation in the core 
block layer which requires quite some explanation.
_And_ relies on the current ordering; if things change here it'll be 
really hard to figure out if that workaround is still required or might 
be obsoleted by the change.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Ming Lei]

On Mon, Apr 25, 2022 at 11:32:15AM +0200, Hannes Reinecke wrote:
> On 4/25/22 11:07, Ming Lei wrote:
> > On Mon, Apr 25, 2022 at 07:10:46AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Apr 25, 2022 at 09:28:27AM +0800, Ming Lei wrote:
> > > > On Sun, Apr 24, 2022 at 03:45:59PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Sun, Apr 24, 2022 at 08:04:59PM +0800, Ming Lei wrote:
> > > > > > On Sun, Apr 24, 2022 at 01:51:45PM +0200, Hannes Reinecke wrote:
> > > > > > > On 4/24/22 11:28, Ming Lei wrote:
> > > > > > > > On Sun, Apr 24, 2022 at 10:53:29AM +0200, Hannes Reinecke wrote:
> > > > > > > > > On 4/23/22 16:39, Ming Lei wrote:
> > > > > > > > > > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > > > > > > > > > created when adding disk, and removed when releasing request queue.
> > > > > > > > > > 
> > > > > > > > > > There is small window between releasing disk and releasing request
> > > > > > > > > > queue, and during the period, one disk with same name may be created
> > > > > > > > > > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > > > > > > > > > with parent 'block' already present!"
> > > > > > > > > > 
> > > > > > > > > > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > > > > > > > > > and the dir name is named with q->id from beginning, and switched to
> > > > > > > > > > disk name when adding disk, and finally changed to q->id in disk_release().
> > > > > > > > > > 
> > > > > > > > > > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > > > > > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > > > > > Cc: yukuai (C) <yukuai3@huawei.com>
> > > > > > > > > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > > > > > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > > > > > > > ---
> > > > > > > > > >     block/blk-core.c  | 4 ++++
> > > > > > > > > >     block/blk-sysfs.c | 4 ++--
> > > > > > > > > >     block/genhd.c     | 8 ++++++++
> > > > > > > > > >     3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > > > > > > 
> > > > > > > > > Errm.
> > > > > > > > > 
> > > > > > > > > Isn't this superfluous now that Jens merged Yu Kuais patch?
> > > > > > > > 
> > > > > > > > Jens has dropped Yu Kuai's patch which caused kernel panic.
> > > > > > > > 
> > > > > > > Right.
> > > > > > > But still, this patch looks really odd.
> > > > > > > How is userspace supposed to use the directories prior to the renaming?
> > > > > > 
> > > > > > That doesn't make any difference for current uses, but we may extend it
> > > > > > to support debugfs for non-blk request queue in future by exporting q->id
> > > > > > somewhere. Even though now the interested q->id can be figured out
> > > > > > easily by very simple ebpf trace prog.
> > > > > > 
> > > > > > > 
> > > > > > > And as you already have identified the places where we can safely create
> > > > > > > (and remove) the debugfs directories, why can't we move the call to create
> > > > > > > and remove the debugfs directories to those locations and do away with the
> > > > > > > renaming?
> > > > > > 
> > > > > > First it needs more change to fix the kernel panic.
> > > > > > 
> > > > > > Second removing debugfs dir in del_gendisk will break blktests block/002.
> > > > > 
> > > > > Then fix the test?  debugfs interactions that cause kernel bugs should
> > > > > be ok to change the functionality of.  Remember, this is for
> > > > > debugging...
> > > > 
> > > > But what is wrong with the test? Isn't it reasonable to keep debugfs dir
> > > > when blktrace is collecting log?
> > > 
> > > How can you collect something from a device that is gone?
> > 
> > Here the 'gone' may be just in logical/soft viewpoint, such as, one disk
> > is removed by sysfs, and the driver still may send sync cache command
> > to make sure the cache inside drive is flushed, such as scsi's
> > SYNCHRONIZE_CACHE.
> > 
> And that is my argument: what does this buy us?

Isn't the posted patch simple enough for fixing the whole issue?

Not only in lines of code, but also in principle.

So far q->debugfs_dir is used by elevator, rq_qos, blktrace and blk-mq
debugfs.

The 1st three can have same lifetime with gendisk, but blk-mq debugfs
more share same lifetime with request_queue.

That is why I make ->debugfs_dir sharing same lifetime with request
queue since request queue has longer lifetime than gendisk.

With this way, we can clean the mess for delaying to add blk-mq debugfs.

Not mention this approach can allow us to add debugfs support for
non-disk request queue.

> Is is relevant (for blktrace) to have the SYNCHRONIZE_CACHE to be present in
> the logs?

SYNCHRONIZE_CACHE is just one example, and there can be more from
/dev/sg or kernel. As one user of trace tool, it is important to get
intact request trace.

> From my POV, blktrace is there to analyze I/O flow; device shutdown is not
> really relevant for that as the results of that operation depend on other
> factors which won't show up in blktrace at all.
> 
> So we're not losing much by (maybe) missing shutdown commands in blktrace;
> if needs be device shutdown can be traced by other means.
> 
> I'd rather keep the code simple, and not having an operation in the core
> block layer which requires quite some explanation.

Please write one workable patch following your idea, then compare yours
and this patch, then you will see which one is simpler.



Thanks,
Ming

Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!"

[Yu Kuai]

Hi, Ming

Is there aggrement on the solution? I really hope this problem can be
solved...

Thansk,
Kuai

在 2022/04/23 22:39, Ming Lei 写道:
> q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> created when adding disk, and removed when releasing request queue.
> 
> There is small window between releasing disk and releasing request
> queue, and during the period, one disk with same name may be created
> and added, so debugfs_create_dir() may complain with "Directory XXXXX
> with parent 'block' already present!"
> 
> Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> and the dir name is named with q->id from beginning, and switched to
> disk name when adding disk, and finally changed to q->id in disk_release().
> 
> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Reported-by: Dan Williams <dan.j.williams@intel.com>
> Cc: yukuai (C) <yukuai3@huawei.com>
> Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/blk-core.c  | 4 ++++
>   block/blk-sysfs.c | 4 ++--
>   block/genhd.c     | 8 ++++++++
>   3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index f305cb66c72a..245ec664753d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -438,6 +438,7 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
>   {
>   	struct request_queue *q;
>   	int ret;
> +	char q_name[16];
>   
>   	q = kmem_cache_alloc_node(blk_get_queue_kmem_cache(alloc_srcu),
>   			GFP_KERNEL | __GFP_ZERO, node_id);
> @@ -495,6 +496,9 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
>   	blk_set_default_limits(&q->limits);
>   	q->nr_requests = BLKDEV_DEFAULT_RQ;
>   
> +	sprintf(q_name, "%d", q->id);
> +	q->debugfs_dir = debugfs_create_dir(q_name, blk_debugfs_root);
> +
>   	return q;
>   
>   fail_stats:
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 88bd41d4cb59..1f986c20a07b 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -837,8 +837,8 @@ int blk_register_queue(struct gendisk *disk)
>   	}
>   
>   	mutex_lock(&q->debugfs_mutex);
> -	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
> -					    blk_debugfs_root);
> +	q->debugfs_dir = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
> +			blk_debugfs_root, kobject_name(q->kobj.parent));
>   	mutex_unlock(&q->debugfs_mutex);
>   
>   	if (queue_is_mq(q)) {
> diff --git a/block/genhd.c b/block/genhd.c
> index 36532b931841..08895f9f7087 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -25,6 +25,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/badblocks.h>
>   #include <linux/part_stat.h>
> +#include <linux/debugfs.h>
>   #include "blk-throttle.h"
>   
>   #include "blk.h"
> @@ -1160,6 +1161,7 @@ static void disk_release_mq(struct request_queue *q)
>   static void disk_release(struct device *dev)
>   {
>   	struct gendisk *disk = dev_to_disk(dev);
> +	char q_name[16];
>   
>   	might_sleep();
>   	WARN_ON_ONCE(disk_live(disk));
> @@ -1173,6 +1175,12 @@ static void disk_release(struct device *dev)
>   	kfree(disk->random);
>   	xa_destroy(&disk->part_tbl);
>   
> +	mutex_lock(&disk->queue->debugfs_mutex);
> +	sprintf(q_name, "%d", disk->queue->id);
> +	disk->queue->debugfs_dir = debugfs_rename(blk_debugfs_root,
> +			disk->queue->debugfs_dir, blk_debugfs_root, q_name);
> +	mutex_unlock(&disk->queue->debugfs_mutex);
> +
>   	disk->queue->disk = NULL;
>   	blk_put_queue(disk->queue);
>   
>