disksort() vs bufqs

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

disksort() vs bufqs

David Gwynne
once upon a time pretty much all block storage was on spinning disks, so all the drivers for disks tried to order queued io to better suit moving a head across a platter by calling disksort. between then and now a lot of the assumptions that disksort relied on are no longer true (eg, SSDs are a thing now) so it's usually more harmful than helpful. with beck@s addition of the nscan backend in bufqs it makes sense to try and unify all disk drivers behind bufqs and deprecate disksort.

so, ive been working through the tree replacing the last direct users of the disksort() over to bufqs, but ive run out of code that i can compile for. there are now 4 drivers in 3 architectures that need conversion, so im asking if anyone is interested in taking some (or all) of them on.

the code in question is:

sys/arch/hp300/dev/hd.c
sys/arch/sparc/dev/fd.c
sys/arch/sparc/dev/xy.c
sys/arch/vax/vsa/hdc9224.c

if anyone has those archs and a some spare time, please feel free to have a go and cut them over. even if you dont have the hardware to test your change, making a compilable diff and sending it to tech@ for testing is better than i can do right now.

cheers,
dlg

Reply | Threaded
Open this post in threaded view
|

Re: disksort() vs bufqs

David Gwynne-5
here's a diff to sys/arch/sparc/dev/fd.c. not compiled, and therefore
not tested.

anyone? ok?

On Sun, Nov 03, 2013 at 09:30:56AM +1000, David Gwynne wrote:

> once upon a time pretty much all block storage was on spinning disks, so all the drivers for disks tried to order queued io to better suit moving a head across a platter by calling disksort. between then and now a lot of the assumptions that disksort relied on are no longer true (eg, SSDs are a thing now) so it's usually more harmful than helpful. with beck@s addition of the nscan backend in bufqs it makes sense to try and unify all disk drivers behind bufqs and deprecate disksort.
>
> so, ive been working through the tree replacing the last direct users of the disksort() over to bufqs, but ive run out of code that i can compile for. there are now 4 drivers in 3 architectures that need conversion, so im asking if anyone is interested in taking some (or all) of them on.
>
> the code in question is:
>
> sys/arch/hp300/dev/hd.c
> sys/arch/sparc/dev/fd.c
> sys/arch/sparc/dev/xy.c
> sys/arch/vax/vsa/hdc9224.c
>
> if anyone has those archs and a some spare time, please feel free to have a go and cut them over. even if you dont have the hardware to test your change, making a compilable diff and sending it to tech@ for testing is better than i can do right now.
>
> cheers,
> dlg


