scsi: restart list search after unlock in scsi_remove_target

Commit Message

Bart Van Assche Oct. 27, 2015, 8:14 p.m. UTC

On 10/26/2015 01:35 AM, Johannes Thumshirn wrote:
> I haven't heard anything from the original reporter of the lockup but
> my test's went all O.K., so
> 
> Tested-by: Johannes Thumshirn <[e-mail]>
> Reviewed-by: Johannes Thumshirn <[e-mail]>

Hello Christoph and Johannes,

How about the patch below, which is a variant of Christoph's patch ?

Thanks,

Bart.

[PATCH] scsi: Restart list search after unlock in scsi_remove_target

When dropping a lock while iterating a list we must restart the search
as other threads could have manipulated the list under us.  Without this
we can get stuck in an endless loop. Introduce a new member variable
"visible" in struct scsi_target to track presence in sysfs.

Reported-by: Johannes Thumshirn <[e-mail]>
---
 drivers/scsi/scsi_scan.c   | 16 +++-------------
 drivers/scsi/scsi_sysfs.c  | 19 ++++++-------------
 include/scsi/scsi_device.h |  1 +
 3 files changed, 10 insertions(+), 26 deletions(-)

Comments

Johannes Thumshirn Oct. 30, 2015, 8:26 a.m. UTC | #1

Hi Bart,
On Tue, 2015-10-27 at 13:14 -0700, Bart Van Assche wrote:
> On 10/26/2015 01:35 AM, Johannes Thumshirn wrote:
> > I haven't heard anything from the original reporter of the lockup
> > but
> > my test's went all O.K., so
> > 
> > Tested-by: Johannes Thumshirn <[e-mail]>
> > Reviewed-by: Johannes Thumshirn <[e-mail]>
> 
> Hello Christoph and Johannes,
> 
> How about the patch below, which is a variant of Christoph's patch ?
> 
> Thanks,
> 
> Bart.


I'm OK with it and I ran tests with it over night, no lockups happened.
I feel a bit reluctant of giving the patch to mt downsream reporter as
it's not their business to validate patches for me.

So from my POV it's OK as well. So please feel free to add

Tested-by: Johannes Thumshirn <[e-mail]>
Reviewed-by: Johannes Thumshirn <[e-mail]>

on the official submission.

Thanks,
	Johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..7b74470 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -314,7 +314,6 @@  static void scsi_target_destroy(struct scsi_target *starget)
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
 	unsigned long flags;
 
-	starget->state = STARGET_DEL;
 	transport_destroy_device(dev);
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->hostt->target_destroy)
@@ -379,12 +378,8 @@  static void scsi_target_reap_ref_release(struct kref *kref)
 	struct scsi_target *starget
 		= container_of(kref, struct scsi_target, reap_ref);
 
-	/*
-	 * if we get here and the target is still in the CREATED state that
-	 * means it was allocated but never made visible (because a scan
-	 * turned up no LUNs), so don't call device_del() on it.
-	 */
-	if (starget->state != STARGET_CREATED) {
+	if (starget->visible) {
+		starget->visible = false;
 		transport_remove_device(&starget->dev);
 		device_del(&starget->dev);
 	}
@@ -507,12 +502,7 @@  static struct scsi_target *scsi_alloc_target(struct device *parent,
  */
 void scsi_target_reap(struct scsi_target *starget)
 {
-	/*
-	 * serious problem if this triggers: STARGET_DEL is only set in the if
-	 * the reap_ref drops to zero, so we're trying to do another final put
-	 * on an already released kref
-	 */
-	BUG_ON(starget->state == STARGET_DEL);
+	starget->state = STARGET_DEL;
 	scsi_target_reap_ref_put(starget);
 }
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5e085d4..de8e202 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -974,7 +974,7 @@  static int scsi_target_add(struct scsi_target *starget)
 {
 	int error;
 
-	if (starget->state != STARGET_CREATED)
+	if (starget->visible)
 		return 0;
 
 	error = device_add(&starget->dev);
@@ -983,6 +983,7 @@  static int scsi_target_add(struct scsi_target *starget)
 		return error;
 	}
 	transport_add_device(&starget->dev);
+	starget->visible = true;
 	starget->state = STARGET_RUNNING;
 
 	pm_runtime_set_active(&starget->dev);
@@ -1166,31 +1167,23 @@  static void __scsi_remove_target(struct scsi_target *starget)
 void scsi_remove_target(struct device *dev)
 {
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
-	struct scsi_target *starget, *last = NULL;
+	struct scsi_target *starget;
 	unsigned long flags;
 
-	/* remove targets being careful to lookup next entry before
-	 * deleting the last
-	 */
+restart:
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_for_each_entry(starget, &shost->__targets, siblings) {
 		if (starget->state == STARGET_DEL)
 			continue;
 		if (starget->dev.parent == dev || &starget->dev == dev) {
-			/* assuming new targets arrive at the end */
 			kref_get(&starget->reap_ref);
 			spin_unlock_irqrestore(shost->host_lock, flags);
-			if (last)
-				scsi_target_reap(last);
-			last = starget;
 			__scsi_remove_target(starget);
-			spin_lock_irqsave(shost->host_lock, flags);
+			scsi_target_reap(starget);
+			goto restart;
 		}
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
-
-	if (last)
-		scsi_target_reap(last);
 }
 EXPORT_SYMBOL(scsi_remove_target);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index fe89d7c..d5a5f21 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -267,6 +267,7 @@  struct scsi_target {
 	unsigned int		expecting_lun_change:1;	/* A device has reported
 						 * a 3F/0E UA, other devices on
 						 * the same target will also. */
+	unsigned int		visible:1; /* visible in sysfs */
 	/* commands actually active on LLD. */
 	atomic_t		target_busy;
 	atomic_t		target_blocked;