Re: bug 2400

[Jens Axboe <axboe@suse.de>]

On Fri, Apr 02 2004, James Bottomley wrote:
> On Thu, 2004-04-01 at 18:32, James Bottomley wrote:
> > Now, the questions are, whose issue is this and how do we fix it?  I can
> > see that a driver needs early notification of unplugs so it can deny all
> > access to a gone device.  On the other hand, for a user land open where
> > we still have to hold resources in the driver, we'd like the driver to
> > have a notify when the device reference count drops to zero so we can
> > clean up.
> 
> OK, I think this is easily fixable in sr.c by moving around the release
> code to do the right thing (and not drop the reference to sdev_gendev
> until we're completely finished).
> 
> Jens, does this look OK (or have I just opened up another race window
> somewhere else)?

Doesn't quite work, how about something like this as a work-around?

diff -ur /mnt/kscratch/linux-2.6.4/drivers/cdrom/cdrom.c linux-2.6.4-SUSE-20040402/drivers/cdrom/cdrom.c
--- /mnt/kscratch/linux-2.6.4/drivers/cdrom/cdrom.c	2004-04-02 14:53:42.000000000 +0200
+++ linux-2.6.4-SUSE-20040402/drivers/cdrom/cdrom.c	2004-04-05 15:43:01.603675727 +0200
@@ -379,6 +379,8 @@
 #endif /* CONFIG_SYSCTL */ 
 	}
 
+	atomic_set(&cdi->all_use, 1);
+
 	ENSURE(drive_status, CDC_DRIVE_STATUS );
 	ENSURE(media_changed, CDC_MEDIA_CHANGED);
 	ENSURE(tray_move, CDC_CLOSE_TRAY | CDC_OPEN_TRAY);
@@ -425,6 +427,9 @@
 	struct cdrom_device_info *cdi, *prev;
 	cdinfo(CD_OPEN, "entering unregister_cdrom\n"); 
 
+	if (!atomic_dec_and_test(&unreg->all_use))
+		return -EBUSY;
+
 	prev = NULL;
 	spin_lock(&cdrom_lock);
 	cdi = topCdromPtr;
@@ -891,6 +896,8 @@
 	if (ret)
 		goto err;
 
+	atomic_inc(&cdi->all_use);
+
 	cdinfo(CD_OPEN, "Use count for \"/dev/%s\" now %d\n",
 			cdi->name, cdi->use_count);
 	/* Do this on open.  Don't wait for mount, because they might
@@ -1096,6 +1103,7 @@
 		    cdi->options & CDO_AUTO_EJECT && CDROM_CAN(CDC_OPEN_TRAY))
 			cdo->tray_move(cdi, 1);
 	}
+
 	return 0;
 }
 
diff -ur /mnt/kscratch/linux-2.6.4/drivers/scsi/sr.c linux-2.6.4-SUSE-20040402/drivers/scsi/sr.c
--- /mnt/kscratch/linux-2.6.4/drivers/scsi/sr.c	2004-04-02 14:53:42.000000000 +0200
+++ linux-2.6.4-SUSE-20040402/drivers/scsi/sr.c	2004-04-05 15:57:35.577714398 +0200
@@ -419,13 +419,36 @@
 static int sr_block_open(struct inode *inode, struct file *file)
 {
 	struct scsi_cd *cd = scsi_cd(inode->i_bdev->bd_disk);
-	return cdrom_open(&cd->cdi, inode, file);
+	struct scsi_device *sdev = cd->device;
+	int retval;
+
+	retval = scsi_device_get(sdev);
+	if (retval)
+		return retval;
+	
+	retval = cdrom_open(&cd->cdi, inode, file);
+	if (retval)
+		scsi_device_put(sdev);
+
+	return retval;
+}
+
+static void sr_cleanup(struct scsi_cd *cd)
+{
+	if (!unregister_cdrom(&cd->cdi)) {
+		scsi_device_put(cd->device);
+		kfree(cd);
+	}
 }
 
 static int sr_block_release(struct inode *inode, struct file *file)
 {
 	struct scsi_cd *cd = scsi_cd(inode->i_bdev->bd_disk);
-	return cdrom_release(&cd->cdi, file);
+	int ret;
+
+	ret = cdrom_release(&cd->cdi, file);
+	sr_cleanup(cd);
+	return ret;
 }
 
 static int sr_block_ioctl(struct inode *inode, struct file *file, unsigned cmd,
@@ -467,10 +490,6 @@
 	struct scsi_device *sdev = cd->device;
 	int retval;
 
-	retval = scsi_device_get(sdev);
-	if (retval)
-		return retval;
-	
 	/*
 	 * If the device is in error recovery, wait until it is done.
 	 * If the device is offline, then disallow any access to it.
@@ -499,8 +518,6 @@
 
 	if (cd->device->sector_size > 2048)
 		sr_set_blocklength(cd, 2048);
-
-	scsi_device_put(cd->device);
 }
 
 static int sr_probe(struct device *dev)
@@ -874,8 +891,8 @@
 	spin_unlock(&sr_index_lock);
 
 	put_disk(cd->disk);
-	unregister_cdrom(&cd->cdi);
-	kfree(cd);
+
+	sr_cleanup(cd);
 
 	return 0;
 }
diff -ur /mnt/kscratch/linux-2.6.4/include/linux/cdrom.h linux-2.6.4-SUSE-20040402/include/linux/cdrom.h
--- /mnt/kscratch/linux-2.6.4/include/linux/cdrom.h	2004-04-02 14:53:42.000000000 +0200
+++ linux-2.6.4-SUSE-20040402/include/linux/cdrom.h	2004-04-05 15:10:46.347243055 +0200
@@ -916,6 +916,7 @@
 
 /* Uniform cdrom data structures for cdrom.c */
 struct cdrom_device_info {
+	atomic_t all_use;
 	struct cdrom_device_ops  *ops;  /* link to device_ops */
 	struct cdrom_device_info *next; /* next device_info for this major */
 	struct gendisk *disk;		/* matching block layer disk */

-- 
Jens Axboe