On Sun, Apr 19, 2020 at 02:55:42PM -0700, Bart Van Assche wrote:
> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > +int __must_check blk_queue_debugfs_register(struct request_queue *q)
> > +{
> > + struct dentry *dir = NULL;
> > +
> > + /* This can happen if we have a bug in the lower layers */
>
> What does "this" refer to? Which layers does "lower layers" refer to? Most
> software developers consider a module that calls directly into another
> module as a higher layer (callbacks through function pointers do not count;
> see also https://en.wikipedia.org/wiki/Modular_programming). According to
> that definition block drivers are a software layer immediately above the
> block layer core.
>
> How about changing that comment into the following to make it unambiguous
> (if this is what you meant)?
>
> /*
> * Check whether the debugfs directory already exists. This can
> * only happen as the result of a bug in a block driver.
> */
But I didn't mean on a block driver. I meant the block core. In our
case, the async request_queue removal is an example. There is nothing
that block drivers could have done to help much with that.
I could just change "lower layers" to "block layer" ?
> > + dir = debugfs_lookup(kobject_name(q->kobj.parent), blk_debugfs_root);
> > + if (dir) {
> > + pr_warn("%s: registering request_queue debugfs directory twice is not allowed\n",
> > + kobject_name(q->kobj.parent));
> > + dput(dir);
> > + return -EALREADY;
> > + }
> > +
> > + q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
> > + blk_debugfs_root);
> > + if (!q->debugfs_dir)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
>
> kobject_name(q->kobj.parent) is used three times in the above function. How
> about introducing a local variable that holds the result of that expression?
Sure.
> > +static bool blk_trace_target_disk(const char *target, const char *diskname)
> > +{
> > + if (strlen(target) != strlen(diskname))
> > + return false;
> > +
> > + if (!strncmp(target, diskname,
> > + min_t(size_t, strlen(target), strlen(diskname))))
> > + return true;
> > +
> > + return false;
> > +}
>
> The above code looks weird to me. When the second if-statement is reached,
> it is guaranteed that 'target' and 'diskname' have the same length. So why
> to calculate the minimum length in the second if-statement of two strings
> that have the same length?
True, no need that that point. Will fix.
> Independent of what the purpose of the above code is, can that code be
> rewritten such that it does not depend on the details of how names are
> assigned to disks and partitions? Would disk_get_part() be useful here?
I did try, but couldn't figure out a way. I'll keep looking but likewise
let me know if you find a way.
Luis
