On 4/19/20 5:04 PM, Luis Chamberlain wrote:
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" ?
That sounds good to me.
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.
How about making blk_trace_setup() pass the result of the following
expression as an additional argument to blk_trace_setup():
bdev != bdev->bd_contains
I think that is a widely used approach to verify after a block device
has been opened whether or not 'bdev' refers to a partition (bdev !=
bdev->bd_contains means that 'bdev' represents a partition).
Thanks,
Bart.
