Mike Christie wrote:
> On 02/10/2010 04:24 PM, Michael Reed wrote:
>>> + /*
>>> + * 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 <mdr@xxxxxxx>
--- a/drivers/scsi/scsi_lib.c 2010-02-08 11:19:57.000000000 -0600
+++ b/drivers/scsi/scsi_lib.c 2010-02-11 14:35:26.249142688 -0600
@@ -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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
