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
