LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] scsi: core: Run queue first after running device.
@ 2021-08-05 14:32 lijinlin3
  2021-08-06  8:58 ` John Garry
  0 siblings, 1 reply; 4+ messages in thread
From: lijinlin3 @ 2021-08-05 14:32 UTC (permalink / raw)
  To: jejb, martin.petersen, linux-scsi, linux-kernel
  Cc: john.garry, bvanassche, qiulaibin, linfeilong, wubo40, lijinlin3

From: Li Jinlin <lijinlin3@huawei.com>

We found a hang issue, the test steps are as follows:
  1. echo "blocked" >/sys/block/sda/device/state
  2. dd if=/dev/sda of=/mnt/t.log bs=1M count=10
  3. echo none > /sys/block/sda/queue/scheduler
  4. echo "running" >/sys/block/sda/device/state

Step3 and Step4 should finish this work after Step4, but them hangs.

  CPU#0               CPU#1                CPU#2
  ---------------     ----------------     ----------------
                                           Step1: blocking device

                                           Step2: dd xxxx
                                                  ^^^^^^ get request
                                                         q_usage_counter++

                      Step3: switching scheculer
                      elv_iosched_store
                        elevator_switch
                          blk_mq_freeze_queue
                            blk_freeze_queue
                              > blk_freeze_queue_start
                                ^^^^^^ mq_freeze_depth++

                              > blk_mq_run_hw_queues
                                ^^^^^^ can't run queue when dev blocked

                              > blk_mq_freeze_queue_wait
                                ^^^^^^ Hang here!!! 
                                       wait q_usage_counter==0

  Step4: running device
  store_state_field
    scsi_rescan_device
      scsi_attach_vpd
        scsi_vpd_inquiry
          __scsi_execute
            blk_get_request
              blk_mq_alloc_request
                blk_queue_enter
                ^^^^^^ Hang here!!!
                       wait mq_freeze_depth==0 

    blk_mq_run_hw_queues                                           
    ^^^^^^ dispatch IO, q_usage_counter will reduce to zero

                            blk_mq_unfreeze_queue
                            ^^^^^ mq_freeze_depth--

Step3 and Step4 wait for each other, caused hangs.

This requires run queue frist to fix this issue when the device state
changes to SDEV_RUNNING.

Fixes: f0f82e2476f6 ("scsi: core: Fix capacity set to zero after offlinining device")
Signed-off-by: Li Jinlin <lijinlin3@huawei.com>
Signed-off-by: Qiu Laibin <qiulaibin@huawei.com>
Signed-off-by: Wu Bo <wubo40@huawei.com>
---
 drivers/scsi/scsi_sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index c3a710bceba0..aa701582c950 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -809,12 +809,12 @@ store_state_field(struct device *dev, struct device_attribute *attr,
 	ret = scsi_device_set_state(sdev, state);
 	/*
 	 * If the device state changes to SDEV_RUNNING, we need to
-	 * rescan the device to revalidate it, and run the queue to
-	 * avoid I/O hang.
+	 * run the queue to avoid I/O hang, and rescan the device
+	 * to revalidate it.
 	 */
 	if (ret == 0 && state == SDEV_RUNNING) {
-		scsi_rescan_device(dev);
 		blk_mq_run_hw_queues(sdev->request_queue, true);
+		scsi_rescan_device(dev);
 	}
 	mutex_unlock(&sdev->state_mutex);
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] scsi: core: Run queue first after running device.
  2021-08-05 14:32 [PATCH] scsi: core: Run queue first after running device lijinlin3
@ 2021-08-06  8:58 ` John Garry
  2021-08-06  9:00   ` John Garry
  0 siblings, 1 reply; 4+ messages in thread
From: John Garry @ 2021-08-06  8:58 UTC (permalink / raw)
  To: lijinlin3, jejb, martin.petersen, linux-scsi, linux-kernel
  Cc: bvanassche, qiulaibin, linfeilong, wubo40

On 05/08/2021 15:32, lijinlin3@huawei.com wrote:
> From: Li Jinlin<lijinlin3@huawei.com>
> 
> We found a hang issue, the test steps are as follows:
>    1. echo "blocked" >/sys/block/sda/device/state
>    2. dd if=/dev/sda of=/mnt/t.log bs=1M count=10
>    3. echo none > /sys/block/sda/queue/scheduler
>    4. echo "running" >/sys/block/sda/device/state
> 
> Step3 and Step4 should finish this work after Step4, but them hangs.
> 
>    CPU#0               CPU#1                CPU#2
>    ---------------     ----------------     ----------------
>                                             Step1: blocking device
> 
>                                             Step2: dd xxxx
>                                                    ^^^^^^ get request
>                                                           q_usage_counter++
> 
>                        Step3: switching scheculer
>                        elv_iosched_store
>                          elevator_switch
>                            blk_mq_freeze_queue
>                              blk_freeze_queue
>                                > blk_freeze_queue_start
>                                  ^^^^^^ mq_freeze_depth++
> 
>                                > blk_mq_run_hw_queues
>                                  ^^^^^^ can't run queue when dev blocked
> 
>                                > blk_mq_freeze_queue_wait
>                                  ^^^^^^ Hang here!!!
>                                         wait q_usage_counter==0
> 
>    Step4: running device
>    store_state_field
>      scsi_rescan_device
>        scsi_attach_vpd
>          scsi_vpd_inquiry
>            __scsi_execute
>              blk_get_request
>                blk_mq_alloc_request
>                  blk_queue_enter
>                  ^^^^^^ Hang here!!!
>                         wait mq_freeze_depth==0
> 
>      blk_mq_run_hw_queues
>      ^^^^^^ dispatch IO, q_usage_counter will reduce to zero
> 
>                              blk_mq_unfreeze_queue
>                              ^^^^^ mq_freeze_depth--
> 
> Step3 and Step4 wait for each other, caused hangs.
> 
> This requires run queue frist to fix this issue when the device state

frist ?

> changes to SDEV_RUNNING.
> 
> Fixes: f0f82e2476f6 ("scsi: core: Fix capacity set to zero after offlinining device")
> Signed-off-by: Li Jinlin<lijinlin3@huawei.com>
> Signed-off-by: Qiu Laibin<qiulaibin@huawei.com>
> Signed-off-by: Wu Bo<wubo40@huawei.com>

what kind of SoB is this?

> ---
>   drivers/scsi/scsi_sysfs.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index c3a710bceba0..aa701582c950 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -809,12 +809,12 @@ store_state_field(struct device *dev, struct device_attribute *attr,
>   	ret = scsi_device_set_state(sdev, state);
>   	/*
>   	 * If the device state changes to SDEV_RUNNING, we need to
> -	 * rescan the device to revalidate it, and run the queue to
> -	 * avoid I/O hang.
> +	 * run the queue to avoid I/O hang, and rescan the device
> +	 * to revalidate it.

A bit more description of the IO hang would be useful

>   	 */
>   	if (ret == 0 && state == SDEV_RUNNING) {
> -		scsi_rescan_device(dev);
>   		blk_mq_run_hw_queues(sdev->request_queue, true);
> +		scsi_rescan_device(dev);

This would not have happened if scsi_rescan_device() was ran outside the 
mutex lock region, like I suggested originally.

Indeed, I doubt blk_mq_run_hw_queues() needs to be run with the sdev 
state_mutex held either.

>   	}
>   	mutex_unlock(&sdev->state_mutex);
>   
> -- 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] scsi: core: Run queue first after running device.
  2021-08-06  8:58 ` John Garry
