More device removal fixes

scsi_run_queue() examines all SCSI devices that are present on the starved list. Since scsi_run_queue() unlocks the SCSI host lock before running a queue a SCSI device can get removed after it has been removed from the starved list and before its queue is run. Protect against that race condition by increasing the sdev refcount before running the sdev queue. Also, remove a SCSI device from the starved list before __scsi_remove_device() decreases the sdev refcount such that the get_device() call added in scsi_run_queue() is guaranteed to succeed.

Signed-off-by Bart Van Assche <[email protected]>

Reported-and-tested-by Chanho Min <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

Acked-by Tejun Heo <[email protected]>

Reviewed-by Mike Christie <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |   16 +++++++++++-----
 drivers/scsi/scsi_sysfs.c |   14 +++++++++++++-
 2 files changed, 24 insertions(+), 6 deletions(-)

drivers/scsi/scsi_lib.c +11 -5

index 86d5220..d6ca072 100644
@@ -456,11 +456,17 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
-		spin_unlock(shost->host_lock);
-		spin_lock(sdev->request_queue->queue_lock);
-		__blk_run_queue(sdev->request_queue);
-		spin_unlock(sdev->request_queue->queue_lock);
-		spin_lock(shost->host_lock);
+		/*
+		 * Obtain a reference before unlocking the host_lock such that
+		 * the sdev can't disappear before blk_run_queue() is invoked.
+		 */
+		get_device(&sdev->sdev_gendev);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+
+		blk_run_queue(sdev->request_queue);
+		put_device(&sdev->sdev_gendev);
+
+		spin_lock_irqsave(shost->host_lock, flags);
 	}
 	/* put any unprocessed entries back */
 	list_splice(&starved_list, &shost->starved_list);

drivers/scsi/scsi_sysfs.c +6 -1

index 931a7d9..34f1b39 100644
@@ -345,7 +345,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);

@@ -953,6 +952,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

@@ -974,6 +975,17 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
+	/*
+	 * Remove a SCSI device from the starved list after blk_cleanup_queue()
+	 * finished such that scsi_request_fn() can't add it back to that list.
+	 * Also remove an sdev from the starved list before invoking

I really don't like this because it's shuffling potentially fragile lifetime rules since you now have to have the sdev deleted from the starved list before final put. That becomes an unstated assumption within the code.

The theory is that the starved list processing may be racing with a scsi_remove_device, so when we unlock the host lock, the device (and the queue) may be destroyed. OK, so I agree with this, remote a possibility though it may be. The easy way of fixing it without making assumptions is this, isn't it? All it requires is that the queue be destroyed after the starved list entry is deleted in the sdev release code.

James

---

drivers/scsi/scsi_lib.c +11 -0

index 86d5220..f294cc6 100644
@@ -434,6 +434,7 @@ static void scsi_run_queue(struct request_queue *q)
  	list_splice_init(&shost->starved_list, &starved_list);
 
  	while (!list_empty(&starved_list)) {
+		struct request_queue *slq;
  		/*
  		 * As long as shost is accepting commands and we have
  		 * starved queues, call blk_run_queue. scsi_request_fn

@@ -456,10 +457,21 @@ static void scsi_run_queue(struct request_queue *q)
  			continue;
  		}
 
+		/*
+		 * once we drop the host lock, a racing scsi_remove_device may
+		 * remove the sdev from the starved list and destroy it and
+		 * the queue.  Mitigate by taking a reference to the queue and
+		 * never touching the sdev again after we drop the host lock.
+		 */
+		slq = sdev->request_queue;
+		if (!blk_get_queue(slq))
+			continue;
+
  		spin_unlock(shost->host_lock);

Quoted 49 lines · James Bottomley

I really don't like this because it's shuffling potentially fragile lifetime rules since you now have to have the sdev deleted from the starved list before final put. That becomes an unstated assumption within the code.

The theory is that the starved list processing may be racing with a scsi_remove_device, so when we unlock the host lock, the device (and the queue) may be destroyed. OK, so I agree with this, remote a possibility though it may be. The easy way of fixing it without making assumptions is this, isn't it? All it requires is that the queue be destroyed after the starved list entry is deleted in the sdev release code.

James

---

drivers/scsi/scsi_lib.c +14 -3

index 86d5220..f294cc6 100644
@@ -434,6 +434,7 @@ static void scsi_run_queue(struct request_queue *q)
  	list_splice_init(&shost->starved_list, &starved_list);
 
  	while (!list_empty(&starved_list)) {
+		struct request_queue *slq;
  		/*
  		 * As long as shost is accepting commands and we have
  		 * starved queues, call blk_run_queue. scsi_request_fn

@@ -456,10 +457,21 @@ static void scsi_run_queue(struct request_queue *q)
  			continue;
  		}
 
+		/*
+		 * once we drop the host lock, a racing scsi_remove_device may
+		 * remove the sdev from the starved list and destroy it and
+		 * the queue.  Mitigate by taking a reference to the queue and
+		 * never touching the sdev again after we drop the host lock.
+		 */
+		slq = sdev->request_queue;
+		if (!blk_get_queue(slq))
+			continue;
+
  		spin_unlock(shost->host_lock);
-		spin_lock(sdev->request_queue->queue_lock);
-		__blk_run_queue(sdev->request_queue);
-		spin_unlock(sdev->request_queue->queue_lock);
+
+		blk_run_queue(slq);
+		blk_put_queue(slq);

drivers/scsi/scsi_lib.c static void scsi_run_queue(struct request_queue *q) · line 477

Quoted 4 lines

drivers/scsi/scsi_lib.c +1 -0

+
  		spin_lock(shost->host_lock);
  	}
  	/* put any unprocessed entries back */

Since the above patch invokes blk_put_queue() with interrupts disabled it may cause blk_release_queue() to be invoked with interrupts disabled. Sorry but I'm not sure whether that will work fine.

Quoted 78 lines · Bart Van Assche

