[src/jdolecek-ncq]: src/sys/dev/ata restart I/O processing after freeing xfer...

[jdolecek <jdolecek%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/d4131be8ce92
branches:  jdolecek-ncq
changeset: 352687:d4131be8ce92
user:      jdolecek <jdolecek%NetBSD.org@localhost>
date:      Fri Jun 23 20:40:51 2017 +0000

description:
restart I/O processing after freeing xfer, i.e. now even after commands
like cache flush or standby; the command handling no longer use on-stack xfer,
hence use queue slot and compete with normal I/O for the xfers

the restart give change to all drives attached to the same channel
in round-robin fashion, for fair usage and to recover from situation
when disk is idle due to all xfers being consumed by other drives

make special concession for flush cache - ignore any new I/O requests
on the particular disk while the flush cache is waiting for xfer,
so that I/O queue won't starve the flush cache and the flush cache would
be done ASAP

tested on piixide(4), ahci(4), siisata(4)

diffstat:

 sys/dev/ata/TODO.ncq |  18 +++++++++----
 sys/dev/ata/ata.c    |  48 ++++++++++++++++++++++++++++++++++++-
 sys/dev/ata/atavar.h |   6 +++-
 sys/dev/ata/wd.c     |  66 +++++++++++++++++++++++++++++++++++++++------------
 sys/dev/ata/wdvar.h  |   3 +-
 5 files changed, 114 insertions(+), 27 deletions(-)

diffs (truncated from 362 to 300 lines):

diff -r 2202ffc67252 -r d4131be8ce92 sys/dev/ata/TODO.ncq
--- a/sys/dev/ata/TODO.ncq      Wed Jun 21 22:40:43 2017 +0000
+++ b/sys/dev/ata/TODO.ncq      Fri Jun 23 20:40:51 2017 +0000
@@ -2,12 +2,15 @@
 ----
 
 fix crashdump for mvsata - request times out (maybe same as HEAD?)
+- HEAD crash due to KASSERT(chp->ch_queue->active_xfer != NULL)
 
-siisata - fix all new XXX and unmergable bits, fix PMP
+siisata - fix all new XXX and unmergable bits
 
 test crashdump with siisata
 - fails with recursive panic via pmap_kremove_local() regardless if
-  drive connected via PMP or direct
+  drive connected via PMP or direct (failed KASSERT(panicstr != NULL))
+- HEAD crashes same as branch
+- see kern/49610 for potential fix
 
 test wd* at umass?, confirm the ata_channel kludge works
 + add detach code (channel detach, free queue)
@@ -19,13 +22,16 @@
 maybe do device error handling in not-interrupt-context (maybe this should be
 done on a mpata branch?)
 
-add mechanics to re-check queue when xfer is finished - needed on PMP
-and for IDE disks, when one drive uses up all available xfers nothing
-ever restarts queue for the other drives
+do not limit openings for xfers for non-NCQ drives in wdstart(), the tag
+will not be used so can use any xfer
+
+in atastart(), restrict NCQ commands to commands for the same drive? it's
+fine for fis-based switching to have outstanding for several drives, but
+not non-FIS
 
 Other random notes (do outside the NCQ branch):
 -----------------------------------------------------
-change wd(4) to use dk_open()
+change wd(4) to use dksubr
 
 dump to unopened disk fails (e.g. dump do wd1b when wd1a not mounted), due
 to the open path executing ata_get_params(), which eventually tsleeps()
diff -r 2202ffc67252 -r d4131be8ce92 sys/dev/ata/ata.c
--- a/sys/dev/ata/ata.c Wed Jun 21 22:40:43 2017 +0000
+++ b/sys/dev/ata/ata.c Fri Jun 23 20:40:51 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata.c,v 1.132.8.15 2017/06/21 22:40:43 jdolecek Exp $  */
+/*     $NetBSD: ata.c,v 1.132.8.16 2017/06/23 20:40:51 jdolecek Exp $  */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.  All rights reserved.
@@ -25,7 +25,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.132.8.15 2017/06/21 22:40:43 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.132.8.16 2017/06/23 20:40:51 jdolecek Exp $");
 
 #include "opt_ata.h"
 
@@ -2143,3 +2143,47 @@
        if (xfer->c_bio.flags & ATA_FUA)
                *device |= WDSD_FUA;
 }
