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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Signed-off-by: James Bottomley <JBottomley@xxxxxxxxxxxxx>
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@xxxxxxxxxx>
CC: Xiaofei Tan <tanxiaofei@xxxxxxxxxx>
CC: John Garry <john.garry@xxxxxxxxxx>
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;
}