@ 2021-08-06  9:00   ` John Garry
  2021-08-06 12:21     ` lijinlin
  0 siblings, 1 reply; 4+ messages in thread
From: John Garry @ 2021-08-06  9:00 UTC (permalink / raw)
  To: lijinlin3, jejb, martin.petersen, linux-scsi, linux-kernel
  Cc: bvanassche, qiulaibin, linfeilong, wubo40

On 06/08/2021 09:58, John Garry wrote:

And the patch subject is ambiguous

> On 05/08/2021 15:32, lijinlin3@huawei.com wrote:
>> From: Li Jinlin<lijinlin3@huawei.com>
>>
>> We found a hang issue, the test steps are as follows:
>>    1. echo "blocked" >/sys/block/sda/device/state
>>    2. dd if=/dev/sda of=/mnt/t.log bs=1M count=10
>>    3. echo none > /sys/block/sda/queue/scheduler
>>    4. echo "running" >/sys/block/sda/device/state
>>
>> Step3 and Step4 should finish this work after Step4, but them hangs.
>>
>>    CPU#0               CPU#1                CPU#2
>>    ---------------     ----------------     ----------------
>>                                             Step1: blocking device
>>
>>                                             Step2: dd xxxx
>>                                                    ^^^^^^ get request
>>                                                           
>> q_usage_counter++
>>
>>                        Step3: switching scheculer
>>                        elv_iosched_store
>>                          elevator_switch
>>                            blk_mq_freeze_queue
>>                              blk_freeze_queue
>>                                > blk_freeze_queue_start
>>                                  ^^^^^^ mq_freeze_depth++
>>
>>                                > blk_mq_run_hw_queues
>>                                  ^^^^^^ can't run queue when dev blocked
>>
>>                                > blk_mq_freeze_queue_wait
>>                                  ^^^^^^ Hang here!!!
>>                                         wait q_usage_counter==0
>>
>>    Step4: running device
>>    store_state_field
>>      scsi_rescan_device
>>        scsi_attach_vpd
>>          scsi_vpd_inquiry
>>            __scsi_execute
>>              blk_get_request
>>                blk_mq_alloc_request
>>                  blk_queue_enter
>>                  ^^^^^^ Hang here!!!
>>                         wait mq_freeze_depth==0
>>
>>      blk_mq_run_hw_queues
>>      ^^^^^^ dispatch IO, q_usage_counter will reduce to zero
>>
>>                              blk_mq_unfreeze_queue
>>                              ^^^^^ mq_freeze_depth--
>>
>> Step3 and Step4 wait for each other, caused hangs.
>>
>> This requires run queue frist to fix this issue when the device state
> 
> frist ?
> 
>> changes to SDEV_RUNNING.
>>
>> Fixes: f0f82e2476f6 ("scsi: core: Fix capacity set to zero after 
>> offlinining device")
>> Signed-off-by: Li Jinlin<lijinlin3@huawei.com>
>> Signed-off-by: Qiu Laibin<qiulaibin@huawei.com>
>> Signed-off-by: Wu Bo<wubo40@huawei.com>
> 
> what kind of SoB is this?
> 
>> ---
>>   drivers/scsi/scsi_sysfs.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index c3a710bceba0..aa701582c950 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -809,12 +809,12 @@ store_state_field(struct device *dev, struct 
>> device_attribute *attr,
>>       ret = scsi_device_set_state(sdev, state);
>>       /*
>>        * If the device state changes to SDEV_RUNNING, we need to
>> -     * rescan the device to revalidate it, and run the queue to
>> -     * avoid I/O hang.
>> +     * run the queue to avoid I/O hang, and rescan the device
>> +     * to revalidate it.
> 
> A bit more description of the IO hang would be useful
> 
>>        */
>>       if (ret == 0 && state == SDEV_RUNNING) {
>> -        scsi_rescan_device(dev);
>>           blk_mq_run_hw_queues(sdev->request_queue, true);
>> +        scsi_rescan_device(dev);
> 
> This would not have happened if scsi_rescan_device() was ran outside the 
> mutex lock region, like I suggested originally.
> 
> Indeed, I doubt blk_mq_run_hw_queues() needs to be run with the sdev 
> state_mutex held either.
> 
>>       }
>>       mutex_unlock(&sdev->state_mutex);
>> -- 
> 
> .


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] scsi: core: Run queue first after running device
  2021-08-06  9:00   ` John Garry
@ 2021-08-06 12:21     ` lijinlin
  0 siblings, 0 replies; 4+ messages in thread
From: lijinlin @ 2021-08-06 12:21 UTC (permalink / raw)
  To: John Garry, jejb, martin.petersen, linux-scsi, linux-kernel
  Cc: bvanassche, qiulaibin, linfeilong, wubo40

On 2021/8/6 17:00, John Garry wrote:
> On 06/08/2021 09:58, John Garry wrote:
> 
> And the patch subject is ambiguous
>

Thanks.
I will modify the patch subject and message in patch v2.
 
>> On 05/08/2021 15:32, lijinlin3@huawei.com wrote:
>>> From: Li Jinlin<lijinlin3@huawei.com>
>>>
>>> We found a hang issue, the test steps are as follows:
>>>    1. echo "blocked" >/sys/block/sda/device/state
>>>    2. dd if=/dev/sda of=/mnt/t.log bs=1M count=10
>>>    3. echo none > /sys/block/sda/queue/scheduler
>>>    4. echo "running" >/sys/block/sda/device/state
>>>
>>> Step3 and Step4 should finish this work after Step4, but them hangs.
>>>
>>>  CPU#0               CPU#1                CPU#2
>>>  ---------------     ----------------     ----------------
>>>                                           Step1: blocking device
>>>
>>>                                           Step2: dd xxxx
>>>                                                  ^^^^^^ get request
>>>                                                         q_usage_counter++
>>>
>>>                      Step3: switching scheculer
>>>                      elv_iosched_store
>>>                        elevator_switch
>>>                          blk_mq_freeze_queue
>>>                            blk_freeze_queue
>>>                              > blk_freeze_queue_start
>>>                                ^^^^^^ mq_freeze_depth++
>>>
>>>                              > blk_mq_run_hw_queues
>>>                                ^^^^^^ can't run queue when dev blocked
>>>
>>>                              > blk_mq_freeze_queue_wait
>>>                                ^^^^^^ Hang here!!! 
>>>                                       wait q_usage_counter==0
>>>
>>>  Step4: running device
>>>  store_state_field
>>>    scsi_rescan_device
>>>      scsi_attach_vpd
>>>        scsi_vpd_inquiry
>>>          __scsi_execute
>>>            blk_get_request
>>>              blk_mq_alloc_request
>>>                blk_queue_enter
>>>                ^^^^^^ Hang here!!!
>>>                       wait mq_freeze_depth==0 
>>>
>>>    blk_mq_run_hw_queues
>>>    ^^^^^^ dispatch IO, q_usage_counter will reduce to zero
>>>
>>>                            blk_mq_unfreeze_queue
>>>                            ^^^^^ mq_freeze_depth--
>>>
>>> Step3 and Step4 wait for each other, caused hangs.
>>>
>>> This requires run queue frist to fix this issue when the device state
>>
>> frist>>
>>> changes to SDEV_RUNNING.
>>>
>>> Fixes: f0f82e2476f6 ("scsi: core: Fix capacity set to zero after offlinining device")
>>> Signed-off-by: Li Jinlin <lijinlin3@huawei.com>
>>> Signed-off-by: Qiu Laibin <qiulaibin@huawei.com>
>>> Signed-off-by: Wu Bo <wubo40@huawei.com>
>>
>> what kind of SoB is this?
>>
>>> ---
>>>   drivers/scsi/scsi_sysfs.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>> index c3a710bceba0..aa701582c950 100644
>>> --- a/drivers/scsi/scsi_sysfs.c
>>> +++ b/drivers/scsi/scsi_sysfs.c
>>> @@ -809,12 +809,12 @@ store_state_field(struct device *dev, struct device_attribute *attr,
>>>       ret = scsi_device_set_state(sdev, state);
>>>       /*
>>>        * If the device state changes to SDEV_RUNNING, we need to
>>> -     * rescan the device to revalidate it, and run the queue to
>>> -     * avoid I/O hang.
>>> +     * run the queue to avoid I/O hang, and rescan the device
>>> +     * to revalidate it.
>>
>> A bit more description of the IO hang would be useful
>>
>>>        */
>>>       if (ret == 0 && state == SDEV_RUNNING) {
>>> -        scsi_rescan_device(dev);
>>>                 blk_mq_run_hw_queues(sdev->request_queue, true);
>>> +        scsi_rescan_device(dev);
>>
>> This would not have happened if scsi_rescan_device() was ran outside the mutex lock region, like I suggested originally.
>>
>> Indeed, I doubt blk_mq_run_hw_queues() needs to be run with the sdev state_mutex held either.

I think blk_mq_run_hw_queues() and scsi_rescan_device() needs to be run with the sdev state_mutex held.
Otherwise, Step4 will hang in the following situations.

  CPU#0               CPU#1                CPU#2                  CPU#3
  ---------------     ----------------     -----------------      ----------------
                                           Step1: blocking device

                                           Step2: dd xxxx
                                                  ^^^^^^ get request
                                                         q_usage_counter++

                      Step3: switching scheculer
                      elv_iosched_store
                        elevator_switch
                          blk_mq_freeze_queue
                            blk_freeze_queue
                              > blk_freeze_queue_start
                                ^^^^^^ mq_freeze_depth++

                              > blk_mq_run_hw_queues
                                ^^^^^^ can't run queue when dev blocked

                              > blk_mq_freeze_queue_wait
                                ^^^^^^ Hang here!!! 
                                       wait q_usage_counter==0

  Step4: running device  
  store_state_field
    mutex_lock(&sdev->state_mutex);
    scsi_device_set_state(sdev, state);
    mutex_unlock(&sdev->state_mutex);
                                                                  Step5: blocking device
                                                                  store_state_field
                                                                     mutex_lock(&sdev->state_mutex);
                                                                     scsi_device_set_state(sdev, state);                                                             
    blk_mq_run_hw_queues 
    ^^^^^^ can't run queue when dev blocked
                                                                     mutex_unlock(&sdev->state_mutex);
    scsi_rescan_device
      scsi_attach_vpd
        scsi_vpd_inquiry
          __scsi_execute
            blk_get_request
              blk_mq_alloc_request
                blk_queue_enter
                ^^^^^^ Hang here!!!
                       wait mq_freeze_depth==0 

>>
>>>       }
>>>       mutex_unlock(&sdev->state_mutex);
>>> -- 
>>
>> .
> 
> .

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-08-06 12:21 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-05 14:32 [PATCH] scsi: core: Run queue first after running device lijinlin3
2021-08-06  8:58 ` John Garry
2021-08-06  9:00   ` John Garry
2021-08-06 12:21     ` lijinlin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).