On Sat, Jun 20, 2020 at 10:31:51AM -0700, Bart Van Assche wrote: > On 2020-06-19 13:47, Luis Chamberlain wrote: > > This goes tested with: > ^^^^ > got? > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > > index 7ff2ea5cd05e..e6e2d25fdbd6 100644 > > --- a/kernel/trace/blktrace.c > > +++ b/kernel/trace/blktrace.c > > @@ -524,10 +524,18 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, > > if (!bt->msg_data) > > goto err; > > > > - ret = -ENOENT; > > - > > - dir = debugfs_lookup(buts->name, blk_debugfs_root); > > - if (!dir) > > +#ifdef CONFIG_BLK_DEBUG_FS > > + /* > > + * When tracing whole make_request drivers (multiqueue) block devices, > > + * reuse the existing debugfs directory created by the block layer on > > + * init. For request-based block devices, all partitions block devices, > ^^^^^^^^^^^^^^^^^^^^^ > It seems like a word is missing from the comment? Or did you perhaps > want to refer to "all partition block devices"? Yes, the later. > > + * and scsi-generic block devices we create a temporary new debugfs > > + * directory that will be removed once the trace ends. > > + */ > > + if (queue_is_mq(q) && bdev && bdev == bdev->bd_contains) > > + dir = q->debugfs_dir; > > + else > > +#endif > > bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root); > > Can it happen that two different threads each try to set up block > tracing and hence that debugfs_create_dir() fails because a directory > with name buts->name already exists? Great question, the answer is no. The reason is that we first use the mutex and then we check for q->blk_trace. If you hold the lock *and* you have checked for q->blk_trace and its NULL, you are sure you should not have a duplicate. Its why the commit log mentioned: The new clarifications on relying on the q->blk_trace_mutex *and* also checking for q->blk_trace *prior* to processing a blktrace ensures the debugfs directories are only created if no possible directory name clashes are possible. These clarifications were prompted through discussions with Jan Kara on the patches he posted which you CC'd me on. I agreed with his patch *but* I suggested it would hold true only if check for the q->blk_trace first, and this is why my patch titled "blktrace: break out of blktrace setup on concurrent calls" got merged prior to Jan Kara's "blktrace: Avoid sparse warnings when assigning q->blk_trace". > > bt->dev = dev; > > @@ -565,8 +573,6 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, > > > > ret = 0; > > err: > > - if (dir && !bt->dir) > > - dput(dir); > > if (ret) > > blk_trace_free(bt); > > return ret; > > Shouldn't bt->dir be removed in this error path for make_request drivers? If there is an error, bt->dir will be removed still, as I never modified the removal of bt->dir in this patch. Luis
