Re: [PATCHv2] libsas: Check for completed commands before calling lldd_abort_task()

[Jason Yan <yanaijie@huawei.com>]

On 2018/1/9 15:34, Hannes Reinecke wrote:
> On 01/09/2018 05:09 AM, Jason Yan wrote:
>> Hannes,
>>
>> On 2018/1/8 20:04, Hannes Reinecke wrote:
>>> The abort handler might be racing with command completion, so the
>>> task might already be NULL by the time the abort handler is called.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>>> ---
>>>    drivers/scsi/libsas/sas_scsi_host.c | 22 +++++++++++++++++++---
>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_scsi_host.c
>>> b/drivers/scsi/libsas/sas_scsi_host.c
>>> index 58476b7..08203fb 100644
>>> --- a/drivers/scsi/libsas/sas_scsi_host.c
>>> +++ b/drivers/scsi/libsas/sas_scsi_host.c
>>> @@ -486,18 +486,34 @@ static int sas_queue_reset(struct domain_device
>>> *dev, int reset_type,
>>>
>>>    int sas_eh_abort_handler(struct scsi_cmnd *cmd)
>>>    {
>>> -    int res;
>>> -    struct sas_task *task = TO_SAS_TASK(cmd);
>>> +    int res = TMF_RESP_FUNC_COMPLETE;
>>> +    struct sas_task *task;
>>>        struct Scsi_Host *host = cmd->device->host;
>>>        struct sas_internal *i = to_sas_internal(host->transportt);
>>> +    struct domain_device *dev = cmd_to_domain_dev(cmd);
>>> +    struct sas_ha_struct *ha = dev->port->ha;
>>> +    unsigned long flags;
>>>
>>>        if (!i->dft->lldd_abort_task)
>>>            return FAILED;
>>>
>>> -    res = i->dft->lldd_abort_task(task);
>>> +    /* Avoid sas_scsi_task_done() interfering */
>>> +    spin_lock_irqsave(&dev->done_lock, flags);
>>> +    task = TO_SAS_TASK(cmd);
>>> +    if (test_bit(SAS_HA_FROZEN, &ha->state)) {
>>> +        res = TMF_RESP_FUNC_FAILED;
>>> +        task = NULL;
>>> +    } else
>>> +        ASSIGN_SAS_TASK(cmd, NULL);
>>> +    spin_unlock_irqrestore(&dev->done_lock, flags);
>>> +    if (task)
>>> +        res = i->dft->lldd_abort_task(task);
>>>        if (res == TMF_RESP_FUNC_SUCC || res == TMF_RESP_FUNC_COMPLETE)
>>>            return SUCCESS;
>>>
>>> +    spin_lock_irqsave(&dev->done_lock, flags);
>>> +    ASSIGN_SAS_TASK(cmd, task);
>>> +    spin_unlock_irqrestore(&dev->done_lock, flags);
>>
>> Why do you assign task back? As I remember, when this cmd dispatch
>> again, we will create a new task and assign to it again. So should we
>> end this task here?
>>
> We only will create a new task if we decide to retry the command.
> But if we return FAILED here the command is not retried but rather the
> SCSI EH is invoked.
>

I got it. The SCSI EH will handle this.

But when we return SUCCESS above, shall we free the task? I don't see
that LLDDs free it.

Jason

> Cheers,
>
> Hannes
>