[PATCH 4/5] SCSI scanning and removal fixes

[PATCH 4/5] SCSI scanning and removal fixes

[Alan Stern]

James:

This patch (as545) fixes the list traversals in __scsi_remove_target and 
scsi_forget_host.  In each case the existing code list_for_each_entry_safe 
in an _unsafe_ manner, because the list was not protected from outside 
modification while the iteration was running.

The new scsi_forget_host routine takes the moderately controversial step 
of iterating over devices for removal rather than iterating over targets.  
This makes more sense to me because the current scheme treats targets as 
second-class citizens, created and removed on demand, rather than as 
objects corresponding to actual hardware.  (Also I couldn't figure out any 
safe way to iterate over the target list, since it's not so easy to tell 
when a target has already been removed.)

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Index: 2613/drivers/scsi/scsi_sysfs.c
===================================================================
--- 2613.orig/drivers/scsi/scsi_sysfs.c
+++ 2613/drivers/scsi/scsi_sysfs.c
@@ -717,17 +717,20 @@ void __scsi_remove_target(struct scsi_ta
 {
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
-	struct scsi_device *sdev, *tmp;
+	struct scsi_device *sdev;
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	starget->reap_ref++;
-	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
+ restart:
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
 		if (sdev->channel != starget->channel ||
-		    sdev->id != starget->id)
+		    sdev->id != starget->id ||
+		    sdev->sdev_state == SDEV_DEL)
 			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		scsi_remove_device(sdev);
 		spin_lock_irqsave(shost->host_lock, flags);
+		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	scsi_target_reap(starget);
Index: 2613/drivers/scsi/scsi_scan.c
===================================================================
--- 2613.orig/drivers/scsi/scsi_scan.c
+++ 2613/drivers/scsi/scsi_scan.c
@@ -1425,23 +1425,17 @@ EXPORT_SYMBOL(scsi_scan_single_target);
 
 void scsi_forget_host(struct Scsi_Host *shost)
 {
-	struct scsi_target *starget, *tmp;
+	struct scsi_device *sdev;
 	unsigned long flags;
 
-	/*
-	 * Ok, this look a bit strange.  We always look for the first device
-	 * on the list as scsi_remove_device removes them from it - thus we
-	 * also have to release the lock.
-	 * We don't need to get another reference to the device before
-	 * releasing the lock as we already own the reference from
-	 * scsi_register_device that's release in scsi_remove_device.  And
-	 * after that we don't look at sdev anymore.
-	 */
+ restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry_safe(starget, tmp, &shost->__targets, siblings) {
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
+		if (sdev->sdev_state == SDEV_DEL)
+			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
-		scsi_remove_target(&starget->dev);
-		spin_lock_irqsave(shost->host_lock, flags);
+		__scsi_remove_device(sdev);
+		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }

Re: [PATCH 4/5] SCSI scanning and removal fixes

[James Bottomley]

On Tue, 2005-07-26 at 10:27 -0400, Alan Stern wrote:
> This patch (as545) fixes the list traversals in __scsi_remove_target and 
> scsi_forget_host.  In each case the existing code list_for_each_entry_safe 
> in an _unsafe_ manner, because the list was not protected from outside 
> modification while the iteration was running.

Yes, but all you're doing is trying to close a race between add and
remove by trying the remove until there's nothing left.  That still
won't close the race window.

The solution is to enforce the state model:  Once the host goes into
cancel or del, no further additions may be made to the host list and if
we add the other patch to enforce the state model on removal then only
one process could be in remove as well.

> The new scsi_forget_host routine takes the moderately controversial step 
> of iterating over devices for removal rather than iterating over targets.  
> This makes more sense to me because the current scheme treats targets as 
> second-class citizens, created and removed on demand, rather than as 
> objects corresponding to actual hardware.  (Also I couldn't figure out any 
> safe way to iterate over the target list, since it's not so easy to tell 
> when a target has already been removed.)

Actually, in doing that, the target now never gets reaped, which would
leave us with dangling target objects.

James