From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Yan Subject: Re: [PATCHv2] libsas: Check for completed commands before calling lldd_abort_task() Date: Tue, 9 Jan 2018 17:04:28 +0800 Message-ID: <5A54859C.6000308@huawei.com> References: <1515413086-34256-1-git-send-email-hare@suse.de> <5A544075.4010705@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from szxga04-in.huawei.com ([45.249.212.190]:3705 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751329AbeAIJFc (ORCPT ); Tue, 9 Jan 2018 04:05:32 -0500 In-Reply-To: Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: Hannes Reinecke , Hannes Reinecke , "Martin K. Petersen" Cc: Christoph Hellwig , James Bottomley , linux-scsi@vger.kernel.org, Yves-Alexis Perez 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 >>> --- >>> 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 >