scsi_run_queue() examines all SCSI devices that are present on the starved list. Since scsi_run_queue() unlocks the SCSI host lock before running a queue a SCSI device can get removed after it has been removed from the starved list and before its queue is run. Protect against that race condition by increasing the sdev refcount before running the sdev queue. Also, remove a SCSI device from the starved list before __scsi_remove_device() decreases the sdev refcount such that the get_device() call added in scsi_run_queue() is guaranteed to succeed.

Signed-off-by Bart Van Assche <[email protected]>

Reported-and-tested-by Chanho Min <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

Acked-by Tejun Heo <[email protected]>

Reviewed-by Mike Christie <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |   16 +++++++++++-----
 drivers/scsi/scsi_sysfs.c |   14 +++++++++++++-
 2 files changed, 24 insertions(+), 6 deletions(-)

drivers/scsi/scsi_lib.c +11 -5

index 86d5220..d6ca072 100644
@@ -456,11 +456,17 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
-		spin_unlock(shost->host_lock);
-		spin_lock(sdev->request_queue->queue_lock);
-		__blk_run_queue(sdev->request_queue);
-		spin_unlock(sdev->request_queue->queue_lock);
-		spin_lock(shost->host_lock);
+		/*
+		 * Obtain a reference before unlocking the host_lock such that
+		 * the sdev can't disappear before blk_run_queue() is invoked.
+		 */
+		get_device(&sdev->sdev_gendev);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+
+		blk_run_queue(sdev->request_queue);
+		put_device(&sdev->sdev_gendev);
+
+		spin_lock_irqsave(shost->host_lock, flags);
 	}
 	/* put any unprocessed entries back */
 	list_splice(&starved_list, &shost->starved_list);

drivers/scsi/scsi_sysfs.c +6 -1

index 931a7d9..34f1b39 100644
@@ -345,7 +345,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);

@@ -953,6 +952,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

@@ -974,6 +975,17 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
+	/*
+	 * Remove a SCSI device from the starved list after blk_cleanup_queue()
+	 * finished such that scsi_request_fn() can't add it back to that list.
+	 * Also remove an sdev from the starved list before invoking

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 991

Quoted 10 lines

drivers/scsi/scsi_sysfs.c +7 -0

+	 * put_device() such that get_device() is guaranteed to succeed for an
+	 * sdev present on the starved list.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);

I really don't like this because it's shuffling potentially fragile lifetime rules since you now have to have the sdev deleted from the starved list before final put. That becomes an unstated assumption within the code.

The theory is that the starved list processing may be racing with a scsi_remove_device, so when we unlock the host lock, the device (and the queue) may be destroyed. OK, so I agree with this, remote a possibility though it may be. The easy way of fixing it without making assumptions is this, isn't it? All it requires is that the queue be destroyed after the starved list entry is deleted in the sdev release code.

James

---

drivers/scsi/scsi_lib.c +11 -0

index 86d5220..f294cc6 100644
@@ -434,6 +434,7 @@ static void scsi_run_queue(struct request_queue *q)
 	list_splice_init(&shost->starved_list, &starved_list);
 
 	while (!list_empty(&starved_list)) {
+		struct request_queue *slq;
 		/*
 		 * As long as shost is accepting commands and we have
 		 * starved queues, call blk_run_queue. scsi_request_fn

@@ -456,10 +457,21 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
+		/*
+		 * once we drop the host lock, a racing scsi_remove_device may
+		 * remove the sdev from the starved list and destroy it and
+		 * the queue.  Mitigate by taking a reference to the queue and
+		 * never touching the sdev again after we drop the host lock.
+		 */
+		slq = sdev->request_queue;
+		if (!blk_get_queue(slq))
+			continue;
+
 		spin_unlock(shost->host_lock);

Quoted 135 lines · James Bottomley

Quoted 78 lines · Bart Van Assche

scsi_run_queue() examines all SCSI devices that are present on the starved list. Since scsi_run_queue() unlocks the SCSI host lock before running a queue a SCSI device can get removed after it has been removed from the starved list and before its queue is run. Protect against that race condition by increasing the sdev refcount before running the sdev queue. Also, remove a SCSI device from the starved list before __scsi_remove_device() decreases the sdev refcount such that the get_device() call added in scsi_run_queue() is guaranteed to succeed.

Signed-off-by Bart Van Assche <[email protected]>

Reported-and-tested-by Chanho Min <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

Acked-by Tejun Heo <[email protected]>

Reviewed-by Mike Christie <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |   16 +++++++++++-----
 drivers/scsi/scsi_sysfs.c |   14 +++++++++++++-
 2 files changed, 24 insertions(+), 6 deletions(-)

drivers/scsi/scsi_lib.c +11 -5

index 86d5220..d6ca072 100644
@@ -456,11 +456,17 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
-		spin_unlock(shost->host_lock);
-		spin_lock(sdev->request_queue->queue_lock);
-		__blk_run_queue(sdev->request_queue);
-		spin_unlock(sdev->request_queue->queue_lock);
-		spin_lock(shost->host_lock);
+		/*
+		 * Obtain a reference before unlocking the host_lock such that
+		 * the sdev can't disappear before blk_run_queue() is invoked.
+		 */
+		get_device(&sdev->sdev_gendev);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+
+		blk_run_queue(sdev->request_queue);
+		put_device(&sdev->sdev_gendev);
+
+		spin_lock_irqsave(shost->host_lock, flags);
 	}
 	/* put any unprocessed entries back */
 	list_splice(&starved_list, &shost->starved_list);

drivers/scsi/scsi_sysfs.c +6 -1

index 931a7d9..34f1b39 100644
@@ -345,7 +345,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);

@@ -953,6 +952,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

