Message ID | 1658489049-232850-1-git-send-email-john.garry@huawei.com |
---|---|
Headers | show |
Series | libsas and drivers: NCQ error handling | expand |
On 2022/07/22 4:24, John Garry wrote: > As reported in [0], the pm8001 driver NCQ error handling more or less > duplicates what libata does in link error handling, as follows: > - abort all commands > - do autopsy with read log ext 10 command > - reset the target to recover > > Indeed for the hisi_sas driver we want to add similar handling for NCQ > errors. > > This series add a new libsas API - sas_ata_link_abort() - to handle host > NCQ errors, and fixes up pm8001 and hisi_sas drivers to use it. As > mentioned in the pm8001 changeover patch, I would prefer a better place to > locate the SATA ABORT command (rather that nexus reset callback). > > I would appreciate some testing of the pm8001 change as the read log ext10 > command mostly hangs on my arm64 machine - these arm64 hangs are a known > issue. I applied this series on top of the current Linus tree and ran some tests: a bunch of fio runs and also ran libzbc test suites on a SATA SMR drive as that generates many command failures. No problems detected, the tests all pass. FYI, messages for failed commands look like this: pm80xx0:: mpi_sata_event 2685: SATA EVENT 0x23 sas: Enter sas_scsi_recover_host busy: 1 failed: 1 sas: sas_scsi_find_task: aborting task 0x00000000ba62a907 pm80xx0:: mpi_sata_completion 2292: task null, freeing CCB tag 2 sas: sas_scsi_find_task: task 0x00000000ba62a907 is aborted sas: sas_eh_handle_sas_errors: task 0x00000000ba62a907 is aborted ata21.00: exception Emask 0x0 SAct 0x20000000 SErr 0x0 action 0x0 ata21.00: failed command: WRITE FPDMA QUEUED ata21.00: cmd 61/02:00:ff:ff:ea/00:00:02:00:00/40 tag 29 ncq dma 8192 out res 43/04:02:ff:ff:ea/00:00:02:00:00/00 Emask 0x400 (NCQ error) <F> ata21.00: status: { DRDY SENSE ERR } ata21.00: error: { ABRT } ata21.00: configured for UDMA/133 ata21: EH complete sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 1 tries: 1 Seems all good to me. > > Finally with these changes we can make the libsas task alloc/free APIs > private, which they should always have been. > > Based on v5.19-rc6 > > [0] https://lore.kernel.org/linux-scsi/8fb3b093-55f0-1fab-81f4-e8519810a978@huawei.com/ > > John Garry (5): > scsi: pm8001: Modify task abort handling for SATA task > scsi: libsas: Add sas_ata_link_abort() > scsi: pm8001: Use sas_ata_link_abort() to handle NCQ errors > scsi: hisi_sas: Don't issue ATA softreset in hisi_sas_abort_task() > scsi: libsas: Make sas_{alloc, alloc_slow, free}_task() private > > Xingui Yang (1): > scsi: hisi_sas: Add SATA_DISK_ERR bit handling for v3 hw > > drivers/scsi/hisi_sas/hisi_sas_main.c | 5 +- > drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 22 ++- > drivers/scsi/libsas/sas_ata.c | 10 ++ > drivers/scsi/libsas/sas_init.c | 3 - > drivers/scsi/libsas/sas_internal.h | 4 + > drivers/scsi/pm8001/pm8001_hwi.c | 194 +++++++------------------ > drivers/scsi/pm8001/pm8001_sas.c | 13 ++ > drivers/scsi/pm8001/pm8001_sas.h | 8 +- > drivers/scsi/pm8001/pm80xx_hwi.c | 177 ++-------------------- > include/scsi/libsas.h | 4 - > include/scsi/sas_ata.h | 5 + > 11 files changed, 132 insertions(+), 313 deletions(-) >
On 2022/08/11 11:54, Damien Le Moal wrote: > On 2022/07/22 4:24, John Garry wrote: >> As reported in [0], the pm8001 driver NCQ error handling more or less >> duplicates what libata does in link error handling, as follows: >> - abort all commands >> - do autopsy with read log ext 10 command >> - reset the target to recover >> >> Indeed for the hisi_sas driver we want to add similar handling for NCQ >> errors. >> >> This series add a new libsas API - sas_ata_link_abort() - to handle host >> NCQ errors, and fixes up pm8001 and hisi_sas drivers to use it. As >> mentioned in the pm8001 changeover patch, I would prefer a better place to >> locate the SATA ABORT command (rather that nexus reset callback). >> >> I would appreciate some testing of the pm8001 change as the read log ext10 >> command mostly hangs on my arm64 machine - these arm64 hangs are a known >> issue. > > I applied this series on top of the current Linus tree and ran some tests: a > bunch of fio runs and also ran libzbc test suites on a SATA SMR drive as that > generates many command failures. No problems detected, the tests all pass. > FYI, messages for failed commands look like this: > > pm80xx0:: mpi_sata_event 2685: SATA EVENT 0x23 > sas: Enter sas_scsi_recover_host busy: 1 failed: 1 > sas: sas_scsi_find_task: aborting task 0x00000000ba62a907 > pm80xx0:: mpi_sata_completion 2292: task null, freeing CCB tag 2 > sas: sas_scsi_find_task: task 0x00000000ba62a907 is aborted > sas: sas_eh_handle_sas_errors: task 0x00000000ba62a907 is aborted > ata21.00: exception Emask 0x0 SAct 0x20000000 SErr 0x0 action 0x0 > ata21.00: failed command: WRITE FPDMA QUEUED > ata21.00: cmd 61/02:00:ff:ff:ea/00:00:02:00:00/40 tag 29 ncq dma 8192 out > res 43/04:02:ff:ff:ea/00:00:02:00:00/00 Emask 0x400 (NCQ error) <F> > ata21.00: status: { DRDY SENSE ERR } > ata21.00: error: { ABRT } > ata21.00: configured for UDMA/133 > ata21: EH complete > sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 1 tries: 1 > > Seems all good to me. Forgot to mention: tested with pm80xx driver. > >> >> Finally with these changes we can make the libsas task alloc/free APIs >> private, which they should always have been. >> >> Based on v5.19-rc6 >> >> [0] https://lore.kernel.org/linux-scsi/8fb3b093-55f0-1fab-81f4-e8519810a978@huawei.com/ >> >> John Garry (5): >> scsi: pm8001: Modify task abort handling for SATA task >> scsi: libsas: Add sas_ata_link_abort() >> scsi: pm8001: Use sas_ata_link_abort() to handle NCQ errors >> scsi: hisi_sas: Don't issue ATA softreset in hisi_sas_abort_task() >> scsi: libsas: Make sas_{alloc, alloc_slow, free}_task() private >> >> Xingui Yang (1): >> scsi: hisi_sas: Add SATA_DISK_ERR bit handling for v3 hw >> >> drivers/scsi/hisi_sas/hisi_sas_main.c | 5 +- >> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 22 ++- >> drivers/scsi/libsas/sas_ata.c | 10 ++ >> drivers/scsi/libsas/sas_init.c | 3 - >> drivers/scsi/libsas/sas_internal.h | 4 + >> drivers/scsi/pm8001/pm8001_hwi.c | 194 +++++++------------------ >> drivers/scsi/pm8001/pm8001_sas.c | 13 ++ >> drivers/scsi/pm8001/pm8001_sas.h | 8 +- >> drivers/scsi/pm8001/pm80xx_hwi.c | 177 ++-------------------- >> include/scsi/libsas.h | 4 - >> include/scsi/sas_ata.h | 5 + >> 11 files changed, 132 insertions(+), 313 deletions(-) >> > >
On Fri, Jul 22, 2022 at 1:30 PM John Garry <john.garry@huawei.com> wrote: > > As reported in [0], the pm8001 driver NCQ error handling more or less > duplicates what libata does in link error handling, as follows: > - abort all commands > - do autopsy with read log ext 10 command > - reset the target to recover > > Indeed for the hisi_sas driver we want to add similar handling for NCQ > errors. > > This series add a new libsas API - sas_ata_link_abort() - to handle host > NCQ errors, and fixes up pm8001 and hisi_sas drivers to use it. As > mentioned in the pm8001 changeover patch, I would prefer a better place to > locate the SATA ABORT command (rather that nexus reset callback). > > I would appreciate some testing of the pm8001 change as the read log ext10 > command mostly hangs on my arm64 machine - these arm64 hangs are a known > issue. > > Finally with these changes we can make the libsas task alloc/free APIs > private, which they should always have been. > > Based on v5.19-rc6 > > [0] https://lore.kernel.org/linux-scsi/8fb3b093-55f0-1fab-81f4-e8519810a978@huawei.com/ > > John Garry (5): > scsi: pm8001: Modify task abort handling for SATA task > scsi: libsas: Add sas_ata_link_abort() > scsi: pm8001: Use sas_ata_link_abort() to handle NCQ errors > scsi: hisi_sas: Don't issue ATA softreset in hisi_sas_abort_task() > scsi: libsas: Make sas_{alloc, alloc_slow, free}_task() private > > Xingui Yang (1): > scsi: hisi_sas: Add SATA_DISK_ERR bit handling for v3 hw > > drivers/scsi/hisi_sas/hisi_sas_main.c | 5 +- > drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 22 ++- > drivers/scsi/libsas/sas_ata.c | 10 ++ > drivers/scsi/libsas/sas_init.c | 3 - > drivers/scsi/libsas/sas_internal.h | 4 + > drivers/scsi/pm8001/pm8001_hwi.c | 194 +++++++------------------ > drivers/scsi/pm8001/pm8001_sas.c | 13 ++ > drivers/scsi/pm8001/pm8001_sas.h | 8 +- > drivers/scsi/pm8001/pm80xx_hwi.c | 177 ++-------------------- > include/scsi/libsas.h | 4 - > include/scsi/sas_ata.h | 5 + > 11 files changed, 132 insertions(+), 313 deletions(-) Thank! John and Damien, for pm80xx. Acked-by: Jack Wang <jinpu.wang@ionos.com> > -- > 2.35.3 >
On 11/08/2022 19:54, Damien Le Moal wrote: > On 2022/07/22 4:24, John Garry wrote: >> As reported in [0], the pm8001 driver NCQ error handling more or less >> duplicates what libata does in link error handling, as follows: >> - abort all commands >> - do autopsy with read log ext 10 command >> - reset the target to recover >> >> Indeed for the hisi_sas driver we want to add similar handling for NCQ >> errors. >> >> This series add a new libsas API - sas_ata_link_abort() - to handle host >> NCQ errors, and fixes up pm8001 and hisi_sas drivers to use it. As >> mentioned in the pm8001 changeover patch, I would prefer a better place to >> locate the SATA ABORT command (rather that nexus reset callback). >> >> I would appreciate some testing of the pm8001 change as the read log ext10 >> command mostly hangs on my arm64 machine - these arm64 hangs are a known >> issue. > Thanks for this! > I applied this series on top of the current Linus tree and ran some tests: a > bunch of fio runs and also ran libzbc test suites on a SATA SMR drive as that > generates many command failures. No problems detected, the tests all pass. > FYI, messages for failed commands look like this: > > pm80xx0:: mpi_sata_event 2685: SATA EVENT 0x23 > sas: Enter sas_scsi_recover_host busy: 1 failed: 1 > sas: sas_scsi_find_task: aborting task 0x00000000ba62a907 > pm80xx0:: mpi_sata_completion 2292: task null, freeing CCB tag 2 > sas: sas_scsi_find_task: task 0x00000000ba62a907 is aborted > sas: sas_eh_handle_sas_errors: task 0x00000000ba62a907 is aborted > ata21.00: exception Emask 0x0 SAct 0x20000000 SErr 0x0 action 0x0 > ata21.00: failed command: WRITE FPDMA QUEUED > ata21.00: cmd 61/02:00:ff:ff:ea/00:00:02:00:00/40 tag 29 ncq dma 8192 out > res 43/04:02:ff:ff:ea/00:00:02:00:00/00 Emask 0x400 (NCQ error) <F> > ata21.00: status: { DRDY SENSE ERR } > ata21.00: error: { ABRT } > ata21.00: configured for UDMA/133 > ata21: EH complete > sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 1 tries: 1 > For this specific test we don't seem to run a hardreset after the autopsy, but we do seem to be getting an NCQ error. That's interesting. We have noticed this scenario for hisi_sas NCQ error, whereby the autopsy decided a reset is not required or useful, such as a medium error. Anyway the pm8001 driver relies on the reset being run always for the NCQ error. So I am thinking of tweaking sas_ata_link_abort() as follows: void sas_ata_link_abort(struct domain_device *device) { struct ata_port *ap = device->sata_dev.ap; struct ata_link *link = &ap->link; link->eh_info.err_mask |= AC_ERR_DEV; + link->eh_info.action |= ATA_EH_RESET; ata_link_abort(link); } This should force a reset. Thanks, John > Seems all good to me. > >> >> Finally with these changes we can make the libsas task alloc/free APIs >> private, which they should always have been. >> >> Based on v5.19-rc6 >> >> [0] https://lore.kernel.org/linux-scsi/8fb3b093-55f0-1fab-81f4-e8519810a978@huawei.com/ >> >> John Garry (5): >> scsi: pm8001: Modify task abort handling for SATA task >> scsi: libsas: Add sas_ata_link_abort() >> scsi: pm8001: Use sas_ata_link_abort() to handle NCQ errors >> scsi: hisi_sas: Don't issue ATA softreset in hisi_sas_abort_task() >> scsi: libsas: Make sas_{alloc, alloc_slow, free}_task() private >> >> Xingui Yang (1): >> scsi: hisi_sas: Add SATA_DISK_ERR bit handling for v3 hw >> >> drivers/scsi/hisi_sas/hisi_sas_main.c | 5 +- >> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 22 ++- >> drivers/scsi/libsas/sas_ata.c | 10 ++ >> drivers/scsi/libsas/sas_init.c | 3 - >> drivers/scsi/libsas/sas_internal.h | 4 + >> drivers/scsi/pm8001/pm8001_hwi.c | 194 +++++++------------------ >> drivers/scsi/pm8001/pm8001_sas.c | 13 ++ >> drivers/scsi/pm8001/pm8001_sas.h | 8 +- >> drivers/scsi/pm8001/pm80xx_hwi.c | 177 ++-------------------- >> include/scsi/libsas.h | 4 - >> include/scsi/sas_ata.h | 5 + >> 11 files changed, 132 insertions(+), 313 deletions(-) >> > >
On 2022/08/12 1:06, John Garry wrote: > On 11/08/2022 19:54, Damien Le Moal wrote: >> On 2022/07/22 4:24, John Garry wrote: >>> As reported in [0], the pm8001 driver NCQ error handling more or less >>> duplicates what libata does in link error handling, as follows: >>> - abort all commands >>> - do autopsy with read log ext 10 command >>> - reset the target to recover >>> >>> Indeed for the hisi_sas driver we want to add similar handling for NCQ >>> errors. >>> >>> This series add a new libsas API - sas_ata_link_abort() - to handle host >>> NCQ errors, and fixes up pm8001 and hisi_sas drivers to use it. As >>> mentioned in the pm8001 changeover patch, I would prefer a better place to >>> locate the SATA ABORT command (rather that nexus reset callback). >>> >>> I would appreciate some testing of the pm8001 change as the read log ext10 >>> command mostly hangs on my arm64 machine - these arm64 hangs are a known >>> issue. >> > > Thanks for this! > >> I applied this series on top of the current Linus tree and ran some tests: a >> bunch of fio runs and also ran libzbc test suites on a SATA SMR drive as that >> generates many command failures. No problems detected, the tests all pass. >> FYI, messages for failed commands look like this: >> >> pm80xx0:: mpi_sata_event 2685: SATA EVENT 0x23 >> sas: Enter sas_scsi_recover_host busy: 1 failed: 1 >> sas: sas_scsi_find_task: aborting task 0x00000000ba62a907 >> pm80xx0:: mpi_sata_completion 2292: task null, freeing CCB tag 2 >> sas: sas_scsi_find_task: task 0x00000000ba62a907 is aborted >> sas: sas_eh_handle_sas_errors: task 0x00000000ba62a907 is aborted >> ata21.00: exception Emask 0x0 SAct 0x20000000 SErr 0x0 action 0x0 >> ata21.00: failed command: WRITE FPDMA QUEUED >> ata21.00: cmd 61/02:00:ff:ff:ea/00:00:02:00:00/40 tag 29 ncq dma 8192 out >> res 43/04:02:ff:ff:ea/00:00:02:00:00/00 Emask 0x400 (NCQ error) <F> >> ata21.00: status: { DRDY SENSE ERR } >> ata21.00: error: { ABRT } >> ata21.00: configured for UDMA/133 >> ata21: EH complete >> sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 1 tries: 1 >> > > For this specific test we don't seem to run a hardreset after the > autopsy, but we do seem to be getting an NCQ error. That's interesting. > > We have noticed this scenario for hisi_sas NCQ error, whereby the > autopsy decided a reset is not required or useful, such as a medium > error. Anyway the pm8001 driver relies on the reset being run always for > the NCQ error. So I am thinking of tweaking sas_ata_link_abort() as follows: > > void sas_ata_link_abort(struct domain_device *device) > { > struct ata_port *ap = device->sata_dev.ap; > struct ata_link *link = &ap->link; > > link->eh_info.err_mask |= AC_ERR_DEV; > + link->eh_info.action |= ATA_EH_RESET; > ata_link_abort(link); > } > > This should force a reset. This is an unaligned write to a sequential write required zone on SMR. So definitely not worth a reset. Forcing hard resetting the link for such error is an overkill. I think it is better to let ata_link_abort() -> ... -> scsi & ata EH decide on the disposition. Note that patch 3 did not apply cleanly to the current Linus tree. So a rebase for the series is needed. > > Thanks, > John > >> Seems all good to me. >> >>> >>> Finally with these changes we can make the libsas task alloc/free APIs >>> private, which they should always have been. >>> >>> Based on v5.19-rc6 >>> >>> [0] https://lore.kernel.org/linux-scsi/8fb3b093-55f0-1fab-81f4-e8519810a978@huawei.com/ >>> >>> John Garry (5): >>> scsi: pm8001: Modify task abort handling for SATA task >>> scsi: libsas: Add sas_ata_link_abort() >>> scsi: pm8001: Use sas_ata_link_abort() to handle NCQ errors >>> scsi: hisi_sas: Don't issue ATA softreset in hisi_sas_abort_task() >>> scsi: libsas: Make sas_{alloc, alloc_slow, free}_task() private >>> >>> Xingui Yang (1): >>> scsi: hisi_sas: Add SATA_DISK_ERR bit handling for v3 hw >>> >>> drivers/scsi/hisi_sas/hisi_sas_main.c | 5 +- >>> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 22 ++- >>> drivers/scsi/libsas/sas_ata.c | 10 ++ >>> drivers/scsi/libsas/sas_init.c | 3 - >>> drivers/scsi/libsas/sas_internal.h | 4 + >>> drivers/scsi/pm8001/pm8001_hwi.c | 194 +++++++------------------ >>> drivers/scsi/pm8001/pm8001_sas.c | 13 ++ >>> drivers/scsi/pm8001/pm8001_sas.h | 8 +- >>> drivers/scsi/pm8001/pm80xx_hwi.c | 177 ++-------------------- >>> include/scsi/libsas.h | 4 - >>> include/scsi/sas_ata.h | 5 + >>> 11 files changed, 132 insertions(+), 313 deletions(-) >>> >> >> >
On 12/08/2022 16:39, Damien Le Moal wrote: >> For this specific test we don't seem to run a hardreset after the >> autopsy, but we do seem to be getting an NCQ error. That's interesting. >> >> We have noticed this scenario for hisi_sas NCQ error, whereby the >> autopsy decided a reset is not required or useful, such as a medium >> error. Anyway the pm8001 driver relies on the reset being run always for >> the NCQ error. So I am thinking of tweaking sas_ata_link_abort() as follows: >> >> void sas_ata_link_abort(struct domain_device *device) >> { >> struct ata_port *ap = device->sata_dev.ap; >> struct ata_link *link = &ap->link; >> >> link->eh_info.err_mask |= AC_ERR_DEV; >> + link->eh_info.action |= ATA_EH_RESET; >> ata_link_abort(link); >> } >> >> This should force a reset. > This is an unaligned write to a sequential write required zone on SMR. So > definitely not worth a reset. Forcing hard resetting the link for such error is > an overkill. I think it is better to let ata_link_abort() -> ... -> scsi & ata > EH decide on the disposition. Do you know if this triggered the pm8001 IO_XFER_ERROR_ABORTED_NCQ_MODE error? If I do not set ATA_EH_RESET then I need to trust that libata will always decide to do the reset for pm8001 IO_XFER_ERROR_ABORTED_NCQ_MODE error. That is because it is in the reset that I send the pm8001 "abort all" command - I could not find a better place for it. > > Note that patch 3 did not apply cleanly to the current Linus tree. So a rebase > for the series is needed. > That might be just git am, which always seems temperamental. The patches still apply from cherry-pick'ing for me. Anyway, I'll send a new version next week. Thanks, John
On 2022/08/12 9:33, John Garry wrote: > On 12/08/2022 16:39, Damien Le Moal wrote: >>> For this specific test we don't seem to run a hardreset after the >>> autopsy, but we do seem to be getting an NCQ error. That's interesting. >>> >>> We have noticed this scenario for hisi_sas NCQ error, whereby the >>> autopsy decided a reset is not required or useful, such as a medium >>> error. Anyway the pm8001 driver relies on the reset being run always for >>> the NCQ error. So I am thinking of tweaking sas_ata_link_abort() as follows: >>> >>> void sas_ata_link_abort(struct domain_device *device) >>> { >>> struct ata_port *ap = device->sata_dev.ap; >>> struct ata_link *link = &ap->link; >>> >>> link->eh_info.err_mask |= AC_ERR_DEV; >>> + link->eh_info.action |= ATA_EH_RESET; >>> ata_link_abort(link); >>> } >>> >>> This should force a reset. >> This is an unaligned write to a sequential write required zone on SMR. So >> definitely not worth a reset. Forcing hard resetting the link for such error is >> an overkill. I think it is better to let ata_link_abort() -> ... -> scsi & ata >> EH decide on the disposition. > > Do you know if this triggered the pm8001 IO_XFER_ERROR_ABORTED_NCQ_MODE > error? > > If I do not set ATA_EH_RESET then I need to trust that libata will > always decide to do the reset for pm8001 IO_XFER_ERROR_ABORTED_NCQ_MODE > error. That is because it is in the reset that I send the pm8001 "abort > all" command - I could not find a better place for it. Not sure what error it was. Will need to add a print of it to check. Easy to do. > >> >> Note that patch 3 did not apply cleanly to the current Linus tree. So a rebase >> for the series is needed. >> > > That might be just git am, which always seems temperamental. The patches > still apply from cherry-pick'ing for me. Anyway, I'll send a new version > next week. Yes, it was a "bad ancestor" thing. Direct patching worked just fine. > > Thanks, > John >