Date: Fri, 15 Apr 2016 05:48:55 +0100 From: Steven Hartland <steven.hartland@multiplay.co.uk> To: Warner Losh <imp@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r298002 - in head/sys: cam cam/ata cam/scsi conf dev/ahci Message-ID: <571072B7.4040909@multiplay.co.uk> In-Reply-To: <201604142147.u3ELlwYo052010@repo.freebsd.org> References: <201604142147.u3ELlwYo052010@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Great to see this hitting the tree Warner, I have a few questions inline below. On 14/04/2016 22:47, Warner Losh wrote: > Author: imp > Date: Thu Apr 14 21:47:58 2016 > New Revision: 298002 > URL: https://svnweb.freebsd.org/changeset/base/298002 > > Log: > New CAM I/O scheduler for FreeBSD. The default I/O scheduler is the same > as before. The common scheduling bits have moved from inline code in > each of the CAM periph drivers into a library that implements the > default scheduling. > > In addition, a number of rate-limiting and I/O preference options can > be enabled by adding CAM_IOSCHED_NETFLIX to your config file. A number > of extra stats are also maintained. CAM_IOSCHED_NETFLIX isn't on by > default because it uses a separate BIO_READ and BIO_WRITE queue, so > doesn't honor BIO_ORDERED between these two types of operations. We > already didn't honor it for BIO_DELETE, and we don't depend on > BIO_ORDERED between reads and writes anywhere in the system (it is > currently used with BIO_FLUSH in ZFS to make sure some writes are > complete before others start and as a poor-man's soft dependency in > one place in UFS where we won't be issuing READs until after the > operation completes). However, out of an abundance of caution, it > isn't enabled by default. > > Plus, this also brings in NCQ TRIM support for those SSDs that support > it. A black list is also provided for known rogues that use NCQ trim > as an excuse to corrupt the drive. It was difficult to separate out > into a separate commit. > > This code has run in production at Netflix for over a year now. > > Sponsored by: Netflix, Inc > Differential Revision: https://reviews.freebsd.org/D4609 snip... > @@ -844,7 +920,7 @@ adadump(void *arg, void *virtual, vm_off > 0, > NULL, > 0, > - ada_default_timeout*1000); > + 5*1000); Not a fan of random constants, is there a reason why this is now just 5 instead if ada_default_timeout, merge issue perhaps? snip... > @@ -1898,6 +2154,31 @@ out: > static int > adaerror(union ccb *ccb, u_int32_t cam_flags, u_int32_t sense_flags) > { > + struct ada_softc *softc; > + struct cam_periph *periph; > + > + periph = xpt_path_periph(ccb->ccb_h.path); > + softc = (struct ada_softc *)periph->softc; > + > + switch (ccb->ccb_h.status & CAM_STATUS_MASK) { > + case CAM_CMD_TIMEOUT: > +#ifdef CAM_IO_STATS > + softc->timeouts++; > +#endif > + break; > + case CAM_REQ_ABORTED: > + case CAM_REQ_CMP_ERR: > + case CAM_REQ_TERMIO: > + case CAM_UNREC_HBA_ERROR: > + case CAM_DATA_RUN_ERR: > + case CAM_ATA_STATUS_ERROR: > +#ifdef CAM_IO_STATS > + softc->errors++; > +#endif > + break; > + default: > + break; > + } Am I missing something or does this entire switch do nothing unless CAM_IO_STATS is set and hence could all be under the #ifdef? Regards Steve
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?571072B7.4040909>