mid-layer unblocks blocked sdev leaving queue stopped

scsi_add_lun() and scsi_sysfs_add_sdev() unconditionally set the sdev state to SDEV_RUNNING. If this sdev was blocked via scsi_internal_device_block() the changing of the state to running will result in the device's queue remaining stopped after the call to scsi_internal_device_unblock() and commands becoming stuck. This patch changes scsi_add_lun() and scsi_sysfs_add_sdev() to not move the sdev to running if it is blocked. It leaves it to the blocker of the sdev to unblock it.

Found during large lun count testing using 13 FC host adapter ports. In my testing the sdev was blocked due to disruptions or events on the fibre channel link.

Signed-off-by Michael Reed <[email protected]>

==

drivers/scsi/scsi_sysfs.c +8 -2

@@ -878,8 +878,14 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 	struct request_queue *rq = sdev->request_queue;
 	struct scsi_target *starget = sdev->sdev_target;
 
-	if ((error = scsi_device_set_state(sdev, SDEV_RUNNING)) != 0)
-		return error;
+	/*
+	 * If device is blocked, leave state alone and let
+	 * blocker unblock when appropriate.
+	 */
+	if (sdev->sdev_state != SDEV_BLOCK) {
+		if ((error = scsi_device_set_state(sdev, SDEV_RUNNING)) != 0)
+			return error;
+	}
 
 	error = scsi_target_add(starget);
 	if (error)

drivers/scsi/scsi_scan.c +13 -9

