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);
