[PATCH] [SCSI] Wake blockdev queue in scsi_internal_device_unblock() for SDEV_RUNNING

[PATCH] [SCSI] Wake blockdev queue in scsi_internal_device_unblock() for SDEV_RUNNING

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

If a SCSI device's old state is already SDEV_RUNNING and we're moving
to the same SDEV_RUNNING state, still wake the blockdev queue in
scsi_internal_device_unblock().  This fixes a case where we silently
hang SCSI commands forever during device discovery.  One way this can
happen is when mpt2sas is discovering a reasonably big SAS topology,
and the sd driver has queued up a bunch of sd_probe_async() instances
that are queueing SCSI commands to various devices.

If at the same time a SAS fabric event goes to the HBA, what can
happen is the following:

     - mpt2sas calls _scsih_block_io_all_device() -> scsi_internal_device_block(sdev)

       (In response to some HBA firmware event like MPI2_EVENT_SAS_BROADCAST_PRIMITIVE)
       Now sdev state is SDEV_BLOCK and blockdev queue has QUEUE_FLAG_STOPPED set.

     - Someone like scsi_add_lun() calls scsi_device_set_state(sdev, SDEV_RUNNING)

       (SCSI bus scanning runs asynchronously to firmware event handling)
       Now sdev state is SDEV_RUNNING but blockdev queue still has QUEUE_FLAG_STOPPED set

     - mpt2sas calls _scsih_ublock_io_all_device() -> scsi_internal_device_unblock(sdev, SDEV_RUNNING)

       (Finishes handling the firmware event)

With the old scsi_lib code, scsi_internal_device_unblock() will return
an error at this point because the sdev state is already SDEV_RUNNING.
This means we skip the call to blk_start_queue() and never actually
start executing commands again.

Fix this by still going ahead and finishing scsi_internal_device_unblock()
even if the sdev state is already SDEV_RUNNING.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/scsi_lib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 765398c..75108ea 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2495,7 +2495,9 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 		else
 			sdev->sdev_state = SDEV_CREATED;
 	} else if (sdev->sdev_state != SDEV_CANCEL &&
-		 sdev->sdev_state != SDEV_OFFLINE)
+		   sdev->sdev_state != SDEV_OFFLINE &&
+		   (sdev->sdev_state != SDEV_RUNNING ||
+		    new_state != SDEV_RUNNING))
 		return -EINVAL;
 
 	spin_lock_irqsave(q->queue_lock, flags);
-- 
1.8.1.2

Re: [PATCH] [SCSI] Wake blockdev queue in scsi_internal_device_unblock() for SDEV_RUNNING

[Roland Dreier]

> With the old scsi_lib code, scsi_internal_device_unblock() will return
> an error at this point because the sdev state is already SDEV_RUNNING.
> This means we skip the call to blk_start_queue() and never actually
> start executing commands again.
>
> Fix this by still going ahead and finishing scsi_internal_device_unblock()
> even if the sdev state is already SDEV_RUNNING.

Any thoughts on this patch?  Without this patch, on a moderate size
system (a couple of mpt2sas adapters and 50 or so SAS devices) we seem
to hit the bug and get stuck every 20 boots or so.  The only way to
recover is to reboot.

 - R.

Re: [PATCH] [SCSI] Wake blockdev queue in scsi_internal_device_unblock() for SDEV_RUNNING

[Mike Christie]

On 02/25/2013 11:55 AM, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> If a SCSI device's old state is already SDEV_RUNNING and we're moving
> to the same SDEV_RUNNING state, still wake the blockdev queue in
> scsi_internal_device_unblock().  This fixes a case where we silently
> hang SCSI commands forever during device discovery.  One way this can
> happen is when mpt2sas is discovering a reasonably big SAS topology,
> and the sd driver has queued up a bunch of sd_probe_async() instances
> that are queueing SCSI commands to various devices.
> 
> If at the same time a SAS fabric event goes to the HBA, what can
> happen is the following:
> 
>      - mpt2sas calls _scsih_block_io_all_device() -> scsi_internal_device_block(sdev)
> 
>        (In response to some HBA firmware event like MPI2_EVENT_SAS_BROADCAST_PRIMITIVE)
>        Now sdev state is SDEV_BLOCK and blockdev queue has QUEUE_FLAG_STOPPED set.
> 
>      - Someone like scsi_add_lun() calls scsi_device_set_state(sdev, SDEV_RUNNING)
> 
>        (SCSI bus scanning runs asynchronously to firmware event handling)
>        Now sdev state is SDEV_RUNNING but blockdev queue still has QUEUE_FLAG_STOPPED set

