Date: Thu, 14 Apr 2016 23:01:35 -0600 From: Warner Losh <imp@bsdimp.com> To: Steven Hartland <steven.hartland@multiplay.co.uk> Cc: Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@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: <CANCZdfqYi4o3QmCzOWkG%2BsJtRPHWczdqz29Svw1VeoDxsuAzdw@mail.gmail.com> In-Reply-To: <571072B7.4040909@multiplay.co.uk> References: <201604142147.u3ELlwYo052010@repo.freebsd.org> <571072B7.4040909@multiplay.co.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Apr 14, 2016 at 10:48 PM, Steven Hartland < steven.hartland@multiplay.co.uk> wrote: > 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? > Already added a comment here. thanks for the suggestion. > 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? > Looks like you are correct. I'll tweak it. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqYi4o3QmCzOWkG%2BsJtRPHWczdqz29Svw1VeoDxsuAzdw>