this reads fine by me, except for one thing. i worry that it looks like wu's get lists of ccbs attached to them that are released when the wu is released. before iopools there was a wu per call to softraids scsi_cmd handler, but now the same wu can be given to scsi_cmd multiple times. sr_scsi_done should probably clean up the ccb list before the xs and wu are given back to the midlayer.
if this isnt a problem and testing goes well, then it has my ok. the wakeups in sr_wu_put are no longer necessary as the iopool code takes over responsibility for sleeping for resources. its just extra code, but it doesnt affect my ok above. dlg On 05/04/2011, at 12:47 AM, Kenneth R Westerback wrote: > On Sun, Apr 03, 2011 at 07:01:04PM -0400, Kenneth R Westerback wrote: >> Works on my crypto volume. People with other volume types would be nice >> to hear from. >> >> .... Ken > > v2. Use scsi_io_[get|put](), stop trying so hard to avoid calling scsi_done() > at SPLBIO as this is nice but not necessary, remove random 'improvements' to > make diff as small as possible. > > Tests still desired! > > .... Ken > > Index: softraid.c > =================================================================== > RCS file: /cvs/src/sys/dev/softraid.c,v > retrieving revision 1.222 > diff -u -p -r1.222 softraid.c > --- softraid.c 15 Mar 2011 13:29:41 -0000 1.222 > +++ softraid.c 4 Apr 2011 14:36:04 -0000 > @@ -1805,7 +1805,7 @@ sr_wu_alloc(struct sr_discipline *sd) > for (i = 0; i < no_wu; i++) { > wu = &sd->sd_wu[i]; > wu->swu_dis = sd; > - sr_wu_put(wu); > + sr_wu_put(sd, wu); > } > > return (0); > @@ -1833,9 +1833,10 @@ sr_wu_free(struct sr_discipline *sd) > } > > void > -sr_wu_put(struct sr_workunit *wu) > +sr_wu_put(void *xsd, void *xwu) > { > - struct sr_discipline *sd = wu->swu_dis; > + struct sr_discipline *sd = (struct sr_discipline *)xsd; > + struct sr_workunit *wu = (struct sr_workunit *)xwu; > struct sr_ccb *ccb; > > int s; > @@ -1864,9 +1865,14 @@ sr_wu_put(struct sr_workunit *wu) > } > TAILQ_INIT(&wu->swu_ccb); > > + splx(s); > + > + mtx_enter(&sd->sd_wu_mtx); > TAILQ_INSERT_TAIL(&sd->sd_wu_freeq, wu, swu_link); > sd->sd_wu_pending--; > + mtx_leave(&sd->sd_wu_mtx); > > + s = splbio(); > /* wake up sleepers */ > #ifdef DIAGNOSTIC > if (sd->sd_wu_sleep < 0) > @@ -1878,30 +1884,20 @@ sr_wu_put(struct sr_workunit *wu) > splx(s); > } > > -struct sr_workunit * > -sr_wu_get(struct sr_discipline *sd, int canwait) > +void * > +sr_wu_get(void *xsd) > { > + struct sr_discipline *sd = (struct sr_discipline *)xsd; > struct sr_workunit *wu; > - int s; > > - s = splbio(); > - > - for (;;) { > - wu = TAILQ_FIRST(&sd->sd_wu_freeq); > - if (wu) { > - TAILQ_REMOVE(&sd->sd_wu_freeq, wu, swu_link); > - wu->swu_state = SR_WU_INPROGRESS; > - sd->sd_wu_pending++; > - break; > - } else if (wu == NULL && canwait) { > - sd->sd_wu_sleep++; > - tsleep(&sd->sd_wu_sleep, PRIBIO, "sr_wu_get", 0); > - sd->sd_wu_sleep--; > - } else > - break; > + mtx_enter(&sd->sd_wu_mtx); > + wu = TAILQ_FIRST(&sd->sd_wu_freeq); > + if (wu) { > + TAILQ_REMOVE(&sd->sd_wu_freeq, wu, swu_link); > + wu->swu_state = SR_WU_INPROGRESS; > + sd->sd_wu_pending++; > } > - > - splx(s); > + mtx_leave(&sd->sd_wu_mtx); > > DNPRINTF(SR_D_WU, "%s: sr_wu_get: %p\n", DEVNAME(sd->sd_sc), wu); > > @@ -1949,18 +1945,7 @@ sr_scsi_cmd(struct scsi_xfer *xs) > goto stuffup; > } > > - /* > - * we'll let the midlayer deal with stalls instead of being clever > - * and sending sr_wu_get !(xs->flags & SCSI_NOSLEEP) in cansleep > - */ > - if ((wu = sr_wu_get(sd, 0)) == NULL) { > - DNPRINTF(SR_D_CMD, "%s: sr_scsi_cmd no wu\n", DEVNAME(sc)); > - xs->error = XS_NO_CCB; > - sr_scsi_done(sd, xs); > - return; > - } > - > - xs->error = XS_NOERROR; > + wu = xs->io; > wu->swu_xs = xs; > > /* the midlayer will query LUNs so report sense to stop scanning */ > @@ -2049,8 +2034,6 @@ stuffup: > xs->error = XS_DRIVER_STUFFUP; > } > complete: > - if (wu) > - sr_wu_put(wu); > sr_scsi_done(sd, xs); > } > int > @@ -3042,6 +3025,8 @@ sr_ioctl_createraid(struct sr_softc *sc, > } > > /* setup scsi midlayer */ > + mtx_init(&sd->sd_wu_mtx, IPL_BIO); > + scsi_iopool_init(&sd->sd_iopool, sd, sr_wu_get, sr_wu_put); > if (sd->sd_openings) > sd->sd_link.openings = sd->sd_openings(sd); > else > @@ -3051,6 +3036,7 @@ sr_ioctl_createraid(struct sr_softc *sc, > sd->sd_link.adapter = &sr_switch; > sd->sd_link.adapter_target = SR_MAX_LD; > sd->sd_link.adapter_buswidth = 1; > + sd->sd_link.pool = &sd->sd_iopool; > bzero(&saa, sizeof(saa)); > saa.saa_sc_link = &sd->sd_link; > > @@ -3954,9 +3940,9 @@ sr_rebuild_thread(void *arg) > lba = blk * sz; > > /* get some wu */ > - if ((wu_r = sr_wu_get(sd, 1)) == NULL) > + if ((wu_r = scsi_io_get(&sd->sd_iopool, 0)) == NULL) > panic("%s: rebuild exhausted wu_r", DEVNAME(sc)); > - if ((wu_w = sr_wu_get(sd, 1)) == NULL) > + if ((wu_w = scsi_io_get(&sd->sd_iopool, 0)) == NULL) > panic("%s: rebuild exhausted wu_w", DEVNAME(sc)); > > /* setup read io */ > @@ -4026,8 +4012,8 @@ queued: > if (slept == 0) > tsleep(sc, PWAIT, "sr_yield", 1); > > - sr_wu_put(wu_r); > - sr_wu_put(wu_w); > + scsi_io_put(&sd->sd_iopool, wu_r); > + scsi_io_put(&sd->sd_iopool, wu_w); > > sd->sd_meta->ssd_rebuild = lba; > > Index: softraid_aoe.c > =================================================================== > RCS file: /cvs/src/sys/dev/softraid_aoe.c,v > retrieving revision 1.17 > diff -u -p -r1.17 softraid_aoe.c > --- softraid_aoe.c 2 Jul 2010 15:49:25 -0000 1.17 > +++ softraid_aoe.c 4 Apr 2011 14:36:05 -0000 > @@ -576,9 +576,6 @@ sr_aoe_input(struct aoe_handler *ah, str > break; > } > } > - > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > sr_scsi_done(sd, xs); > } > > Index: softraid_crypto.c > =================================================================== > RCS file: /cvs/src/sys/dev/softraid_crypto.c,v > retrieving revision 1.63 > diff -u -p -r1.63 softraid_crypto.c > --- softraid_crypto.c 6 Mar 2011 21:41:57 -0000 1.63 > +++ softraid_crypto.c 4 Apr 2011 14:36:06 -0000 > @@ -1345,8 +1345,6 @@ sr_crypto_finish_io(struct sr_workunit * > sr_crypto_putcryptop(ccb->ccb_opaque); > } > > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > sr_scsi_done(sd, xs); > > if (sd->sd_sync && sd->sd_wu_pending == 0) > Index: softraid_raid0.c > =================================================================== > RCS file: /cvs/src/sys/dev/softraid_raid0.c,v > retrieving revision 1.22 > diff -u -p -r1.22 softraid_raid0.c > --- softraid_raid0.c 2 Jul 2010 09:20:26 -0000 1.22 > +++ softraid_raid0.c 4 Apr 2011 14:36:06 -0000 > @@ -456,8 +456,6 @@ sr_raid0_intr(struct buf *bp) > printf("%s: wu: %p not on pending queue\n", > DEVNAME(sc), wu); > > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > sr_scsi_done(sd, xs); > > if (sd->sd_sync && sd->sd_wu_pending == 0) > @@ -468,7 +466,6 @@ sr_raid0_intr(struct buf *bp) > return; > bad: > xs->error = XS_DRIVER_STUFFUP; > - sr_wu_put(wu); > sr_scsi_done(sd, xs); > splx(s); > } > Index: softraid_raid1.c > =================================================================== > RCS file: /cvs/src/sys/dev/softraid_raid1.c,v > retrieving revision 1.26 > diff -u -p -r1.26 softraid_raid1.c > --- softraid_raid1.c 6 Nov 2010 23:01:56 -0000 1.26 > +++ softraid_raid1.c 4 Apr 2011 14:36:06 -0000 > @@ -615,8 +615,6 @@ sr_raid1_intr(struct buf *bp) > wakeup(wu); > } > } else { > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > scsi_done(xs); > } > > @@ -633,8 +631,6 @@ bad: > wu->swu_flags |= SR_WUF_REBUILDIOCOMP; > wakeup(wu); > } else { > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > scsi_done(xs); > } > > Index: softraid_raid6.c > =================================================================== > RCS file: /cvs/src/sys/dev/softraid_raid6.c,v > retrieving revision 1.19 > diff -u -p -r1.19 softraid_raid6.c > --- softraid_raid6.c 7 Aug 2010 03:50:01 -0000 1.19 > +++ softraid_raid6.c 4 Apr 2011 14:36:06 -0000 > @@ -480,7 +480,7 @@ sr_raid6_rw(struct sr_workunit *wu) > rwmode = (xs->flags & SCSI_DATA_IN) ? M_RFLG : M_WFLG; > if (xs->flags & SCSI_DATA_OUT) > /* create write workunit */ > - if ((wu_w = sr_wu_get(sd, 0)) == NULL) { > + if ((wu_w = scsi_io_get(&sd->sd_iopool, SCSI_NOSLEEP)) == NULL){ > printf("%s: can't get wu_w", DEVNAME(sd->sd_sc)); > goto bad; > } > @@ -744,7 +744,7 @@ queued: > bad: > /* wu is unwound by sr_wu_put */ > if (wu_w) > - sr_wu_put(wu_w); > + scsi_io_put(&sd->sd_iopool, wu_w); > return (1); > } > > @@ -889,10 +889,10 @@ sr_raid6_intr(struct buf *bp) > wakeup(wu); > } > } else { > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > if (xs != NULL) > scsi_done(xs); > + else > + scsi_io_put(&sd->sd_iopool, wu); > } > > if (sd->sd_sync && sd->sd_wu_pending == 0) > @@ -908,8 +908,6 @@ bad: > wu->swu_flags |= SR_WUF_REBUILDIOCOMP; > wakeup(wu); > } else { > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > scsi_done(xs); > } > > Index: softraid_raidp.c > =================================================================== > RCS file: /cvs/src/sys/dev/softraid_raidp.c,v > retrieving revision 1.19 > diff -u -p -r1.19 softraid_raidp.c > --- softraid_raidp.c 7 Aug 2010 03:50:01 -0000 1.19 > +++ softraid_raidp.c 4 Apr 2011 14:36:07 -0000 > @@ -396,7 +396,7 @@ sr_raidp_rw(struct sr_workunit *wu) > > if (xs->flags & SCSI_DATA_OUT) > /* create write workunit */ > - if ((wu_w = sr_wu_get(sd, 0)) == NULL) { > + if ((wu_w = scsi_io_get(&sd->sd_iopool, SCSI_NOSLEEP)) == NULL){ > printf("%s: can't get wu_w", DEVNAME(sd->sd_sc)); > goto bad; > } > @@ -553,7 +553,7 @@ queued: > bad: > /* wu is unwound by sr_wu_put */ > if (wu_w) > - sr_wu_put(wu_w); > + scsi_io_put(&sd->sd_iopool, wu_w); > return (1); > } > > @@ -669,10 +669,10 @@ sr_raidp_intr(struct buf *bp) > wakeup(wu); > } > } else { > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > if (xs != NULL) > scsi_done(xs); > + else > + scsi_io_put(&sd->sd_iopool, wu); > } > > if (sd->sd_sync && sd->sd_wu_pending == 0) > @@ -688,8 +688,6 @@ bad: > wu->swu_flags |= SR_WUF_REBUILDIOCOMP; > wakeup(wu); > } else { > - /* do not change the order of these 2 functions */ > - sr_wu_put(wu); > scsi_done(xs); > } > > @@ -831,9 +829,9 @@ sr_raidp_scrub(struct sr_discipline *sd) > int s, slept; > void *xorbuf; > > - if ((wu_r = sr_wu_get(sd, 1)) == NULL) > + if ((wu_w = scsi_io_get(&sd->sd_iopool, 0)) == NULL) > goto done; > - if ((wu_w = sr_wu_get(sd, 1)) == NULL) > + if ((wu_r = scsi_io_get(&sd->sd_iopool, 0)) == NULL) > goto done; > > no_chunk = sd->sd_meta->ssdi.ssd_chunk_no - 1; > Index: softraidvar.h > =================================================================== > RCS file: /cvs/src/sys/dev/softraidvar.h,v > retrieving revision 1.98 > diff -u -p -r1.98 softraidvar.h > --- softraidvar.h 15 Mar 2011 13:29:41 -0000 1.98 > +++ softraidvar.h 4 Apr 2011 14:36:07 -0000 > @@ -526,6 +526,9 @@ struct sr_discipline { > struct sr_wu_list sd_wu_defq; /* deferred wu queue */ > int sd_wu_sleep; /* wu sleepers counter */ > > + struct mutex sd_wu_mtx; > + struct scsi_iopool sd_iopool; > + > /* discipline stats */ > int sd_wu_pending; > u_int64_t sd_wu_collisions; > @@ -604,8 +607,8 @@ struct sr_ccb *sr_ccb_get(struct sr_dis > void sr_ccb_put(struct sr_ccb *); > int sr_wu_alloc(struct sr_discipline *); > void sr_wu_free(struct sr_discipline *); > -struct sr_workunit *sr_wu_get(struct sr_discipline *, int); > -void sr_wu_put(struct sr_workunit *); > +void *sr_wu_get(void *); > +void sr_wu_put(void *, void *); > > /* misc functions */ > int32_t sr_validate_stripsize(u_int32_t);