[PATCH] update sd to use kref and fix open/release race

[PATCH] update sd to use kref and fix open/release race

[James Bottomley]

All this does is replace our kobject usage with a kref.

James

===== sd.c 1.144 vs edited =====
--- 1.144/drivers/scsi/sd.c	Tue Mar 16 19:57:15 2004
+++ edited/sd.c	Fri Apr  9 09:35:31 2004
@@ -48,6 +48,7 @@
 #include <linux/vmalloc.h>
 #include <linux/blkdev.h>
 #include <linux/blkpg.h>
+#include <linux/kref.h>
 #include <asm/uaccess.h>
 
 #include "scsi.h"
@@ -77,16 +78,12 @@
  */
 #define SD_MAX_RETRIES		5
 
-static void scsi_disk_release (struct kobject *kobj);
-
-static struct kobj_type scsi_disk_kobj_type = {
-	.release = scsi_disk_release,
-};
+static void scsi_disk_release(struct kref *kref);
 
 struct scsi_disk {
 	struct scsi_driver *driver;	/* always &sd_template */
 	struct scsi_device *device;
-	struct kobject	kobj;
+	struct kref	kref;
 	struct gendisk	*disk;
 	unsigned int	openers;	/* protected by BKL for now, yuck */
 	sector_t	capacity;	/* size in 512-byte sectors */
@@ -100,6 +97,7 @@
 
 static unsigned long sd_index_bits[SD_DISKS / BITS_PER_LONG];
 static spinlock_t sd_index_lock = SPIN_LOCK_UNLOCKED;
+static DECLARE_MUTEX(sd_ref_sem);
 
 static int sd_revalidate_disk(struct gendisk *disk);
 static void sd_rw_intr(struct scsi_cmnd * SCpnt);
@@ -161,31 +159,33 @@
 
 /* reverse mapping dev -> (sd_nr, part) not currently needed */
 
-#define to_scsi_disk(obj) container_of(obj,struct scsi_disk,kobj);
+#define to_scsi_disk(obj) container_of(obj,struct scsi_disk,kref);
 
 static inline struct scsi_disk *scsi_disk(struct gendisk *disk)
 {
 	return container_of(disk->private_data, struct scsi_disk, driver);
 }
 
-static int scsi_disk_get(struct scsi_disk *sdkp)
+static struct scsi_disk *scsi_disk_get(struct gendisk *disk)
 {
-	if (!kobject_get(&sdkp->kobj))
-		goto out;
-	if (scsi_device_get(sdkp->device))
-		goto out_put_kobj;
-	return 0;
+	struct scsi_disk *sdkp = NULL;
 
-out_put_kobj:
-	kobject_put(&sdkp->kobj);
-out:
-	return -ENXIO;
+	down(&sd_ref_sem);
+	if (disk->private_data == NULL)
+		goto out;
+	sdkp = scsi_disk(disk);
+	if (!kref_get(&sdkp->kref))
+		sdkp = NULL;
+ out:
+	up(&sd_ref_sem);
+	return sdkp;
 }
 
 static void scsi_disk_put(struct scsi_disk *sdkp)
 {
-	scsi_device_put(sdkp->device);
-	kobject_put(&sdkp->kobj);
+	down(&sd_ref_sem);
+	kref_put(&sdkp->kref);
+	up(&sd_ref_sem);
 }
 
 /**
@@ -406,15 +406,15 @@
 static int sd_open(struct inode *inode, struct file *filp)
 {
 	struct gendisk *disk = inode->i_bdev->bd_disk;
-	struct scsi_disk *sdkp = scsi_disk(disk);
+	struct scsi_disk *sdkp;
 	struct scsi_device *sdev;
 	int retval;
 
-	SCSI_LOG_HLQUEUE(3, printk("sd_open: disk=%s\n", disk->disk_name));
+	if (!(sdkp = scsi_disk_get(disk)))
+		return -ENXIO;
 
-	retval = scsi_disk_get(sdkp);
-	if (retval)
-		return retval;
+
+	SCSI_LOG_HLQUEUE(3, printk("sd_open: disk=%s\n", disk->disk_name));
 
 	sdev = sdkp->device;
 
@@ -1338,17 +1338,19 @@
 	if ((sdp->type != TYPE_DISK) && (sdp->type != TYPE_MOD))
 		goto out;
 
+	if ((error = scsi_device_get(sdp)) != 0)
+		goto out;
+
 	SCSI_LOG_HLQUEUE(3, printk("sd_attach: scsi device: <%d,%d,%d,%d>\n", 
 			 sdp->host->host_no, sdp->channel, sdp->id, sdp->lun));
 
 	error = -ENOMEM;
 	sdkp = kmalloc(sizeof(*sdkp), GFP_KERNEL);
 	if (!sdkp)
-		goto out;
+		goto out_put_sdev;
 
 	memset (sdkp, 0, sizeof(*sdkp));
-	kobject_init(&sdkp->kobj);
-	sdkp->kobj.ktype = &scsi_disk_kobj_type;
+	kref_init(&sdkp->kref, scsi_disk_release);
 
 	/* Note: We can accomodate 64 partitions, but the genhd code
 	 * assumes partitions allocate consecutive minors, which they don't.
@@ -1421,6 +1423,8 @@
 	put_disk(gd);
 out_free:
 	kfree(sdkp);
+out_put_sdev:
+	scsi_device_put(sdp);
 out:
 	return error;
 }
@@ -1442,26 +1446,32 @@
 
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
-	kobject_put(&sdkp->kobj);
+	scsi_disk_put(sdkp);
 
 	return 0;
 }
 
 /**
  *	scsi_disk_release - Called to free the scsi_disk structure
- *	@kobj: pointer to embedded kobject
+ *	@kref: pointer to embedded kref
  **/
-static void scsi_disk_release(struct kobject *kobj)
+static void scsi_disk_release(struct kref *kref)
 {
-	struct scsi_disk *sdkp = to_scsi_disk(kobj);
+	struct scsi_disk *sdkp = to_scsi_disk(kref);
+	struct scsi_device *sdev = sdkp->device;
+	struct gendisk *disk = sdkp->disk;
 	
-	put_disk(sdkp->disk);
-
 	spin_lock(&sd_index_lock);
 	clear_bit(sdkp->index, sd_index_bits);
 	spin_unlock(&sd_index_lock);
 
+	disk->private_data = NULL;
+
+	put_disk(disk);
+
 	kfree(sdkp);
+
+	scsi_device_put(sdev);
 }
 
 /*

Re: [PATCH] update sd to use kref and fix open/release race

[Patrick Mansfield]

On Fri, Apr 09, 2004 at 08:52:59AM -0500, James Bottomley wrote:
> All this does is replace our kobject usage with a kref.

Nice work, I did not think this could be fixed without changing gendisk
code. It took me a bit to walk through the code paths, I still don't
fully grok the kobj_unmap/map/lookup. 

I was about to post on the other thread that you missed the semaphore
around the put in sd_remove but I see it is fixed here.

Per other thread, comments about using the private_data to mark the device
as removed, and about the get/put race would be good.

Code comments can't clarify the overall ref counting that occurs, maybe
Mike A can update his ref counting slides.

-- Patrick Mansfield

Re: [PATCH] update sd to use kref and fix open/release race

[Patrick Mansfield]

I spoke a bit too soon, a remove module is giving me an oops.

Running scsi-misc-2.6 + this patch. I did not try scsi-misc-2.6 plain.

I loaded the qla2300 module, removed a single lun via the sysfs interface,
and then rmmod qla2300.

Let me know if you need any other information.

elm3b79.beaverton.ibm.com login: Unable to handle kernel NULL pointer dereference at virtual address 00000000
 printing eip:
c01ea4b3
*pde = 33da1001
Oops: 0000 [#1]
SMP 
CPU:    2
EIP:    0060:[<c01ea4b3>]    Not tainted
EFLAGS: 00010286   (2.6.5-rc2) 
EIP is at scsi_device_set_state+0xa3/0xe4
eax: 00000000   ebx: 00000004   ecx: 00000003   edx: 00000018
esi: f416f000   edi: c02eae38   ebp: f3d22000   esp: f3d23e94
ds: 007b   es: 007b   ss: 0068
Process modprobe (pid: 1493, threadinfo=f3d22000 task=f3e6a6d0)
Stack: f416f1e0 c02b5688 c02b5690 00000003 f416f000 f4182000 c01ec39a f416f000 
       00000003 f3e40000 f4182000 c01eba42 f416f000 f3e40000 f4ba0c44 c01e57cd 
       f3e40000 f3e40000 f3e40000 00000000 f3e401c8 f88af667 f3e40000 f3e400e8 
Call Trace:
 [<c01ec39a>] scsi_remove_device+0xe/0x88
 [<c01eba42>] scsi_forget_host+0x32/0x60
 [<c01e57cd>] scsi_remove_host+0x19/0x48
 [<f88af667>] qla2x00_remove_one+0x6f/0x8c [qla2xxx]
 [<f88c9022>] qla2300_remove_one+0xa/0x10 [qla2300]
 [<c01a5fe2>] pci_device_remove+0x1a/0x34
 [<c01c77aa>] device_release_driver+0x46/0x58
 [<c01c77d9>] driver_detach+0x1d/0x2c
 [<c01c79b5>] bus_remove_driver+0x29/0x5c
 [<c01c7ceb>] driver_unregister+0xb/0x1f
 [<c01a6166>] pci_unregister_driver+0xe/0x1c
 [<f88c9032>] qla2300_exit+0xa/0x10 [qla2300]
 [<c012d0d5>] sys_delete_module+0x141/0x174
 [<c01405f0>] sys_munmap+0x38/0x58
 [<c0106b93>] syscall_call+0x7/0xb

Code: 8b 00 50 68 60 4c 2b c0 e8 24 f7 f2 ff 83 c4 18 68 68 06 00 

-- Patrick Mansfield

Re: [PATCH] update sd to use kref and fix open/release race

[James Bottomley]

On Fri, 2004-04-09 at 12:17, Patrick Mansfield wrote:
> I spoke a bit too soon, a remove module is giving me an oops.
> 
> Running scsi-misc-2.6 + this patch. I did not try scsi-misc-2.6 plain.
> 
> I loaded the qla2300 module, removed a single lun via the sysfs interface,
> and then rmmod qla2300.
> 
> Let me know if you need any other information.
> 
> elm3b79.beaverton.ibm.com login: Unable to handle kernel NULL pointer dereference at virtual address 00000000
>  printing eip:
> c01ea4b3
> *pde = 33da1001
> Oops: 0000 [#1]
> SMP 
> CPU:    2
> EIP:    0060:[<c01ea4b3>]    Not tainted
> EFLAGS: 00010286   (2.6.5-rc2) 
> EIP is at scsi_device_set_state+0xa3/0xe4
> eax: 00000000   ebx: 00000004   ecx: 00000003   edx: 00000018
> esi: f416f000   edi: c02eae38   ebp: f3d22000   esp: f3d23e94
> ds: 007b   es: 007b   ss: 0068
> Process modprobe (pid: 1493, threadinfo=f3d22000 task=f3e6a6d0)
> Stack: f416f1e0 c02b5688 c02b5690 00000003 f416f000 f4182000 c01ec39a f416f000 
>        00000003 f3e40000 f4182000 c01eba42 f416f000 f3e40000 f4ba0c44 c01e57cd 
>        f3e40000 f3e40000 f3e40000 00000000 f3e401c8 f88af667 f3e40000 f3e400e8 
> Call Trace:
>  [<c01ec39a>] scsi_remove_device+0xe/0x88
>  [<c01eba42>] scsi_forget_host+0x32/0x60

This looks odd.  I'm guessing that scsi_device_set_state+0xa3/0xe4 is
right around the dev_printk() in the illegal: label?

I'm guessing it did this because the driver had already detached so the
dev->driver->name deref is the NULL pointer one.

Really, we need to make dev_printk a lot more robust if it's actually
going to be useful.  Can you fix it and then tell me what the illegal
state transition actually was?

I guess it's because we don't drop off the siblings list until release
time, and the device was already being deleted.

Thanks,

James

Re: [PATCH] update sd to use kref and fix open/release race

[Greg KH]

On Fri, Apr 09, 2004 at 02:19:34PM -0500, James Bottomley wrote:
> 
> Really, we need to make dev_printk a lot more robust if it's actually
> going to be useful.

It is useful, you just have to be careful in how you use it :)

I have a debugging patch in my kernel tree that checks and prevents
oopses in those calls, but I've been told that it will not make it into
the main kernel tree.

thanks,

greg k-h

Re: [PATCH] update sd to use kref and fix open/release race

[Patrick Mansfield]

On Fri, Apr 09, 2004 at 02:19:34PM -0500, James Bottomley wrote:
> This looks odd.  I'm guessing that scsi_device_set_state+0xa3/0xe4 is
> right around the dev_printk() in the illegal: label?

Yep.

> I'm guessing it did this because the driver had already detached so the
> dev->driver->name deref is the NULL pointer one.
> 
> Really, we need to make dev_printk a lot more robust if it's actually
> going to be useful.  

IMO we should have a sdev_printk not using dev_printk, as during
scan/remove time we use all the normal code paths, and we don't have a
driver bound at those times. We could also bind a "dummy" driver and still
use dev_printk, but that could be problematic given the removing and
adding races.

> Can you fix it and then tell me what the illegal
> state transition actually was?

OK.

> I guess it's because we don't drop off the siblings list until release
> time, and the device was already being deleted.

I ran the delete a few seconds before the modprobe -r, the delete should
have been complete. The device being deleted was not open at the time.

Here is the stack and debug output:

scsi device <6:0:3:0> Illegal state transition deleted->cancel
Badness in scsi_device_set_state at drivers/scsi/scsi_lib.c:1646
Call Trace:
 [<c01ea4d8>] scsi_device_set_state+0xc8/0xd4
 [<c01ec38a>] scsi_remove_device+0xe/0x88
 [<c01eba32>] scsi_forget_host+0x32/0x60
 [<c01e57cd>] scsi_remove_host+0x19/0x48
 [<f88b5667>] qla2x00_remove_one+0x6f/0x8c [qla2xxx]
 [<f88cf022>] qla2300_remove_one+0xa/0x10 [qla2300]
 [<c01a5fe2>] pci_device_remove+0x1a/0x34
 [<c01c77aa>] device_release_driver+0x46/0x58
 [<c01c77d9>] driver_detach+0x1d/0x2c
 [<c01c79b5>] bus_remove_driver+0x29/0x5c
 [<c01c7ceb>] driver_unregister+0xb/0x1f
 [<c01a6166>] pci_unregister_driver+0xe/0x1c
 [<f88cf032>] qla2300_exit+0xa/0x10 [qla2300]
 [<c012d0d5>] sys_delete_module+0x141/0x174
 [<c01405f0>] sys_munmap+0x38/0x58
 [<c0106b93>] syscall_call+0x7/0xb

Re: [PATCH] update sd to use kref and fix open/release race

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> @@ -1338,17 +1338,19 @@
>  	if ((sdp->type != TYPE_DISK) && (sdp->type != TYPE_MOD))
>  		goto out;
>  
> +	if ((error = scsi_device_get(sdp)) != 0)
> +		goto out;
> +
>  	SCSI_LOG_HLQUEUE(3, printk("sd_attach: scsi device: <%d,%d,%d,%d>\n", 
>  			 sdp->host->host_no, sdp->channel, sdp->id, sdp->lun));
>  
>  	error = -ENOMEM;
>  	sdkp = kmalloc(sizeof(*sdkp), GFP_KERNEL);

Did we decide through the thread on this issue that we will not allow a
rmmod to happen anymore unless you manually delete the children devices
off the host? In using scsi_debug I am not able to rmmod until I delete
all the children (i.e. sd's).

The parent child hierarchy should be held in place through kobject_add /
kobject_get of the parent so I would believe that the extra
scsi_device_get should not be needed.

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

Re: [PATCH] update sd to use kref and fix open/release race

[James Bottomley]

On Tue, 2004-04-13 at 12:12, Mike Anderson wrote:
> Did we decide through the thread on this issue that we will not allow a
> rmmod to happen anymore unless you manually delete the children devices
> off the host? In using scsi_debug I am not able to rmmod until I delete
> all the children (i.e. sd's).

Well, I don't think anything was decided.  However, it's a consequence
of the compound reference strategy because now we hold a scsi_device for
the life of the ULD object.

> The parent child hierarchy should be held in place through kobject_add /
> kobject_get of the parent so I would believe that the extra
> scsi_device_get should not be needed.

In the ULD?  It's just simpler to do it that way.

The slight problem in all this is that the module refcounting model is
different from the device one (in modules, you can't send a deletion
(rmmod) unless your "ref" count is zero).

The best fix would be to move the module model over to the device model.
So you can rmmod at any point, removal triggers the appropriate
scsi_remove_device etc calls and the rmmod command hangs around until
it's safe to remove the module ... for SCSI we could do this before the
device references are dropped because there's a point where we can
guarantee no more commands will be sent to the host adapter.

To conform to the existing model, we can simply move the scsi_device_get
and scsi_device_put under the semaphore in the ULD get and put
routines.  This adds some assymmetry because now the put in sd_remove()
can't be the uld wrappered put, it would have to be a direct kref put
(with the semaphore), but it's doable.

James

Re: [PATCH] update sd to use kref and fix open/release race

[Mike Anderson]

James Bottomley [James.Bottomley@SteelEye.com] wrote:
> The slight problem in all this is that the module refcounting model is
> different from the device one (in modules, you can't send a deletion
> (rmmod) unless your "ref" count is zero).
> 
> The best fix would be to move the module model over to the device model.
> So you can rmmod at any point, removal triggers the appropriate
> scsi_remove_device etc calls and the rmmod command hangs around until
> it's safe to remove the module ... for SCSI we could do this before the
> device references are dropped because there's a point where we can
> guarantee no more commands will be sent to the host adapter.

Yes it would be good to convert the module model someday. We already
allow today to hot unplug an adapter with the module ref count positive
and cover almost the same code paths as a rmmod.

> 
> To conform to the existing model, we can simply move the scsi_device_get
> and scsi_device_put under the semaphore in the ULD get and put
> routines.  This adds some assymmetry because now the put in sd_remove()
> can't be the uld wrappered put, it would have to be a direct kref put
> (with the semaphore), but it's doable.

Yes, The addition of this asymmetric put is a side effect that would be
good to avoid. I believe the patch below is similar to what you are
talking about. I only ran a light ref count check on it. I will try a
more complete run tomorrow.

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


DESC
Move scsi_device_get out of sd probe path to allow module to be unloaded
when devices are not open.
EDESC


 patched-2.6-andmike/drivers/scsi/sd.c |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)

diff -puN drivers/scsi/sd.c~sd_ref drivers/scsi/sd.c
--- patched-2.6/drivers/scsi/sd.c~sd_ref	Thu Apr 22 04:52:00 2004
+++ patched-2.6-andmike/drivers/scsi/sd.c	Thu Apr 22 05:20:13 2004
@@ -179,7 +179,16 @@ static struct scsi_disk *scsi_disk_get(s
 		goto out;
 	sdkp = scsi_disk(disk);
 	if (!kref_get(&sdkp->kref))
-		sdkp = NULL;
+		goto out_sdkp;
+	if (scsi_device_get(sdkp->device))
+		goto out_put;
+	up(&sd_ref_sem);
+	return sdkp;
+
+ out_put:
+	kref_put(&sdkp->kref);
+ out_sdkp:
+	sdkp = NULL;
  out:
 	up(&sd_ref_sem);
 	return sdkp;
@@ -188,6 +197,7 @@ static struct scsi_disk *scsi_disk_get(s
 static void scsi_disk_put(struct scsi_disk *sdkp)
 {
 	down(&sd_ref_sem);
+	scsi_device_put(sdkp->device);
 	kref_put(&sdkp->kref);
 	up(&sd_ref_sem);
 }
@@ -1342,16 +1352,13 @@ static int sd_probe(struct device *dev)
 	if ((sdp->type != TYPE_DISK) && (sdp->type != TYPE_MOD))
 		goto out;
 
-	if ((error = scsi_device_get(sdp)) != 0)
-		goto out;
-
 	SCSI_LOG_HLQUEUE(3, printk("sd_attach: scsi device: <%d,%d,%d,%d>\n", 
 			 sdp->host->host_no, sdp->channel, sdp->id, sdp->lun));
 
 	error = -ENOMEM;
 	sdkp = kmalloc(sizeof(*sdkp), GFP_KERNEL);
 	if (!sdkp)
-		goto out_put_sdev;
+		goto out;
 
 	memset (sdkp, 0, sizeof(*sdkp));
 	kref_init(&sdkp->kref, scsi_disk_release);
@@ -1427,8 +1434,6 @@ out_put:
 	put_disk(gd);
 out_free:
 	kfree(sdkp);
-out_put_sdev:
-	scsi_device_put(sdp);
 out:
 	return error;
 }
@@ -1450,7 +1455,9 @@ static int sd_remove(struct device *dev)
 
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
-	scsi_disk_put(sdkp);
+	down(&sd_ref_sem);
+	kref_put(&sdkp->kref);
+	up(&sd_ref_sem);
 
 	return 0;
 }
@@ -1479,8 +1486,6 @@ static void scsi_disk_release(struct kre
 	put_disk(disk);
 
 	kfree(sdkp);
-
-	scsi_device_put(sdev);
 }
 
 /*

_

Re: [PATCH] update sd to use kref and fix open/release race

[viro]

On Wed, Apr 21, 2004 at 10:57:45PM -0700, Mike Anderson wrote:
> James Bottomley [James.Bottomley@SteelEye.com] wrote:
> > The slight problem in all this is that the module refcounting model is
> > different from the device one (in modules, you can't send a deletion
> > (rmmod) unless your "ref" count is zero).
> > 
> > The best fix would be to move the module model over to the device model.
> > So you can rmmod at any point, removal triggers the appropriate
> > scsi_remove_device etc calls and the rmmod command hangs around until
> > it's safe to remove the module ... for SCSI we could do this before the
> > device references are dropped because there's a point where we can
> > guarantee no more commands will be sent to the host adapter.

Or we could just admit that there is more to life than a plain refcount.
 
> Yes it would be good to convert the module model someday. We already
> allow today to hot unplug an adapter with the module ref count positive
> and cover almost the same code paths as a rmmod.

I bow to your stunning power of observation.  Of *course* all modules are
like that - so obviously that it would be an unpardonable waste of time to
explore what else might be out there.  If some isolated pathological cases
do not fit that model, they are better off broken, really.

Sheesh...