@@ -902,17 +902,22 @@ static int scsi_add_lun(struct scsi_devi
 	if (*bflags & BLIST_USE_10_BYTE_MS)
 		sdev->use_10_for_ms = 1;
 
-	/* set the device running here so that slave configure
-	 * may do I/O */
-	ret = scsi_device_set_state(sdev, SDEV_RUNNING);
-	if (ret) {
-		ret = scsi_device_set_state(sdev, SDEV_BLOCK);
-
+	/*
+	 * If device is blocked, leave state alone and let blocker
+	 * unblock when appropriate.  Otherwise, set the device
+	 * running here so that slave configure may perform i/o.
+	 */
+	if (sdev->sdev_state != SDEV_BLOCK) {
+		ret = scsi_device_set_state(sdev, SDEV_RUNNING);
 		if (ret) {
-			sdev_printk(KERN_ERR, sdev,
+			ret = scsi_device_set_state(sdev, SDEV_BLOCK);
+
+			if (ret) {
+				sdev_printk(KERN_ERR, sdev,
 				    "in wrong state %s to complete scan\n",
 				    scsi_device_state_name(sdev->sdev_state));
-			return SCSI_SCAN_NO_RESPONSE;
+				return SCSI_SCAN_NO_RESPONSE;
+			}
 		}
 	}
 
--

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

drivers/scsi/scsi_scan.c static int scsi_add_lun(struct scsi_devi · line 911

Quoted 7 lines

drivers/scsi/scsi_scan.c +7 -0

+	/*
+	 * If device is blocked, leave state alone and let blocker
+	 * unblock when appropriate.  Otherwise, set the device
+	 * running here so that slave configure may perform i/o.
+	 */
+	if (sdev->sdev_state != SDEV_BLOCK) {
+		ret = scsi_device_set_state(sdev, SDEV_RUNNING);

Quoted 7 lines · Michael Reed

Review

Quoted 7 lines

(quoted diff) +7 -0

+	/*
+	 * If device is blocked, leave state alone and let blocker
+	 * unblock when appropriate.  Otherwise, set the device
+	 * running here so that slave configure may perform i/o.
+	 */
+	if (sdev->sdev_state != SDEV_BLOCK) {
+		ret = scsi_device_set_state(sdev, SDEV_RUNNING);

Do we need locking here? Is it possible that right after we check the sdev_state for being blocked, it could be be set to blocked?

Quoted 10 lines · Mike Christie

Quoted 7 lines · Michael Reed

Review

Quoted 7 lines

(quoted diff) +7 -0

+	/*
+	 * If device is blocked, leave state alone and let blocker
+	 * unblock when appropriate.  Otherwise, set the device
+	 * running here so that slave configure may perform i/o.
+	 */
+	if (sdev->sdev_state != SDEV_BLOCK) {
+		ret = scsi_device_set_state(sdev, SDEV_RUNNING);

Do we need locking here? Is it possible that right after we check the sdev_state for being blocked, it could be be set to blocked?

Since the sdev_state can be set to blocked upon entry to the routines, it seems likely that it could transition as you describe. The host_lock appears to be the right lock for this. Nice catch. It does, however, open a can of worms. The documentation for the _block()/_unblock() functions state that it is assumed the host_lock is held upon entry. This doesn't line up with the code, which explicitly releases the host_lock before calling scsi_internal_device_[un]block().

I previously toyed around with an alternate method of resolving this which put the onus on the unblocking thread which doesn't care about the sdev_state. As scsi_internal_device_block() is the routine which stops the queue, I made scsi_internal_device_unblock() allow for the possibility that another thread had transitioned sdev_state from SDEV_BLOCK.

It's been long enough that I don't recall why I chose the patch I posted to resolve the issue, but this patch was also effective at resolving the issue. It also confines the change to the _block()/ _unblock() pair of routines and doesn't require changes to the existing locking logic. In retrospect, this patch was probably the one I should have chosen to post.

Please review.

Signed-off-by Michael Reed <[email protected]>

drivers/scsi/scsi_lib.c +12 -5

@@ -2376,7 +2376,7 @@ EXPORT_SYMBOL(scsi_target_resume);
  *	(which must be a legal transition).  When the device is in this
  *	state, all commands are deferred until the scsi lld reenables
  *	the device with scsi_device_unblock or device_block_tmo fires.
- *	This routine assumes the host_lock is held on entry.
+ *	This routine assumes the host_lock is not held on entry.
  */
 int
 scsi_internal_device_block(struct scsi_device *sdev)

@@ -2420,7 +2420,7 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_b
  *	This routine transitions the device to the SDEV_RUNNING state
  *	(which must be a legal transition) allowing the midlayer to
  *	goose the queue for this device.  This routine assumes the 
- *	host_lock is held upon entry.
+ *	host_lock is not held upon entry.
  */
 int
 scsi_internal_device_unblock(struct scsi_device *sdev)

@@ -2431,15 +2431,23 @@ scsi_internal_device_unblock(struct scsi
 	/* 
 	 * Try to transition the scsi device to SDEV_RUNNING
 	 * and goose the device queue if successful.  
+	 * It's possible that an sdev may be blocked before it is
+	 * completely set up.  scsi_sysfs_add_sdev() and scsi_add_lun()
+	 * will unconditionally attempt to transition the sdev to
+	 * SDEV_RUNNING.  To avoid leaving the queue stopped, we
+	 * allow for the sdev to already be in the SDEV_RUNNING state.
 	 */
+	spin_lock_irqsave(q->queue_lock, flags);
+
 	if (sdev->sdev_state == SDEV_BLOCK)
 		sdev->sdev_state = SDEV_RUNNING;
 	else if (sdev->sdev_state == SDEV_CREATED_BLOCK)
 		sdev->sdev_state = SDEV_CREATED;
-	else
+	else if (sdev->sdev_state != SDEV_RUNNING || !blk_queue_stopped(q)) {
+		spin_unlock_irqrestore(q->queue_lock, flags);
 		return -EINVAL;
+	}
 
-	spin_lock_irqsave(q->queue_lock, flags);
 	blk_start_queue(q);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
 
--

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

Quoted 9 lines · Mike Christie

Quoted 7 lines · Michael Reed

Review

Quoted 7 lines

(quoted diff) +7 -0

+	/*
+	 * If device is blocked, leave state alone and let blocker
+	 * unblock when appropriate.  Otherwise, set the device
+	 * running here so that slave configure may perform i/o.
+	 */
+	if (sdev->sdev_state != SDEV_BLOCK) {
+		ret = scsi_device_set_state(sdev, SDEV_RUNNING);

Do we need locking here? Is it possible that right after we check the sdev_state for being blocked, it could be be set to blocked?

Since the sdev_state can be set to blocked upon entry to the routines, it seems likely that it could transition as you describe. The host_lock appears to be the right lock for this. Nice catch. It does, however, open a can of worms. The documentation for the _block()/_unblock() functions state that it is assumed the host_lock is held upon entry. This doesn't line up with the code, which explicitly releases the host_lock before calling scsi_internal_device_[un]block().

I previously toyed around with an alternate method of resolving this which put the onus on the unblocking thread which doesn't care about the sdev_state. As scsi_internal_device_block() is the routine which stops the queue, I made scsi_internal_device_unblock() allow for the possibility that another thread had transitioned sdev_state from SDEV_BLOCK.

It's been long enough that I don't recall why I chose the patch I posted to resolve the issue, but this patch was also effective at resolving the issue. It also confines the change to the _block()/ _unblock() pair of routines and doesn't require changes to the existing locking logic. In retrospect, this patch was probably the one I should have chosen to post.

Please review.

Signed-off-by Michael Reed <[email protected]>

drivers/scsi/scsi_lib.c +12 -4

@@ -2376,7 +2376,7 @@ EXPORT_SYMBOL(scsi_target_resume);
  *	(which must be a legal transition).  When the device is in this
  *	state, all commands are deferred until the scsi lld reenables
  *	the device with scsi_device_unblock or device_block_tmo fires.
- *	This routine assumes the host_lock is held on entry.
+ *	This routine assumes the host_lock is not held on entry.
  */
 int
 scsi_internal_device_block(struct scsi_device *sdev)

@@ -2420,7 +2420,7 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_b
  *	This routine transitions the device to the SDEV_RUNNING state
  *	(which must be a legal transition) allowing the midlayer to
  *	goose the queue for this device.  This routine assumes the 
- *	host_lock is held upon entry.
+ *	host_lock is not held upon entry.
  */
 int
 scsi_internal_device_unblock(struct scsi_device *sdev)

@@ -2431,15 +2431,23 @@ scsi_internal_device_unblock(struct scsi
 	/* 
 	 * Try to transition the scsi device to SDEV_RUNNING
 	 * and goose the device queue if successful.  
+	 * It's possible that an sdev may be blocked before it is
+	 * completely set up.  scsi_sysfs_add_sdev() and scsi_add_lun()
+	 * will unconditionally attempt to transition the sdev to
+	 * SDEV_RUNNING.  To avoid leaving the queue stopped, we
+	 * allow for the sdev to already be in the SDEV_RUNNING state.
 	 */
+	spin_lock_irqsave(q->queue_lock, flags);
+
 	if (sdev->sdev_state == SDEV_BLOCK)
 		sdev->sdev_state = SDEV_RUNNING;
 	else if (sdev->sdev_state == SDEV_CREATED_BLOCK)
 		sdev->sdev_state = SDEV_CREATED;
-	else
+	else if (sdev->sdev_state != SDEV_RUNNING || !blk_queue_stopped(q)) {
+		spin_unlock_irqrestore(q->queue_lock, flags);
 		return -EINVAL;
+	}
 
-	spin_lock_irqsave(q->queue_lock, flags);
 	blk_start_queue(q);
 	spin_unlock_irqrestore(q->queue_lock, flags);