On Thu, Aug 07 2003, Jeff Garzik wrote:
> I like the patch, but see two problems:
>
> 1) You convert drivers to dynamically allocated queues... who is freeing
> the queues? unregister_blkdev? It's a bit non-obvious to say the
> least, since you patches (for example, the first one, to stram.c)
> obviously switch blk_init_queue to dynamically allocate a queue... but
> you do not add code to remove the final reference in modprobe. The
> standard driver-facing API dictates that the driver calls foo_put
> itself, in the driver, rather than have it done implicitly.
blk_cleanup_queue() still does that, as does blk_put_queue() (same deal,
each drop a reference, last reference frees the queue).
This first patch is just the frame work, the only thing that's
referenced counted right now is that the returned object has one
reference and when the driver cleans the queue it drops the reference
causing it to be freed. Next step is making sure others that hold a
reference to the queue also grab a reference to it, using
blk_get_queue(). That's stuff like bdev_get_queue(), for instance.
> 2) the blk_init_queue really should change names, IMO. The other
> subsystems in the kernel tend to use a "foo_alloc" or "alloc_foo"
> pattern when creating new objects. blk_alloc_queue, or simply blk_alloc?
blk_alloc_queue() would be fine. However, it's hard to screw the usage
up since it returns a queue, so... And people with out-of-tree drivers
that need to be converted need only look at the blk_init_queue()
changes, easy to grep for.
-- Jens Axboe- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Thu Aug 07 2003 - 22:00:38 EST