On Wed, Apr 22, 2020 at 01:10:11AM -0700, Christoph Hellwig wrote: > On Wed, Apr 22, 2020 at 07:48:02AM +0000, Luis Chamberlain wrote: > > > I don't see why we need this check. If it is valueable enough we > > > should have a debugfs_create_dir_exclusive or so that retunrns an error > > > for an exsting directory, instead of reimplementing it in the caller in > > > a racy way. But I'm not really sure we need it to start with. > > > > In short races, and even with synchronous request_queue removal I'm > > seeing the race is still possible, but that's due to some other races > > I'm going to chase down now. > > > > The easier solution really is to just have a debugfs dir created for > > each partition if debugfs is enabled, this way the directory will > > always be there, and the lookups are gone. > > That sounds like the best plan to me. Groovy. > > > > + > > > > + q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent), > > > > + blk_debugfs_root); > > > > + if (!q->debugfs_dir) > > > > + return -ENOMEM; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +void blk_queue_debugfs_unregister(struct request_queue *q) > > > > +{ > > > > + debugfs_remove_recursive(q->debugfs_dir); > > > > + q->debugfs_dir = NULL; > > > > +} > > > > > > Which to me suggests we can just fold these two into the callers, > > > with an IS_ENABLED for the creation case given that we check for errors > > > and the stub will always return an error. > > > > Sorry not sure I follow this. > > Don't both with the two above functions and just open code them in > the callers. IFF you still want to check for errors after the > discussion with Greg, wrap the call in a > > if (IS_ENABLED(CONFIG_DEBUG_FS)) > > to ensure that you don't fail queue creation in the !DEBUG_FS > case. Got it, thanks. Luis