Is this a valid state change? Should it be failed in
scsi_device_set_state when we try to go from SDEV_BLOCKED ->
SDEV_RUNNING? I am not sure if it make senses to ever have a device in
SDEV_RUNNING but have the stopped queue bit set.

It used to be that scsi_internal_device_unblock called
scsi_device_set_state so the transition from SDEV_BLOCKED to
SDEV_RUNNUNG had to be a valid transition. scsi_internal_device_unblock
then started the queue. I am not sure if the sequence you were hitting
was actually a transition that was intended or just worked by accident.

Should we be failing the above call to scsi_device_set_state, and then
the call to scsi_internal_device_unblock would work as expected.

OTOH, your patch makes the block/unblock API easier to use.



> 
>      - mpt2sas calls _scsih_ublock_io_all_device() -> scsi_internal_device_unblock(sdev, SDEV_RUNNING)
> 
>        (Finishes handling the firmware event)
> 
> With the old scsi_lib code, scsi_internal_device_unblock() will return
> an error at this point because the sdev state is already SDEV_RUNNING.
> This means we skip the call to blk_start_queue() and never actually
> start executing commands again.
> 
> Fix this by still going ahead and finishing scsi_internal_device_unblock()
> even if the sdev state is already SDEV_RUNNING.
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/scsi/scsi_lib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 765398c..75108ea 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2495,7 +2495,9 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
>  		else
>  			sdev->sdev_state = SDEV_CREATED;
>  	} else if (sdev->sdev_state != SDEV_CANCEL &&
> -		 sdev->sdev_state != SDEV_OFFLINE)
> +		   sdev->sdev_state != SDEV_OFFLINE &&
> +		   (sdev->sdev_state != SDEV_RUNNING ||
> +		    new_state != SDEV_RUNNING))
>  		return -EINVAL;
>  
>  	spin_lock_irqsave(q->queue_lock, flags);
> 

Re: [PATCH] [SCSI] Wake blockdev queue in scsi_internal_device_unblock() for SDEV_RUNNING

[Roland Dreier]

On Mon, Mar 11, 2013 at 11:08 AM, Mike Christie <michaelc@cs.wisc.edu> wrote:
>> If at the same time a SAS fabric event goes to the HBA, what can
>> happen is the following:
>>
>>      - mpt2sas calls _scsih_block_io_all_device() -> scsi_internal_device_block(sdev)
>>
>>        (In response to some HBA firmware event like MPI2_EVENT_SAS_BROADCAST_PRIMITIVE)
>>        Now sdev state is SDEV_BLOCK and blockdev queue has QUEUE_FLAG_STOPPED set.
>>
>>      - Someone like scsi_add_lun() calls scsi_device_set_state(sdev, SDEV_RUNNING)
>>
>>        (SCSI bus scanning runs asynchronously to firmware event handling)
>>        Now sdev state is SDEV_RUNNING but blockdev queue still has QUEUE_FLAG_STOPPED set
>
> Is this a valid state change? Should it be failed in
> scsi_device_set_state when we try to go from SDEV_BLOCKED ->
> SDEV_RUNNING? I am not sure if it make senses to ever have a device in
> SDEV_RUNNING but have the stopped queue bit set.
>
> It used to be that scsi_internal_device_unblock called
> scsi_device_set_state so the transition from SDEV_BLOCKED to
> SDEV_RUNNUNG had to be a valid transition. scsi_internal_device_unblock
> then started the queue. I am not sure if the sequence you were hitting
> was actually a transition that was intended or just worked by accident.
>
> Should we be failing the above call to scsi_device_set_state, and then
> the call to scsi_internal_device_unblock would work as expected.

Dunno ... I was a bit scared to go too deep into this sdev_state
stuff, since it looks very old and fragile.  With that said the big
question to me is who really owns the state -- is it the low-level
driver or the midlayer?  I don't quite understand what it means for
the midlayer to try and start a device if the low-level driver has
temporarily paused things.  However since we have the blockdev queue
stopped state also, it seems to make things work out OK.

 - R.