Hi Ming
On 3/21/19 11:55 AM, Ming Lei wrote:
> On Thu, Mar 21, 2019 at 11:21:20AM +0800, zhengbin wrote:
>> When I use dd test a SCSI device which use blk-mq in the following steps:
>> 1.echo "blocked" >/sys/block/sda/device/state
>> 2.dd if=/dev/sda of=/mnt/t.log bs=1M count=10
>> 3.echo "running" >/sys/block/sda/device/state
>> dd should finish this work after step 3, unfortunately, still hung.
>>
>> After step2, the key code process is like this:
>> blk_mq_dispatch_rq_list-->scsi_queue_rq-->prep_to_mq
>>
>> prep_to_mq will return BLK_STS_RESOURCE, and scsi_queue_rq will transter
>> it to BLK_STS_DEV_RESOURCE, which means that driver can guarantee that
>> IO dispatch will be triggered in future when the resource is available.
>> Need to follow the rule if we set the device state to running.
>
> IMO, it is the correct direction for fixing the issue.
Do you think we should provide more guarantee when setting sdev state through sysfs ?
In the current implementation, store_state_field does nothing but just modify state.
For example, when we set state to 'blocked', it cannot ensure the tasks that has escaped
the checking of state in scsi_queue_rq has quit, when we return from the sysfs.
Thanks
Jianchao
>
>>
>> Signed-off-by: zhengbin <zhengbin13@xxxxxxxxxx>
>> ---
>> drivers/scsi/scsi_sysfs.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index 6a9040f..ce60408 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -770,7 +770,20 @@ store_state_field(struct device *dev, struct device_attribute *attr,
>> return -EINVAL;
>>
>> mutex_lock(&sdev->state_mutex);
>> + enum scsi_device_state oldstate = sdev->sdev_state;
>> ret = scsi_device_set_state(sdev, state);
>> + if (ret == 0) {
>> + /* If device use blk-mq, the device state changes from
>> + * SDEV_BLOCK to SDEV_RUNNING, we need to run hw queue
>> + * to avoid io hung.
>> + */
>> + if ((state == SDEV_RUNNING) && (oldstate == SDEV_BLOCK)) {
>> + struct request_queue *q = sdev->request_queue;
>
> It is fine to run queue unconditionally in case of 'state ==
> SDEV_RUNNING'.
>
>> +
>> + if (q->mq_ops)
>
> The above check isn't needed.
>
>> + blk_mq_run_hw_queues(q, true);
>
> I suggest to move blk_mq_run_hw_queues() out of the mutex for
> avoiding any potential trouble.
>
> Thanks,
> Ming
>
