svn commit: r298002 - in head/sys: cam cam/ata cam/scsi conf dev/ahci

[Warner Losh]

On Thu, Apr 14, 2016 at 10:48 PM, Steven Hartland <
steven.hartland at 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