Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] scsi: core: Run queue first after running device
To:     John Garry <john.garry@huawei.com>, <jejb@linux.ibm.com>,
        <martin.petersen@oracle.com>, <linux-scsi@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     <bvanassche@acm.org>, <qiulaibin@huawei.com>,
        <linfeilong@huawei.com>, <wubo40@huawei.com>
References: <20210805143231.1713299-1-lijinlin3@huawei.com>
 <908bb2bb-c511-06a4-e0b6-577d90bb9b57@huawei.com>
 <0d27db3f-4236-4a30-97a0-ad1dcbf4bcfa@huawei.com>
From:   lijinlin <lijinlin3@huawei.com>
Message-ID: <b9894e12-c214-8cea-7f02-96ad99aca2f8@huawei.com>
Date:   Fri, 6 Aug 2021 20:21:42 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <0d27db3f-4236-4a30-97a0-ad1dcbf4bcfa@huawei.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk

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);
>>> -- 
>>
>> .
> 
> .