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

[Hannes Reinecke <hare@suse.com>]

On 01/09/2018 10:04 AM, Jason Yan wrote:
> 
> 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.
> 
Hmm. Possibly.
I'll check.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)