@@ -974,6 +975,17 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
+	/*
+	 * Remove a SCSI device from the starved list after blk_cleanup_queue()
+	 * finished such that scsi_request_fn() can't add it back to that list.
+	 * Also remove an sdev from the starved list before invoking

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 991

Quoted 10 lines

drivers/scsi/scsi_sysfs.c +7 -0

+	 * put_device() such that get_device() is guaranteed to succeed for an
+	 * sdev present on the starved list.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);

I really don't like this because it's shuffling potentially fragile lifetime rules since you now have to have the sdev deleted from the starved list before final put. That becomes an unstated assumption within the code.

The theory is that the starved list processing may be racing with a scsi_remove_device, so when we unlock the host lock, the device (and the queue) may be destroyed. OK, so I agree with this, remote a possibility though it may be. The easy way of fixing it without making assumptions is this, isn't it? All it requires is that the queue be destroyed after the starved list entry is deleted in the sdev release code.

James

---

drivers/scsi/scsi_lib.c +11 -0

index 86d5220..f294cc6 100644
@@ -434,6 +434,7 @@ static void scsi_run_queue(struct request_queue *q)
 	list_splice_init(&shost->starved_list, &starved_list);
 
 	while (!list_empty(&starved_list)) {
+		struct request_queue *slq;
 		/*
 		 * As long as shost is accepting commands and we have
 		 * starved queues, call blk_run_queue. scsi_request_fn

@@ -456,10 +457,21 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
+		/*
+		 * once we drop the host lock, a racing scsi_remove_device may
+		 * remove the sdev from the starved list and destroy it and
+		 * the queue.  Mitigate by taking a reference to the queue and
+		 * never touching the sdev again after we drop the host lock.
+		 */
+		slq = sdev->request_queue;
+		if (!blk_get_queue(slq))
+			continue;
+
 		spin_unlock(shost->host_lock);

drivers/scsi/scsi_lib.c static void scsi_run_queue(struct request_queue *q) · line 477

Quoted 10 lines

drivers/scsi/scsi_lib.c +4 -3

-		spin_lock(sdev->request_queue->queue_lock);
-		__blk_run_queue(sdev->request_queue);
-		spin_unlock(sdev->request_queue->queue_lock);
+
+		blk_run_queue(slq);
+		blk_put_queue(slq);
+
 		spin_lock(shost->host_lock);
 	}
 	/* put any unprocessed entries back */

I think we will hit issues with the scsi_device being freed too soon still.

1. In thread 1, __scsi_remove_device runs. It cleans up the commands and it does the last put_device. It left the sdev on the starved list though.

2. In thread 2, scsi_run_queue runs and takes the dev off the starved list and calls into block layer __blk_run_queue.

3. scsi_device_dev_release_usercontext runs and frees the scsi_device.

4. __blk_run_queue from #2 runs and calls into scsi_request_fn. We now reference the freed sdev at the top of scsi_request_fn.

Now that all scsi_request_fn() callers hold a reference on the SCSI device that function is invoked for and since blk_cleanup_queue() waits until scsi_request_fn() has finished it is safe to remove the get_device() / put_device() pair from scsi_request_fn().

Signed-off-by Bart Van Assche <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc James Bottomley <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Mike Christie <[email protected]>

---

Now that all scsi_request_fn() callers hold a reference on the SCSI device that function is invoked for

Quoted 2 lines · Bart Van Assche

Now that all scsi_request_fn() callers hold a reference on the SCSI device that function is invoked for

What makes you think that this is a true statement? The usual caller is the block layer, which doesn't really know anything about the sdev->sdev_gendev.

Quoted 7 lines · James Bottomley

Quoted 2 lines · Bart Van Assche

Now that all scsi_request_fn() callers hold a reference on the SCSI device that function is invoked for

What makes you think that this is a true statement? The usual caller is the block layer, which doesn't really know anything about the sdev->sdev_gendev.

The reasoning behind that comment is as follows:
* The block layer guarantees that the reference count of a request queue is >= 1 as long as a request_fn() call is in progress (see also blk_cleanup_queue(), the __blk_drain_queue() call in that function and the loop in __blk_drain_queue() that waits until request_fn_active == 0).
* The SCSI core guarantees that blk_cleanup_queue() is invoked before the final put on sdev->sdev_gendev.

Quoted 9 lines · Bart Van Assche

Quoted 7 lines · James Bottomley

Quoted 2 lines · Bart Van Assche

Now that all scsi_request_fn() callers hold a reference on the SCSI device that function is invoked for

What makes you think that this is a true statement? The usual caller is the block layer, which doesn't really know anything about the sdev->sdev_gendev.

Quoted 8 lines

The reasoning behind that comment is as follows:
* The block layer guarantees that the reference count of a request queue is >= 1 as long as a request_fn() call is in progress (see also blk_cleanup_queue(), the __blk_drain_queue() call in that function and the loop in __blk_drain_queue() that waits until request_fn_active == 0).
* The SCSI core guarantees that blk_cleanup_queue() is invoked before

    the final put on sdev->sdev_gendev.

That's still unclear.

I think a clear explanation is something like:

scsi_devices may only be removed by by scsi_remove_device() which has a call to blk_cleanup_queue() before the final put of sdev->sdev_gendev. Since blk_cleanup_queue() waits for the block queue to drain and then tears it down, scsi_request_fn cannot be active once blk_cleanup_queue() returns and hence the get_device/put_device pairs in scsi_request_fn are unnecessary because the final put will always be done by scsi_remove_device().

This, by the way, is an optimisation not a fix, so it shouldn't really be part of a series labelled "device removal fixes"

SCSI devices are added to the shost->__devices list from inside scsi_alloc_sdev(). If something goes wrong during LUN scanning, e.g. a transport layer failure occurs, then __scsi_remove_device() can get invoked by the LUN scanning code for a SCSI device in state SDEV_CREATED_BLOCK or SDEV_BLOCKED. If this happens then the SCSI device has not yet been added to sysfs (is_visible == 0). Make sure that if this happens these devices are transitioned into state SDEV_DEL. This avoids that __scsi_remove_device() gets invoked a second time by scsi_forget_host().