+
+/*
+ * Must be called without any locks, i.e. with both drive and channel locks
+ * released.
+ */ 
+void
+ata_channel_start(struct ata_channel *chp, int drive)
+{
+       int i;
+       struct ata_drive_datas *drvp;
+
+       KASSERT(chp->ch_ndrives > 0);
+
+#define ATA_DRIVE_START(chp, drive) \
+       do {                                                    \
+               KASSERT(drive < chp->ch_ndrives);               \
+               drvp = &chp->ch_drive[drive];                   \
+                                                               \
+               if (drvp->drive_type != ATA_DRIVET_ATA &&       \
+                   drvp->drive_type != ATA_DRIVET_ATAPI &&     \
+                   drvp->drive_type != ATA_DRIVET_OLD)         \
+                       continue;                               \
+                                                               \
+               if (drvp->drv_start != NULL)                    \
+                       (*drvp->drv_start)(drvp->drv_softc);    \
+       } while (0)
+
+       /*
+        * Process drives in round robin fashion starting with next one after
+        * the one which finished transfer. Thus no single drive would
+        * completely starve other drives on same channel. 
+        * This loop processes all but the current drive, so won't do anything
+        * if there is only one drive in channel.
+        */
+       for (i = (drive + 1) % chp->ch_ndrives; i != drive;
+           i = (i + 1) % chp->ch_ndrives) {
+               ATA_DRIVE_START(chp, i);
+       }
+
+       /* Now try to kick off xfers on the current drive */
+       ATA_DRIVE_START(chp, drive);
+
+#undef ATA_DRIVE_START
+}
diff -r 2202ffc67252 -r d4131be8ce92 sys/dev/ata/atavar.h
--- a/sys/dev/ata/atavar.h      Wed Jun 21 22:40:43 2017 +0000
+++ b/sys/dev/ata/atavar.h      Fri Jun 23 20:40:51 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: atavar.h,v 1.92.8.14 2017/06/21 22:40:43 jdolecek Exp $        */
+/*     $NetBSD: atavar.h,v 1.92.8.15 2017/06/23 20:40:51 jdolecek Exp $        */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.
@@ -299,7 +299,8 @@
 #endif
 
        /* Callbacks into the drive's driver. */
-       void    (*drv_done)(void *, struct ata_xfer *); /* transfer is done */
+       void    (*drv_done)(device_t, struct ata_xfer *); /* xfer is done */
+       void    (*drv_start)(device_t);                   /* start queue */
 
        device_t drv_softc;             /* ATA drives softc, if any */
        struct ata_channel *chnl_softc; /* channel softc */
@@ -499,6 +500,7 @@
 void   ata_reset_channel(struct ata_channel *, int);
 void   ata_channel_freeze(struct ata_channel *);
 void   ata_channel_thaw(struct ata_channel *);
+void   ata_channel_start(struct ata_channel *, int);
 
 int    ata_addref(struct ata_channel *);
 void   ata_delref(struct ata_channel *);
diff -r 2202ffc67252 -r d4131be8ce92 sys/dev/ata/wd.c
--- a/sys/dev/ata/wd.c  Wed Jun 21 22:40:43 2017 +0000
+++ b/sys/dev/ata/wd.c  Fri Jun 23 20:40:51 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: wd.c,v 1.428.2.19 2017/06/20 20:58:22 jdolecek Exp $ */
+/*     $NetBSD: wd.c,v 1.428.2.20 2017/06/23 20:40:51 jdolecek Exp $ */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.  All rights reserved.
@@ -54,7 +54,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: wd.c,v 1.428.2.19 2017/06/20 20:58:22 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: wd.c,v 1.428.2.20 2017/06/23 20:40:51 jdolecek Exp $");
 
 #include "opt_ata.h"
 
@@ -192,10 +192,10 @@
 
 void  wdgetdefaultlabel(struct wd_softc *, struct disklabel *);
 void  wdgetdisklabel(struct wd_softc *);
-void  wdstart(struct wd_softc *);
+void  wdstart(device_t);
 void  wdstart1(struct wd_softc *, struct buf *, struct ata_xfer *);
 void  wdrestart(void *);
-void  wddone(void *, struct ata_xfer *);
+void  wddone(device_t, struct ata_xfer *);
 static void wd_params_to_properties(struct wd_softc *);
 int   wd_get_params(struct wd_softc *, uint8_t, struct ataparams *);
 int   wd_flushcache(struct wd_softc *, int);
@@ -310,6 +310,7 @@
        wd->drvp = adev->adev_drv_data;
 
        wd->drvp->drv_openings = 1;
