Hi, Niklas
On 2024/10/31 22:07, Niklas Cassel wrote:
libata is responsible to ensure that NCQ and non-NCQ commands are not mixed
in the command list for the same device.
This is handled using the .qc_defer callback (ata_std_qc_defer()), which
will defer a non-NCQ command as long as there are NCQ commands in flight.
The problem is that if an application is continuously submitting NCQ
commands (e.g. fio with a queue depth greater than 1), this can completely
starve out another application that is sending a non-NCQ command (because
the non-NCQ command will be deferred forever).
Solve this by triggering EH if there are NCQ commands in flight when a
non-NCQ is submitted. If EH is scheduled, no new commands will be accepted,
and EH will wake up when there are no commands in flight. We will then
submit the non-NCQ command from EH context, and synchronously wait for the
completion. When EH is finished, libata will continue to accept new
commands like normal.
Reported-by: Xingui Yang <yangxingui@xxxxxxxxxx>
Closes: https://lore.kernel.org/linux-block/eef1e927-c9b2-c61d-7f48-92e65d8b0418@xxxxxxxxxx/
Suggested-by: Hannes Reinecke <hare@xxxxxxx>
Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
---
drivers/ata/libata-core.c | 169 +++++++++++++++++++++++++++++++++++---
drivers/ata/libata-eh.c | 60 +++++++++++++-
drivers/ata/libata-scsi.c | 16 +++-
drivers/ata/libata.h | 1 +
include/linux/libata.h | 7 +-
5 files changed, 237 insertions(+), 16 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2b7d265e4a7b..c53de1d3baba 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1633,6 +1633,134 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
return err_mask;
}
+/**
+ * ata_issue_via_eh - issue non-NCQ command via EH synchronously
+ * @qc: command to issue to device
+ *
+ * Issues a non-NCQ command via EH and waits for completion. @qc contains
+ * the command on entry and the result on return. Timeout and error
+ * conditions are reported via the return value. No recovery action is
+ * needed, since flag ATA_QCFLAG_EH is set on entry and on exit, so in case
+ * of error, EH will clean it up during ata_eh_finish().
+ *
+ * LOCKING:
+ * None. Should be called with kernel context, might sleep.
+ *
+ * RETURNS:
+ * Zero on success, AC_ERR_* mask on failure
+ */
+unsigned int ata_issue_via_eh(struct ata_queued_cmd *qc)
After testing, the issues we encountered were resolved.
But the kernel prints the following log:
[246993.392832] sas: Enter sas_scsi_recover_host busy: 1 failed: 1
[246993.392839] sas: ata5: end_device-4:0: cmd error handler
[246993.392855] sas: ata5: end_device-4:0: dev error handler
[246993.392860] sas: ata6: end_device-4:3: dev error handler
[246993.392863] sas: ata7: end_device-4:4: dev error handler
[246993.606491] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 1
tries: 1
And because the current EH will set the host to the recovery state, when
we test and execute the smartctl command, it will affect the performance
of all other disks under the same host.
Perhaps we can continue to improve the EH mechanism that Wenchao tried
to do before, and implement EH for a single disk. After a single disk
enters EH, it may not affect other disks under the same host.
https://lore.kernel.org/linux-scsi/20230901094127.2010873-1-haowenchao2@xxxxxxxxxx/
Thanks，
Xingui
