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@xxxxxxx> > > > > > > > Reported-by: Dan Williams <dan.j.williams@xxxxxxxxx> > > > > > > > Cc: yukuai (C) <yukuai3@xxxxxxxxxx> > > > > > > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx> > > > > > > > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx> > > > > > > > --- > > > > > > > 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
