* [PATCH V2 0/2] block: fix "Directory XXXXX with parent 'block' already present!" @ 2022-04-23 14:39 Ming Lei 2022-04-23 14:39 ` [PATCH V2 1/2] debugfs: fix declaration of debugfs_rename Ming Lei 2022-04-23 14:39 ` [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei 0 siblings, 2 replies; 20+ messages in thread From: Ming Lei @ 2022-04-23 14:39 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Greg Kroah-Hartman, 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH V2 1/2] debugfs: fix declaration of debugfs_rename 2022-04-23 14:39 [PATCH V2 0/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei @ 2022-04-23 14:39 ` Ming Lei 2022-04-23 14:39 ` [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei 1 sibling, 0 replies; 20+ messages in thread From: Ming Lei @ 2022-04-23 14:39 UTC (permalink / raw) To: Jens Axboe Cc: linux-block, Christoph Hellwig, Greg Kroah-Hartman, Ming Lei, kernel test robot 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-23 14:39 [PATCH V2 0/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei 2022-04-23 14:39 ` [PATCH V2 1/2] debugfs: fix declaration of debugfs_rename Ming Lei @ 2022-04-23 14:39 ` Ming Lei 2022-04-23 16:29 ` Christoph Hellwig ` (2 more replies) 1 sibling, 3 replies; 20+ messages in thread From: Ming Lei @ 2022-04-23 14:39 UTC (permalink / raw) To: Jens Axboe Cc: linux-block, Christoph Hellwig, Greg Kroah-Hartman, Ming Lei, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-23 14:39 ` [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei @ 2022-04-23 16:29 ` Christoph Hellwig 2022-04-24 9:24 ` Ming Lei 2022-04-24 8:53 ` Hannes Reinecke 2022-05-23 13:11 ` Yu Kuai 2 siblings, 1 reply; 20+ messages in thread From: Christoph Hellwig @ 2022-04-23 16:29 UTC (permalink / raw) To: Ming Lei Cc: Jens Axboe, linux-block, Christoph Hellwig, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams, yukuai 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-23 16:29 ` Christoph Hellwig @ 2022-04-24 9:24 ` Ming Lei 2022-04-25 7:49 ` Christoph Hellwig 0 siblings, 1 reply; 20+ messages in thread From: Ming Lei @ 2022-04-24 9:24 UTC (permalink / raw) To: Christoph Hellwig Cc: Jens Axboe, linux-block, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-24 9:24 ` Ming Lei @ 2022-04-25 7:49 ` Christoph Hellwig 2022-04-25 9:18 ` Ming Lei 0 siblings, 1 reply; 20+ messages in thread From: Christoph Hellwig @ 2022-04-25 7:49 UTC (permalink / raw) To: Ming Lei Cc: Christoph Hellwig, Jens Axboe, linux-block, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams, yukuai 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-25 7:49 ` Christoph Hellwig @ 2022-04-25 9:18 ` Ming Lei 0 siblings, 0 replies; 20+ messages in thread From: Ming Lei @ 2022-04-25 9:18 UTC (permalink / raw) To: Christoph Hellwig Cc: Jens Axboe, linux-block, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-23 14:39 ` [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei 2022-04-23 16:29 ` Christoph Hellwig @ 2022-04-24 8:53 ` Hannes Reinecke 2022-04-24 9:28 ` Ming Lei 2022-05-23 13:11 ` Yu Kuai 2 siblings, 1 reply; 20+ messages in thread From: Hannes Reinecke @ 2022-04-24 8:53 UTC (permalink / raw) To: Ming Lei, Jens Axboe Cc: linux-block, Christoph Hellwig, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-24 8:53 ` Hannes Reinecke @ 2022-04-24 9:28 ` Ming Lei 2022-04-24 11:51 ` Hannes Reinecke 0 siblings, 1 reply; 20+ messages in thread From: Ming Lei @ 2022-04-24 9:28 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, linux-block, Christoph Hellwig, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-24 9:28 ` Ming Lei @ 2022-04-24 11:51 ` Hannes Reinecke 2022-04-24 12:04 ` Ming Lei 0 siblings, 1 reply; 20+ messages in thread From: Hannes Reinecke @ 2022-04-24 11:51 UTC (permalink / raw) To: Ming Lei Cc: Jens Axboe, linux-block, Christoph Hellwig, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-24 11:51 ` Hannes Reinecke @ 2022-04-24 12:04 ` Ming Lei 2022-04-24 13:45 ` Greg Kroah-Hartman 0 siblings, 1 reply; 20+ messages in thread From: Ming Lei @ 2022-04-24 12:04 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, linux-block, Christoph Hellwig, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-24 12:04 ` Ming Lei @ 2022-04-24 13:45 ` Greg Kroah-Hartman 2022-04-25 1:28 ` Ming Lei 0 siblings, 1 reply; 20+ messages in thread From: Greg Kroah-Hartman @ 2022-04-24 13:45 UTC (permalink / raw) To: Ming Lei Cc: Hannes Reinecke, Jens Axboe, linux-block, Christoph Hellwig, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-24 13:45 ` Greg Kroah-Hartman @ 2022-04-25 1:28 ` Ming Lei 2022-04-25 5:10 ` Greg Kroah-Hartman 0 siblings, 1 reply; 20+ messages in thread From: Ming Lei @ 2022-04-25 1:28 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Hannes Reinecke, Jens Axboe, linux-block, Christoph Hellwig, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-25 1:28 ` Ming Lei @ 2022-04-25 5:10 ` Greg Kroah-Hartman 2022-04-25 7:48 ` Christoph Hellwig 2022-04-25 9:07 ` Ming Lei 0 siblings, 2 replies; 20+ messages in thread From: Greg Kroah-Hartman @ 2022-04-25 5:10 UTC (permalink / raw) To: Ming Lei Cc: Hannes Reinecke, Jens Axboe, linux-block, Christoph Hellwig, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-25 5:10 ` Greg Kroah-Hartman @ 2022-04-25 7:48 ` Christoph Hellwig 2022-04-25 7:53 ` Hannes Reinecke 2022-04-25 9:07 ` Ming Lei 1 sibling, 1 reply; 20+ messages in thread From: Christoph Hellwig @ 2022-04-25 7:48 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Ming Lei, Hannes Reinecke, Jens Axboe, linux-block, Christoph Hellwig, Shin'ichiro Kawasaki, Dan Williams, yukuai 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-25 7:48 ` Christoph Hellwig @ 2022-04-25 7:53 ` Hannes Reinecke 0 siblings, 0 replies; 20+ messages in thread From: Hannes Reinecke @ 2022-04-25 7:53 UTC (permalink / raw) To: Christoph Hellwig, Greg Kroah-Hartman Cc: Ming Lei, Jens Axboe, linux-block, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-25 5:10 ` Greg Kroah-Hartman 2022-04-25 7:48 ` Christoph Hellwig @ 2022-04-25 9:07 ` Ming Lei 2022-04-25 9:32 ` Hannes Reinecke 1 sibling, 1 reply; 20+ messages in thread From: Ming Lei @ 2022-04-25 9:07 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Hannes Reinecke, Jens Axboe, linux-block, Christoph Hellwig, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-25 9:07 ` Ming Lei @ 2022-04-25 9:32 ` Hannes Reinecke 2022-04-26 3:07 ` Ming Lei 0 siblings, 1 reply; 20+ messages in thread From: Hannes Reinecke @ 2022-04-25 9:32 UTC (permalink / raw) To: Ming Lei, Greg Kroah-Hartman Cc: Jens Axboe, linux-block, Christoph Hellwig, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-25 9:32 ` Hannes Reinecke @ 2022-04-26 3:07 ` Ming Lei 0 siblings, 0 replies; 20+ messages in thread From: Ming Lei @ 2022-04-26 3:07 UTC (permalink / raw) To: Hannes Reinecke Cc: Greg Kroah-Hartman, Jens Axboe, linux-block, Christoph Hellwig, Shin'ichiro Kawasaki, Dan Williams, yukuai 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" 2022-04-23 14:39 ` [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei 2022-04-23 16:29 ` Christoph Hellwig 2022-04-24 8:53 ` Hannes Reinecke @ 2022-05-23 13:11 ` Yu Kuai 2 siblings, 0 replies; 20+ messages in thread From: Yu Kuai @ 2022-05-23 13:11 UTC (permalink / raw) To: Ming Lei, Jens Axboe Cc: linux-block, Christoph Hellwig, Greg Kroah-Hartman, Shin'ichiro Kawasaki, Dan Williams 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); > > ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-05-23 13:11 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-23 14:39 [PATCH V2 0/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei 2022-04-23 14:39 ` [PATCH V2 1/2] debugfs: fix declaration of debugfs_rename Ming Lei 2022-04-23 14:39 ` [PATCH V2 2/2] block: fix "Directory XXXXX with parent 'block' already present!" Ming Lei 2022-04-23 16:29 ` Christoph Hellwig 2022-04-24 9:24 ` Ming Lei 2022-04-25 7:49 ` Christoph Hellwig 2022-04-25 9:18 ` Ming Lei 2022-04-24 8:53 ` Hannes Reinecke 2022-04-24 9:28 ` Ming Lei 2022-04-24 11:51 ` Hannes Reinecke 2022-04-24 12:04 ` Ming Lei 2022-04-24 13:45 ` Greg Kroah-Hartman 2022-04-25 1:28 ` Ming Lei 2022-04-25 5:10 ` Greg Kroah-Hartman 2022-04-25 7:48 ` Christoph Hellwig 2022-04-25 7:53 ` Hannes Reinecke 2022-04-25 9:07 ` Ming Lei 2022-04-25 9:32 ` Hannes Reinecke 2022-04-26 3:07 ` Ming Lei 2022-05-23 13:11 ` Yu Kuai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.