update sd to use kref and fix open/release race

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

James

===== sd.c 1.144 vs edited =====

@@ -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);
 }
 
 /*

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

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

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

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

Yep.

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.

OK.

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

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.

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

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.

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
[email protected]

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
@@ -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);
 }
 
 /*

_

Or we could just admit that there is more to life than a plain refcount.

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