* [PATCH v1 0/2] fixup handling of timeouts with deferred QCs
@ 2026-07-09 8:39 Damien Le Moal
2026-07-09 8:39 ` [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out Damien Le Moal
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Damien Le Moal @ 2026-07-09 8:39 UTC (permalink / raw)
To: linux-ide, Niklas Cassel, linux-scsi, Martin K . Petersen
Cc: Igor Pylypiv, John Garry, Jason Yan
This patch series fixes libata and libsas to correctly handles deferred
queued commands in case of a timeout error, to avoid excessive delays in
waking up the scsi EH task.
Igor,
Please retest !
Martin,
Once reviewed, I or you can take both patches ?
Damien Le Moal (2):
ata: libata-scsi: terminate deferred commands on time out
scsi: libsas: terminate deferred commands on time out
drivers/ata/libata-eh.c | 2 +-
drivers/ata/libata-scsi.c | 54 +++++++++++++++++++++++++----
drivers/ata/libata.h | 3 +-
drivers/scsi/libsas/sas_scsi_host.c | 17 +++++++++
include/linux/libata.h | 4 +++
include/scsi/libsas.h | 2 ++
6 files changed, 73 insertions(+), 9 deletions(-)
--
2.55.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out 2026-07-09 8:39 [PATCH v1 0/2] fixup handling of timeouts with deferred QCs Damien Le Moal @ 2026-07-09 8:39 ` Damien Le Moal 2026-07-09 9:00 ` sashiko-bot 2026-07-09 8:39 ` [PATCH v1 2/2] scsi: libsas: " Damien Le Moal 2026-07-09 17:36 ` [PATCH v1 0/2] fixup handling of timeouts with deferred QCs Igor Pylypiv 2 siblings, 1 reply; 10+ messages in thread From: Damien Le Moal @ 2026-07-09 8:39 UTC (permalink / raw) To: linux-ide, Niklas Cassel, linux-scsi, Martin K . Petersen Cc: Igor Pylypiv, John Garry, Jason Yan If a command timeout occurs while we have a deferred non-NCQ command waiting to be issued, the scsi EH task is never woken up as the waiting deferred command is never issued nor completed, thus leaving this command to always be counted as "busy" for the scsi host. This results in the scsi_error_handler() function test "shost->host_failed != scsi_host_busy(shost))" to always be true, keeping the EH task sleeping. Eventuially, when the deferred command also times out, the EH task is woken up and the timeout processing occurs. Avoid this unnecessary EH trigger wait time using the eh_timed_out scsi host template operation. The function ata_scsi_eh_timed_out() is introduced to implement this operation using the helper function ata_scsi_port_eh_timed_out(). This function forces a requeue, or a timed out completion, of any deferred queued command when a time out error occurs. Since the timeout itself and eventual re-issuing of commands is not handled by this helper, SCSI_EH_NOT_HANDLED is returned to have scsi_timeout() continue with the regular timeout handling. Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-eh.c | 2 +- drivers/ata/libata-scsi.c | 53 +++++++++++++++++++++++++++++++++------ drivers/ata/libata.h | 3 ++- include/linux/libata.h | 2 ++ 4 files changed, 51 insertions(+), 9 deletions(-) diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 05df7ea6954a..57d3d2d11dd8 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -951,7 +951,7 @@ static void ata_eh_set_pending(struct ata_port *ap, bool fastdrain) * If we have a deferred qc, requeue it so that it is retried once EH * completes. */ - ata_scsi_requeue_deferred_qc(ap); + ata_scsi_requeue_deferred_qc(ap, NULL); if (!fastdrain) return; diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 5868526301a2..8328778ec046 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1685,7 +1685,8 @@ void ata_scsi_deferred_qc_work(struct work_struct *work) spin_unlock_irqrestore(ap->lock, flags); } -void ata_scsi_requeue_deferred_qc(struct ata_port *ap) +void ata_scsi_requeue_deferred_qc(struct ata_port *ap, + struct scsi_cmnd *timed_out_scmd) { struct ata_link *link; @@ -1698,12 +1699,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap) */ ata_for_each_link(link, ap, PMP_FIRST) { struct ata_queued_cmd *qc = link->deferred_qc; + u32 host_byte; - if (qc) { - link->deferred_qc = NULL; - cancel_work(&link->deferred_qc_work); - ata_scsi_qc_done(qc, true, DID_REQUEUE << 16); - } + if (!qc) + continue; + + link->deferred_qc = NULL; + cancel_work(&link->deferred_qc_work); + + if (qc->scsicmd == timed_out_scmd) + host_byte = DID_TIME_OUT; + else + host_byte = DID_REQUEUE; + ata_scsi_qc_done(qc, true, host_byte << 16); } } @@ -1723,13 +1731,44 @@ static void ata_scsi_schedule_deferred_qc(struct ata_link *link) return; if (ata_port_eh_scheduled(ap)) { - ata_scsi_requeue_deferred_qc(ap); + ata_scsi_requeue_deferred_qc(ap, NULL); return; } if (!ap->ops->qc_defer(qc)) queue_work(system_highpri_wq, &link->deferred_qc_work); } +static enum scsi_timeout_action +ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd) +{ + unsigned long flags; + + /* + * We had a timeout, either for an NCQ command or for one deferred + * queued command. If we have deferred QCs and we do not release them + * immediately, we will have shost->host_failed != scsi_host_busy() + * until the deferred QCs also timeout. This unnecessarilly increases + * the time it takes for scsi EH to start. Terminate all deferred QCs + * to avoid that. + */ + spin_lock_irqsave(ap->lock, flags); + ata_scsi_requeue_deferred_qc(ap, scmd); + spin_unlock_irqrestore(ap->lock, flags); + + /* + * Let scsi_timeout() know that it must continue with handling the + * timeout as we in fact did not do much here. + */ + return SCSI_EH_NOT_HANDLED; +} + +enum scsi_timeout_action ata_scsi_eh_timed_out(struct scsi_cmnd *scmd) +{ + return ata_scsi_port_eh_timed_out(ata_shost_to_port(scmd->device->host), + scmd); +} +EXPORT_SYMBOL_GPL(ata_scsi_eh_timed_out); + static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) { struct ata_link *link = qc->dev->link; diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 700627596ce1..14912e140686 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -180,7 +180,8 @@ enum scsi_qc_status __ata_scsi_queuecmd(struct scsi_cmnd *scmd, struct ata_port *ap) __must_hold(ap->lock); void ata_scsi_deferred_qc_work(struct work_struct *work); -void ata_scsi_requeue_deferred_qc(struct ata_port *ap); +void ata_scsi_requeue_deferred_qc(struct ata_port *ap, + struct scsi_cmnd *timed_out_scmd); /* libata-eh.c */ extern unsigned int ata_internal_cmd_timeout(struct ata_device *dev, u8 cmd); diff --git a/include/linux/libata.h b/include/linux/libata.h index 96e626d6a7ca..327da43d7496 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1153,6 +1153,7 @@ extern int ata_scsi_ioctl(struct scsi_device *dev, unsigned int cmd, #endif extern enum scsi_qc_status ata_scsi_queuecmd(struct Scsi_Host *h, struct scsi_cmnd *cmd); +enum scsi_timeout_action ata_scsi_eh_timed_out(struct scsi_cmnd *cmd); #if IS_REACHABLE(CONFIG_ATA) bool ata_scsi_dma_need_drain(struct request *rq); #else @@ -1464,6 +1465,7 @@ extern const struct attribute_group *ata_common_sdev_groups[]; .ioctl = ata_scsi_ioctl, \ ATA_SCSI_COMPAT_IOCTL \ .queuecommand = ata_scsi_queuecmd, \ + .eh_timed_out = ata_scsi_eh_timed_out, \ .dma_need_drain = ata_scsi_dma_need_drain, \ .this_id = ATA_SHT_THIS_ID, \ .emulated = ATA_SHT_EMULATED, \ -- 2.55.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out 2026-07-09 8:39 ` [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out Damien Le Moal @ 2026-07-09 9:00 ` sashiko-bot 2026-07-09 9:14 ` Damien Le Moal 0 siblings, 1 reply; 10+ messages in thread From: sashiko-bot @ 2026-07-09 9:00 UTC (permalink / raw) To: Damien Le Moal; +Cc: linux-ide, dlemoal, linux-scsi, cassel Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The eh_timed_out handler prematurely completes the timed-out deferred command, freeing the libata `qc` and causing the SCSI error handler to permanently fail the command instead of retrying it. -- --- Patch [1]: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out --- commit 5d7ac082b6ce256e2174fbf7d209e8acc3d6b3a9 Author: Damien Le Moal <dlemoal@kernel.org> ata: libata-scsi: terminate deferred commands on time out If a command timeout occurs while we have a deferred non-NCQ command waiting to be issued, the scsi EH task is never woken up. Avoid this unnecessary EH trigger wait time by using the eh_timed_out scsi host template operation to terminate deferred QCs. Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation") Cc: stable@vger.kernel.org > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 5868526301a2b..8328778ec0464 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c [ ... ] > @@ -1698,12 +1699,19 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap) > */ > ata_for_each_link(link, ap, PMP_FIRST) { > struct ata_queued_cmd *qc = link->deferred_qc; > + u32 host_byte; > > - if (qc) { > - link->deferred_qc = NULL; > - cancel_work(&link->deferred_qc_work); > - ata_scsi_qc_done(qc, true, DID_REQUEUE << 16); > - } > + if (!qc) > + continue; > + > + link->deferred_qc = NULL; > + cancel_work(&link->deferred_qc_work); > + > + if (qc->scsicmd == timed_out_scmd) > + host_byte = DID_TIME_OUT; > + else > + host_byte = DID_REQUEUE; > + ata_scsi_qc_done(qc, true, host_byte << 16); [Severity: High] Does calling ata_scsi_qc_done() here prematurely complete the timed-out deferred command and free the libata qc? When a deferred command times out, ata_scsi_eh_timed_out() calls this function which sets the host byte to DID_TIME_OUT and calls ata_scsi_qc_done(). ata_scsi_qc_done() then frees the qc and invokes the qc->scsidone callback. Later, when the SCSI completion softirq routes the DID_TIME_OUT command to the EH queue and the EH thread runs ata_scsi_cmd_error_handler(): drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() { ... set_host_byte(scmd, DID_OK); ata_qc_for_each_raw(ap, qc, i) { if (qc->scsicmd != scmd) continue; if ((qc->flags & ATA_QCFLAG_ACTIVE) || qc == qc->dev->link->deferred_qc) break; } ... } Because the qc was already freed, the ATA_QCFLAG_ACTIVE flag is cleared and deferred_qc is NULL. The loop will fail to find the active command. It seems this causes the handler to fall into the 'raced completion' branch: drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() { ... } else { /* Normal completion occurred after * SCSI timeout but before this point. * Successfully complete it. */ scmd->retries = scmd->allowed; scsi_eh_finish_cmd(scmd, &ap->eh_done_q); } ... } Could this result in the midlayer completely dropping the timed-out deferred command (completed with DID_OK and no retries) instead of properly retrying it? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260709083934.1116862-1-dlemoal@kernel.org?part=1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out 2026-07-09 9:00 ` sashiko-bot @ 2026-07-09 9:14 ` Damien Le Moal 2026-07-09 17:27 ` Niklas Cassel 2026-07-09 17:56 ` Niklas Cassel 0 siblings, 2 replies; 10+ messages in thread From: Damien Le Moal @ 2026-07-09 9:14 UTC (permalink / raw) To: sashiko-reviews; +Cc: linux-ide, linux-scsi, cassel On 7/9/26 18:00, sashiko-bot@kernel.org wrote: > [Severity: High] > Does calling ata_scsi_qc_done() here prematurely complete the timed-out > deferred command and free the libata qc? Yes it does. But unless it is the deferred QC that suffered the timeout, the scsi command will be requeued and retried. > When a deferred command times out, ata_scsi_eh_timed_out() calls this function > which sets the host byte to DID_TIME_OUT and calls ata_scsi_qc_done(). > ata_scsi_qc_done() then frees the qc and invokes the qc->scsidone callback. > > Later, when the SCSI completion softirq routes the DID_TIME_OUT command to the > EH queue and the EH thread runs ata_scsi_cmd_error_handler(): > > drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() { > ... > set_host_byte(scmd, DID_OK); > ata_qc_for_each_raw(ap, qc, i) { > if (qc->scsicmd != scmd) > continue; > if ((qc->flags & ATA_QCFLAG_ACTIVE) || > qc == qc->dev->link->deferred_qc) > break; > } > ... > } > > Because the qc was already freed, the ATA_QCFLAG_ACTIVE flag is cleared > and deferred_qc is NULL. The loop will fail to find the active command. > > It seems this causes the handler to fall into the 'raced completion' branch: > > drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() { > ... > } else { > /* Normal completion occurred after > * SCSI timeout but before this point. > * Successfully complete it. > */ > scmd->retries = scmd->allowed; > scsi_eh_finish_cmd(scmd, &ap->eh_done_q); > } > ... > } And after this, scsi_eh_flush_done_q() will retry the command. Tested and I do not see failure of the deferred QC :) -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out 2026-07-09 9:14 ` Damien Le Moal @ 2026-07-09 17:27 ` Niklas Cassel 2026-07-09 17:56 ` Niklas Cassel 1 sibling, 0 replies; 10+ messages in thread From: Niklas Cassel @ 2026-07-09 17:27 UTC (permalink / raw) To: Damien Le Moal; +Cc: sashiko-reviews, linux-ide, linux-scsi On Thu, Jul 09, 2026 at 06:14:08PM +0900, Damien Le Moal wrote: > > It seems this causes the handler to fall into the 'raced completion' branch: > > > > drivers/ata/libata-eh.c:ata_scsi_cmd_error_handler() { > > ... > > } else { > > /* Normal completion occurred after > > * SCSI timeout but before this point. > > * Successfully complete it. > > */ > > scmd->retries = scmd->allowed; > > scsi_eh_finish_cmd(scmd, &ap->eh_done_q); > > } > > ... > > } > > And after this, scsi_eh_flush_done_q() will retry the command. > Tested and I do not see failure of the deferred QC :) I don't see how scsi_eh_flush_done_q() will retry the command. Since ata_scsi_eh_timed_out() calls ata_scsi_qc_done(), which calls scsi_done(), which calls scsi_done_internal(cmd, false), which sets SCMD_STATE_COMPLETE, scsi_timeout() will never add the scmd to the list of failed scmds using scsi_eh_scmd_add(). Thus, when ata_scsi_cmd_error_handler() loops though the list of failed scmds, the scmd in question will never have been added in the first place. Most likely the command is retried via: ata_scsi_qc_done() -> scsi_done() -> scsi_done_internal() softirq -> scsi_complete() -> scsi_decide_disposition() scsi_decide_disposition() returns ADD_TO_MLQUEUE for DID_REQUEUE, scsi_complete() calls scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY); when scsi_decide_disposition() returns ADD_DO_MLQUEUE. Kind regards, Niklas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out 2026-07-09 9:14 ` Damien Le Moal 2026-07-09 17:27 ` Niklas Cassel @ 2026-07-09 17:56 ` Niklas Cassel 2026-07-09 18:02 ` Niklas Cassel 2026-07-10 0:00 ` Damien Le Moal 1 sibling, 2 replies; 10+ messages in thread From: Niklas Cassel @ 2026-07-09 17:56 UTC (permalink / raw) To: Damien Le Moal; +Cc: sashiko-reviews, linux-ide, linux-scsi, Igor Pylypiv On Thu, Jul 09, 2026 at 06:14:08PM +0900, Damien Le Moal wrote: > On 7/9/26 18:00, sashiko-bot@kernel.org wrote: > > [Severity: High] > > Does calling ata_scsi_qc_done() here prematurely complete the timed-out > > deferred command and free the libata qc? > > Yes it does. But unless it is the deferred QC that suffered the timeout, the > scsi command will be requeued and retried. I think Sashiko has a good point. After this patch: We will have some deferred QC handling in ata_scsi_eh_timed_out(scmd): -Sets link->deferred_qc = NULL; -Cancels the workqueue. ata_scsi_eh_timed_out() will do the following: If scmd (the QC that timed out) was the deferred QC: -Sets DID_REQUEUE on the deferred QC. Else: -Sets DID_TIMEOUT on the deferred QC. -Calls ata_scsi_qc_done(), which frees the deferred QC and calls scsi_done() -scsi_done() will call scsi_done_internal(cmd, false); scsi_done_internal(cmd, false) will set SCMD_STATE_COMPLETE, but will defer the actual completion (scsi_complete() to softirq context. -ata_scsi_eh_timed_out() will return SCSI_EH_NOT_HANDLED to scsi_timeout(), which will break; and will then evaluate if SCMD_STATE_COMPLETE is set, if it is, it will do nothing. (If it is not set it will add the scmd to the list of failed scmds.) In case scmd == the deferred QC, since scsi_done_internal() will set SCMD_STATE_COMPLETE, before deferring the completion to softirq context, the code in scsi_timeout() will not add the scmd to the error list using scsi_eh_scmd_add(), instead it will return BLK_EH_DONE; Thus, Sashikos comment can not happen in realtity, because if the deferred QC timed out, it will never be added to the list of scmds which ata_scsi_cmd_error_handler() will loop over. I think it would be cleaner if we: 1) Modify ata_scsi_eh_timed_out(): if scmd == the deferred QC, set DID_TIMEOUT, but return SCSI_EH_DONE. This way it is more obvious that no further EH will be done. (Instead of relying on SCMD_STATE_COMPLETE already have been set, even though the completion was deferred to softirq context.) If scmd != the deferred QC, continue to set DID_REQUEUE and return SCSI_EH_NOT_HANDLED. 2) If it was a timeout of the deferred QC, then we know that ata_scsi_eh_timed_out() has been called, before ata_scsi_cmd_error_handler() is called. On NCQ error, while having a deferred non-NCQ QC, ata_scsi_cmd_error_handler() will be called without ata_scsi_eh_timed_out() having been called first. Thus, we should be able to remove the code which specifically handles the case where the deferred QC timed out in ata_scsi_eh_timed_out(). If the deferred QC timed out, ata_scsi_eh_timed_out() must have been called with the timed out deferred QC as argument. And ata_scsi_eh_timed_out() will set link->deferred_qc = NULL; so the code which handles the deferred QC timing out, should now be dead code that can never be reached. (On a hard NCQ error, the deferred QC will be requeued using: ata_do_link_abort() -> ata_eh_set_pending() -> ata_scsi_requeue_deferred_qc(), which sets link->deferred_qc = NULL; cancels the workqueue and calls ata_scsi_qc_done(qc, true, DID_REQUEUE << 16)) Kind regards, Niklas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out 2026-07-09 17:56 ` Niklas Cassel @ 2026-07-09 18:02 ` Niklas Cassel 2026-07-10 0:00 ` Damien Le Moal 1 sibling, 0 replies; 10+ messages in thread From: Niklas Cassel @ 2026-07-09 18:02 UTC (permalink / raw) To: Damien Le Moal; +Cc: sashiko-reviews, linux-ide, linux-scsi, Igor Pylypiv On Thu, Jul 09, 2026 at 07:56:06PM +0200, Niklas Cassel wrote: > If scmd != the deferred QC, continue to set DID_REQUEUE and return > SCSI_EH_NOT_HANDLED. Just to clarify: If scmd != the deferred QC, continue to set DID_REQUEUE on the deferred QC, and return SCSI_EH_NOT_HANDLED, since we did not touch scmd. Kind regards, Niklas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out 2026-07-09 17:56 ` Niklas Cassel 2026-07-09 18:02 ` Niklas Cassel @ 2026-07-10 0:00 ` Damien Le Moal 1 sibling, 0 replies; 10+ messages in thread From: Damien Le Moal @ 2026-07-10 0:00 UTC (permalink / raw) To: Niklas Cassel; +Cc: sashiko-reviews, linux-ide, linux-scsi, Igor Pylypiv On 7/10/26 02:56, Niklas Cassel wrote: > On Thu, Jul 09, 2026 at 06:14:08PM +0900, Damien Le Moal wrote: >> On 7/9/26 18:00, sashiko-bot@kernel.org wrote: >>> [Severity: High] >>> Does calling ata_scsi_qc_done() here prematurely complete the timed-out >>> deferred command and free the libata qc? >> >> Yes it does. But unless it is the deferred QC that suffered the timeout, the >> scsi command will be requeued and retried. > > I think Sashiko has a good point. > > > After this patch: > > We will have some deferred QC handling in ata_scsi_eh_timed_out(scmd): > > -Sets link->deferred_qc = NULL; > > -Cancels the workqueue. > > ata_scsi_eh_timed_out() will do the following: > If scmd (the QC that timed out) was the deferred QC: > -Sets DID_REQUEUE on the deferred QC. > Else: > -Sets DID_TIMEOUT on the deferred QC. > > -Calls ata_scsi_qc_done(), which frees the deferred QC and calls scsi_done() > > -scsi_done() will call scsi_done_internal(cmd, false); > scsi_done_internal(cmd, false) will set SCMD_STATE_COMPLETE, > but will defer the actual completion (scsi_complete() to softirq context. > -ata_scsi_eh_timed_out() will return SCSI_EH_NOT_HANDLED to scsi_timeout(), > which will break; and will then evaluate if SCMD_STATE_COMPLETE is set, > if it is, it will do nothing. > (If it is not set it will add the scmd to the list of failed scmds.) > > In case scmd == the deferred QC, since scsi_done_internal() will set > SCMD_STATE_COMPLETE, before deferring the completion to softirq context, > the code in scsi_timeout() will not add the scmd to the error list using > scsi_eh_scmd_add(), instead it will return BLK_EH_DONE; > > Thus, Sashikos comment can not happen in realtity, because if the deferred > QC timed out, it will never be added to the list of scmds which > ata_scsi_cmd_error_handler() will loop over. > > I think it would be cleaner if we: > > 1) Modify ata_scsi_eh_timed_out(): > if scmd == the deferred QC, set DID_TIMEOUT, but return SCSI_EH_DONE. > This way it is more obvious that no further EH will be done. (Instead of > relying on SCMD_STATE_COMPLETE already have been set, even though the > completion was deferred to softirq context.) Yes, you are right. For a timedout command, we always get +1 to the scsi host failed counter, so we do not need to do anything special to decrement the busy counter for a deferred QC when it times out. As for the change, since ata_scsi_cmd_error_handler() already correctly handles timed-out deferred QCs, when we have scmd == the deferred QC, we should just not do anything at all in ata_scsi_requeue_deferred_qc(). ata_scsi_cmd_error_handler() will take care of it. Sending a v2 with that change. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 2/2] scsi: libsas: terminate deferred commands on time out 2026-07-09 8:39 [PATCH v1 0/2] fixup handling of timeouts with deferred QCs Damien Le Moal 2026-07-09 8:39 ` [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out Damien Le Moal @ 2026-07-09 8:39 ` Damien Le Moal 2026-07-09 17:36 ` [PATCH v1 0/2] fixup handling of timeouts with deferred QCs Igor Pylypiv 2 siblings, 0 replies; 10+ messages in thread From: Damien Le Moal @ 2026-07-09 8:39 UTC (permalink / raw) To: linux-ide, Niklas Cassel, linux-scsi, Martin K . Petersen Cc: Igor Pylypiv, John Garry, Jason Yan If a command timeout occurs while we have a deferred non-NCQ command waiting to be issued, the scsi EH task is never woken up as the waiting deferred command is never issued nor completed, thus leaving this command to always be counted as "busy" for the scsi host. This results in the scsi_error_handler() function test "shost->host_failed != scsi_host_busy(shost))" to always be true, keeping the EH task sleeping. Eventuially, when the deferred command also times out, the EH task is woken up and the timeout processing occurs. Avoid this unnecessary EH trigger wait time with the same method as implemented in libata-scsi, using the eh_timed_out scsi host template operation. The function sas_eh_timed_out() implements this operation and executes the helper function ata_scsi_port_eh_timed_out() if the device is a sata one. Co-developed-by: Igor Pylypiv <ipylypiv@google.com> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-scsi.c | 5 +++-- drivers/scsi/libsas/sas_scsi_host.c | 17 +++++++++++++++++ include/linux/libata.h | 2 ++ include/scsi/libsas.h | 2 ++ 4 files changed, 24 insertions(+), 2 deletions(-) diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 8328778ec046..43762afa58ac 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1738,8 +1738,8 @@ static void ata_scsi_schedule_deferred_qc(struct ata_link *link) queue_work(system_highpri_wq, &link->deferred_qc_work); } -static enum scsi_timeout_action -ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd) +enum scsi_timeout_action ata_scsi_port_eh_timed_out(struct ata_port *ap, + struct scsi_cmnd *scmd) { unsigned long flags; @@ -1761,6 +1761,7 @@ ata_scsi_port_eh_timed_out(struct ata_port *ap, struct scsi_cmnd *scmd) */ return SCSI_EH_NOT_HANDLED; } +EXPORT_SYMBOL_GPL(ata_scsi_port_eh_timed_out); enum scsi_timeout_action ata_scsi_eh_timed_out(struct scsi_cmnd *scmd) { diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index c83282733ec4..578fa8fd70f0 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -502,6 +502,23 @@ int sas_eh_target_reset_handler(struct scsi_cmnd *cmd) } EXPORT_SYMBOL_GPL(sas_eh_target_reset_handler); +/* + * Handle deferred QCs in case of a timeout. + * See ata_scsi_port_eh_timed_out() for details. + */ +enum scsi_timeout_action sas_eh_timed_out(struct scsi_cmnd *cmd) +{ + struct domain_device *dev = NULL; + + if (cmd) + dev = cmd_to_domain_dev(cmd); + if (!dev || !dev_is_sata(dev)) + return SCSI_EH_NOT_HANDLED; + + return ata_scsi_port_eh_timed_out(dev->sata_dev.ap, cmd); +} +EXPORT_SYMBOL_GPL(sas_eh_timed_out); + /* Try to reset a device */ static int try_to_reset_cmd_device(struct scsi_cmnd *cmd) { diff --git a/include/linux/libata.h b/include/linux/libata.h index 327da43d7496..2958a2fa3da0 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1153,6 +1153,8 @@ extern int ata_scsi_ioctl(struct scsi_device *dev, unsigned int cmd, #endif extern enum scsi_qc_status ata_scsi_queuecmd(struct Scsi_Host *h, struct scsi_cmnd *cmd); +enum scsi_timeout_action ata_scsi_port_eh_timed_out(struct ata_port *ap, + struct scsi_cmnd *cmd); enum scsi_timeout_action ata_scsi_eh_timed_out(struct scsi_cmnd *cmd); #if IS_REACHABLE(CONFIG_ATA) bool ata_scsi_dma_need_drain(struct request *rq); diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index 163f23c92b41..c7017ae76c61 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -705,6 +705,7 @@ void sas_task_abort(struct sas_task *); int sas_eh_abort_handler(struct scsi_cmnd *cmd); int sas_eh_device_reset_handler(struct scsi_cmnd *cmd); int sas_eh_target_reset_handler(struct scsi_cmnd *cmd); +enum scsi_timeout_action sas_eh_timed_out(struct scsi_cmnd *cmd); extern void sas_target_destroy(struct scsi_target *); extern int sas_sdev_init(struct scsi_device *); @@ -743,6 +744,7 @@ void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event, .this_id = -1, \ .eh_device_reset_handler = sas_eh_device_reset_handler, \ .eh_target_reset_handler = sas_eh_target_reset_handler, \ + .eh_timed_out = sas_eh_timed_out, \ .target_destroy = sas_target_destroy, \ .ioctl = sas_ioctl, \ -- 2.55.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/2] fixup handling of timeouts with deferred QCs 2026-07-09 8:39 [PATCH v1 0/2] fixup handling of timeouts with deferred QCs Damien Le Moal 2026-07-09 8:39 ` [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out Damien Le Moal 2026-07-09 8:39 ` [PATCH v1 2/2] scsi: libsas: " Damien Le Moal @ 2026-07-09 17:36 ` Igor Pylypiv 2 siblings, 0 replies; 10+ messages in thread From: Igor Pylypiv @ 2026-07-09 17:36 UTC (permalink / raw) To: Damien Le Moal Cc: linux-ide, Niklas Cassel, linux-scsi, Martin K . Petersen, John Garry, Jason Yan On Thu, Jul 09, 2026 at 05:39:32PM +0900, Damien Le Moal wrote: > This patch series fixes libata and libsas to correctly handles deferred > queued commands in case of a timeout error, to avoid excessive delays in > waking up the scsi EH task. > > Igor, > > Please retest ! Looks good! Thank you, Damien! Tested-by: Igor Pylypiv <ipylypiv@google.com> > > Martin, > > Once reviewed, I or you can take both patches ? > > Damien Le Moal (2): > ata: libata-scsi: terminate deferred commands on time out > scsi: libsas: terminate deferred commands on time out > > drivers/ata/libata-eh.c | 2 +- > drivers/ata/libata-scsi.c | 54 +++++++++++++++++++++++++---- > drivers/ata/libata.h | 3 +- > drivers/scsi/libsas/sas_scsi_host.c | 17 +++++++++ > include/linux/libata.h | 4 +++ > include/scsi/libsas.h | 2 ++ > 6 files changed, 73 insertions(+), 9 deletions(-) > > -- > 2.55.0 > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-07-10 0:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-09 8:39 [PATCH v1 0/2] fixup handling of timeouts with deferred QCs Damien Le Moal 2026-07-09 8:39 ` [PATCH v1 1/2] ata: libata-scsi: terminate deferred commands on time out Damien Le Moal 2026-07-09 9:00 ` sashiko-bot 2026-07-09 9:14 ` Damien Le Moal 2026-07-09 17:27 ` Niklas Cassel 2026-07-09 17:56 ` Niklas Cassel 2026-07-09 18:02 ` Niklas Cassel 2026-07-10 0:00 ` Damien Le Moal 2026-07-09 8:39 ` [PATCH v1 2/2] scsi: libsas: " Damien Le Moal 2026-07-09 17:36 ` [PATCH v1 0/2] fixup handling of timeouts with deferred QCs Igor Pylypiv
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox