[PATCH 0/3] export scsi_forget_host

[PATCH 0/3] export scsi_forget_host

[Mike Anderson]

In regards to the mail the other day about rmmod of a LLDD and the
errors generated if a child device of the LLDD had write cache enabled.
The url reference to the thread is provided below.

http://marc.theaimsgroup.com/?t=108689054100009&r=1&w=2

The sequence of patches is against 2.6 bk current but should also
apply with some offsets to scsi-misc-2.6.

The first patch is the export of the function. The second patch is an
example of a LLDD (scsi_debug) using the function. The third patch is
not functionally needed, but the scsi_forget_host function name did not
appear to fit the other api naming.

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

[PATCH 1/3] scsi_forget_host - export function

[Mike Anderson]

DESC
scsi_forget_host - export function

Export scsi_forget_host for use by LLDD
Signed-off-by: Mike Anderson <andmike@us.ibm.com>
EDESC


 patched-2.6-andmike/drivers/scsi/scsi_priv.h |    1 -
 patched-2.6-andmike/drivers/scsi/scsi_syms.c |    1 +
 patched-2.6-andmike/include/scsi/scsi_host.h |    1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff -puN drivers/scsi/scsi_priv.h~exp_scsi_forget_host drivers/scsi/scsi_priv.h
--- patched-2.6/drivers/scsi/scsi_priv.h~exp_scsi_forget_host	Sat Jun 12 00:47:35 2004
+++ patched-2.6-andmike/drivers/scsi/scsi_priv.h	Sat Jun 12 00:47:35 2004
@@ -137,7 +137,6 @@ extern void scsi_exit_procfs(void);
 /* scsi_scan.c */
 extern int scsi_scan_host_selected(struct Scsi_Host *, unsigned int,
 				   unsigned int, unsigned int, int);
-extern void scsi_forget_host(struct Scsi_Host *);
 extern void scsi_rescan_device(struct device *);
 
 /* scsi_sysctl.c */
diff -puN drivers/scsi/scsi_syms.c~exp_scsi_forget_host drivers/scsi/scsi_syms.c
--- patched-2.6/drivers/scsi/scsi_syms.c~exp_scsi_forget_host	Sat Jun 12 00:47:35 2004
+++ patched-2.6-andmike/drivers/scsi/scsi_syms.c	Sat Jun 12 00:52:50 2004
@@ -36,6 +36,7 @@ EXPORT_SYMBOL(scsi_register_interface);
 EXPORT_SYMBOL(scsi_host_alloc);
 EXPORT_SYMBOL(scsi_add_host);
 EXPORT_SYMBOL(scsi_scan_host);
+EXPORT_SYMBOL(scsi_forget_host);
 EXPORT_SYMBOL(scsi_remove_host);
 EXPORT_SYMBOL(scsi_host_get);
 EXPORT_SYMBOL(scsi_host_put);
diff -puN include/scsi/scsi_host.h~exp_scsi_forget_host include/scsi/scsi_host.h
--- patched-2.6/include/scsi/scsi_host.h~exp_scsi_forget_host	Sat Jun 12 00:47:35 2004
+++ patched-2.6-andmike/include/scsi/scsi_host.h	Sat Jun 12 00:52:50 2004
@@ -501,6 +501,7 @@ struct Scsi_Host {
 extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
 extern int scsi_add_host(struct Scsi_Host *, struct device *);
 extern void scsi_scan_host(struct Scsi_Host *);
+extern void scsi_forget_host(struct Scsi_Host *);
 extern void scsi_remove_host(struct Scsi_Host *);
 extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);
 extern void scsi_host_put(struct Scsi_Host *t);

_

[PATCH 2/3] scsi_forget_host - scsi_debug usage

[Mike Anderson]

DESC
scsi_forget_host - scsi_debug usage

Use the export scsi_forget_host interface to remove child scsi devices
from the scsi_host prior to removal.

Signed-off-by: Mike Anderson <andmike@us.ibm.com>
EDESC


 patched-2.6-andmike/drivers/scsi/scsi_debug.c |    1 +
 1 files changed, 1 insertion(+)

