Re: bug 2400

[Mike Anderson <andmike@us.ibm.com>]

James Bottomley [James.Bottomley@SteelEye.com] wrote:
> On Fri, 2004-04-02 at 12:44, Mike Anderson wrote:
> > Where does the last put come from? How do you close the open race or
> > know when the final put_disk has been called? SCSI cannot do this alone
> > as we have created and registered an object in another subsystem
> > (alloc_disk and add_disk) and we have no indication when that objects
> > ref count has reached zero. 
> > 
> > Or
> > 
> > As I previous stated in the thread below I have the gendisk /
> > block layer locking mis-understood and there is something that SCSI can
> > do.
> 
> well, sr has elected to merge these, so it takes a reference to
> sdev_gendev on first open and releases it on last close of the block
> device.  This is what ties the SCSI model into the final put_disk().

I think there is a little more to tie this together. see below.

> 
> We founder on calling driver ->remove before the final put of
> sdev_gendev.

I do not believe we founder by calling, but what we do in the remove
function. The way sd is split out seems to match what you think remove
should do, but I do not have externally callable a release as it is
handled inside sd.

> 
> Anything with objects in more than one refcounted subsystem is
> responsible for tying the refcounts together uniformly.
> 
> There should be no open race as long as we error out correctly if a
> reference to the underlying sdev_gendev cannot be obtained (because the
> object is being destroyed).  sr seems to do this correctly.  The
> indication when the non-scsi object's refcount reaches zero is given to
> us because at that point the sr code does a put of the sdev_gendev (and
> if this is the last put, that should trigger cleanup).

Greg stopped by and after talking this over I think I see why sd is
racing in its current form. The race happens when sd_remove and do_open
race. Even though I do not like adding a lock_kernel it would appear
adding on to sd_remove would serialize sd_remove and do_open. This would
ensure either do_open's get_gendisk returns a gendisk struct and sd
ref's are incremented or we will start cleaning up and sd_open will not
be called.

I would believe similar alignment in sr.c to what sd is doing plus
agreement on the lock_kernel should fix both drivers.

I think the "error out correctly" on trying to get a ref on sdev_gendev
may need some higher serialization as I think there is a race on a release
function starting and the reference count trying to be taken to 1 (i.e.
you need something subsystem wide as you cannot look at the item you
maybe deleting.

-andmike
--
Michael Anderson
andmike@us.ibm.com