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>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] 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’t pushed these changes into -head. That level of detail doesn’t 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: > > Any reason why you don't retry the failed NCQ TRIM as a none NCQ TRIM instead of returning success then it failed? > > Regards > Steve > > 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 >> >> 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. >> >> 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 >> >> Modified: projects/iosched/sys/cam/ata/ata_all.h >> ============================================================================== >> --- 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; >> >> Modified: projects/iosched/sys/cam/ata/ata_da.c >> ============================================================================== >> --- 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 = 0x0400, >> ADA_FLAG_CAN_POWERMGT = 0x0800, >> ADA_FLAG_CAN_DMA48 = 0x1000, >> - ADA_FLAG_DIRTY = 0x2000 >> + ADA_FLAG_DIRTY = 0x2000, >> + ADA_FLAG_CAN_NCQ_TRIM = 0x4000 /* CAN_TRIM also set */ >> } ada_flags; >> typedef enum { >> @@ -1018,10 +1019,22 @@ adaasync(void *callback_arg, u_int32_t c >> else >> softc->flags &= ~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 |= ADA_FLAG_CAN_TRIM; >> - else >> - softc->flags &= ~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) != 0 && */ /* Don't know how to do this here */ >> + (cgd.ident_data.satacapabilities2 & ATA_SUPPORT_RCVSND_FPDMA_QUEUED) != 0 && >> + (softc->flags & ADA_FLAG_CAN_TRIM) != 0) >> + softc->flags |= ADA_FLAG_CAN_NCQ_TRIM; >> + else >> + softc->flags &= ~ADA_FLAG_CAN_NCQ_TRIM; >> + } else >> + softc->flags &= ~(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 = maxio; >> if ((cpi.hba_misc & PIM_UNMAPPED) != 0) >> softc->disk->d_flags |= 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) != 0 && >> + (cgd->ident_data.satacapabilities2 & >> + ATA_SUPPORT_RCVSND_FPDMA_QUEUED) != 0 && >> + (softc->flags & ADA_FLAG_CAN_TRIM) != 0) >> + softc->flags |= 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 = &softc->trim_req; >> uint64_t lastlba = (uint64_t)-1; >> int c, lastcount = 0, off, ranges = 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 = &softc->trim_req; >> + int ranges; >> + >> + ranges = 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 = &softc->trim_req; >> + int ranges; >> + >> + ranges = 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 = ATA_SFPDMA_DSM; >> + ataio->cmd.flags |= CAM_ATAIO_AUX_HACK; >> +} >> + >> +static void >> ada_cfaerase(struct ada_softc *softc, struct bio *bp, struct ccb_ataio *ataio) >> { >> struct trim_request *req = &softc->trim_req; >> @@ -1523,7 +1581,9 @@ adastart(struct cam_periph *periph, unio >> /* Run TRIM if not running yet. */ >> if (!softc->trim_running && >> (bp = bioq_first(&softc->trim_queue)) != 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 = (struct bio *)done_ccb->ccb_h.ccb_bp; >> if ((done_ccb->ccb_h.status & CAM_STATUS_MASK) != CAM_REQ_CMP) { >> error = adaerror(done_ccb, 0, 0); >> if (error == 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 == ADA_CCB_TRIM && >> + error != 0 && >> + (softc->flags & ADA_FLAG_CAN_NCQ_TRIM) != 0) { >> + softc->flags &= ~ADA_FLAG_CAN_NCQ_TRIM; >> + error = 0; >> + } >> } else { >> if ((done_ccb->ccb_h.status & CAM_DEV_QFRZN) != 0) >> panic("REQ_CMP with QFRZN"); >> error = 0; >> } >> - bp = (struct bio *)done_ccb->ccb_h.ccb_bp; >> bp->bio_error = error; >> if (error != 0) { >> bp->bio_resid = bp->bio_bcount; >> >> Modified: projects/iosched/sys/cam/cam_ccb.h >> ============================================================================== >> --- 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 = 0x200, /* Supports the sata ncq trim kludge */ >> PIM_EXTLUNS = 0x100,/* 64bit extended LUNs supported */ >> PIM_SCANHILO = 0x80, /* Bus scans from high ID to low ID */ >> PIM_NOREMOVE = 0x40, /* Removeable devices not included in scan */ >> >> Modified: projects/iosched/sys/dev/ahci/ahci.c >> ============================================================================== >> --- 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] = ccb->ataio.cmd.sector_count_exp; >> } >> fis[15] = 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] = 1; >> } else { >> fis[15] = ccb->ataio.cmd.control; >> } >> @@ -2618,7 +2621,7 @@ ahciaction(struct cam_sim *sim, union cc >> if (ch->caps & AHCI_CAP_SPM) >> cpi->hba_inquiry |= PI_SATAPM; >> cpi->target_sprt = 0; >> - cpi->hba_misc = PIM_SEQSCAN | PIM_UNMAPPED; >> + cpi->hba_misc = PIM_SEQSCAN | PIM_UNMAPPED | PIM_NCQ_KLUDGE; >> cpi->hba_eng_cnt = 0; >> if (ch->caps & AHCI_CAP_SPM) >> cpi->max_target = 15; >> > [-- Attachment #2 --] -----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-----help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5637B469-F10D-4D7A-8036-DE417368F71E>