Re: [PATCH] Proper block queue reference counting

Re: [PATCH] Proper block queue reference counting

[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, 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

Re: [PATCH] Proper block queue reference counting

[Jens Axboe]

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

Re: [PATCH] Proper block queue reference counting

[Jeff Garzik]

Jens Axboe wrote:
> 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.

Groovy, thanks for explaining.


>>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.

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

Re: [PATCH] Proper block queue reference counting

[Jens Axboe]

On Thu, Aug 07 2003, Jeff Garzik wrote:
> Jens Axboe wrote:
> >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.
> 
> Groovy, thanks for explaining.

No problem

> >>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.
> 
> 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 :)

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 :)

> (this is a minor point, anyway.  I'm happy about the patch as a whole)

Great

-- 
Jens Axboe