[PATCH] correct module refcounting

[PATCH] correct module refcounting

[James Bottomley]

There's a race window between doing the last scsi_device_put and having
the devices actually released where the module may be unloaded.  The fix
is to move the module accounting into the
scsi_allocate_sdev/scsi_free_sdev pieces of the SCSI routines.

I also think we probably want to move sdev_gendev population into
scsi_allocate_sdev.  Note this will change how we do initial scans
because now we'll be using the generic device release infrastructure to
free even temporary devices.  Unless anyone has a better idea...?

Finally, when this is done, we can take a device reference over both
scsi_get_command/scsi_put_command and also within the scsi_request_fn so
we know the device can never be released while it has outstanding
commands or while we are running its queues.

James

===== drivers/scsi/scsi.c 1.130 vs edited =====
--- 1.130/drivers/scsi/scsi.c	Mon Oct 27 06:01:49 2003
+++ edited/drivers/scsi/scsi.c	Tue Oct 28 15:39:10 2003
@@ -897,10 +897,6 @@
 		return -ENXIO;
 	if (!get_device(&sdev->sdev_gendev))
 		return -ENXIO;
-	if (!try_module_get(sdev->host->hostt->module)) {
-		put_device(&sdev->sdev_gendev);
-		return -ENXIO;
-	}
 	return 0;
 }
 EXPORT_SYMBOL(scsi_device_get);
@@ -915,7 +911,6 @@
  */
 void scsi_device_put(struct scsi_device *sdev)
 {
-	module_put(sdev->host->hostt->module);
 	put_device(&sdev->sdev_gendev);
 }
 EXPORT_SYMBOL(scsi_device_put);
===== drivers/scsi/scsi_scan.c 1.110 vs edited =====
--- 1.110/drivers/scsi/scsi_scan.c	Mon Oct 27 06:01:50 2003
+++ edited/drivers/scsi/scsi_scan.c	Tue Oct 28 15:39:00 2003
@@ -192,6 +192,9 @@
 	struct scsi_device *sdev, *device;
 	unsigned long flags;
 
+	if (!try_module_get(sdev->host->hostt->module))
+		goto out;
+
 	sdev = kmalloc(sizeof(*sdev), GFP_ATOMIC);
 	if (!sdev)
 		goto out;
@@ -299,6 +302,7 @@
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	kfree(sdev->inquiry);
+	module_put(sdev->host->hostt->module);
 	kfree(sdev);
 }
 

Re: [PATCH] correct module refcounting

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> There's a race window between doing the last scsi_device_put and having
> the devices actually released where the module may be unloaded.  The fix
> is to move the module accounting into the
> scsi_allocate_sdev/scsi_free_sdev pieces of the SCSI routines.
> 
> I also think we probably want to move sdev_gendev population into
> scsi_allocate_sdev.  Note this will change how we do initial scans
> because now we'll be using the generic device release infrastructure to
> free even temporary devices.  Unless anyone has a better idea...?
> 

If we do this we will need to do a two phase init of the sdev_gendev
like we do with the host. Otherwise every device_register will cause the
sd_probe to be called.

> Finally, when this is done, we can take a device reference over both
> scsi_get_command/scsi_put_command and also within the scsi_request_fn so
> we know the device can never be released while it has outstanding
> commands or while we are running its queues.
> 

What case of IO in-flight post a upper level driver release call are you
trying to protect. Do you think these extra atomic_incs will have any
negative impact.

> James
> 
> ===== drivers/scsi/scsi_scan.c 1.110 vs edited =====
> --- 1.110/drivers/scsi/scsi_scan.c	Mon Oct 27 06:01:50 2003
> +++ edited/drivers/scsi/scsi_scan.c	Tue Oct 28 15:39:00 2003
> @@ -192,6 +192,9 @@
>  	struct scsi_device *sdev, *device;
>  	unsigned long flags;
>  
> +	if (!try_module_get(sdev->host->hostt->module))
> +		goto out;
> +
>  	sdev = kmalloc(sizeof(*sdev), GFP_ATOMIC);
>  	if (!sdev)
>  		goto out;

Shouldn't this chunk be after the setting of sdev. I applied your patch
and it oops on me. After I moved it I am booted, but I have not done any
adds / deletes.

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

Re: [PATCH] correct module refcounting

[James Bottomley]

On Tue, 2003-10-28 at 18:17, Mike Anderson wrote:
> > I also think we probably want to move sdev_gendev population into
> > scsi_allocate_sdev.  Note this will change how we do initial scans
> > because now we'll be using the generic device release infrastructure to
> > free even temporary devices.  Unless anyone has a better idea...?
> > 
> 
> If we do this we will need to do a two phase init of the sdev_gendev
> like we do with the host. Otherwise every device_register will cause the
> sd_probe to be called.

