Calling sd_shutdown when in state SDEV_DEL

Calling sd_shutdown when in state SDEV_DEL

[Heiko Carstens]

Hi,

upon scsi_remove_device the sd driver tries to send out a SYNCHRONIZE_CACHE
command (sd_shutdown) which is not possible because the sdev_state for the
device in question is already SDEV_DEL.
The patch below solves the problem, but I'm not sure if this is the right
way to do it. Also setting the state to SDEV_DEL after calling device_del
is probably pointless...

Heiko

diff -urN 2.5/drivers/scsi/scsi_sysfs.c 2.5-q/drivers/scsi/scsi_sysfs.c
--- 2.5/drivers/scsi/scsi_sysfs.c	Wed Mar 24 14:42:57 2004
+++ 2.5-q/drivers/scsi/scsi_sysfs.c	Wed Mar 24 14:39:21 2004
@@ -437,11 +437,12 @@
 void scsi_remove_device(struct scsi_device *sdev)
 {
 	if (sdev->sdev_state == SDEV_RUNNING || sdev->sdev_state == SDEV_CANCEL) {
-		scsi_device_set_state(sdev, SDEV_DEL);
+		scsi_device_quiesce(sdev);
 		class_device_unregister(&sdev->sdev_classdev);
 		if(sdev->transport_classdev.class)
 			class_device_unregister(&sdev->transport_classdev);
 		device_del(&sdev->sdev_gendev);
+		scsi_device_set_state(sdev, SDEV_DEL);
 		if (sdev->host->hostt->slave_destroy)
 			sdev->host->hostt->slave_destroy(sdev);
 		if (sdev->host->transportt->cleanup)

Re: Calling sd_shutdown when in state SDEV_DEL

[James Bottomley]

On Wed, 2004-03-24 at 10:03, Heiko Carstens wrote:
> upon scsi_remove_device the sd driver tries to send out a SYNCHRONIZE_CACHE
> command (sd_shutdown) which is not possible because the sdev_state for the
> device in question is already SDEV_DEL.
> The patch below solves the problem, but I'm not sure if this is the right
> way to do it. Also setting the state to SDEV_DEL after calling device_del
> is probably pointless...

Actually, I don't think quiesce, because that defers I/O waiting to
restart (also the transition QUIESCE->DEL is currently forbidden).

How about the attached instead.  That forces every deleted device to go
through CANCEL (uncompiled, but you can test it...)

James

===== scsi_sysfs.c 1.43 vs edited =====
--- 1.43/drivers/scsi/scsi_sysfs.c	Sat Mar 13 07:09:30 2004
+++ edited/scsi_sysfs.c	Wed Mar 24 10:32:56 2004
@@ -436,18 +436,19 @@
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
-	if (sdev->sdev_state == SDEV_RUNNING || sdev->sdev_state == SDEV_CANCEL) {
-		scsi_device_set_state(sdev, SDEV_DEL);
-		class_device_unregister(&sdev->sdev_classdev);
-		if(sdev->transport_classdev.class)
-			class_device_unregister(&sdev->transport_classdev);
-		device_del(&sdev->sdev_gendev);
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		if (sdev->host->transportt->cleanup)
-			sdev->host->transportt->cleanup(sdev);
-		put_device(&sdev->sdev_gendev);
-	}
+	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+		return;
+
+	class_device_unregister(&sdev->sdev_classdev);
+	if(sdev->transport_classdev.class)
+		class_device_unregister(&sdev->transport_classdev);
+	device_del(&sdev->sdev_gendev);
+	scsi_device_set_state(sdev, SDEV_DEL);
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	if (sdev->host->transportt->cleanup)
+		sdev->host->transportt->cleanup(sdev);
+	put_device(&sdev->sdev_gendev);
 }
 
 int scsi_register_driver(struct device_driver *drv)

Re: Calling sd_shutdown when in state SDEV_DEL

[Heiko Carstens]

> > upon scsi_remove_device the sd driver tries to send out a 
SYNCHRONIZE_CACHE
> > command (sd_shutdown) which is not possible because the sdev_state for 
the
> > device in question is already SDEV_DEL.
> How about the attached instead.  That forces every deleted device to go
> through CANCEL (uncompiled, but you can test it...)

Your patch works fine for me. Are you going to apply that one?

Heiko

Re: Calling sd_shutdown when in state SDEV_DEL

[Mike Anderson]

Heiko Carstens [heiko.carstens@de.ibm.com] wrote:
> The patch below solves the problem, but I'm not sure if this is the right
> way to do it. Also setting the state to SDEV_DEL after calling device_del
> is probably pointless...

One point of setting SDEV_DEL is to reduce future scsi_device_get calls from
incrementing the ref count.

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

Re: Calling sd_shutdown when in state SDEV_DEL

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> How about the attached instead.  That forces every deleted device to go
> through CANCEL (uncompiled, but you can test it...)
> 

Why setting CANCEL on the device? Future plans for the CANCEL state?
You are setting the state to cancel, but not going through
scsi_device_cancel as you want IO to still flow.

While this will solve the problem for removing a single device, this
same error will show up when you remove a host unless you remove all
devices first through the sysfs interface. Is this ok?

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

Re: Calling sd_shutdown when in state SDEV_DEL

[James Bottomley]

On Wed, 2004-03-24 at 12:04, Mike Anderson wrote:
> Why setting CANCEL on the device? Future plans for the CANCEL state?
> You are setting the state to cancel, but not going through
> scsi_device_cancel as you want IO to still flow.

Mainly because it was there.  Using CANCEL instead of DEL is appealing
since it will still allow special I/O.  Would there be an issue with
refusing new device references in the CANCEL state?

> While this will solve the problem for removing a single device, this
> same error will show up when you remove a host unless you remove all
> devices first through the sysfs interface. Is this ok?

Sigh, that's a symptom of the host not having a state model ... I need
to think about adding that.

James

Re: Calling sd_shutdown when in state SDEV_DEL

[Mike Anderson]

James Bottomley [James.Bottomley@SteelEye.com] wrote:
> On Wed, 2004-03-24 at 12:04, Mike Anderson wrote:
> > Why setting CANCEL on the device? Future plans for the CANCEL state?
> > You are setting the state to cancel, but not going through
> > scsi_device_cancel as you want IO to still flow.
> 
> Mainly because it was there.  Using CANCEL instead of DEL is appealing
> since it will still allow special I/O.  Would there be an issue with
> refusing new device references in the CANCEL state?

I think this should be ok as this is our (scsi core) own internal
solution to keep ref counts from increasing for the purpose of
eventually allowing everything to cleanup. 

> 
> > While this will solve the problem for removing a single device, this
> > same error will show up when you remove a host unless you remove all
> > devices first through the sysfs interface. Is this ok?
> 
> Sigh, that's a symptom of the host not having a state model ... I need
> to think about adding that.

Well this is my mess. We really have two usages models here but only one
scsi_remove_host interface. One usage calls scsi_remove_host when the
transport is dead and does not want anymore IO to flow through
queuecommand.  The other usage calls during a clean removal (i.e., rmmod
or hotplug) while there are still scsi_device children of the host
adapter as this is easier than deleting the children first.


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

Re: Calling sd_shutdown when in state SDEV_DEL

[James Bottomley]

On Wed, 2004-03-24 at 20:48, Mike Anderson wrote:
> James Bottomley [James.Bottomley@SteelEye.com] wrote:
> > On Wed, 2004-03-24 at 12:04, Mike Anderson wrote:
> > > Why setting CANCEL on the device? Future plans for the CANCEL state?
> > > You are setting the state to cancel, but not going through
> > > scsi_device_cancel as you want IO to still flow.
> > 
> > Mainly because it was there.  Using CANCEL instead of DEL is appealing
> > since it will still allow special I/O.  Would there be an issue with
> > refusing new device references in the CANCEL state?
> 
> I think this should be ok as this is our (scsi core) own internal
> solution to keep ref counts from increasing for the purpose of
> eventually allowing everything to cleanup. 

So can we agree on this as the final patch?

James

===== drivers/scsi/scsi.c 1.140 vs edited =====
--- 1.140/drivers/scsi/scsi.c	Tue Mar 16 20:10:09 2004
+++ edited/drivers/scsi/scsi.c	Fri Apr  2 15:36:08 2004
@@ -977,7 +977,7 @@
  */
 int scsi_device_get(struct scsi_device *sdev)
 {
-	if (sdev->sdev_state == SDEV_DEL)
+	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
 		return -ENXIO;
 	if (!get_device(&sdev->sdev_gendev))
 		return -ENXIO;
===== drivers/scsi/scsi_sysfs.c 1.44 vs edited =====
--- 1.44/drivers/scsi/scsi_sysfs.c	Tue Mar 16 10:00:19 2004
+++ edited/drivers/scsi/scsi_sysfs.c	Fri Apr  2 17:31:06 2004
@@ -502,18 +502,19 @@
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
-	if (sdev->sdev_state == SDEV_RUNNING || sdev->sdev_state == SDEV_CANCEL) {
-		scsi_device_set_state(sdev, SDEV_DEL);
-		class_device_unregister(&sdev->sdev_classdev);
-		if (sdev->transport_classdev.class)
-			class_device_unregister(&sdev->transport_classdev);
-		device_del(&sdev->sdev_gendev);
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		if (sdev->host->transportt->cleanup)
-			sdev->host->transportt->cleanup(sdev);
-		put_device(&sdev->sdev_gendev);
-	}
+	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+		return;
+
+	class_device_unregister(&sdev->sdev_classdev);
+	if (sdev->transport_classdev.class)
+		class_device_unregister(&sdev->transport_classdev);
+	device_del(&sdev->sdev_gendev);
+	scsi_device_set_state(sdev, SDEV_DEL);
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	if (sdev->host->transportt->cleanup)
+		sdev->host->transportt->cleanup(sdev);
+	put_device(&sdev->sdev_gendev);
 }
 
 int scsi_register_driver(struct device_driver *drv)