Signed-off-by Bart Van Assche <[email protected]>

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Tejun Heo <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    2 ++
 drivers/scsi/scsi_sysfs.c |    7 ++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

drivers/scsi/scsi_lib.c +2 -0

index 03d4165..d57b5e1 100644
@@ -2175,6 +2175,8 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 		case SDEV_OFFLINE:
 		case SDEV_TRANSPORT_OFFLINE:
 		case SDEV_CANCEL:
+		case SDEV_BLOCK:
+		case SDEV_CREATED_BLOCK:
 			break;
 		default:
 			goto illegal;

drivers/scsi/scsi_sysfs.c +3 -2

index 34f1b39..f869ef85 100644
@@ -956,8 +956,9 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	unsigned long flags;
 
 	if (sdev->is_visible) {
-		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-			return;
+		WARN(scsi_device_set_state(sdev, SDEV_CANCEL) != 0,
+		     "%s: unexpected state %d\n", dev_name(&sdev->sdev_gendev),
+		     sdev->sdev_state);
 
 		bsg_unregister_queue(sdev->request_queue);
 		device_unregister(&sdev->sdev_dev);

Quoted 47 lines · Bart Van Assche

SCSI devices are added to the shost->__devices list from inside scsi_alloc_sdev(). If something goes wrong during LUN scanning, e.g. a transport layer failure occurs, then __scsi_remove_device() can get invoked by the LUN scanning code for a SCSI device in state SDEV_CREATED_BLOCK or SDEV_BLOCKED. If this happens then the SCSI device has not yet been added to sysfs (is_visible == 0). Make sure that if this happens these devices are transitioned into state SDEV_DEL. This avoids that __scsi_remove_device() gets invoked a second time by scsi_forget_host().

Signed-off-by Bart Van Assche <[email protected]>

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Tejun Heo <[email protected]>

---
  drivers/scsi/scsi_lib.c   |    2 ++
  drivers/scsi/scsi_sysfs.c |    7 ++++---
  2 files changed, 6 insertions(+), 3 deletions(-)

drivers/scsi/scsi_lib.c +2 -0

index 03d4165..d57b5e1 100644
@@ -2175,6 +2175,8 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
  		case SDEV_OFFLINE:
  		case SDEV_TRANSPORT_OFFLINE:
  		case SDEV_CANCEL:
+		case SDEV_BLOCK:
+		case SDEV_CREATED_BLOCK:
  			break;
  		default:
  			goto illegal;

drivers/scsi/scsi_sysfs.c +3 -2

index 34f1b39..f869ef85 100644
@@ -956,8 +956,9 @@ void __scsi_remove_device(struct scsi_device *sdev)
  	unsigned long flags;
 
  	if (sdev->is_visible) {
-		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-			return;
+		WARN(scsi_device_set_state(sdev, SDEV_CANCEL) != 0,
+		     "%s: unexpected state %d\n", dev_name(&sdev->sdev_gendev),
+		     sdev->sdev_state);
 

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 977

Quoted 10 lines

drivers/scsi/scsi_sysfs.c +1 -1

  		bsg_unregister_queue(sdev->request_queue);
  		device_unregister(&sdev->sdev_dev);

@@ -971,7 +972,7 @@ void __scsi_remove_device(struct scsi_device *sdev)
  	 * scsi_run_queue() invocations have finished before tearing down the
  	 * device.
  	 */
-	scsi_device_set_state(sdev, SDEV_DEL);
+	WARN_ON_ONCE(scsi_device_set_state(sdev, SDEV_DEL) != 0);
  	blk_cleanup_queue(sdev->request_queue);
  	cancel_work_sync(&sdev->requeue_work);

The Addition to scsi_device_set_state looks ok. I was not sure why we need the new WARNs though. Do we still think there are other cases?

SCSI devices are added to the shost->__devices list from inside scsi_alloc_sdev(). If something goes wrong during LUN scanning, e.g. a transport layer failure occurs, then __scsi_remove_device() can get invoked by the LUN scanning code for a SCSI device in state SDEV_CREATED_BLOCK or SDEV_BLOCKED. If this happens then the SCSI device has not yet been added to sysfs (is_visible == 0). Make sure that if this happens these devices are transitioned into state SDEV_DEL. This avoids that __scsi_remove_device() gets invoked a second time by scsi_forget_host().

Quoted 9 lines · Bart Van Assche

SCSI devices are added to the shost->__devices list from inside scsi_alloc_sdev(). If something goes wrong during LUN scanning, e.g. a transport layer failure occurs, then __scsi_remove_device() can get invoked by the LUN scanning code for a SCSI device in state SDEV_CREATED_BLOCK or SDEV_BLOCKED. If this happens then the SCSI device has not yet been added to sysfs (is_visible == 0). Make sure that if this happens these devices are transitioned into state SDEV_DEL. This avoids that __scsi_remove_device() gets invoked a second time by scsi_forget_host().

The current principle is that scsi_remove_device can fail, so the condition you're avoiding is expected. If you want to make it always succeed, we have to worry about any device state racing with an asynchronous remove, which looks like a whole nasty can of worms.

The change log makes it sound like what you actually want to enable is the ability to remove devices which fail probing but which are in the blocked state, so why not just respin with only that, which is just adding the blocked states to the ->SDEV_DEL state transitions?

Quoted 16 lines · James Bottomley

Quoted 9 lines · Bart Van Assche

SCSI devices are added to the shost->__devices list from inside scsi_alloc_sdev(). If something goes wrong during LUN scanning, e.g. a transport layer failure occurs, then __scsi_remove_device() can get invoked by the LUN scanning code for a SCSI device in state SDEV_CREATED_BLOCK or SDEV_BLOCKED. If this happens then the SCSI device has not yet been added to sysfs (is_visible == 0). Make sure that if this happens these devices are transitioned into state SDEV_DEL. This avoids that __scsi_remove_device() gets invoked a second time by scsi_forget_host().

The current principle is that scsi_remove_device can fail, so the condition you're avoiding is expected. If you want to make it always succeed, we have to worry about any device state racing with an asynchronous remove, which looks like a whole nasty can of worms.

Quoted 4 lines

The change log makes it sound like what you actually want to enable is the ability to remove devices which fail probing but which are in the blocked state, so why not just respin with only that, which is just adding the blocked states to the ->SDEV_DEL state transitions?

If what you had in mind is the patch below, I think we agree:

drivers/scsi/scsi_lib.c +0 -0

index e3d6276..eaea242 100644

Quoted 24 lines · Bart Van Assche

Quoted 16 lines · James Bottomley

Quoted 9 lines · Bart Van Assche

SCSI devices are added to the shost->__devices list from inside scsi_alloc_sdev(). If something goes wrong during LUN scanning, e.g. a transport layer failure occurs, then __scsi_remove_device() can get invoked by the LUN scanning code for a SCSI device in state SDEV_CREATED_BLOCK or SDEV_BLOCKED. If this happens then the SCSI device has not yet been added to sysfs (is_visible == 0). Make sure that if this happens these devices are transitioned into state SDEV_DEL. This avoids that __scsi_remove_device() gets invoked a second time by scsi_forget_host().

The current principle is that scsi_remove_device can fail, so the condition you're avoiding is expected. If you want to make it always succeed, we have to worry about any device state racing with an asynchronous remove, which looks like a whole nasty can of worms.

Quoted 4 lines

The change log makes it sound like what you actually want to enable is the ability to remove devices which fail probing but which are in the blocked state, so why not just respin with only that, which is just adding the blocked states to the ->SDEV_DEL state transitions?

If what you had in mind is the patch below, I think we agree:

drivers/scsi/scsi_lib.c scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state) · line 2189

Quoted 10 lines

drivers/scsi/scsi_lib.c +2 -0

index e3d6276..eaea242 100644
@@ -2185,6 +2185,8 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
  		case SDEV_OFFLINE:
  		case SDEV_TRANSPORT_OFFLINE:
  		case SDEV_CANCEL:
+		case SDEV_BLOCK:
+		case SDEV_CREATED_BLOCK:

Something like this, yes. For the probe lun case, we have to be in CREATED, so any block action transitions only to CREATED_BLOCK. The BLOCK->DEL transition can only be a result of an async remove racing with bringup, can't it? Which is something I think we still want to forbid.

Changing the state of a SCSI device via sysfs into "cancel" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this. Also, introduce the symbolic name INVALID_SDEV_STATE, representing a value different from any valid

Quoted 4 lines · Bart Van Assche

Changing the state of a SCSI device via sysfs into "cancel" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this. Also, introduce the symbolic name INVALID_SDEV_STATE, representing a value different from any valid

Maybe it should be named SDEV_INVALID_STATE to match the current naming scheme? Other than that, it seems ok.

Changing the state of a SCSI device via sysfs into "cancel" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this. Also, introduce the symbolic name INVALID_SDEV_STATE, representing a value different from any valid SCSI device state. Update scsi_device_set_state() such that gcc does not issue a warning about an enumeration value not being handled inside a switch statement.

Quoted 7 lines · Bart Van Assche

Changing the state of a SCSI device via sysfs into "cancel" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this. Also, introduce the symbolic name INVALID_SDEV_STATE, representing a value different from any valid SCSI device state. Update scsi_device_set_state() such that gcc does not issue a warning about an enumeration value not being handled inside a switch statement.

zero is the invalid state, that's why the SDEV_ states start at 1. Using a bare zero also means that gcc doesn't have to consider it in the switch statement, so there's no need to introduce a new one.

If we want to try to babysit user initiated state changes, then it looks like OFFLINE<->RUNNING might be the only useful ones?

Quoted 13 lines · James Bottomley

Quoted 7 lines · Bart Van Assche

Changing the state of a SCSI device via sysfs into "cancel" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this. Also, introduce the symbolic name INVALID_SDEV_STATE, representing a value different from any valid SCSI device state. Update scsi_device_set_state() such that gcc does not issue a warning about an enumeration value not being handled inside a switch statement.

zero is the invalid state, that's why the SDEV_ states start at 1. Using a bare zero also means that gcc doesn't have to consider it in the switch statement, so there's no need to introduce a new one.

Quoted 2 lines

If we want to try to babysit user initiated state changes, then it looks like OFFLINE<->RUNNING might be the only useful ones?

How about the BLOCKED<>RUNNING and QUIESCE<>RUNNING transitions ? I think it may be useful for a user to trigger these as well.

Since it is not allowed to invoke scsi_remove_host() with interrupts disabled, avoid saving and restoring the interrupt state inside scsi_remove_host(). This patch does not change the functionality of the function scsi_remove_host().

Signed-off-by Bart Van Assche <[email protected]>

Acked-by Tejun Heo <[email protected]>

Acked-by Hannes Reinecke <[email protected]>

Cc Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

A SCSI LLD may start cleaning up host resources as soon as scsi_remove_host() returns. These host resources may be needed by the LLD in an implementation of one of the eh_* functions. So if one of the eh_* functions is in progress when scsi_remove_host() is invoked, wait until the eh_* function has finished. Also, do not invoke any of the eh_* functions after scsi_remove_host() has started. Remove Scsi_Host.tmf_in_progress because it is now superfluous.

Quoted 8 lines · Bart Van Assche

A SCSI LLD may start cleaning up host resources as soon as scsi_remove_host() returns. These host resources may be needed by the LLD in an implementation of one of the eh_* functions. So if one of the eh_* functions is in progress when scsi_remove_host() is invoked, wait until the eh_* function has finished. Also, do not invoke any of the eh_* functions after scsi_remove_host() has started. Remove Scsi_Host.tmf_in_progress because it is now superfluous.

I think the patch looks ok for drivers that do not implement their own eh_strategy_handler, but what about SAS? If you added a scsi_begin_eh in scsi_error_handler before the eh_strategy_handler is called and then add a scsi_end_eh after it is called, I think it would cover them too.

Quoted 4 lines

(quoted diff) +0 -0

@@ -1894,6 +1962,9 @@ int scsi_error_handler(void *data)
      }
      __set_current_state(TASK_RUNNING);
 

