From owner-svn-src-all@freebsd.org Fri Apr 15 05:01:36 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6CFCBAED2BC for ; Fri, 15 Apr 2016 05:01:36 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ig0-x243.google.com (mail-ig0-x243.google.com [IPv6:2607:f8b0:4001:c05::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 34FFF1A09 for ; Fri, 15 Apr 2016 05:01:36 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-ig0-x243.google.com with SMTP id fn8so1555885igb.2 for ; Thu, 14 Apr 2016 22:01:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc; bh=eToRlesg6tSn6os/J9NAKJGCtNcH3Hj+b6PO3rmenNU=; b=IcWXED32KCSrxHZ4pcZmrHgw1Wd+LDXAzj/xEq/MSS3HCUmvGYJfFoZqct3tGco+d9 umG9uUaluCeH1g4QUFc0EVyBnpblpHi/5IWKdYIgKJByGqgg1dQ5g1wG7tE2CdjZaUyn yiMid4KEuitQqNl6tYCwvFtQXi8Hkzumbr6SBWyEt+DxgCCKRY//rAy+7PYSCNGPV3Mc 5bgkAnhyhacrODbKFwL7fibQ1cyu7iF+29deOW61LI9/rv7kDiZv/zBAFV+8KRDXjh1p FA7Qt+18ebqzklbazz0ZZmTqh0satss5QUDV6wTBjaqHuM92hexsj0+iNhkDn+Q7se5c qYnQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to:cc; bh=eToRlesg6tSn6os/J9NAKJGCtNcH3Hj+b6PO3rmenNU=; b=a07Hy+X2SGTdGlSBRzeomQ+kka20jCzip53B5iULQ/KrWJGYKSvWLBMbmdd0LTWIMk Z0IGGzwN+w9Agpck47Krm/UIo0bFGHEg5DqX/47D9kEJbD3a0+SG54VbyhBx8mcYcC4x GCyIZhpWdEwbCTzQiopp7gTWO2RYZ79FxfSujIK2Ez9OlALcsvi9wKzPK9+1Z4J9RmjV 3zAdu+jNGGS6kUVcYaRXWmrW+p1raqYo2MQOIH4wSxG3vc1hI/BOjoRaWYaEgphWMIaa Pt6uAZVxvLsnruJ897mDpsb31t9mdkKtA8dtWpg1NHPhKJAXdv6ONPvGu8CjI+Gk7qsP VNVA== X-Gm-Message-State: AOPr4FXn6AKnruKx3djouG4UMuZOu8uXRZE0uL0C8+4WDcaWugsN8Kt4Fho2o9rFL5T7E7NsvBRyNxcLFpuK9w== MIME-Version: 1.0 X-Received: by 10.50.110.99 with SMTP id hz3mr2398445igb.16.1460696495472; Thu, 14 Apr 2016 22:01:35 -0700 (PDT) Sender: wlosh@bsdimp.com Received: by 10.79.104.197 with HTTP; Thu, 14 Apr 2016 22:01:35 -0700 (PDT) X-Originating-IP: [50.253.99.174] In-Reply-To: <571072B7.4040909@multiplay.co.uk> References: <201604142147.u3ELlwYo052010@repo.freebsd.org> <571072B7.4040909@multiplay.co.uk> Date: Thu, 14 Apr 2016 23:01:35 -0600 X-Google-Sender-Auth: rI7QSqxrlKIcjntuDXiOPRJdx7M Message-ID: Subject: Re: svn commit: r298002 - in head/sys: cam cam/ata cam/scsi conf dev/ahci From: Warner Losh To: Steven Hartland Cc: Warner Losh , src-committers , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Apr 2016 05:01:36 -0000 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