Proper block queue reference counting
A message that is not in the archive (sent off-list or not yet received): [email protected]
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.
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?
Jeff
Quoted 9 lines · Jeff Garzik
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, tostram.c) obviously switchblk_init_queueto 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 callsfoo_putitself, 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.
Quoted 3 lines
2) the
blk_init_queuereally 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 simplyblk_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.
Signature · 1 line
Jens Axboe
Quoted 9 lines · Jens Axboe
blk_cleanup_queue()still does that, as doesblk_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 likebdev_get_queue(), for instance.
Groovy, thanks for explaining.
Quoted 7 lines
Quoted 1 line
2) the
blk_init_queuereally 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 simplyblk_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 theblk_init_queue()changes, easy to grep for.
OTOH, blk_init_queue is changing quite radically, and people converting drivers will have to change that area of code _anyway_, so why not change the name too? :) It might create more confusion than it solves, to have the same function radically changing its behavior. So I respectfully disagree :)
(this is a minor point, anyway. I'm happy about the patch as a whole)
Jeff
Quoted 4 lines · Jeff Garzik
Quoted 3 lines · Jens Axboe
blk_cleanup_queue()still does that, as doesblk_put_queue()(same deal, each drop a reference, last reference frees the queue).
Quoted 8 lines
Quoted 6 lines
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 likebdev_get_queue(), for instance.Groovy, thanks for explaining.
No problem
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
2) the
blk_init_queuereally 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 simplyblk_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 theblk_init_queue()changes, easy to grep for.
Quoted 5 lines
OTOH,
blk_init_queueis changing quite radically, and people converting drivers will have to change that area of code _anyway_, so why not change the name too? :) It might create more confusion than it solves, to have the same function radically changing its behavior. So I respectfully disagree :)
I wouldn't mind that change. I don't have time to do it right now though, I'll soon be outta here for 2 weeks :)
Quoted 1 line
(this is a minor point, anyway. I'm happy about the patch as a whole)
Great
Signature · 1 line
Jens Axboe