well...we really already have this: slave_alloc and slave_configure, so
I think we may be able to slot it into the infrastructure.

> > Finally, when this is done, we can take a device reference over both
> > scsi_get_command/scsi_put_command and also within the scsi_request_fn so
> > we know the device can never be released while it has outstanding
> > commands or while we are running its queues.
> > 
> 
> What case of IO in-flight post a upper level driver release call are you
> trying to protect. Do you think these extra atomic_incs will have any
> negative impact.

It's more symmetry...since the elimination of scsi_device.access_count
we've been moving over to relying on the generic device reference
counting.  This would just be an extension.

I don't think an atomic_inc is particularly expensive compared with the
slab allocation of the commmand.

> > +	if (!try_module_get(sdev->host->hostt->module))
> > +		goto out;
> > +
> >  	sdev = kmalloc(sizeof(*sdev), GFP_ATOMIC);
> >  	if (!sdev)
> >  		goto out;
> 
> Shouldn't this chunk be after the setting of sdev. I applied your patch
> and it oops on me. After I moved it I am booted, but I have not done any
> adds / deletes.

Cut and paste error.  That should read if
(!try_module_get(shost->hostt->module))

but it should be before the kmalloc.

James

Re: [PATCH] correct module refcounting

[Mike Anderson]

James Bottomley [James.Bottomley@SteelEye.com] wrote:
> On Tue, 2003-10-28 at 18:17, Mike Anderson wrote:
> > If we do this we will need to do a two phase init of the sdev_gendev
> > like we do with the host. Otherwise every device_register will cause the
> > sd_probe to be called.
> 
> well...we really already have this: slave_alloc and slave_configure, so
> I think we may be able to slot it into the infrastructure.

ok. Since you already have this patch started are you going to add this,
or did you want me to look at this?

> 
> > > Finally, when this is done, we can take a device reference over both
> > > scsi_get_command/scsi_put_command and also within the scsi_request_fn so
> > > we know the device can never be released while it has outstanding
> > > commands or while we are running its queues.
> > > 
> > 
> > What case of IO in-flight post a upper level driver release call are you
> > trying to protect. Do you think these extra atomic_incs will have any
> > negative impact.
> 
> It's more symmetry...since the elimination of scsi_device.access_count
> we've been moving over to relying on the generic device reference
> counting.  This would just be an extension.
> 
> I don't think an atomic_inc is particularly expensive compared with the
> slab allocation of the commmand.
> 

This is probably true. In the past I had thought refs for every
scsi_get_command/scsi_put_command where excessive when you should have
other refs (i.e., open, direct get calls) keeping the device in place.
Though I still cannot track down the usb issue from lkml where the
elv_queue_empty function is oopsing :-(.

> > > +	if (!try_module_get(sdev->host->hostt->module))
> > > +		goto out;
> > > +
> > >  	sdev = kmalloc(sizeof(*sdev), GFP_ATOMIC);
> > >  	if (!sdev)
> > >  		goto out;
> > 
> > Shouldn't this chunk be after the setting of sdev. I applied your patch
> > and it oops on me. After I moved it I am booted, but I have not done any
> > adds / deletes.
> 
> Cut and paste error.  That should read if
> (!try_module_get(shost->hostt->module))
> 
> but it should be before the kmalloc.
> 

ok. You will also need a module_put on failures, but maybe I need to
wait for an update patch.

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

Re: [PATCH] correct module refcounting

[Christoph Hellwig]

On Tue, Oct 28, 2003 at 03:46:02PM -0600, James Bottomley wrote:
> There's a race window between doing the last scsi_device_put and having
> the devices actually released where the module may be unloaded.  The fix
> is to move the module accounting into the
> scsi_allocate_sdev/scsi_free_sdev pieces of the SCSI routines.

That would mean we can't rmmod a LLDD without first removing all device
through sysfs, or did I get it completely wrong?

--
Christoph Hellwig <hch@lst.de>		-	Freelance Hacker
Contact me for driver hacking and kernel development consulting

Re: [PATCH] correct module refcounting

[James Bottomley]

On Fri, 2003-10-31 at 06:18, Christoph Hellwig wrote:
> That would mean we can't rmmod a LLDD without first removing all device
> through sysfs, or did I get it completely wrong?

Yes, I was thinking the wrong way around.  I think we need to use the
scsi_get_device() scsi_put_device() to hold a reference to struct
scsi_device in more places than just ULD open/close.  However, obviously
we only need a reference to the module when it shouldn't be removed.


Perhaps all that's really necessary is something like splitting
scsi_device_get() into an __ version that does everything but the module
get.  Then we can use the __scsi_device_get() in the critical sections.

James