Michael Reed wrote:
> 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.
I've encountered a test system which exhibits this problem regularly.
This patch consistently corrects the hang and allows the system to
boot. I used a printk to confirm the condition was encountered.
Please consider this patch for integration to stable as well as
any other appropriate trees.
Thank you.
Mike
>
> 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
--
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