Review int scsi_error_handler(void *data) · line 1970

Quoted 6 lines

(quoted diff) +3 -0

+    WARN_ONCE(shost->eh_active, "scsi_eh_%d: eh_active = %d\n",
+          shost->host_no, shost->eh_active);
+
      SCSI_LOG_ERROR_RECOVERY(1,
          printk("Error handler scsi_eh_%d exiting\n", shost->host_no));
      shost->ehandler = NULL;

What is the warn for? Is there a chance this can happen with some non upstream driver or are you just adding it just in case?

A SCSI LLD may start cleaning up host resources as soon as scsi_remove_host() returns. These host resources may be needed by the LLD in an implementation of one of the eh_* functions. So if one of the eh_* functions is in progress when scsi_remove_host() is invoked, wait until the eh_* function has finished. Also, do not invoke any of the eh_* functions after scsi_remove_host() has started.

Quoted 7 lines · Bart Van Assche

A SCSI LLD may start cleaning up host resources as soon as scsi_remove_host() returns. These host resources may be needed by the LLD in an implementation of one of the eh_* functions. So if one of the eh_* functions is in progress when scsi_remove_host() is invoked, wait until the eh_* function has finished. Also, do not invoke any of the eh_* functions after scsi_remove_host() has started.

We already have state guards for this, don't we? That's the SHOST_*_RECOVERY ones. When eh functions are active, the host transitions to a recovery state, so the wait could just wait on that state rather than implement an open coded counting semaphore.

