| Message ID | 20221114172200.1475962-1-niklas.cassel@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ata: libata: fix commands incorrectly not getting retried during NCQ error | expand |
On 11/15/22 02:21, Niklas Cassel wrote: > A NCQ error means that the device has aborted processing of all active > commands. > To get the single NCQ command that caused the NCQ error, host software has > to read the NCQ error log, which also takes the device out of error state. > > When the device encounters a NCQ error, we receive an error interrupt from > the HBA, and call ata_do_link_abort() to mark all outstanding commands on > the link as ATA_QCFLAG_FAILED (which means that these commands are owned > by libata EH), and then call ata_qc_complete() on them. > > ata_qc_complete() will call fill_result_tf() for all commands marked as > ATA_QCFLAG_FAILED. > > The taskfile is simply the latest status/error as seen from the device's > perspective. The taskfile will have ATA_ERR set in the status field and > ATA_ABORTED set in the error field. > > When we fill the current taskfile values for all outstanding commands, > that means that qc->result_tf will have ATA_ERR set for all commands > owned by libata EH. > > When ata_eh_link_autopsy() later analyzes all commands owned by libata EH, > it will call ata_eh_analyze_tf(), which will check if qc->result_tf has > ATA_ERR set, if it does, it will set qc->err_mask (which marks the command > as an error). > > When ata_eh_finish() later calls __ata_qc_complete() on all commands owned > by libata EH, it will call qc->complete_fn() (ata_scsi_qc_complete()), > ata_scsi_qc_complete() will call ata_gen_ata_sense() to generate sense > data if qc->err_mask is set. > > This means that we will generate sense data for commands that should not > have any sense data set. Having sense data set for the non-failed commands > will cause SCSI to finish these commands instead of retrying them. > > While this incorrect behavior has existed for a long time, this first > became a problem once we started reading the correct taskfile register in > commit 4ba09d202657 ("ata: libahci: read correct status and error field > for NCQ commands"). > > Before this commit, NCQ commands would read the taskfile values received > from the last non-NCQ command completion, which most likely did not have > ATA_ERR set, since the last non-NCQ command was most likely not an error. > > Fix this by changing ata_eh_analyze_ncq_error() to mark all non-failed > commands as ATA_QCFLAG_RETRY, and change the loop in ata_eh_link_autopsy() > to skip commands marked as ATA_QCFLAG_RETRY. > > While at it, make sure that we clear ATA_ERR and any error bits for all > commands except the actual command that caused the NCQ error, so that no > other libata code will be able to misinterpret these commands as errors. > > Fixes: 4ba09d202657 ("ata: libahci: read correct status and error field for NCQ commands") > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> Applied to for-6.1-fixes. Thanks ! > --- > Changes since v1: > -Squashed patch 1/2 with 2/2. > > drivers/ata/libata-eh.c | 1 + > drivers/ata/libata-sata.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index bde15f855f70..34303ce67c14 100644 > --- a/drivers/ata/libata-eh.c > +++ b/drivers/ata/libata-eh.c > @@ -1955,6 +1955,7 @@ static void ata_eh_link_autopsy(struct ata_link *link) > > ata_qc_for_each_raw(ap, qc, tag) { > if (!(qc->flags & ATA_QCFLAG_FAILED) || > + qc->flags & ATA_QCFLAG_RETRY || > ata_dev_phys_link(qc->dev) != link) > continue; > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 283ce1ab29cf..18ef14e749a0 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -1476,6 +1476,33 @@ void ata_eh_analyze_ncq_error(struct ata_link *link) > } > } > > + ata_qc_for_each_raw(ap, qc, tag) { > + if (!(qc->flags & ATA_QCFLAG_FAILED) || > + ata_dev_phys_link(qc->dev) != link) > + continue; > + > + /* Skip the single QC which caused the NCQ error. */ > + if (qc->err_mask) > + continue; > + > + /* > + * For SATA, the STATUS and ERROR fields are shared for all NCQ > + * commands that were completed with the same SDB FIS. > + * Therefore, we have to clear the ATA_ERR bit for all QCs > + * except the one that caused the NCQ error. > + */ > + qc->result_tf.status &= ~ATA_ERR; > + qc->result_tf.error = 0; > + > + /* > + * If we get a NCQ error, that means that a single command was > + * aborted. All other failed commands for our link should be > + * retried and has no business of going though further scrutiny > + * by ata_eh_link_autopsy(). > + */ > + qc->flags |= ATA_QCFLAG_RETRY; > + } > + > ehc->i.err_mask &= ~AC_ERR_DEV; > } > EXPORT_SYMBOL_GPL(ata_eh_analyze_ncq_error);
On Fri, Nov 18, 2022 at 01:40:17PM +0900, Damien Le Moal wrote: > On 11/15/22 02:21, Niklas Cassel wrote: > > A NCQ error means that the device has aborted processing of all active > > commands. > > To get the single NCQ command that caused the NCQ error, host software has > > to read the NCQ error log, which also takes the device out of error state. > > > > When the device encounters a NCQ error, we receive an error interrupt from > > the HBA, and call ata_do_link_abort() to mark all outstanding commands on > > the link as ATA_QCFLAG_FAILED (which means that these commands are owned > > by libata EH), and then call ata_qc_complete() on them. > > > > ata_qc_complete() will call fill_result_tf() for all commands marked as > > ATA_QCFLAG_FAILED. > > > > The taskfile is simply the latest status/error as seen from the device's > > perspective. The taskfile will have ATA_ERR set in the status field and > > ATA_ABORTED set in the error field. > > > > When we fill the current taskfile values for all outstanding commands, > > that means that qc->result_tf will have ATA_ERR set for all commands > > owned by libata EH. > > > > When ata_eh_link_autopsy() later analyzes all commands owned by libata EH, > > it will call ata_eh_analyze_tf(), which will check if qc->result_tf has > > ATA_ERR set, if it does, it will set qc->err_mask (which marks the command > > as an error). > > > > When ata_eh_finish() later calls __ata_qc_complete() on all commands owned > > by libata EH, it will call qc->complete_fn() (ata_scsi_qc_complete()), > > ata_scsi_qc_complete() will call ata_gen_ata_sense() to generate sense > > data if qc->err_mask is set. > > > > This means that we will generate sense data for commands that should not > > have any sense data set. Having sense data set for the non-failed commands > > will cause SCSI to finish these commands instead of retrying them. > > > > While this incorrect behavior has existed for a long time, this first > > became a problem once we started reading the correct taskfile register in > > commit 4ba09d202657 ("ata: libahci: read correct status and error field > > for NCQ commands"). > > > > Before this commit, NCQ commands would read the taskfile values received > > from the last non-NCQ command completion, which most likely did not have > > ATA_ERR set, since the last non-NCQ command was most likely not an error. > > > > Fix this by changing ata_eh_analyze_ncq_error() to mark all non-failed > > commands as ATA_QCFLAG_RETRY, and change the loop in ata_eh_link_autopsy() > > to skip commands marked as ATA_QCFLAG_RETRY. > > > > While at it, make sure that we clear ATA_ERR and any error bits for all > > commands except the actual command that caused the NCQ error, so that no > > other libata code will be able to misinterpret these commands as errors. > > > > Fixes: 4ba09d202657 ("ata: libahci: read correct status and error field for NCQ commands") > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > > Applied to for-6.1-fixes. Thanks ! So, the Fixes-tag points to a commit that is only in your for-next branch. If you this patch to 6.1-fixes, then the Fixes-tag points to a commit that does not (yet) exist in the tree. If you prefer to this patch to 6.1-fixes, then we should probably change the Fixes-tag to point to: e8ee84518c15 ("[PATCH] libata-ncq: update EH to handle NCQ") While the problem could happen even on v6.1-rc5, it is highly unlikely, as v6.1-rc5 is reading the wrong status register for NCQ commands, which means that during an NCQ error, analyze_tf() will read the status from the last non-NCQ command, which is most likely does not have ATA_ERR set in status. I think the only way it is a problem on v6.1-rc5 is if: 1) A non-NCQ command fails 2a) No D2H FIS (non-NCQ command) is received with ATA_ERR bit cleared, before 3) happens 2b) The device is not reset, before 3) happens 3) A NCQ error occurs Perhaps just queue this up for 6.2 instead? Kind regards, Niklas
On 11/18/22 18:41, Niklas Cassel wrote: > On Fri, Nov 18, 2022 at 01:40:17PM +0900, Damien Le Moal wrote: >> On 11/15/22 02:21, Niklas Cassel wrote: >>> A NCQ error means that the device has aborted processing of all active >>> commands. >>> To get the single NCQ command that caused the NCQ error, host software has >>> to read the NCQ error log, which also takes the device out of error state. >>> >>> When the device encounters a NCQ error, we receive an error interrupt from >>> the HBA, and call ata_do_link_abort() to mark all outstanding commands on >>> the link as ATA_QCFLAG_FAILED (which means that these commands are owned >>> by libata EH), and then call ata_qc_complete() on them. >>> >>> ata_qc_complete() will call fill_result_tf() for all commands marked as >>> ATA_QCFLAG_FAILED. >>> >>> The taskfile is simply the latest status/error as seen from the device's >>> perspective. The taskfile will have ATA_ERR set in the status field and >>> ATA_ABORTED set in the error field. >>> >>> When we fill the current taskfile values for all outstanding commands, >>> that means that qc->result_tf will have ATA_ERR set for all commands >>> owned by libata EH. >>> >>> When ata_eh_link_autopsy() later analyzes all commands owned by libata EH, >>> it will call ata_eh_analyze_tf(), which will check if qc->result_tf has >>> ATA_ERR set, if it does, it will set qc->err_mask (which marks the command >>> as an error). >>> >>> When ata_eh_finish() later calls __ata_qc_complete() on all commands owned >>> by libata EH, it will call qc->complete_fn() (ata_scsi_qc_complete()), >>> ata_scsi_qc_complete() will call ata_gen_ata_sense() to generate sense >>> data if qc->err_mask is set. >>> >>> This means that we will generate sense data for commands that should not >>> have any sense data set. Having sense data set for the non-failed commands >>> will cause SCSI to finish these commands instead of retrying them. >>> >>> While this incorrect behavior has existed for a long time, this first >>> became a problem once we started reading the correct taskfile register in >>> commit 4ba09d202657 ("ata: libahci: read correct status and error field >>> for NCQ commands"). >>> >>> Before this commit, NCQ commands would read the taskfile values received >>> from the last non-NCQ command completion, which most likely did not have >>> ATA_ERR set, since the last non-NCQ command was most likely not an error. >>> >>> Fix this by changing ata_eh_analyze_ncq_error() to mark all non-failed >>> commands as ATA_QCFLAG_RETRY, and change the loop in ata_eh_link_autopsy() >>> to skip commands marked as ATA_QCFLAG_RETRY. >>> >>> While at it, make sure that we clear ATA_ERR and any error bits for all >>> commands except the actual command that caused the NCQ error, so that no >>> other libata code will be able to misinterpret these commands as errors. >>> >>> Fixes: 4ba09d202657 ("ata: libahci: read correct status and error field for NCQ commands") >>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> >> >> Applied to for-6.1-fixes. Thanks ! > > So, the Fixes-tag points to a commit that is only in your for-next branch. Arg. I got confused with last week fix... > > If you this patch to 6.1-fixes, then the Fixes-tag points to a commit that > does not (yet) exist in the tree. > > If you prefer to this patch to 6.1-fixes, then we should probably change > the Fixes-tag to point to: > e8ee84518c15 ("[PATCH] libata-ncq: update EH to handle NCQ") > > While the problem could happen even on v6.1-rc5, it is highly unlikely, > as v6.1-rc5 is reading the wrong status register for NCQ commands, > which means that during an NCQ error, analyze_tf() will read the status > from the last non-NCQ command, which is most likely does not have ATA_ERR > set in status. > > I think the only way it is a problem on v6.1-rc5 is if: > 1) A non-NCQ command fails > 2a) No D2H FIS (non-NCQ command) is received with ATA_ERR bit cleared, > before 3) happens > 2b) The device is not reset, before 3) happens > 3) A NCQ error occurs > > Perhaps just queue this up for 6.2 instead? Indeed. It needs to be there. Moving it. Thanks. > > > Kind regards, > Niklas
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index bde15f855f70..34303ce67c14 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -1955,6 +1955,7 @@ static void ata_eh_link_autopsy(struct ata_link *link) ata_qc_for_each_raw(ap, qc, tag) { if (!(qc->flags & ATA_QCFLAG_FAILED) || + qc->flags & ATA_QCFLAG_RETRY || ata_dev_phys_link(qc->dev) != link) continue; diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 283ce1ab29cf..18ef14e749a0 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1476,6 +1476,33 @@ void ata_eh_analyze_ncq_error(struct ata_link *link) } } + ata_qc_for_each_raw(ap, qc, tag) { + if (!(qc->flags & ATA_QCFLAG_FAILED) || + ata_dev_phys_link(qc->dev) != link) + continue; + + /* Skip the single QC which caused the NCQ error. */ + if (qc->err_mask) + continue; + + /* + * For SATA, the STATUS and ERROR fields are shared for all NCQ + * commands that were completed with the same SDB FIS. + * Therefore, we have to clear the ATA_ERR bit for all QCs + * except the one that caused the NCQ error. + */ + qc->result_tf.status &= ~ATA_ERR; + qc->result_tf.error = 0; + + /* + * If we get a NCQ error, that means that a single command was + * aborted. All other failed commands for our link should be + * retried and has no business of going though further scrutiny + * by ata_eh_link_autopsy(). + */ + qc->flags |= ATA_QCFLAG_RETRY; + } + ehc->i.err_mask &= ~AC_ERR_DEV; } EXPORT_SYMBOL_GPL(ata_eh_analyze_ncq_error);
A NCQ error means that the device has aborted processing of all active commands. To get the single NCQ command that caused the NCQ error, host software has to read the NCQ error log, which also takes the device out of error state. When the device encounters a NCQ error, we receive an error interrupt from the HBA, and call ata_do_link_abort() to mark all outstanding commands on the link as ATA_QCFLAG_FAILED (which means that these commands are owned by libata EH), and then call ata_qc_complete() on them. ata_qc_complete() will call fill_result_tf() for all commands marked as ATA_QCFLAG_FAILED. The taskfile is simply the latest status/error as seen from the device's perspective. The taskfile will have ATA_ERR set in the status field and ATA_ABORTED set in the error field. When we fill the current taskfile values for all outstanding commands, that means that qc->result_tf will have ATA_ERR set for all commands owned by libata EH. When ata_eh_link_autopsy() later analyzes all commands owned by libata EH, it will call ata_eh_analyze_tf(), which will check if qc->result_tf has ATA_ERR set, if it does, it will set qc->err_mask (which marks the command as an error). When ata_eh_finish() later calls __ata_qc_complete() on all commands owned by libata EH, it will call qc->complete_fn() (ata_scsi_qc_complete()), ata_scsi_qc_complete() will call ata_gen_ata_sense() to generate sense data if qc->err_mask is set. This means that we will generate sense data for commands that should not have any sense data set. Having sense data set for the non-failed commands will cause SCSI to finish these commands instead of retrying them. While this incorrect behavior has existed for a long time, this first became a problem once we started reading the correct taskfile register in commit 4ba09d202657 ("ata: libahci: read correct status and error field for NCQ commands"). Before this commit, NCQ commands would read the taskfile values received from the last non-NCQ command completion, which most likely did not have ATA_ERR set, since the last non-NCQ command was most likely not an error. Fix this by changing ata_eh_analyze_ncq_error() to mark all non-failed commands as ATA_QCFLAG_RETRY, and change the loop in ata_eh_link_autopsy() to skip commands marked as ATA_QCFLAG_RETRY. While at it, make sure that we clear ATA_ERR and any error bits for all commands except the actual command that caused the NCQ error, so that no other libata code will be able to misinterpret these commands as errors. Fixes: 4ba09d202657 ("ata: libahci: read correct status and error field for NCQ commands") Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> --- Changes since v1: -Squashed patch 1/2 with 2/2. drivers/ata/libata-eh.c | 1 + drivers/ata/libata-sata.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+)