| Message ID | 20180227065909.43541-1-yanaijie@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 27/02/2018 06:59, Jason Yan wrote: > When ata device doing EH, some commands still attached with tasks are not > passed to libata when abort failed or recover failed, so libata did not > handle these commands. After these commands done, sas task is freed, but > ata qc is not freed. This will cause ata qc leak and trigger a warning > like below: It's seems like a bug that we're just killing the ATA command in libsas error handling and not deferring them to ATA EH also. And this WARN, below, in ata_eh_finish() is a longterm issue I see (but maybe because of other issue also). As mentioned to Jason privately, I wonder why Dan's patch excluded the change here: commit 3944f50995f947558c35fb16ae0288354756762c Author: Dan Williams <dan.j.williams@intel.com> Date: Tue Nov 29 12:08:50 2011 -0800 [SCSI] libsas: let libata handle command timeouts libsas-eh if it successfully aborts an ata command will hide the timeout condition (AC_ERR_TIMEOUT) from libata. The command likely completes with the all-zero task->task_status it started with. Instead, interpret a TMF_RESP_FUNC_COMPLETE as the end of the sas_task but keep the scmd around for libata-eh to handle. Tested-by: Andrzej Jakowski <andrzej.jakowski@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: James Bottomley <JBottomley@Parallels.com> > > WARNING: CPU: 0 PID: 28512 at drivers/ata/libata-eh.c:4037 > ata_eh_finish+0xb4/0xcc > CPU: 0 PID: 28512 Comm: kworker/u32:2 Tainted: G W OE 4.14.0#1 > ...... > Call trace: > [<ffff0000088b7bd0>] ata_eh_finish+0xb4/0xcc > [<ffff0000088b8420>] ata_do_eh+0xc4/0xd8 > [<ffff0000088b8478>] ata_std_error_handler+0x44/0x8c > [<ffff0000088b8068>] ata_scsi_port_error_handler+0x480/0x694 > [<ffff000008875fc4>] async_sas_ata_eh+0x4c/0x80 > [<ffff0000080f6be8>] async_run_entry_fn+0x4c/0x170 > [<ffff0000080ebd70>] process_one_work+0x144/0x390 > [<ffff0000080ec100>] worker_thread+0x144/0x418 > [<ffff0000080f2c98>] kthread+0x10c/0x138 > [<ffff0000080855dc>] ret_from_fork+0x10/0x18 > > If ata qc leaked too many, ata tag allocation will fail and io blocked > for ever. > > It is always safe to defer all commands to libata if it is sata device. > The ata_scsi_cmd_error_handler() will flag the timeout command as > ATA_QCFLAG_FAILED if it is not set already. The libata EH will handle these > qcs correctly. > > Signed-off-by: Jason Yan <yanaijie@huawei.com> > CC: Xiaofei Tan <tanxiaofei@huawei.com> > CC: John Garry <john.garry@huawei.com> The change looks ok to me. But we need more review here from domain expert please. > --- > drivers/scsi/libsas/sas_scsi_host.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index 6de9681ace82..fd00e432112b 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -274,7 +274,7 @@ static void sas_scsi_clear_queue_I_T(struct list_head *error_q, > struct domain_device *x = cmd_to_domain_dev(cmd); > > if (x == dev) > - sas_eh_finish_cmd(cmd); > + sas_eh_defer_cmd(cmd); > } > } > > @@ -288,7 +288,7 @@ static void sas_scsi_clear_queue_port(struct list_head *error_q, > struct asd_sas_port *x = dev->port; > > if (x == port) > - sas_eh_finish_cmd(cmd); > + sas_eh_defer_cmd(cmd); > } > } > > @@ -662,7 +662,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * > struct domain_device *dev = task->dev; > SAS_DPRINTK("I_T %016llx recovered\n", > SAS_ADDR(task->dev->sas_addr)); > - sas_eh_finish_cmd(cmd); > + sas_eh_defer_cmd(cmd); > sas_scsi_clear_queue_I_T(work_q, dev); > goto Again; > } > @@ -676,7 +676,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * > if (res == TMF_RESP_FUNC_COMPLETE) { > SAS_DPRINTK("clear nexus port:%d " > "succeeded\n", port->id); > - sas_eh_finish_cmd(cmd); > + sas_eh_defer_cmd(cmd); > sas_scsi_clear_queue_port(work_q, > port); > goto Again; > @@ -688,7 +688,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * > if (res == TMF_RESP_FUNC_COMPLETE) { > SAS_DPRINTK("clear nexus ha " > "succeeded\n"); > - sas_eh_finish_cmd(cmd); > + sas_eh_defer_cmd(cmd); > goto clear_q; > } > } > @@ -701,7 +701,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * > SAS_ADDR(task->dev->sas_addr), > cmd->device->lun); > > - sas_eh_finish_cmd(cmd); > + sas_eh_defer_cmd(cmd); > goto clear_q; > } > } > @@ -713,7 +713,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * > clear_q: > SAS_DPRINTK("--- Exit %s -- clear_q\n", __func__); > list_for_each_entry_safe(cmd, n, work_q, eh_entry) > - sas_eh_finish_cmd(cmd); > + sas_eh_defer_cmd(cmd); > goto out; > } > >
2018-02-27 12:50 GMT+01:00 John Garry <john.garry@huawei.com>: > On 27/02/2018 06:59, Jason Yan wrote: >> >> When ata device doing EH, some commands still attached with tasks are not >> passed to libata when abort failed or recover failed, so libata did not >> handle these commands. After these commands done, sas task is freed, but >> ata qc is not freed. This will cause ata qc leak and trigger a warning >> like below: > > > It's seems like a bug that we're just killing the ATA command in libsas > error handling and not deferring them to ATA EH also. > > And this WARN, below, in ata_eh_finish() is a longterm issue I see (but > maybe because of other issue also). > > As mentioned to Jason privately, I wonder why Dan's patch excluded the > change here: > commit 3944f50995f947558c35fb16ae0288354756762c > Author: Dan Williams <dan.j.williams@intel.com> > Date: Tue Nov 29 12:08:50 2011 -0800 > > [SCSI] libsas: let libata handle command timeouts > > libsas-eh if it successfully aborts an ata command will hide the timeout/ > condition (AC_ERR_TIMEOUT) from libata. The command likely completes > with the all-zero task->task_status it started with. Instead, interpret > a TMF_RESP_FUNC_COMPLETE as the end of the sas_task but keep the scmd > around for libata-eh to handle. > > Tested-by: Andrzej Jakowski <andrzej.jakowski@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: James Bottomley <JBottomley@Parallels.com> > > >> >> WARNING: CPU: 0 PID: 28512 at drivers/ata/libata-eh.c:4037 >> ata_eh_finish+0xb4/0xcc >> CPU: 0 PID: 28512 Comm: kworker/u32:2 Tainted: G W OE 4.14.0#1 >> ...... >> Call trace: >> [<ffff0000088b7bd0>] ata_eh_finish+0xb4/0xcc >> [<ffff0000088b8420>] ata_do_eh+0xc4/0xd8 >> [<ffff0000088b8478>] ata_std_error_handler+0x44/0x8c >> [<ffff0000088b8068>] ata_scsi_port_error_handler+0x480/0x694 >> [<ffff000008875fc4>] async_sas_ata_eh+0x4c/0x80 >> [<ffff0000080f6be8>] async_run_entry_fn+0x4c/0x170 >> [<ffff0000080ebd70>] process_one_work+0x144/0x390 >> [<ffff0000080ec100>] worker_thread+0x144/0x418 >> [<ffff0000080f2c98>] kthread+0x10c/0x138 >> [<ffff0000080855dc>] ret_from_fork+0x10/0x18 Hi John, hi Jason, We've seen this warning once on pm80xx with sata SSD in production (on 3.12 kernel), but failed to see the root cause. In my case, it's a chain sequence, one SSD failed, lead to error handle & IO stuck. Do you have reproducer? Your change looks good to me, but would be good to hear from Dan & James. Thanks, Jack >> >> If ata qc leaked too many, ata tag allocation will fail and io blocked >> for ever. >> >> It is always safe to defer all commands to libata if it is sata device. >> The ata_scsi_cmd_error_handler() will flag the timeout command as >> ATA_QCFLAG_FAILED if it is not set already. The libata EH will handle >> these >> qcs correctly. >> >> Signed-off-by: Jason Yan <yanaijie@huawei.com> >> CC: Xiaofei Tan <tanxiaofei@huawei.com> >> CC: John Garry <john.garry@huawei.com> > > > The change looks ok to me. > > But we need more review here from domain expert please. > > >> --- >> drivers/scsi/libsas/sas_scsi_host.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/scsi/libsas/sas_scsi_host.c >> b/drivers/scsi/libsas/sas_scsi_host.c >> index 6de9681ace82..fd00e432112b 100644 >> --- a/drivers/scsi/libsas/sas_scsi_host.c >> +++ b/drivers/scsi/libsas/sas_scsi_host.c >> @@ -274,7 +274,7 @@ static void sas_scsi_clear_queue_I_T(struct list_head >> *error_q, >> struct domain_device *x = cmd_to_domain_dev(cmd); >> >> if (x == dev) >> - sas_eh_finish_cmd(cmd); >> + sas_eh_defer_cmd(cmd); >> } >> } >> >> @@ -288,7 +288,7 @@ static void sas_scsi_clear_queue_port(struct list_head >> *error_q, >> struct asd_sas_port *x = dev->port; >> >> if (x == port) >> - sas_eh_finish_cmd(cmd); >> + sas_eh_defer_cmd(cmd); >> } >> } >> >> @@ -662,7 +662,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host >> *shost, struct list_head * >> struct domain_device *dev = task->dev; >> SAS_DPRINTK("I_T %016llx recovered\n", >> >> SAS_ADDR(task->dev->sas_addr)); >> - sas_eh_finish_cmd(cmd); >> + sas_eh_defer_cmd(cmd); >> sas_scsi_clear_queue_I_T(work_q, dev); >> goto Again; >> } >> @@ -676,7 +676,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host >> *shost, struct list_head * >> if (res == TMF_RESP_FUNC_COMPLETE) { >> SAS_DPRINTK("clear nexus port:%d " >> "succeeded\n", >> port->id); >> - sas_eh_finish_cmd(cmd); >> + sas_eh_defer_cmd(cmd); >> sas_scsi_clear_queue_port(work_q, >> port); >> goto Again; >> @@ -688,7 +688,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host >> *shost, struct list_head * >> if (res == TMF_RESP_FUNC_COMPLETE) { >> SAS_DPRINTK("clear nexus ha " >> "succeeded\n"); >> - sas_eh_finish_cmd(cmd); >> + sas_eh_defer_cmd(cmd); >> goto clear_q; >> } >> } >> @@ -701,7 +701,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host >> *shost, struct list_head * >> SAS_ADDR(task->dev->sas_addr), >> cmd->device->lun); >> >> - sas_eh_finish_cmd(cmd); >> + sas_eh_defer_cmd(cmd); >> goto clear_q; >> } >> } >> @@ -713,7 +713,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host >> *shost, struct list_head * >> clear_q: >> SAS_DPRINTK("--- Exit %s -- clear_q\n", __func__); >> list_for_each_entry_safe(cmd, n, work_q, eh_entry) >> - sas_eh_finish_cmd(cmd); >> + sas_eh_defer_cmd(cmd); >> goto out; >> } >> >> > >
On 2018/2/27 23:00, Jack Wang wrote: > 2018-02-27 12:50 GMT+01:00 John Garry <john.garry@huawei.com>: >> On 27/02/2018 06:59, Jason Yan wrote: >>> >>> When ata device doing EH, some commands still attached with tasks are not >>> passed to libata when abort failed or recover failed, so libata did not >>> handle these commands. After these commands done, sas task is freed, but >>> ata qc is not freed. This will cause ata qc leak and trigger a warning >>> like below: >> >> >> It's seems like a bug that we're just killing the ATA command in libsas >> error handling and not deferring them to ATA EH also. >> >> And this WARN, below, in ata_eh_finish() is a longterm issue I see (but >> maybe because of other issue also). >> >> As mentioned to Jason privately, I wonder why Dan's patch excluded the >> change here: >> commit 3944f50995f947558c35fb16ae0288354756762c >> Author: Dan Williams <dan.j.williams@intel.com> >> Date: Tue Nov 29 12:08:50 2011 -0800 >> >> [SCSI] libsas: let libata handle command timeouts >> >> libsas-eh if it successfully aborts an ata command will hide the timeout/ >> condition (AC_ERR_TIMEOUT) from libata. The command likely completes >> with the all-zero task->task_status it started with. Instead, interpret >> a TMF_RESP_FUNC_COMPLETE as the end of the sas_task but keep the scmd >> around for libata-eh to handle. >> >> Tested-by: Andrzej Jakowski <andrzej.jakowski@intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: James Bottomley <JBottomley@Parallels.com> >> >> >>> >>> WARNING: CPU: 0 PID: 28512 at drivers/ata/libata-eh.c:4037 >>> ata_eh_finish+0xb4/0xcc >>> CPU: 0 PID: 28512 Comm: kworker/u32:2 Tainted: G W OE 4.14.0#1 >>> ...... >>> Call trace: >>> [<ffff0000088b7bd0>] ata_eh_finish+0xb4/0xcc >>> [<ffff0000088b8420>] ata_do_eh+0xc4/0xd8 >>> [<ffff0000088b8478>] ata_std_error_handler+0x44/0x8c >>> [<ffff0000088b8068>] ata_scsi_port_error_handler+0x480/0x694 >>> [<ffff000008875fc4>] async_sas_ata_eh+0x4c/0x80 >>> [<ffff0000080f6be8>] async_run_entry_fn+0x4c/0x170 >>> [<ffff0000080ebd70>] process_one_work+0x144/0x390 >>> [<ffff0000080ec100>] worker_thread+0x144/0x418 >>> [<ffff0000080f2c98>] kthread+0x10c/0x138 >>> [<ffff0000080855dc>] ret_from_fork+0x10/0x18 > Hi John, hi Jason, > > We've seen this warning once on pm80xx with sata SSD in production (on > 3.12 kernel), but failed to see the root cause. > In my case, it's a chain sequence, one SSD failed, lead to error > handle & IO stuck. > > Do you have reproducer? > I have found this warning several times when our test team running a very large test suite. Sorry to say that I do not have a simple reproducer yet. > Your change looks good to me, but would be good to hear from Dan & James. > > Thanks, > Jack > > > >
On 28/02/2018 04:14, Jason Yan wrote: > > > On 2018/2/27 23:00, Jack Wang wrote: >> 2018-02-27 12:50 GMT+01:00 John Garry <john.garry@huawei.com>: >>> On 27/02/2018 06:59, Jason Yan wrote: >>>> >>>> When ata device doing EH, some commands still attached with tasks >>>> are not >>>> passed to libata when abort failed or recover failed, so libata did not >>>> handle these commands. After these commands done, sas task is freed, >>>> but >>>> ata qc is not freed. This will cause ata qc leak and trigger a warning >>>> like below: >>> >>> >>> It's seems like a bug that we're just killing the ATA command in libsas >>> error handling and not deferring them to ATA EH also. >>> >>> And this WARN, below, in ata_eh_finish() is a longterm issue I see (but >>> maybe because of other issue also). >>> >>> As mentioned to Jason privately, I wonder why Dan's patch excluded the >>> change here: >>> commit 3944f50995f947558c35fb16ae0288354756762c >>> Author: Dan Williams <dan.j.williams@intel.com> >>> Date: Tue Nov 29 12:08:50 2011 -0800 >>> >>> [SCSI] libsas: let libata handle command timeouts >>> >>> libsas-eh if it successfully aborts an ata command will hide the >>> timeout/ >>> condition (AC_ERR_TIMEOUT) from libata. The command likely >>> completes >>> with the all-zero task->task_status it started with. Instead, >>> interpret >>> a TMF_RESP_FUNC_COMPLETE as the end of the sas_task but keep the >>> scmd >>> around for libata-eh to handle. >>> >>> Tested-by: Andrzej Jakowski <andrzej.jakowski@intel.com> >>> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >>> Signed-off-by: James Bottomley <JBottomley@Parallels.com> >>> >>> >>>> >>>> WARNING: CPU: 0 PID: 28512 at drivers/ata/libata-eh.c:4037 >>>> ata_eh_finish+0xb4/0xcc >>>> CPU: 0 PID: 28512 Comm: kworker/u32:2 Tainted: G W OE 4.14.0#1 >>>> ...... >>>> Call trace: >>>> [<ffff0000088b7bd0>] ata_eh_finish+0xb4/0xcc >>>> [<ffff0000088b8420>] ata_do_eh+0xc4/0xd8 >>>> [<ffff0000088b8478>] ata_std_error_handler+0x44/0x8c >>>> [<ffff0000088b8068>] ata_scsi_port_error_handler+0x480/0x694 >>>> [<ffff000008875fc4>] async_sas_ata_eh+0x4c/0x80 >>>> [<ffff0000080f6be8>] async_run_entry_fn+0x4c/0x170 >>>> [<ffff0000080ebd70>] process_one_work+0x144/0x390 >>>> [<ffff0000080ec100>] worker_thread+0x144/0x418 >>>> [<ffff0000080f2c98>] kthread+0x10c/0x138 >>>> [<ffff0000080855dc>] ret_from_fork+0x10/0x18 >> Hi John, hi Jason, >> >> We've seen this warning once on pm80xx with sata SSD in production (on >> 3.12 kernel), but failed to see the root cause. >> In my case, it's a chain sequence, one SSD failed, lead to error >> handle & IO stuck. >> >> Do you have reproducer? >> > > I have found this warning several times when our test team running a > very large test suite. Sorry to say that I do not have a simple > reproducer yet. > Typically we would see this when commands timeout after we unplug a SATA disk with IO in flight, right? John >> Your change looks good to me, but would be good to hear from Dan & James. >> >> Thanks, >> Jack >> >> >> >> > > > > . >
On 2018/2/28 16:09, John Garry wrote: > On 28/02/2018 04:14, Jason Yan wrote: >> >> >> On 2018/2/27 23:00, Jack Wang wrote: >>> 2018-02-27 12:50 GMT+01:00 John Garry <john.garry@huawei.com>: >>>> On 27/02/2018 06:59, Jason Yan wrote: >>>>> >>>>> When ata device doing EH, some commands still attached with tasks >>>>> are not >>>>> passed to libata when abort failed or recover failed, so libata did >>>>> not >>>>> handle these commands. After these commands done, sas task is freed, >>>>> but >>>>> ata qc is not freed. This will cause ata qc leak and trigger a warning >>>>> like below: >>>> >>>> >>>> It's seems like a bug that we're just killing the ATA command in libsas >>>> error handling and not deferring them to ATA EH also. >>>> >>>> And this WARN, below, in ata_eh_finish() is a longterm issue I see (but >>>> maybe because of other issue also). >>>> >>>> As mentioned to Jason privately, I wonder why Dan's patch excluded the >>>> change here: >>>> commit 3944f50995f947558c35fb16ae0288354756762c >>>> Author: Dan Williams <dan.j.williams@intel.com> >>>> Date: Tue Nov 29 12:08:50 2011 -0800 >>>> >>>> [SCSI] libsas: let libata handle command timeouts >>>> >>>> libsas-eh if it successfully aborts an ata command will hide the >>>> timeout/ >>>> condition (AC_ERR_TIMEOUT) from libata. The command likely >>>> completes >>>> with the all-zero task->task_status it started with. Instead, >>>> interpret >>>> a TMF_RESP_FUNC_COMPLETE as the end of the sas_task but keep the >>>> scmd >>>> around for libata-eh to handle. >>>> >>>> Tested-by: Andrzej Jakowski <andrzej.jakowski@intel.com> >>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >>>> Signed-off-by: James Bottomley <JBottomley@Parallels.com> >>>> >>>> >>>>> >>>>> WARNING: CPU: 0 PID: 28512 at drivers/ata/libata-eh.c:4037 >>>>> ata_eh_finish+0xb4/0xcc >>>>> CPU: 0 PID: 28512 Comm: kworker/u32:2 Tainted: G W OE 4.14.0#1 >>>>> ...... >>>>> Call trace: >>>>> [<ffff0000088b7bd0>] ata_eh_finish+0xb4/0xcc >>>>> [<ffff0000088b8420>] ata_do_eh+0xc4/0xd8 >>>>> [<ffff0000088b8478>] ata_std_error_handler+0x44/0x8c >>>>> [<ffff0000088b8068>] ata_scsi_port_error_handler+0x480/0x694 >>>>> [<ffff000008875fc4>] async_sas_ata_eh+0x4c/0x80 >>>>> [<ffff0000080f6be8>] async_run_entry_fn+0x4c/0x170 >>>>> [<ffff0000080ebd70>] process_one_work+0x144/0x390 >>>>> [<ffff0000080ec100>] worker_thread+0x144/0x418 >>>>> [<ffff0000080f2c98>] kthread+0x10c/0x138 >>>>> [<ffff0000080855dc>] ret_from_fork+0x10/0x18 >>> Hi John, hi Jason, >>> >>> We've seen this warning once on pm80xx with sata SSD in production (on >>> 3.12 kernel), but failed to see the root cause. >>> In my case, it's a chain sequence, one SSD failed, lead to error >>> handle & IO stuck. >>> >>> Do you have reproducer? >>> >> >> I have found this warning several times when our test team running a >> very large test suite. Sorry to say that I do not have a simple >> reproducer yet. >> > > Typically we would see this when commands timeout after we unplug a SATA > disk with IO in flight, right? > Yes, that's true. > John > >>> Your change looks good to me, but would be good to hear from Dan & >>> James. >>> >>> Thanks, >>> Jack >>> >>> >>> >>> >> >> >> >> . >> > > > > . >
> When ata device doing EH, some commands still attached with tasks are not > passed to libata when abort failed or recover failed, so libata did not > handle these commands. After these commands done, sas task is freed, but > ata qc is not freed. Somebody from the libsas camp, please review! https://patchwork.kernel.org/patch/10244375/
On Tue, Mar 6, 2018 at 10:04 AM, Martin K. Petersen <martin.petersen@oracle.com> wrote: > >> When ata device doing EH, some commands still attached with tasks are not >> passed to libata when abort failed or recover failed, so libata did not >> handle these commands. After these commands done, sas task is freed, but >> ata qc is not freed. > > Somebody from the libsas camp, please review! > > https://patchwork.kernel.org/patch/10244375/ It's been a while since I've had access to libsas hardware, but this patch looks like a good idea to me. However, it seems that it could be narrowed to just the sas_eh_finish_cmd() that need to be converted to sas_eh_defer_cmd(). Otherwise, if there are no more direct usages of sas_eh_finish_cmd() then we should consider just renaming sas_eh_defer_cmd() to sas_eh_finish_cmd() and delete the old sas_eh_finish_cmd(), i.e. remove the distinction.
On 2018/3/7 2:29, Dan Williams wrote: > On Tue, Mar 6, 2018 at 10:04 AM, Martin K. Petersen > <martin.petersen@oracle.com> wrote: >> >>> When ata device doing EH, some commands still attached with tasks are not >>> passed to libata when abort failed or recover failed, so libata did not >>> handle these commands. After these commands done, sas task is freed, but >>> ata qc is not freed. >> >> Somebody from the libsas camp, please review! >> >> https://patchwork.kernel.org/patch/10244375/ > > It's been a while since I've had access to libsas hardware, but this > patch looks like a good idea to me. However, it seems that it could be > narrowed to just the sas_eh_finish_cmd() that need to be converted to > sas_eh_defer_cmd(). Otherwise, if there are no more direct usages of > sas_eh_finish_cmd() then we should consider just renaming > sas_eh_defer_cmd() to sas_eh_finish_cmd() and delete the old > sas_eh_finish_cmd(), i.e. remove the distinction. > Delete the old sas_eh_finish_cmd() seems better. Thanks for the suggestion. I will respin this patch. > . >
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 6de9681ace82..fd00e432112b 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -274,7 +274,7 @@ static void sas_scsi_clear_queue_I_T(struct list_head *error_q, struct domain_device *x = cmd_to_domain_dev(cmd); if (x == dev) - sas_eh_finish_cmd(cmd); + sas_eh_defer_cmd(cmd); } } @@ -288,7 +288,7 @@ static void sas_scsi_clear_queue_port(struct list_head *error_q, struct asd_sas_port *x = dev->port; if (x == port) - sas_eh_finish_cmd(cmd); + sas_eh_defer_cmd(cmd); } } @@ -662,7 +662,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * struct domain_device *dev = task->dev; SAS_DPRINTK("I_T %016llx recovered\n", SAS_ADDR(task->dev->sas_addr)); - sas_eh_finish_cmd(cmd); + sas_eh_defer_cmd(cmd); sas_scsi_clear_queue_I_T(work_q, dev); goto Again; } @@ -676,7 +676,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * if (res == TMF_RESP_FUNC_COMPLETE) { SAS_DPRINTK("clear nexus port:%d " "succeeded\n", port->id); - sas_eh_finish_cmd(cmd); + sas_eh_defer_cmd(cmd); sas_scsi_clear_queue_port(work_q, port); goto Again; @@ -688,7 +688,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * if (res == TMF_RESP_FUNC_COMPLETE) { SAS_DPRINTK("clear nexus ha " "succeeded\n"); - sas_eh_finish_cmd(cmd); + sas_eh_defer_cmd(cmd); goto clear_q; } } @@ -701,7 +701,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * SAS_ADDR(task->dev->sas_addr), cmd->device->lun); - sas_eh_finish_cmd(cmd); + sas_eh_defer_cmd(cmd); goto clear_q; } } @@ -713,7 +713,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head * clear_q: SAS_DPRINTK("--- Exit %s -- clear_q\n", __func__); list_for_each_entry_safe(cmd, n, work_q, eh_entry) - sas_eh_finish_cmd(cmd); + sas_eh_defer_cmd(cmd); goto out; }
When ata device doing EH, some commands still attached with tasks are not passed to libata when abort failed or recover failed, so libata did not handle these commands. After these commands done, sas task is freed, but ata qc is not freed. This will cause ata qc leak and trigger a warning like below: WARNING: CPU: 0 PID: 28512 at drivers/ata/libata-eh.c:4037 ata_eh_finish+0xb4/0xcc CPU: 0 PID: 28512 Comm: kworker/u32:2 Tainted: G W OE 4.14.0#1 ...... Call trace: [<ffff0000088b7bd0>] ata_eh_finish+0xb4/0xcc [<ffff0000088b8420>] ata_do_eh+0xc4/0xd8 [<ffff0000088b8478>] ata_std_error_handler+0x44/0x8c [<ffff0000088b8068>] ata_scsi_port_error_handler+0x480/0x694 [<ffff000008875fc4>] async_sas_ata_eh+0x4c/0x80 [<ffff0000080f6be8>] async_run_entry_fn+0x4c/0x170 [<ffff0000080ebd70>] process_one_work+0x144/0x390 [<ffff0000080ec100>] worker_thread+0x144/0x418 [<ffff0000080f2c98>] kthread+0x10c/0x138 [<ffff0000080855dc>] ret_from_fork+0x10/0x18 If ata qc leaked too many, ata tag allocation will fail and io blocked for ever. It is always safe to defer all commands to libata if it is sata device. The ata_scsi_cmd_error_handler() will flag the timeout command as ATA_QCFLAG_FAILED if it is not set already. The libata EH will handle these qcs correctly. Signed-off-by: Jason Yan <yanaijie@huawei.com> CC: Xiaofei Tan <tanxiaofei@huawei.com> CC: John Garry <john.garry@huawei.com> --- drivers/scsi/libsas/sas_scsi_host.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)