Skip site navigation (1)Skip section navigation (2)
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>