Date: Thu, 5 Mar 2015 18:50:30 -0700 From: Warner Losh <imp@bsdimp.com> To: Steven Hartland <steven@multiplay.co.uk> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org, Warner Losh <imp@FreeBSD.org> Subject: Re: svn commit: r279678 - in projects/iosched/sys: cam cam/ata dev/ahci Message-ID: <5637B469-F10D-4D7A-8036-DE417368F71E@bsdimp.com> In-Reply-To: <54F8F9F7.8080605@freebsd.org> References: <201503060023.t260NOA8076981@svn.freebsd.org> <54F8F9F7.8080605@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail=_BCDC3C1D-6567-4014-BC2A-916B7D97BBFE Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Pure laziness on my part. I should be retrying a failed NCQ DSM TRIM as = a DSM TRIM. This is one reason why I haven=E2=80=99t pushed these = changes into -head. That level of detail doesn=E2=80=99t matter for the = I/O Scheduler which this branch exists for, but bits of that code depend = on these changes, alas. Warner > On Mar 5, 2015, at 5:51 PM, Steven Hartland <steven@multiplay.co.uk> = wrote: >=20 > Any reason why you don't retry the failed NCQ TRIM as a none NCQ TRIM = instead of returning success then it failed? >=20 > Regards > Steve >=20 > On 06/03/2015 00:23, Warner Losh wrote: >> Author: imp >> Date: Fri Mar 6 00:23:23 2015 >> New Revision: 279678 >> URL: https://svnweb.freebsd.org/changeset/base/279678 >>=20 >> Log: >> Implement "NCQ TRIM" by using Send First Party DMA commands with a >> Dataset Management subcommand and setting the TRIM bit. For drives >> that support this method of TRIM, better latency is expected = because >> TRIMs no longer act as a gating event forcing the queue to drain >> before they are executed and holding off new commands while the = TRIM >> finishes. We still impose a one-at-a-time limit of the TRIMs, = though >> that could be relaxed (though you start running into fairness = issues >> if you have a lot of TRIMs competing with other I/Os), so that >> optimization is left for the future. >> Add a flag for supporting the NCQ DSM TRIM extention to the = ata_cmd >> structure. >> Add a flag to adversize that the SIM knows how to do said = kludge. >> When NCQ DSM Trim fails, fall back to normal DSM Trim. However, = treat >> the original trim as advisory and don't reissue it. The stack is = too >> fragile at this point in the code to risk it. >>=20 >> Modified: >> projects/iosched/sys/cam/ata/ata_all.h >> projects/iosched/sys/cam/ata/ata_da.c >> projects/iosched/sys/cam/cam_ccb.h >> projects/iosched/sys/dev/ahci/ahci.c >>=20 >> Modified: projects/iosched/sys/cam/ata/ata_all.h >> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >> --- projects/iosched/sys/cam/ata/ata_all.h Thu Mar 5 21:41:58 2015 = (r279677) >> +++ projects/iosched/sys/cam/ata/ata_all.h Fri Mar 6 00:23:23 2015 = (r279678) >> @@ -46,6 +46,7 @@ struct ata_cmd { >> #define CAM_ATAIO_CONTROL 0x04 /* Control, not = a command */ >> #define CAM_ATAIO_NEEDRESULT 0x08 /* Request = requires result. */ >> #define CAM_ATAIO_DMA 0x10 /* DMA command = */ >> +#define CAM_ATAIO_AUX_HACK 0x20 /* Kludge to = make FPDMA DSM TRIM work */ >> u_int8_t command; >> u_int8_t features; >>=20 >> Modified: projects/iosched/sys/cam/ata/ata_da.c >> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >> --- projects/iosched/sys/cam/ata/ata_da.c Thu Mar 5 21:41:58 2015 = (r279677) >> +++ projects/iosched/sys/cam/ata/ata_da.c Fri Mar 6 00:23:23 2015 = (r279678) >> @@ -87,7 +87,8 @@ typedef enum { >> ADA_FLAG_CAN_CFA =3D 0x0400, >> ADA_FLAG_CAN_POWERMGT =3D 0x0800, >> ADA_FLAG_CAN_DMA48 =3D 0x1000, >> - ADA_FLAG_DIRTY =3D 0x2000 >> + ADA_FLAG_DIRTY =3D 0x2000, >> + ADA_FLAG_CAN_NCQ_TRIM =3D 0x4000 /* CAN_TRIM also set */ >> } ada_flags; >> typedef enum { >> @@ -1018,10 +1019,22 @@ adaasync(void *callback_arg, u_int32_t c >> else >> softc->flags &=3D ~ADA_FLAG_CAN_NCQ; >> if ((cgd.ident_data.support_dsm & ATA_SUPPORT_DSM_TRIM) = && >> - (cgd.inq_flags & SID_DMA)) >> + (cgd.inq_flags & SID_DMA)) { >> softc->flags |=3D ADA_FLAG_CAN_TRIM; >> - else >> - softc->flags &=3D ~ADA_FLAG_CAN_TRIM; >> + /* >> + * If we can do RCVSND_FPDMA_QUEUED commands, we = may be able to do >> + * NCQ trims, if we support trims at all. We = also need support from >> + * the sim do do things properly. Perhaps we = should look at log 13 >> + * dword 0 bit 0 and dword 1 bit 0 are set = too... >> + */ >> + if (/* (cpi.hba_misc & PIM_NCQ_KLUDGE) !=3D 0 && = */ /* Don't know how to do this here */ >> + (cgd.ident_data.satacapabilities2 & = ATA_SUPPORT_RCVSND_FPDMA_QUEUED) !=3D 0 && >> + (softc->flags & ADA_FLAG_CAN_TRIM) !=3D 0) >> + softc->flags |=3D ADA_FLAG_CAN_NCQ_TRIM; >> + else >> + softc->flags &=3D = ~ADA_FLAG_CAN_NCQ_TRIM; >> + } else >> + softc->flags &=3D ~(ADA_FLAG_CAN_TRIM | = ADA_FLAG_CAN_NCQ_TRIM); >> cam_periph_async(periph, code, path, arg); >> break; >> @@ -1293,6 +1306,17 @@ adaregister(struct cam_periph *periph, v >> softc->disk->d_delmaxsize =3D maxio; >> if ((cpi.hba_misc & PIM_UNMAPPED) !=3D 0) >> softc->disk->d_flags |=3D DISKFLAG_UNMAPPED_BIO; >> + /* >> + * If we can do RCVSND_FPDMA_QUEUED commands, we may be able to = do >> + * NCQ trims, if we support trims at all. We also need support = from >> + * the sim do do things properly. Perhaps we should look at log = 13 >> + * dword 0 bit 0 and dword 1 bit 0 are set too... >> + */ >> + if ((cpi.hba_misc & PIM_NCQ_KLUDGE) !=3D 0 && >> + (cgd->ident_data.satacapabilities2 & >> + ATA_SUPPORT_RCVSND_FPDMA_QUEUED) !=3D 0 && >> + (softc->flags & ADA_FLAG_CAN_TRIM) !=3D 0) >> + softc->flags |=3D ADA_FLAG_CAN_NCQ_TRIM; >> strlcpy(softc->disk->d_descr, cgd->ident_data.model, >> MIN(sizeof(softc->disk->d_descr), = sizeof(cgd->ident_data.model))); >> strlcpy(softc->disk->d_ident, cgd->ident_data.serial, >> @@ -1410,10 +1434,9 @@ adaregister(struct cam_periph *periph, v >> return(CAM_REQ_CMP); >> } >> -static void >> -ada_dsmtrim(struct ada_softc *softc, struct bio *bp, struct = ccb_ataio *ataio) >> +static int >> +ada_dsmtrim_req_create(struct ada_softc *softc, struct bio *bp, = struct trim_request *req) >> { >> - struct trim_request *req =3D &softc->trim_req; >> uint64_t lastlba =3D (uint64_t)-1; >> int c, lastcount =3D 0, off, ranges =3D 0; >> @@ -1466,6 +1489,17 @@ ada_dsmtrim(struct ada_softc *softc, str >> (softc->trim_max_ranges - ranges) * = ATA_DSM_RANGE_MAX) >> break; >> } while (1); >> + >> + return (ranges); >> +} >> + >> +static void >> +ada_dsmtrim(struct ada_softc *softc, struct bio *bp, struct = ccb_ataio *ataio) >> +{ >> + struct trim_request *req =3D &softc->trim_req; >> + int ranges; >> + >> + ranges =3D ada_dsmtrim_req_create(softc, bp, req); >> cam_fill_ataio(ataio, >> ada_retry_count, >> adadone, >> @@ -1481,6 +1515,30 @@ ada_dsmtrim(struct ada_softc *softc, str >> } >> static void >> +ada_ncq_dsmtrim(struct ada_softc *softc, struct bio *bp, struct = ccb_ataio *ataio) >> +{ >> + struct trim_request *req =3D &softc->trim_req; >> + int ranges; >> + >> + ranges =3D ada_dsmtrim_req_create(softc, bp, req); >> + cam_fill_ataio(ataio, >> + ada_retry_count, >> + adadone, >> + CAM_DIR_OUT, >> + 0, >> + req->data, >> + ((ranges + ATA_DSM_BLK_RANGES - 1) / >> + ATA_DSM_BLK_RANGES) * ATA_DSM_BLK_SIZE, >> + ada_default_timeout * 1000); >> + ata_ncq_cmd(ataio, >> + ATA_SEND_FPDMA_QUEUED, >> + 0, >> + (ranges + ATA_DSM_BLK_RANGES - 1) / ATA_DSM_BLK_RANGES); >> + ataio->cmd.sector_count_exp =3D ATA_SFPDMA_DSM; >> + ataio->cmd.flags |=3D CAM_ATAIO_AUX_HACK; >> +} >> + >> +static void >> ada_cfaerase(struct ada_softc *softc, struct bio *bp, struct = ccb_ataio *ataio) >> { >> struct trim_request *req =3D &softc->trim_req; >> @@ -1523,7 +1581,9 @@ adastart(struct cam_periph *periph, unio >> /* Run TRIM if not running yet. */ >> if (!softc->trim_running && >> (bp =3D bioq_first(&softc->trim_queue)) !=3D 0) { >> - if (softc->flags & ADA_FLAG_CAN_TRIM) { >> + if (softc->flags & ADA_FLAG_CAN_NCQ_TRIM) { >> + ada_ncq_dsmtrim(softc, bp, ataio); >> + } else if (softc->flags & ADA_FLAG_CAN_TRIM) { >> ada_dsmtrim(softc, bp, ataio); >> } else if ((softc->flags & ADA_FLAG_CAN_CFA) && >> !(softc->flags & ADA_FLAG_CAN_48BIT)) { >> @@ -1749,6 +1809,7 @@ adadone(struct cam_periph *periph, union >> int error; >> cam_periph_lock(periph); >> + bp =3D (struct bio *)done_ccb->ccb_h.ccb_bp; >> if ((done_ccb->ccb_h.status & CAM_STATUS_MASK) !=3D = CAM_REQ_CMP) { >> error =3D adaerror(done_ccb, 0, 0); >> if (error =3D=3D ERESTART) { >> @@ -1762,12 +1823,24 @@ adadone(struct cam_periph *periph, union >> /*reduction*/0, >> /*timeout*/0, >> /*getcount_only*/0); >> + /* >> + * If we get an error on an NCQ DSM TRIM, fall = back >> + * to a non-NCQ DSM TRIM forever. Please note = that if >> + * CAN_NCQ_TRIM is set, CAN_TRIM is necessarily = set too. >> + * However, for this one trim, we treat it as = advisory >> + * and return success up the stack. >> + */ >> + if (state =3D=3D ADA_CCB_TRIM && >> + error !=3D 0 && >> + (softc->flags & ADA_FLAG_CAN_NCQ_TRIM) !=3D = 0) { >> + softc->flags &=3D = ~ADA_FLAG_CAN_NCQ_TRIM; >> + error =3D 0; >> + } >> } else { >> if ((done_ccb->ccb_h.status & CAM_DEV_QFRZN) !=3D = 0) >> panic("REQ_CMP with QFRZN"); >> error =3D 0; >> } >> - bp =3D (struct bio *)done_ccb->ccb_h.ccb_bp; >> bp->bio_error =3D error; >> if (error !=3D 0) { >> bp->bio_resid =3D bp->bio_bcount; >>=20 >> Modified: projects/iosched/sys/cam/cam_ccb.h >> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >> --- projects/iosched/sys/cam/cam_ccb.h Thu Mar 5 21:41:58 2015 = (r279677) >> +++ projects/iosched/sys/cam/cam_ccb.h Fri Mar 6 00:23:23 2015 = (r279678) >> @@ -573,6 +573,7 @@ typedef enum { >> } pi_tmflag; >> typedef enum { >> + PIM_NCQ_KLUDGE =3D 0x200, /* Supports the sata ncq trim kludge = */ >> PIM_EXTLUNS =3D 0x100,/* 64bit extended LUNs supported */ >> PIM_SCANHILO =3D 0x80, /* Bus scans from high ID to low = ID */ >> PIM_NOREMOVE =3D 0x40, /* Removeable devices not = included in scan */ >>=20 >> Modified: projects/iosched/sys/dev/ahci/ahci.c >> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >> --- projects/iosched/sys/dev/ahci/ahci.c Thu Mar 5 21:41:58 2015 = (r279677) >> +++ projects/iosched/sys/dev/ahci/ahci.c Fri Mar 6 00:23:23 2015 = (r279678) >> @@ -2361,6 +2361,9 @@ ahci_setup_fis(struct ahci_channel *ch, >> fis[13] =3D ccb->ataio.cmd.sector_count_exp; >> } >> fis[15] =3D ATA_A_4BIT; >> + /* Gross and vile hack -- makes ncq trim work w/o = changing ataio size */ >> + if (ccb->ataio.cmd.flags & CAM_ATAIO_AUX_HACK) >> + fis[16] =3D 1; >> } else { >> fis[15] =3D ccb->ataio.cmd.control; >> } >> @@ -2618,7 +2621,7 @@ ahciaction(struct cam_sim *sim, union cc >> if (ch->caps & AHCI_CAP_SPM) >> cpi->hba_inquiry |=3D PI_SATAPM; >> cpi->target_sprt =3D 0; >> - cpi->hba_misc =3D PIM_SEQSCAN | PIM_UNMAPPED; >> + cpi->hba_misc =3D PIM_SEQSCAN | PIM_UNMAPPED | = PIM_NCQ_KLUDGE; >> cpi->hba_eng_cnt =3D 0; >> if (ch->caps & AHCI_CAP_SPM) >> cpi->max_target =3D 15; >>=20 >=20 --Apple-Mail=_BCDC3C1D-6567-4014-BC2A-916B7D97BBFE Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP using GPGMail -----BEGIN PGP SIGNATURE----- Comment: GPGTools - https://gpgtools.org iQIcBAEBCgAGBQJU+QfmAAoJEGwc0Sh9sBEAK8kP/3d+IhMb3+EDshU/MdsfpFy3 XWB7+WtEilrWRcYCBJGGFDZm0Nq8byS2PQHY76OEjWDwIN82gWymaNDAMCs6FFE3 +Jy2aDjUVIL1Fp4xkMiFGXIThOXzb1rK4Gd7QWz1J8K+3BRHczBtxPtjWyTRAhnS ou+niJyJnW3sFZ7Vt9pW3Fm81xIElf5jE68z+AP82Aekw/fG5yRjkJDmTeE8BOIQ vdV7VnoyrpCbjlj1yGNqQud2KGOC49QbcLKnMPn3IvNrvyYotEIz1+OzwAdFkmSV mIkg/GpGmIQF14F3/9W5gzdbVTVmGV3PZNVqoPn5xG6ZZrzJ2vR91cdPWUAY+gon BXi3LBbSydcuXUaBmOg9LjmQwJzZ6E6k4ppd1+sbJ6uspZpdXqix+55wZmCb7eyI cjnVSrkPYGrMwPUiJasGJosS5ZqjMAiYoQEhtcDgiaqnp1qhGZUl4+00wmGUdjjf aQSg6VoeLUUhhTztwZBUAs8ywY6hSlXylsSJsJAqZlXoUtdPF04XFuxBU6fWmVOU /XpM0LA7Qy7ifFuFYEC80sMgOqpy5UgF52zU2r6Qq2oIMRlxfxpizAEF7O3Adcpf n1S+purMpqqenrZ1sMQ50LJQyEfYdrHa6wgDiBotUiA6ukSfV5Fi6UtEqla6zMW+ Pq7XhtzaV1X7Dj2ZqyjM =aNHU -----END PGP SIGNATURE----- --Apple-Mail=_BCDC3C1D-6567-4014-BC2A-916B7D97BBFE--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5637B469-F10D-4D7A-8036-DE417368F71E>