+       wd->drvp->drv_start = wdstart;
        wd->drvp->drv_done = wddone;
        wd->drvp->drv_softc = wd->sc_dev; /* done in atabusconfig_thread()
                                             but too late */
@@ -518,6 +519,7 @@
        /* Unhook the entropy source. */
        rnd_detach_source(&sc->rnd_source);
 
+       mutex_destroy(&sc->sc_lock);
        callout_destroy(&sc->sc_restart_ch);
 
        sc->drvp->drive_type = ATA_DRIVET_NONE; /* no drive any more here */
@@ -620,7 +622,8 @@
        bufq_put(wd->sc_q, bp);
        mutex_exit(&wd->sc_lock);
 
-       wdstart(wd);
+       /* Try to queue on the current drive only */
+       wdstart(wd->sc_dev);
        return;
 done:
        /* Toss transfer; we're done early. */
@@ -632,8 +635,9 @@
  * Queue a drive for I/O.
  */
 void
-wdstart(struct wd_softc *wd)
+wdstart(device_t self)
 {
+       struct wd_softc *wd = device_private(self);
        struct buf *bp;
        struct ata_xfer *xfer;
 
@@ -645,11 +649,19 @@
 
        mutex_enter(&wd->sc_lock);
 
+       /*
+        * Do not queue any transfers until flush is finished, so that
+        * once flush is pending, it will get handled as soon as xfer
+        * is available.
+        */
+       if (ISSET(wd->sc_flags, WDF_FLUSH_PEND))
+               goto out;
+
        while (bufq_peek(wd->sc_q) != NULL) {
                /* First try to get command */
                xfer = ata_get_xfer_ext(wd->drvp->chnl_softc, false,
                    wd->drvp->drv_openings);
-               if (xfer == NULL) 
+               if (xfer == NULL)
                        break;
 
                /* There is got to be a buf for us */
@@ -660,6 +672,7 @@
                wdstart1(wd, bp, xfer);
        }
 
+out:
        mutex_exit(&wd->sc_lock);
 }
 
@@ -739,9 +752,9 @@
 }
 
 void
-wddone(void *v, struct ata_xfer *xfer)
+wddone(device_t self, struct ata_xfer *xfer)
 {
-       struct wd_softc *wd = device_private(v);
+       struct wd_softc *wd = device_private(self);
        const char *errmsg;
        int do_perror = 0;
        struct buf *bp;
@@ -854,7 +867,7 @@
        ata_free_xfer(wd->drvp->chnl_softc, xfer);
        mutex_exit(&wd->sc_lock);
        biodone(bp);
-       wdstart(wd);
+       ata_channel_start(wd->drvp->chnl_softc, wd->drvp->drive);
 }
 
 void
@@ -1861,6 +1874,7 @@
 
 out:
        ata_free_xfer(wd->drvp->chnl_softc, xfer);
+       ata_channel_start(wd->drvp->chnl_softc, wd->drvp->drive);
        return error;
 }
 
@@ -1903,6 +1917,7 @@
 
 out:
        ata_free_xfer(wd->drvp->chnl_softc, xfer);
+       ata_channel_start(wd->drvp->chnl_softc, wd->drvp->drive);
        return error;
 }
 
@@ -1921,9 +1936,20 @@
            wd->sc_params.atap_cmd_set2 == 0xffff))
                return ENODEV;
 
+       mutex_enter(&wd->sc_lock);
+       SET(wd->sc_flags, WDF_FLUSH_PEND);
+       mutex_exit(&wd->sc_lock);
+
        xfer = ata_get_xfer(wd->drvp->chnl_softc);
-       if (xfer == NULL)
-               return EINTR;
+
+       mutex_enter(&wd->sc_lock);
+       CLR(wd->sc_flags, WDF_FLUSH_PEND);
+       mutex_exit(&wd->sc_lock);
+
+       if (xfer == NULL) {
+               error = EINTR;
+               goto out;
+       }
 
        if ((wd->sc_params.atap_cmd2_en & ATA_CMD2_LBA48) != 0 &&
            (wd->sc_params.atap_cmd2_en & ATA_CMD2_FCE) != 0) {
@@ -1939,13 +1965,13 @@
                aprint_error_dev(wd->sc_dev,
                    "flush cache command didn't complete\n");
                error = EIO;
-               goto out;
+               goto out_xfer;
        }
        if (xfer->c_ata_c.flags & AT_ERROR) {