However, what's the reasoning behind wanting to do this? In theory all necessary resources for the eh thread should only be freed in the release callback. That means they aren't freed until all error recovery completes.

Quoted 9 lines · James Bottomley

Quoted 7 lines · Bart Van Assche

A SCSI LLD may start cleaning up host resources as soon as scsi_remove_host() returns. These host resources may be needed by the LLD in an implementation of one of the eh_* functions. So if one of the eh_* functions is in progress when scsi_remove_host() is invoked, wait until the eh_* function has finished. Also, do not invoke any of the eh_* functions after scsi_remove_host() has started.

Quoted 5 lines

We already have state guards for this, don't we? That's the SHOST_*_RECOVERY ones. When eh functions are active, the host transitions to a recovery state, so the wait could just wait on that state rather than implement an open coded counting semaphore.

That seems better. For the sg_reset_provider case we just would have to also wait on the tmf_in_progress bit.

I think I used all my credits messing up reviewing this patchset.

Quoted 4 lines

However, what's the reasoning behind wanting to do this? In theory all necessary resources for the eh thread should only be freed in the release callback. That means they aren't freed until all error recovery completes.

I think it makes it easier to handle cleanup of driver resources needed
for aborts/resets for some drivers. If after host removal, the scsi eh can call into the driver after scsi_remove_host is called then we have to set some internal bits to fail future eh callback calls and handle waiting on or flushing running eh operations. If we know that after scsi_host_remove is called the eh callbacks will not be running and will
not be called we can just free the driver resources.

For iscsi and I think drivers that do scsi_remove_target it would be helpful to have something similar at the target level.

Quoted 15 lines

Quoted 4 lines

However, what's the reasoning behind wanting to do this? In theory all necessary resources for the eh thread should only be freed in the release callback. That means they aren't freed until all error recovery completes.

I think it makes it easier to handle cleanup of driver resources needed
for aborts/resets for some drivers. If after host removal, the scsi eh can call into the driver after scsi_remove_host is called then we have to set some internal bits to fail future eh callback calls and handle waiting on or flushing running eh operations. If we know that after scsi_host_remove is called the eh callbacks will not be running and will
not be called we can just free the driver resources.

Quoted 2 lines

For iscsi and I think drivers that do scsi_remove_target it would be helpful to have something similar at the target level.

I'm wary of this because it combines two models: a definite state model (where we move from state to state waiting for the completions) with an event driven one (in theory the current one); such combinations rarely lead to good things because you get a mixture of actions causing state transitions some of which are waited for and some of which have an async transition and that ends up confusing the heck out of everybody no matter how carefully documented. Can you give me some use cases of what you're trying to achieve? Could it be as simple as an event that fires on release?

Quoted 18 lines · James Bottomley

Quoted 15 lines

Quoted 4 lines

However, what's the reasoning behind wanting to do this? In theory all necessary resources for the eh thread should only be freed in the release callback. That means they aren't freed until all error recovery completes.

I think it makes it easier to handle cleanup of driver resources needed
for aborts/resets for some drivers. If after host removal, the scsi eh can call into the driver after scsi_remove_host is called then we have to set some internal bits to fail future eh callback calls and handle waiting on or flushing running eh operations. If we know that after scsi_host_remove is called the eh callbacks will not be running and will
not be called we can just free the driver resources.

Quoted 2 lines

For iscsi and I think drivers that do scsi_remove_target it would be helpful to have something similar at the target level.

Quoted 9 lines

I'm wary of this because it combines two models: a definite state model (where we move from state to state waiting for the completions) with an event driven one (in theory the current one); such combinations rarely lead to good things because you get a mixture of actions causing state transitions some of which are waited for and some of which have an async transition and that ends up confusing the heck out of everybody no matter how carefully documented. Can you give me some use cases of what you're trying to achieve? Could it be as simple as an event that fires on release?