diff -puN drivers/scsi/scsi_debug.c~use_scsi_forget_host drivers/scsi/scsi_debug.c
--- patched-2.6/drivers/scsi/scsi_debug.c~use_scsi_forget_host	Sat Jun 12 00:47:43 2004
+++ patched-2.6-andmike/drivers/scsi/scsi_debug.c	Sat Jun 12 00:52:50 2004
@@ -1725,6 +1725,7 @@ static int sdebug_driver_remove(struct d
 		return -ENODEV;
 	}
 
+	scsi_forget_host(sdbg_host->shost);
         scsi_remove_host(sdbg_host->shost);
 
         list_for_each_safe(lh, lh_sf, &sdbg_host->dev_info_list) {

_

[PATCH 3/3] scsi_forget_host - rename scsi_forget_host to scsi_unscan_host

[Mike Anderson]

DESC
scsi_forget_host - rename scsi_forget_host to scsi_unscan_host

This patch changes the name of the newly export scsi_forget_host function
to scsi_unscan_host. The scsi_unscan_host function name appears to create
a more symmetric LLDD interface
scsi_add_host
scsi_scan_host
...
scsi_unscan_host
scsi_remove_host

Signed-off-by: Mike Anderson <andmike@us.ibm.com>
EDESC


 patched-2.6-andmike/drivers/scsi/hosts.c      |    2 +-
 patched-2.6-andmike/drivers/scsi/scsi_debug.c |    2 +-
 patched-2.6-andmike/drivers/scsi/scsi_scan.c  |    2 +-
 patched-2.6-andmike/drivers/scsi/scsi_syms.c  |    2 +-
 patched-2.6-andmike/include/scsi/scsi_host.h  |    2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff -puN drivers/scsi/scsi_syms.c~scsi_unscan_host drivers/scsi/scsi_syms.c
--- patched-2.6/drivers/scsi/scsi_syms.c~scsi_unscan_host	Sat Jun 12 00:47:46 2004
+++ patched-2.6-andmike/drivers/scsi/scsi_syms.c	Sat Jun 12 00:47:46 2004
@@ -36,7 +36,7 @@ EXPORT_SYMBOL(scsi_register_interface);
 EXPORT_SYMBOL(scsi_host_alloc);
 EXPORT_SYMBOL(scsi_add_host);
 EXPORT_SYMBOL(scsi_scan_host);
-EXPORT_SYMBOL(scsi_forget_host);
+EXPORT_SYMBOL(scsi_unscan_host);
 EXPORT_SYMBOL(scsi_remove_host);
 EXPORT_SYMBOL(scsi_host_get);
 EXPORT_SYMBOL(scsi_host_put);
diff -puN include/scsi/scsi_host.h~scsi_unscan_host include/scsi/scsi_host.h
--- patched-2.6/include/scsi/scsi_host.h~scsi_unscan_host	Sat Jun 12 00:47:46 2004
+++ patched-2.6-andmike/include/scsi/scsi_host.h	Sat Jun 12 00:47:46 2004
@@ -501,7 +501,7 @@ struct Scsi_Host {
 extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
 extern int scsi_add_host(struct Scsi_Host *, struct device *);
 extern void scsi_scan_host(struct Scsi_Host *);
-extern void scsi_forget_host(struct Scsi_Host *);
+extern void scsi_unscan_host(struct Scsi_Host *);
 extern void scsi_remove_host(struct Scsi_Host *);
 extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);
 extern void scsi_host_put(struct Scsi_Host *t);
diff -puN drivers/scsi/scsi_debug.c~scsi_unscan_host drivers/scsi/scsi_debug.c
--- patched-2.6/drivers/scsi/scsi_debug.c~scsi_unscan_host	Sat Jun 12 00:47:46 2004
+++ patched-2.6-andmike/drivers/scsi/scsi_debug.c	Sat Jun 12 00:47:46 2004
@@ -1725,7 +1725,7 @@ static int sdebug_driver_remove(struct d
 		return -ENODEV;
 	}
 
-	scsi_forget_host(sdbg_host->shost);
+	scsi_unscan_host(sdbg_host->shost);
         scsi_remove_host(sdbg_host->shost);
 
         list_for_each_safe(lh, lh_sf, &sdbg_host->dev_info_list) {
diff -puN drivers/scsi/scsi_scan.c~scsi_unscan_host drivers/scsi/scsi_scan.c
--- patched-2.6/drivers/scsi/scsi_scan.c~scsi_unscan_host	Sat Jun 12 00:47:46 2004
+++ patched-2.6-andmike/drivers/scsi/scsi_scan.c	Sat Jun 12 00:47:46 2004
@@ -1259,7 +1259,7 @@ void scsi_scan_host(struct Scsi_Host *sh
 				SCAN_WILD_CARD, 0);
 }
 
-void scsi_forget_host(struct Scsi_Host *shost)
+void scsi_unscan_host(struct Scsi_Host *shost)
 {
 	struct scsi_device *sdev, *tmp;
 	unsigned long flags;
diff -puN drivers/scsi/hosts.c~scsi_unscan_host drivers/scsi/hosts.c
--- patched-2.6/drivers/scsi/hosts.c~scsi_unscan_host	Sat Jun 12 00:47:46 2004
+++ patched-2.6-andmike/drivers/scsi/hosts.c	Sat Jun 12 00:47:46 2004
@@ -79,7 +79,7 @@ void scsi_remove_host(struct Scsi_Host *
 {
 	scsi_host_cancel(shost, 0);
 	scsi_proc_host_rm(shost);
-	scsi_forget_host(shost);
+	scsi_unscan_host(shost);
 
 	set_bit(SHOST_DEL, &shost->shost_state);
 

_

Re: [PATCH 2/3] scsi_forget_host - scsi_debug usage

[Christoph Hellwig]

On Fri, Jun 11, 2004 at 09:22:32PM -0700, Mike Anderson wrote:
> DESC
> scsi_forget_host - scsi_debug usage
> 
> Use the export scsi_forget_host interface to remove child scsi devices
> from the scsi_host prior to removal.

Moving this into drivers sounds like a rather bad idea.  But with our
refcounting in place I wonder whether moving scsi_forget_host first in
scsi_remove_host wouldn't simply work.

Re: [PATCH 2/3] scsi_forget_host - scsi_debug usage

[Jens Axboe]

On Mon, Jun 14 2004, Christoph Hellwig wrote:
> On Fri, Jun 11, 2004 at 09:22:32PM -0700, Mike Anderson wrote:
> > DESC
> > scsi_forget_host - scsi_debug usage
> > 
> > Use the export scsi_forget_host interface to remove child scsi devices
> > from the scsi_host prior to removal.
> 
> Moving this into drivers sounds like a rather bad idea.  But with our

Agree

> refcounting in place I wonder whether moving scsi_forget_host first in
> scsi_remove_host wouldn't simply work.

It doesn't, device has already been deleted there. So either that needs
to be reworked if we want to allow devices to talk to themselves from
->remove(), or we just move the sync cache call to sd_release() instead.

The latter is attached.

===== drivers/scsi/sd.c 1.151 vs edited =====
--- 1.151/drivers/scsi/sd.c	2004-05-29 19:50:44 +02:00
+++ edited/drivers/scsi/sd.c	2004-06-14 15:05:34 +02:00
@@ -108,7 +108,6 @@
 
 static int sd_probe(struct device *);
 static int sd_remove(struct device *);
-static void sd_shutdown(struct device *dev);
 static void sd_rescan(struct device *);
 static int sd_init_command(struct scsi_cmnd *);
 static void sd_read_capacity(struct scsi_disk *sdkp, char *diskname,
@@ -120,7 +119,6 @@
 		.name		= "sd",
 		.probe		= sd_probe,
 		.remove		= sd_remove,
-		.shutdown	= sd_shutdown,
 	},
 	.rescan			= sd_rescan,
 	.init_command		= sd_init_command,
@@ -402,6 +400,50 @@
 	return 1;
 }
 
+/*
+ * Send a SYNCHRONIZE CACHE instruction down to the device through
+ * the normal SCSI command structure.  Wait for the command to
+ * complete.
+ */
+static void sd_sync_cache(struct scsi_device *sdp)
+{
+	struct scsi_request *sreq;
+	int retries, res;
+
+	sreq = scsi_allocate_request(sdp, GFP_KERNEL);
+	if (!sreq) {
+		printk("FAILED\n  No memory for request\n");
+		return;
+	}
+
+	sreq->sr_data_direction = DMA_NONE;
+	for (retries = 3; retries > 0; --retries) {
+		unsigned char cmd[10] = { 0 };
+
+		cmd[0] = SYNCHRONIZE_CACHE;
+		/*
+		 * Leave the rest of the command zero to indicate
+		 * flush everything.
+		 */
+		scsi_wait_req(sreq, cmd, NULL, 0, SD_TIMEOUT, SD_MAX_RETRIES);
+		if (sreq->sr_result == 0)
+			break;
+	}
+
+	res = sreq->sr_result;
+	if (res) {
+		printk(KERN_WARNING "FAILED\n  status = %x, message = %02x, "
+				    "host = %d, driver = %02x\n  ",
+				    status_byte(res), msg_byte(res),
+				    host_byte(res), driver_byte(res));
+			if (driver_byte(res) & DRIVER_SENSE)
+				print_req_sense("sd", sreq);
+	}
+	
+	scsi_release_request(sreq);
+	printk("\n");
+}
+
 /**
  *	sd_open - open a scsi disk device
  *	@inode: only i_rdev member may be used
@@ -498,8 +540,9 @@
 
 	SCSI_LOG_HLQUEUE(3, printk("sd_release: disk=%s\n", disk->disk_name));
 
-	if (!--sdkp->openers && sdev->removable) {
-		if (scsi_block_when_processing_errors(sdev))
+	if (!--sdkp->openers) {
+		sd_sync_cache(sdev);
+		if (sdev->removable && scsi_block_when_processing_errors(sdev))
 			scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
 	}
 
@@ -1460,7 +1503,6 @@
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
 
 	del_gendisk(sdkp->disk);
-	sd_shutdown(dev);
 	down(&sd_ref_sem);
 	kref_put(&sdkp->kref);
 	up(&sd_ref_sem);
@@ -1492,62 +1534,6 @@
 
 	kfree(sdkp);
 }
-
-/*
- * Send a SYNCHRONIZE CACHE instruction down to the device through
- * the normal SCSI command structure.  Wait for the command to
- * complete.
- */
-static void sd_shutdown(struct device *dev)
-{
-	struct scsi_device *sdp = to_scsi_device(dev);
-	struct scsi_disk *sdkp;
-	struct scsi_request *sreq;
-	int retries, res;
-
-	sdkp = dev_get_drvdata(dev);
-	if (!sdkp)
-               return;         /* this can happen */
-
-	if (!scsi_device_online(sdp) || !sdkp->WCE)
-		return;
-
-	printk(KERN_NOTICE "Synchronizing SCSI cache for disk %s: ",
-			sdkp->disk->disk_name);
-
-	sreq = scsi_allocate_request(sdp, GFP_KERNEL);
-	if (!sreq) {
-		printk("FAILED\n  No memory for request\n");
-		return;
-	}
-
-	sreq->sr_data_direction = DMA_NONE;
-	for (retries = 3; retries > 0; --retries) {
-		unsigned char cmd[10] = { 0 };
-
-		cmd[0] = SYNCHRONIZE_CACHE;
-		/*
-		 * Leave the rest of the command zero to indicate
-		 * flush everything.
-		 */
-		scsi_wait_req(sreq, cmd, NULL, 0, SD_TIMEOUT, SD_MAX_RETRIES);
-		if (sreq->sr_result == 0)
-			break;
-	}
-
-	res = sreq->sr_result;
-	if (res) {
-		printk(KERN_WARNING "FAILED\n  status = %x, message = %02x, "
-				    "host = %d, driver = %02x\n  ",
-				    status_byte(res), msg_byte(res),
-				    host_byte(res), driver_byte(res));
-			if (driver_byte(res) & DRIVER_SENSE)
-				print_req_sense("sd", sreq);
-	}
-	
-	scsi_release_request(sreq);
-	printk("\n");
-}	
 
 /**
  *	init_sd - entry point for this driver (both when built in or when

-- 
Jens Axboe

Re: [PATCH 2/3] scsi_forget_host - scsi_debug usage

[Christoph Hellwig]

On Mon, Jun 14, 2004 at 03:06:19PM +0200, Jens Axboe wrote:
> > refcounting in place I wonder whether moving scsi_forget_host first in
> > scsi_remove_host wouldn't simply work.
> 
> It doesn't, device has already been deleted there. So either that needs
> to be reworked if we want to allow devices to talk to themselves from
> ->remove(), or we just move the sync cache call to sd_release() instead.

Doing sync cache in sd_release sounds good anyway.  But for the rootfs
case we need to do it in shutdown aswell.

Re: [PATCH 2/3] scsi_forget_host - scsi_debug usage

[Jens Axboe]

On Mon, Jun 14 2004, Christoph Hellwig wrote:
> On Mon, Jun 14, 2004 at 03:06:19PM +0200, Jens Axboe wrote:
> > > refcounting in place I wonder whether moving scsi_forget_host first in
> > > scsi_remove_host wouldn't simply work.
> > 
> > It doesn't, device has already been deleted there. So either that needs
> > to be reworked if we want to allow devices to talk to themselves from
> > ->remove(), or we just move the sync cache call to sd_release() instead.
> 
> Doing sync cache in sd_release sounds good anyway.  But for the rootfs

Yep

> case we need to do it in shutdown aswell.

But the device is already SDEV_DEL there, so it can't be done without
fixing that. What makes the rootfs case special, btw?

-- 
Jens Axboe

Re: [PATCH 2/3] scsi_forget_host - scsi_debug usage

[Christoph Hellwig]

On Mon, Jun 14, 2004 at 03:26:09PM +0200, Jens Axboe wrote:
> > case we need to do it in shutdown aswell.
> 
> But the device is already SDEV_DEL there, so it can't be done without
> fixing that. What makes the rootfs case special, btw?

The root device won't ever be closed, so you don't get the final sd_release
call.

Re: [PATCH 2/3] scsi_forget_host - scsi_debug usage

[Jens Axboe]

On Mon, Jun 14 2004, Christoph Hellwig wrote:
> On Mon, Jun 14, 2004 at 03:26:09PM +0200, Jens Axboe wrote:
> > > case we need to do it in shutdown aswell.
> > 
> > But the device is already SDEV_DEL there, so it can't be done without
> > fixing that. What makes the rootfs case special, btw?
> 
> The root device won't ever be closed, so you don't get the final sd_release
> call.

Ah yes, of course. If you want that to work, then there's no way around
juggling the device shutdown stuff around. Or flush cache on 2nd to last
release too :-)

-- 
Jens Axboe

Re: [PATCH 2/3] scsi_forget_host - scsi_debug usage

[Mike Anderson]

Christoph Hellwig [hch@infradead.org] wrote:
> On Fri, Jun 11, 2004 at 09:22:32PM -0700, Mike Anderson wrote:
> > DESC
> > scsi_forget_host - scsi_debug usage
> > 
> > Use the export scsi_forget_host interface to remove child scsi devices
> > from the scsi_host prior to removal.
> 
> Moving this into drivers sounds like a rather bad idea.  But with our
> refcounting in place I wonder whether moving scsi_forget_host first in
> scsi_remove_host wouldn't simply work.

Why is this a bad idea? We allow the LLDD to control scanning through
scsi_scan_host. We also have LLDDs calling scsi_remove_device directly
which scsi_forget_host uses.

The issue I was trying to address is there are callers that want to call
scsi_remove_host during unexpected disconnect situations where they want
to reduce more IO being sent to the LLDDs queuecommand. It should be the
case that if a LLDD can handle unexpected disconnect that they already
have code in there queuecommand to stop the flow of IO in these cases so
your suggested change to scsi_remove_host may not negatively effect
LLDDs.

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