Index: fd.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc/dev/fd.c,v
retrieving revision 1.86
diff -u -p -r1.86 fd.c
--- fd.c 12 Nov 2013 16:04:09 -0000 1.86
+++ fd.c 17 Nov 2013 06:40:34 -0000
@@ -217,7 +217,8 @@ struct fd_softc {
 
  TAILQ_ENTRY(fd_softc) sc_drivechain;
  int sc_ops; /* I/O ops since last switch */
- struct buf sc_q; /* head of buf chain */
+ struct bufq sc_bufq; /* pending I/O requests */
+ struct buf *sc_bp; /* current I/O */
 
  struct timeout fd_motor_on_to;
  struct timeout fd_motor_off_to;
@@ -642,6 +643,7 @@ fdattach(parent, self, aux)
  */
  fd->sc_dk.dk_flags = DKF_NOLABELREAD;
  fd->sc_dk.dk_name = fd->sc_dv.dv_xname;
+ bufq_init(&fd->sc_bufq, BUFQ_DEFAULT);
  disk_attach(&fd->sc_dv, &fd->sc_dk);
 
  /*
@@ -732,11 +734,13 @@ fdstrategy(bp)
  (long long)fd->sc_blkno, bp->b_cylinder);
 #endif
 
+ /* Queue transfer */
+ bufq_queue(&fd->sc_bufq, bp);
+
  /* Queue transfer on drive, activate drive and controller if idle. */
  s = splbio();
- disksort(&fd->sc_q, bp);
  timeout_del(&fd->fd_motor_off_to); /* a good idea */
- if (!fd->sc_q.b_active)
+ if (fd->sc_bp == NULL)
  fdstart(fd);
 #ifdef DIAGNOSTIC
  else {
@@ -767,7 +771,7 @@ fdstart(fd)
  int active = !TAILQ_EMPTY(&fdc->sc_drives);
 
  /* Link into controller queue. */
- fd->sc_q.b_active = 1;
+ fd->sc_bp = bufq_dequeue(&fd->sc_bufq);
  TAILQ_INSERT_TAIL(&fdc->sc_drives, fd, sc_drivechain);
 
  /* If controller not already active, start it. */
@@ -782,6 +786,9 @@ fdfinish(fd, bp)
 {
  struct fdc_softc *fdc = (void *)fd->sc_dv.dv_parent;
 
+ fd->sc_skip = 0;
+ fd->sc_bp = bufq_dequeue(&fd->sc_bufq);
+
  /*
  * Move this drive to the end of the queue to give others a `fair'
  * chance.  We only force a switch if N operations are completed while
@@ -791,15 +798,11 @@ fdfinish(fd, bp)
  if (TAILQ_NEXT(fd, sc_drivechain) != NULL && ++fd->sc_ops >= 8) {
  fd->sc_ops = 0;
  TAILQ_REMOVE(&fdc->sc_drives, fd, sc_drivechain);
- if (bp->b_actf) {
+ if (fd->sc_bp != NULL)
  TAILQ_INSERT_TAIL(&fdc->sc_drives, fd, sc_drivechain);
- } else
- fd->sc_q.b_active = 0;
  }
- bp->b_resid = fd->sc_bcount;
- fd->sc_skip = 0;
- fd->sc_q.b_actf = bp->b_actf;
 
+ bp->b_resid = fd->sc_bcount;
  biodone(bp);
  /* turn off motor 5s from now */
  timeout_add_sec(&fd->fd_motor_off_to, 5);
@@ -1117,7 +1120,7 @@ fdctimeout(arg)
  goto out;
  }
 
- if (fd->sc_q.b_actf)
+ if (fd->sc_bp != NULL)
  fdc->sc_state++;
  else
  fdc->sc_state = DEVIDLE;
@@ -1285,11 +1288,10 @@ loop:
  }
 
  /* Is there a transfer to this drive?  If not, deactivate drive. */
- bp = fd->sc_q.b_actf;
+ bp = fd->sc_bp;
  if (bp == NULL) {
  fd->sc_ops = 0;
  TAILQ_REMOVE(&fdc->sc_drives, fd, sc_drivechain);
- fd->sc_q.b_active = 0;
  goto loop;
  }
 
@@ -1663,7 +1665,7 @@ fdcretry(fdc)
  int error = EIO;
 
  fd = TAILQ_FIRST(&fdc->sc_drives);
- bp = fd->sc_q.b_actf;
+ bp = fd->sc_bp;
 
  fdc->sc_overruns = 0;
  if (fd->sc_opts & FDOPT_NORETRY)

Reply | Threaded
Open this post in threaded view
|

Re: disksort() vs bufqs

David Gwynne-5
In reply to this post by David Gwynne
this one is for hd(4) on hp300s. again, i cant compile and therefore cant test.

anyone able to compile this for me? does anyone have such hardware?

On Sun, Nov 03, 2013 at 09:30:56AM +1000, David Gwynne wrote:

> once upon a time pretty much all block storage was on spinning disks, so all the drivers for disks tried to order queued io to better suit moving a head across a platter by calling disksort. between then and now a lot of the assumptions that disksort relied on are no longer true (eg, SSDs are a thing now) so it's usually more harmful than helpful. with beck@s addition of the nscan backend in bufqs it makes sense to try and unify all disk drivers behind bufqs and deprecate disksort.
>
> so, ive been working through the tree replacing the last direct users of the disksort() over to bufqs, but ive run out of code that i can compile for. there are now 4 drivers in 3 architectures that need conversion, so im asking if anyone is interested in taking some (or all) of them on.
>
> the code in question is:
>
> sys/arch/hp300/dev/hd.c
> sys/arch/sparc/dev/fd.c
> sys/arch/sparc/dev/xy.c
> sys/arch/vax/vsa/hdc9224.c
>
> if anyone has those archs and a some spare time, please feel free to have a go and cut them over. even if you dont have the hardware to test your change, making a compilable diff and sending it to tech@ for testing is better than i can do right now.
>
> cheers,
> dlg

Index: hd.c
===================================================================
RCS file: /cvs/src/sys/arch/hp300/dev/hd.c,v
retrieving revision 1.73
diff -u -p -r1.73 hd.c
--- hd.c 1 Nov 2013 17:36:19 -0000 1.73
+++ hd.c 19 Nov 2013 05:22:44 -0000
@@ -294,6 +294,7 @@ hdattach(parent, self, aux)
  */
  sc->sc_dkdev.dk_name = sc->sc_dev.dv_xname;
  disk_attach(&sc->sc_dev, &sc->sc_dkdev);
+ bufq_init(&sc->sc_bufq, BUFQ_DEFAULT);
 
  sc->sc_slave = ha->ha_slave;
  sc->sc_punit = ha->ha_punit;
@@ -630,9 +631,9 @@ hdclose(dev, flag, mode, p)
  if (dk->dk_openmask == 0) {
  rs->sc_flags |= HDF_CLOSING;
  s = splbio();
- while (rs->sc_tab.b_active) {
+ while (rs->sc_bp == NULL) {
  rs->sc_flags |= HDF_WANTED;
- tsleep((caddr_t)&rs->sc_tab, PRIBIO, "hdclose", 0);
+ tsleep((caddr_t)&rs->sc_bp, PRIBIO, "hdclose", 0);
  }
  splx(s);
  rs->sc_flags &= ~(HDF_CLOSING);
@@ -669,11 +670,11 @@ hdstrategy(bp)
  if (bounds_check_with_label(bp, rs->sc_dkdev.dk_label) == -1)
  goto done;
 
+ bufq_queue(&rs->sc_bufq, bp);
+
  s = splbio();
- dp = &rs->sc_tab;
- disksort(dp, bp);
- if (dp->b_active == 0) {
- dp->b_active = 1;
+ if (rs->b_bp == NULL) {
+ rs->sc_bp = bufq_dequeue(rs->sc_bufq);
  hdustart(rs);
  }
  splx(s);
@@ -710,7 +711,7 @@ hdustart(rs)
 {
  struct buf *bp;
 
- bp = rs->sc_tab.b_actf;
+ bp = rs->sc_bp;
  rs->sc_addr = bp->b_data;
  rs->sc_resid = bp->b_bcount;
  if (hpibreq(rs->sc_dev.dv_parent, &rs->sc_hq))
@@ -722,24 +723,23 @@ hdfinish(rs, bp)
  struct hd_softc *rs;
  struct buf *bp;
 {
- struct buf *dp = &rs->sc_tab;
  int s;
 
  rs->sc_errcnt = 0;
- dp->b_actf = bp->b_actf;
  bp->b_resid = 0;
  s = splbio();
  biodone(bp);
  splx(s);
+
  hpibfree(rs->sc_dev.dv_parent, &rs->sc_hq);
- if (dp->b_actf)
- return (dp->b_actf);
- dp->b_active = 0;
- if (rs->sc_flags & HDF_WANTED) {
+ rs->sc_bp = bufq_dequeue(&rs->sc_bufq);
+
+ if (rs->sc_bp == NULL && rs->sc_flags & HDF_WANTED) {
  rs->sc_flags &= ~HDF_WANTED;
- wakeup((caddr_t)dp);
+ wakeup((caddr_t)&rs->sc_bp);
  }
- return (NULL);
+
+ return (rs->sc_bp);
 }
 
 void
@@ -748,7 +748,7 @@ hdstart(arg)
 {
  struct hd_softc *rs = arg;
  struct disklabel *lp;
- struct buf *bp = rs->sc_tab.b_actf;
+ struct buf *bp = rs->sc_bp;
  int ctlr, slave;
  daddr_t bn;
 
@@ -831,7 +831,7 @@ hdgo(arg)
  void *arg;
 {
  struct hd_softc *rs = arg;
- struct buf *bp = rs->sc_tab.b_actf;
+ struct buf *bp = rs->sc_bp;
  int rw, ctlr, slave;
 
  ctlr = rs->sc_dev.dv_parent->dv_unit;
@@ -855,7 +855,7 @@ hdinterrupt(arg)
 {
  struct hd_softc *rs = arg;
  int unit = rs->sc_dev.dv_unit;
- struct buf *bp = rs->sc_tab.b_actf;
+ struct buf *bp = rs->sc_bp;
  u_char stat = 13; /* in case hpibrecv fails */
  int rv, restart, ctlr, slave;
 
@@ -1025,7 +1025,7 @@ hderror(unit)
  * Note that not all errors report a block number, in that case
  * we just use b_blkno.
  */
- bp = rs->sc_tab.b_actf;
+ bp = rs->sc_bp;
  pbn = DL_GETPOFFSET(&rs->sc_dkdev.dk_label->d_partitions[DISKPART(bp->b_dev)]);
  if ((sp->c_fef & FEF_CU) || (sp->c_fef & FEF_DR) ||
     (sp->c_ief & IEF_RRMASK)) {
Index: hdvar.h
===================================================================
RCS file: /cvs/src/sys/arch/hp300/dev/hdvar.h,v
retrieving revision 1.11
diff -u -p -r1.11 hdvar.h
--- hdvar.h 5 Jun 2011 18:40:33 -0000 1.11
+++ hdvar.h 19 Nov 2013 05:22:44 -0000
@@ -70,7 +70,8 @@ struct hd_softc {
  struct hd_iocmd sc_ioc;
  struct hd_rscmd sc_rsc;
  struct hd_stat sc_stat;
- struct buf sc_tab; /* buffer queue */
+ struct bufq sc_bufq; /* buffer queue */
+ struct buf *sc_bp;
 #ifdef DEBUG
  struct hdstats sc_stats;
 #endif

Reply | Threaded
Open this post in threaded view
|

Re: disksort() vs bufqs

David Gwynne-5
In reply to this post by David Gwynne
this one is for xy(4) on sparc. again, i cant test, so if anyone
could do that for me it would be appreciated.

does anyone have one of these?

Index: xy.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc/dev/xy.c,v
retrieving revision 1.57
diff -u -p -r1.57 xy.c
--- xy.c 1 Nov 2013 17:36:19 -0000 1.57
+++ xy.c 19 Nov 2013 05:54:05 -0000
@@ -508,10 +508,7 @@ xyattach(parent, self, aux)
  xy->parent = xyc;
 
  /* init queue of waiting bufs */
-
- xy->xyq.b_active = 0;
- xy->xyq.b_actf = 0;
- xy->xyq.b_actb = &xy->xyq.b_actf; /* XXX b_actb: not used? */
+ bufq_init(&xy->xy_bufq, BUFQ_DEFAULT);
 
  xy->xyrq = &xyc->reqs[xa->driveno];
 
@@ -1003,14 +1000,14 @@ xystrategy(bp)
  if (bounds_check_with_label(bp, xy->sc_dk.dk_label) == -1)
  goto done;
 
+ bufq_queue(&xy->xy_bufq, bp);
+
  /*
  * now we know we have a valid buf structure that we need to do I/O
  * on.
  */
  s = splbio(); /* protect the queues */
 
- disksort(&xy->xyq, bp);
-
  /* start 'em up */
 
  xyc_start(xy->parent, NULL);
@@ -1604,7 +1601,6 @@ xyc_reset(xycsc, quiet, blastmode, error
     dvma_mapout((vaddr_t)iorq->dbufbase,
  (vaddr_t)iorq->buf->b_data,
  iorq->buf->b_bcount);
-    iorq->xy->xyq.b_actf = iorq->buf->b_actf;
     disk_unbusy(&xycsc->reqs[lcv].xy->sc_dk,
  (xycsc->reqs[lcv].buf->b_bcount -
  xycsc->reqs[lcv].buf->b_resid),
@@ -1651,9 +1647,9 @@ xyc_start(xycsc, iorq)
  if (iorq == NULL) {
  for (lcv = 0; lcv < XYC_MAXDEV ; lcv++) {
  if ((xy = xycsc->sc_drives[lcv]) == NULL) continue;
- if (xy->xyq.b_actf == NULL) continue;
+ if (!bufq_peek(&xy->xy_bufq)) continue;
  if (xy->xyrq->mode != XY_SUB_FREE) continue;
- xyc_startbuf(xycsc, xy, xy->xyq.b_actf);
+ xyc_startbuf(xycsc, xy, bufq_dequeue(&xy->xy_bufq));
  }
  }
  xyc_submit_iorq(xycsc, iorq, XY_SUB_NOQ);
@@ -1783,7 +1779,6 @@ xyc_remove_iorq(xycsc)
  dvma_mapout((vaddr_t) iorq->dbufbase,
     (vaddr_t) bp->b_data,
     bp->b_bcount);
- iorq->xy->xyq.b_actf = bp->b_actf;
  disk_unbusy(&iorq->xy->sc_dk,
     (bp->b_bcount - bp->b_resid),
     (bp->b_flags & B_READ));
Index: xyvar.h
===================================================================
RCS file: /cvs/src/sys/arch/sparc/dev/xyvar.h,v
retrieving revision 1.7
diff -u -p -r1.7 xyvar.h
--- xyvar.h 5 Jun 2011 18:40:33 -0000 1.7
+++ xyvar.h 19 Nov 2013 05:54:05 -0000
@@ -115,7 +115,7 @@ struct xy_softc {
   u_char nsect;                    /* number of sectors per track */
   u_char hw_spt;                   /* as above, but includes spare sectors */
   struct xy_iorq *xyrq;   /* this disk's ioreq structure */
-  struct buf xyq;   /* queue'd I/O requests */
+  struct bufq xy_bufq;   /* queue'd I/O requests */
   struct dkbad dkb;                /* bad144 sectors */
 };
 

Reply | Threaded
Open this post in threaded view
|

Re: disksort() vs bufqs

David Gwynne-5
In reply to this post by David Gwynne
and here's the last one, the hdc stuff on vax.

can anyone compile this? test?

Index: hdc9224.c
===================================================================
RCS file: /cvs/src/sys/arch/vax/vsa/hdc9224.c,v
retrieving revision 1.41
diff -u -p -r1.41 hdc9224.c
--- hdc9224.c 14 Oct 2013 23:26:22 -0000 1.41
+++ hdc9224.c 19 Nov 2013 06:43:24 -0000
@@ -134,7 +134,7 @@ struct hdcsoftc {
  struct evcount sc_intrcnt;
  struct vsbus_dma sc_vd;
  vaddr_t sc_regs; /* register addresses */
- struct buf sc_buf_queue;
+ struct bufq sc_bufq;
  struct buf *sc_active;
  struct hdc9224_UDCreg sc_creg; /* (command) registers to be written */
  struct hdc9224_UDCreg sc_sreg; /* (status) registers being read */
@@ -387,7 +387,7 @@ hdcintr(void *arg)
  struct buf *bp;
 
  sc->sc_status = HDC_RSTAT;
- if (sc->sc_active == 0)
+ if (sc->sc_active == NULL)
  return; /* Complain? */
 
  if ((sc->sc_status & (DKC_ST_INTPEND | DKC_ST_DONE)) !=
@@ -395,7 +395,7 @@ hdcintr(void *arg)
  return; /* Why spurious ints sometimes??? */
 
  bp = sc->sc_active;
- sc->sc_active = 0;
+ sc->sc_active = NULL;
  if ((sc->sc_status & DKC_ST_TERMCOD) != DKC_TC_SUCCESS) {
  int i;
  u_char *g = (u_char *)&sc->sc_sreg;
@@ -455,8 +455,9 @@ hdstrategy(struct buf *bp)
  if (bounds_check_with_label(bp, hd->sc_disk.dk_label) == -1)
  goto done;
 
+ bufq_queue(&sc->sc_bufq, bp);
+
  s = splbio();
- disksort(&sc->sc_buf_queue, bp);
  if (inq == 0) {
  inq = 1;
  vsbus_dma_start(&sc->sc_vd);
@@ -479,8 +480,7 @@ hdc_qstart(void *arg)
  inq = 0;
 
  hdcstart(sc, NULL);
- dp = &sc->sc_buf_queue;
- if (dp->b_actf != NULL) {
+ if (bufq_peek(&sc->sc_bufq)) {
  vsbus_dma_start(&sc->sc_vd); /* More to go */
  inq = 1;
  }
@@ -499,15 +499,14 @@ hdcstart(struct hdcsoftc *sc, struct buf
 
  splassert(IPL_BIO);
 
- if (sc->sc_active)
+ if (sc->sc_active != NULL)
  return; /* Already doing something */
 
  if (ob == NULL) {
- dp = &sc->sc_buf_queue;
- if ((bp = dp->b_actf) == NULL)
+ bp = bufq_dequeue(&sc->sc_bufq);
+ if (bp == NULL)
  return; /* Nothing to do */
 
- dp->b_actf = bp->b_actf;
  sc->sc_bufaddr = bp->b_data;
  sc->sc_bytecnt = bp->b_bcount;
  sc->sc_retries = 0;