The problem that we hit in iscsi is that we will call scsi_remove_target (we used to call scsi_remove_host when we always did a host per target so we hit the problem at that level before). That will complete, but the scsi eh might still be trying to abort/reset devices accessed through that target. To avoid freeing resources that the iscsi scsi eh might be using, we set internal state bits and wait on host_busy to go to zero before we tear down the iscsi session, conn and task structs.

I think Bart was hitting a similar issue but a level up in the host removal case, because srp always does a host per target and so it just does a scsi_remove_host..

Quoted 15 lines · Mike Christie

Quoted 13 lines · James Bottomley

Quoted 7 lines · Bart Van Assche

A SCSI LLD may start cleaning up host resources as soon as scsi_remove_host() returns. These host resources may be needed by the LLD in an implementation of one of the eh_* functions. So if one of the eh_* functions is in progress when scsi_remove_host() is invoked, wait until the eh_* function has finished. Also, do not invoke any of the eh_* functions after scsi_remove_host() has started.

We already have state guards for this, don't we? That's the SHOST_*_RECOVERY ones. When eh functions are active, the host transitions to a recovery state, so the wait could just wait on that state rather than implement an open coded counting semaphore.

Quoted 2 lines

That seems better. For the sg_reset_provider case we just would have to also wait on the tmf_in_progress bit.

The simplest way is may just be to move the kthread_stop() from release to remove. That synchronously waits for the outstanding error handling to complete and the eh thread to stop. Perhaps the eh thread should also wait for tmf in progress before it dies?

Quoted 16 lines · James Bottomley

Quoted 15 lines · Mike Christie

Quoted 13 lines · James Bottomley

Quoted 7 lines · Bart Van Assche

A SCSI LLD may start cleaning up host resources as soon as scsi_remove_host() returns. These host resources may be needed by the LLD in an implementation of one of the eh_* functions. So if one of the eh_* functions is in progress when scsi_remove_host() is invoked, wait until the eh_* function has finished. Also, do not invoke any of the eh_* functions after scsi_remove_host() has started.

We already have state guards for this, don't we? That's the SHOST_*_RECOVERY ones. When eh functions are active, the host transitions to a recovery state, so the wait could just wait on that state rather than implement an open coded counting semaphore.

Quoted 7 lines

Quoted 2 lines

That seems better. For the sg_reset_provider case we just would have to also wait on the tmf_in_progress bit.

The simplest way is may just be to move the kthread_stop() from release to remove. That synchronously waits for the outstanding error handling to complete and the eh thread to stop. Perhaps the eh thread should also wait for tmf in progress before it dies?

Regarding TMF that are in progress: my preference is to leave it to the LLD to wait for any TMF in progress if necessary. At least with SRP over RDMA it is possible to prevent receiving further TMF completion notifications by closing the connection over which these TMF were sent.

There is a difference though between moving the EH kthread_stop() call and the patch at the start of this thread: moving the EH kthread_stop() call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_* callback after scsi_remove_host() has finished. However, the scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can cause an eh_* callback to be invoked after scsi_remove_device() finished.

Quoted 6 lines · Bart Van Assche

There is a difference though between moving the EH kthread_stop() call and the patch at the start of this thread: moving the EH kthread_stop() call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_* callback after scsi_remove_host() has finished. However, the scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can cause an eh_* callback to be invoked after scsi_remove_device() finished.

OK, but this doesn't tell me what you're trying to achieve.

An eh function is allowable as long as the host hadn't had the release callback executed. That means you must have to have a reference to the device/host to execute the eh function, which is currently guaranteed for all invocations.

Quoted 8 lines · James Bottomley

Quoted 6 lines · Bart Van Assche

There is a difference though between moving the EH kthread_stop() call and the patch at the start of this thread: moving the EH kthread_stop() call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_* callback after scsi_remove_host() has finished. However, the scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can cause an eh_* callback to be invoked after scsi_remove_device() finished.

Quoted 6 lines

OK, but this doesn't tell me what you're trying to achieve.

An eh function is allowable as long as the host hadn't had the release callback executed. That means you must have to have a reference to the device/host to execute the eh function, which is currently guaranteed for all invocations.

That raises a new question: how is an LLD expected to clean up resources without triggering a race condition ? What you wrote means that it's not safe for an LLD to start cleaning up the resources needed by the eh_* callbacks immediately after scsi_remove_device() returns since it it not guaranteed that at that time all references to the device have already been dropped.

Quoted 16 lines · Bart Van Assche

Quoted 14 lines · James Bottomley

Quoted 6 lines · Bart Van Assche

There is a difference though between moving the EH kthread_stop() call and the patch at the start of this thread: moving the EH kthread_stop() call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_* callback after scsi_remove_host() has finished. However, the scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can cause an eh_* callback to be invoked after scsi_remove_device() finished.

OK, but this doesn't tell me what you're trying to achieve.

An eh function is allowable as long as the host hadn't had the release callback executed. That means you must have to have a reference to the device/host to execute the eh function, which is currently guaranteed for all invocations.

Quoted 2 lines

That raises a new question: how is an LLD expected to clean up resources without triggering a race condition ? What you wrote means that it's not safe for an LLD to start cleaning up the resources needed by the eh_* callbacks immediately after scsi_remove_device() returns since it it not guaranteed that at that time all references to the device have already been dropped.

A callback in the device/target/host (whatever is needed) release function would do this right? If I understand James right, I think he suggested something like this in another mail.

If I look at what we actually do: all the HBAs treat scsi_remove_host as a waited for transition. The reason this works is the loop over __scsi_remove_device() in scsi_forget_host(). By the time that loop returns, every scsi_device is gone (and so is every target). Because blk_cleanup_queue() induces a synchronous wait for the queue to die in __scsi_remove_device(), there can be no outstanding I/O and no eh activity for the device when it returns (and no possibility of starting any). Thus at the end of scsi_forget_host, we have no devices to start I/O and no eh activity, so the final put will be the last.

Quoted 16 lines · Bart Van Assche

Quoted 8 lines · James Bottomley

Quoted 6 lines · Bart Van Assche

