Re: BUG in: Driver core: convert block from raw kobjects to core devices (fwd)

[Greg KH <greg@kroah.com>]

On Wed, Oct 31, 2007 at 11:46:30AM +0100, Kay Sievers wrote:
> On Tue, 2007-10-30 at 21:25 -0700, Greg KH wrote:
> > On Mon, Oct 29, 2007 at 08:13:17PM +0100, Kay Sievers wrote:
> > > On Mon, 2007-10-29 at 14:47 -0400, Alan Stern wrote:
> > > > On Mon, 29 Oct 2007, James Bottomley wrote:
> > > > 
> > > > > I'm not sure if we can do this patch.  If you kill a device, you see
> > > > > there's a processing state in scsi_prep_fn() for SDEV_DEL, which has a
> > > > > printk message I see quite often when I unplug devices while they're
> > > > > operating.
> > > > > 
> > > > > If you NULL out and free the request queue immediately after setting
> > > > > SDEV_DEL, without allowing the devices and filesystems to finish, are
> > > > > you sure we're not going to get a null deref on sdev->request_queue?
> > > > 
> > > > Let's get at this another way.  The patch below probably should be 
> > > > applied in any case.  It's essentially a reversion of this patch from 
> > > > 2003:
> > > > 
> > > > http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=10921a8f1305b8ec97794941db78b825db5839bc
> > > > 
> > > > which was written to compensate for problems in the SCSI stack.  The
> > > > idea was to avoid releasing a kobject before all its children were
> > > > released.  However as far as I can see, in general we should be able to
> > > > release a kobject as soon as all its children are removed and its
> > > > refcount drops to 0, without waiting for the children to be released.  
> > > > To put it another way, once a child is removed from visibility it
> > > > should not try (or need) to access its parent at all.  If it has to
> > > > then it should take an explicit reference to the parent.
> > > > 
> > > > Since the SCSI stack is now in much better shape, there doesn't seem to 
> > > > be any reason to keep the old code.  Do you agree that the patch below 
> > > > is worth merging?  I submitted it to Greg some time ago, but he didn't 
> > > > want to accept it without some assurance that it is now safe.
> > > > 
> > > > It would fix the problem Kay and saw with the circular references, 
> > > > because the references would all get dropped when the scsi_device is 
> > > > removed instead of waiting for a release that will never come.
> > > 
> > > It indeed fixes my problem. And it sounds you are right, the "fix" from
> > > 2003 is probably just a paper-over for a missing explicit reference that
> > > time.
> > > 
> > > I'm all for giving the reversion a try, and add explicit parent get/put
> > > if needed somewhere. If, for some reason, that will not happen, I'll
> > > need to do something like this in the block patch, which will then be a
> > > "fix for the paper-over solution for an unknown bug". :)
> > > 
> > > 
> > > --- a/fs/partitions/check.c
> > > +++ b/fs/partitions/check.c
> > > ...
> > > +       device_del(&disk->dev);
> > > +
> > > +       /*
> > > +        * disconnect from parent device, so the parent can clean up
> > > +        * without waiting for us to clean up
> > > +        *
> > > +        * the driver core took this reference while we added ourself as
> > > +        * a child of the parent device
> > > +        */
> > > +       parent = disk->dev.kobj.parent;
> > 
> > Save off the parent before calling device_del() otherwise it could be a
> > stale pointer, right?
> 
> I still hold a ref, the one I drop a few lines later. It should be safe.

Ok, but it's still wrong that this is needed :)

> > > +       disk->dev.kobj.parent = NULL;
> > > +       disk->dev.parent = NULL;
> > > +       kobject_put(parent);
> > 
> > No, that's just wrong, if this is needed, something else really is
> > broken, and I don't think it's the driver core...
> 
> It's just a kobject_orphan() function. We need to break the circiular
> reference somewhere, one of the objects in the circle has to clean up,
> to allow the others to clean up. The objects are properly deleted, but
> all final references are only dropped on cleanup. The kobject_orphan()
> here is just what Alan's patch is doing for the whole core.
> 
> With the current logic, if you have any parent referencing a child, the
> core will deadlock, and never reach any cleanup funtion, right? That's
> the loop I need to break here.

Hm, I seem to have missed the part in this thread where someone said
that it was valid to have a parent reference a child device.  That's
just wrong and needs to be fixed.  Is that in the scsi layer somewhere?
The block layer?  It sure isn't in the driver core...

thanks,

greg k-h