There is a difference though between moving the EH kthread_stop() call and the patch at the start of this thread: moving the EH kthread_stop() call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_* callback after scsi_remove_host() has finished. However, the scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can cause an eh_* callback to be invoked after scsi_remove_device() finished.

Quoted 6 lines

OK, but this doesn't tell me what you're trying to achieve.

An eh function is allowable as long as the host hadn't had the release callback executed. That means you must have to have a reference to the device/host to execute the eh function, which is currently guaranteed for all invocations.

Quoted 6 lines

That raises a new question: how is an LLD expected to clean up resources without triggering a race condition ? What you wrote means that it's not safe for an LLD to start cleaning up the resources needed by the eh_* callbacks immediately after scsi_remove_device() returns since it it not guaranteed that at that time all references to the device have already been dropped.

A callback in the device/target/host (whatever is needed) release function would do this right? If I understand James right, I think he suggested something like this in another mail.

Quoted 24 lines · Michael Christie

Quoted 16 lines · Bart Van Assche

Quoted 8 lines · James Bottomley

Quoted 6 lines · Bart Van Assche

There is a difference though between moving the EH kthread_stop() call and the patch at the start of this thread: moving the EH kthread_stop() call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_* callback after scsi_remove_host() has finished. However, the scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can cause an eh_* callback to be invoked after scsi_remove_device() finished.

Quoted 6 lines

OK, but this doesn't tell me what you're trying to achieve.

An eh function is allowable as long as the host hadn't had the release callback executed. That means you must have to have a reference to the device/host to execute the eh function, which is currently guaranteed for all invocations.

Quoted 6 lines

That raises a new question: how is an LLD expected to clean up resources without triggering a race condition ? What you wrote means that it's not safe for an LLD to start cleaning up the resources needed by the eh_* callbacks immediately after scsi_remove_device() returns since it it not guaranteed that at that time all references to the device have already been dropped.

Quoted 3 lines

A callback in the device/target/host (whatever is needed) release function would do this right? If I understand James right, I think he suggested something like this in another mail.

(replying to an e-mail of seven months ago - see also
http://thread.gmane.org/gmane.linux.scsi/82572/focus=82822)

Hello Mike,

Sorry but I'm afraid that making the SCSI core invoke a callback function from a device, target or host release function would create a new challenge, namely making sure that all these callback functions have finished before the module is unloaded that contains the SCSI host template and the code implementing such a callback function. That challenge is not specific to the SCSI infrastructure but is a general question that has not yet been solved in the Linux kernel (see e.g. "[PATCH] kobject: provide kobject_put_wait to fix module unload race" for a more general discussion about how to ensure that kobject release functions are invoked before the module is unloaded that owns the release function - http://thread.gmane.org/gmane.linux.kernel/1622885).

Or maybe this just means that I misunderstood you ?

In case it is not clear why I'm reviving this discussion: now that the "improved eh timeout handler" patch is upstream (commit e494f6a728394ab0df194342549ee20e6f0752df) there is an additional way in which the SCSI core can invoke an EH function concurrently with or after scsi_remove_host() has finished, namely from the TMF work queue (tmf_work_q).

Quoted 4 lines · Bart Van Assche

On 06/25/13 18:13, Michael Christie wrote:
Sorry but I'm afraid that making the SCSI core invoke a callback function from a device, target or host release function would create a new challenge, namely making sure that all these callback functions have

Quoted 8 lines

finished before the module is unloaded that contains the SCSI host template and the code implementing such a callback function. That challenge is not specific to the SCSI infrastructure but is a general question that has not yet been solved in the Linux kernel (see e.g. "[PATCH] kobject: provide kobject_put_wait to fix module unload race" for a more general discussion about how to ensure that kobject release functions are invoked before the module is unloaded that owns the release function - http://thread.gmane.org/gmane.linux.kernel/1622885).

For callbacks, that's easy: it's module_get/module_put

Quoted 6 lines

In case it is not clear why I'm reviving this discussion: now that the "improved eh timeout handler" patch is upstream (commit e494f6a728394ab0df194342549ee20e6f0752df) there is an additional way in which the SCSI core can invoke an EH function concurrently with or after scsi_remove_host() has finished, namely from the TMF work queue (tmf_work_q).

But the fundamental guarantee is that the eh thread for the host (the eh context if you will) has to be dead before the host can be removed and the module unloaded. The thread doesn't die until all the work is done.

For a variety of reasons this patch set is incredibly hard to review:
Almost every patch touches pieces in the mid layer where you have to be sure in minute detail you know what's going on (and what should be going on), so usually it's a couple of hours with the source code just making sure you do know this. Plus it's code where the underlying usage model has evolved over the years meaning the original guarantees might have

been violated silently somewhere and the ramifications spread out like tentacles everywhere. Finally, it's not clear from the change logs why the changes are actually being made: for instance sentence one of this change log says "A SCSI LLD may start cleaning up host resources as soon as scsi_remove_host() returns." which causes my sanity checker to go off immediately because in a refcounted model, like we use for dev, target and host, nothing essential is supposed to be freed until the last put which may or may not happen in the remove function.

A SCSI LLD may overwrite host_scribble in its queuecommand() implementation. Several drivers need that field to process requests and aborts correctly. Hence this field must be saved by scsi_eh_prep_cmnd() and must be restored by scsi_eh_restore_cmnd().

Signed-off-by Bart Van Assche <[email protected]>

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Tejun Heo <[email protected]>

Quoted 6 lines · Bart Van Assche

A SCSI LLD may overwrite host_scribble in its queuecommand() implementation. Several drivers need that field to process requests and aborts correctly. Hence this field must be saved by scsi_eh_prep_cmnd() and must be restored by scsi_eh_restore_cmnd().

Quoted 5 lines

Signed-off-by Bart Van Assche <[email protected]>

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Tejun Heo <[email protected]>

How have we not hit this before :) ?

Looks ok.