From 32d02b2c6ba5f2b495e2e9f996e170930c6bda61 Mon Sep 17 00:00:00 2001 From: dlg Date: Fri, 23 Jul 2010 09:41:16 +0000 Subject: [PATCH] matthew@ noted a possible misuse of the rwlock used to protect vscsi from being opened by multiple processes at the same time. then he pointed out that pool_get can sleep in the middle of scsi_cmd, during which time the process processing the vscsi requests can go away. this uses a state variable to protect vscsi from multiple opens which is checked by all the process and scsi midlayer entrypoints. it avoids a potential sleep in the middle of the scsi_cmd handler by moving to iopools. lots of tweaks and feedback by matthew@ --- sys/dev/vscsi.c | 215 ++++++++++++++++++++++++++++++------------------ 1 file changed, 137 insertions(+), 78 deletions(-) diff --git a/sys/dev/vscsi.c b/sys/dev/vscsi.c index 3e9c95acc08..ce48f495df2 100644 --- a/sys/dev/vscsi.c +++ b/sys/dev/vscsi.c @@ -1,4 +1,4 @@ -/* $OpenBSD: vscsi.c,v 1.14 2010/06/28 18:31:01 krw Exp $ */ +/* $OpenBSD: vscsi.c,v 1.15 2010/07/23 09:41:16 dlg Exp $ */ /* * Copyright (c) 2008 David Gwynne @@ -49,23 +49,33 @@ struct vscsi_ccb { TAILQ_HEAD(vscsi_ccb_list, vscsi_ccb); +enum vscsi_state { + VSCSI_S_CLOSED, + VSCSI_S_CONFIG, + VSCSI_S_RUNNING +}; + struct vscsi_softc { struct device sc_dev; struct scsi_link sc_link; struct scsibus_softc *sc_scsibus; + struct mutex sc_state_mtx; + enum vscsi_state sc_state; + u_int sc_ccb_count; struct pool sc_ccb_pool; + + struct scsi_iopool sc_iopool; + struct vscsi_ccb_list sc_ccb_i2t; struct vscsi_ccb_list sc_ccb_t2i; int sc_ccb_tag; struct mutex sc_ccb_mtx; - struct rwlock sc_ccb_polling; + struct mutex sc_poll_mtx; + struct rwlock sc_ioc_lock; struct selinfo sc_sel; struct mutex sc_sel_mtx; - - struct rwlock sc_open; - volatile int sc_opened; }; #define DEVNAME(_s) ((_s)->sc_dev.dv_xname) @@ -93,15 +103,16 @@ struct scsi_adapter vscsi_switch = { NULL }; -void vscsi_xs_stuffup(struct scsi_xfer *); - +int vscsi_running(struct vscsi_softc *); int vscsi_i2t(struct vscsi_softc *, struct vscsi_ioc_i2t *); int vscsi_data(struct vscsi_softc *, struct vscsi_ioc_data *, int); int vscsi_t2i(struct vscsi_softc *, struct vscsi_ioc_t2i *); -struct vscsi_ccb * vscsi_ccb_get(struct vscsi_softc *, int); -#define vscsi_ccb_put(_s, _c) pool_put(&(_s)->sc_ccb_pool, (_c)) +void vscsi_done(struct vscsi_softc *, struct vscsi_ccb *); + +void * vscsi_ccb_get(void *); +void vscsi_ccb_put(void *, void *); void filt_vscsidetach(struct knote *); int filt_vscsiread(struct knote *, long); @@ -128,14 +139,23 @@ vscsi_attach(struct device *parent, struct device *self, void *aux) printf("\n"); - rw_init(&sc->sc_open, DEVNAME(sc)); - rw_init(&sc->sc_ccb_polling, DEVNAME(sc)); + mtx_init(&sc->sc_state_mtx, IPL_BIO); + sc->sc_state = VSCSI_S_CLOSED; + + TAILQ_INIT(&sc->sc_ccb_i2t); + TAILQ_INIT(&sc->sc_ccb_t2i); + mtx_init(&sc->sc_ccb_mtx, IPL_BIO); + mtx_init(&sc->sc_poll_mtx, IPL_BIO); + mtx_init(&sc->sc_sel_mtx, IPL_BIO); + rw_init(&sc->sc_ioc_lock, "vscsiioc"); + scsi_iopool_init(&sc->sc_iopool, sc, vscsi_ccb_get, vscsi_ccb_put); sc->sc_link.adapter = &vscsi_switch; sc->sc_link.adapter_softc = sc; sc->sc_link.adapter_target = 256; sc->sc_link.adapter_buswidth = 256; sc->sc_link.openings = 1; + sc->sc_link.pool = &sc->sc_iopool; bzero(&saa, sizeof(saa)); saa.saa_sc_link = &sc->sc_link; @@ -144,29 +164,37 @@ vscsi_attach(struct device *parent, struct device *self, void *aux) &saa, scsiprint); } +int +vscsi_running(struct vscsi_softc *sc) +{ + int running; + + mtx_enter(&sc->sc_state_mtx); + running = (sc->sc_state == VSCSI_S_RUNNING); + mtx_leave(&sc->sc_state_mtx); + + return (running); +} + void vscsi_cmd(struct scsi_xfer *xs) { struct scsi_link *link = xs->sc_link; struct vscsi_softc *sc = link->adapter_softc; - struct vscsi_ccb *ccb; + struct vscsi_ccb *ccb = xs->io; int polled = ISSET(xs->flags, SCSI_POLL); - if (sc->sc_opened == 0) { - vscsi_xs_stuffup(xs); + if (!vscsi_running(sc)) { + xs->error = XS_DRIVER_STUFFUP; + scsi_done(xs); return; } if (ISSET(xs->flags, SCSI_POLL) && ISSET(xs->flags, SCSI_NOSLEEP)) { printf("%s: POLL && NOSLEEP for 0x%02x\n", DEVNAME(sc), xs->cmd->opcode); - vscsi_xs_stuffup(xs); - return; - } - - ccb = vscsi_ccb_get(sc, ISSET(xs->flags, SCSI_NOSLEEP) ? 0 : 1); - if (ccb == NULL) { - vscsi_xs_stuffup(xs); + xs->error = XS_DRIVER_STUFFUP; + scsi_done(xs); return; } @@ -178,19 +206,26 @@ vscsi_cmd(struct scsi_xfer *xs) selwakeup(&sc->sc_sel); if (polled) { - rw_enter_read(&sc->sc_ccb_polling); + mtx_enter(&sc->sc_poll_mtx); while (ccb->ccb_xs != NULL) - tsleep(ccb, PRIBIO, "vscsipoll", 0); - vscsi_ccb_put(sc, ccb); - rw_exit_read(&sc->sc_ccb_polling); + msleep(ccb, &sc->sc_poll_mtx, PRIBIO, "vscsipoll", 0); + mtx_leave(&sc->sc_poll_mtx); + scsi_done(xs); } } void -vscsi_xs_stuffup(struct scsi_xfer *xs) +vscsi_done(struct vscsi_softc *sc, struct vscsi_ccb *ccb) { - xs->error = XS_DRIVER_STUFFUP; - scsi_done(xs); + struct scsi_xfer *xs = ccb->ccb_xs; + + if (ISSET(xs->flags, SCSI_POLL)) { + mtx_enter(&sc->sc_poll_mtx); + ccb->ccb_xs = NULL; + wakeup(ccb); + mtx_leave(&sc->sc_poll_mtx); + } else + scsi_done(xs); } int @@ -198,7 +233,7 @@ vscsi_probe(struct scsi_link *link) { struct vscsi_softc *sc = link->adapter_softc; - if (sc->sc_opened == 0) + if (!vscsi_running(sc)) return (ENXIO); return (0); @@ -208,26 +243,38 @@ int vscsiopen(dev_t dev, int flags, int mode, struct proc *p) { struct vscsi_softc *sc = DEV2SC(dev); - int rv; + enum vscsi_state state = VSCSI_S_RUNNING; + int rv = 0; if (sc == NULL) return (ENXIO); - rv = rw_enter(&sc->sc_open, RW_WRITE | RW_NOSLEEP); + mtx_enter(&sc->sc_state_mtx); + if (sc->sc_state != VSCSI_S_CLOSED) + rv = EBUSY; + else + sc->sc_state = VSCSI_S_CONFIG; + mtx_leave(&sc->sc_state_mtx); + if (rv != 0) return (rv); pool_init(&sc->sc_ccb_pool, sizeof(struct vscsi_ccb), 0, 0, 0, "vscsiccb", NULL); - pool_setipl(&sc->sc_ccb_pool, IPL_BIO); - TAILQ_INIT(&sc->sc_ccb_i2t); - TAILQ_INIT(&sc->sc_ccb_t2i); - mtx_init(&sc->sc_ccb_mtx, IPL_BIO); - mtx_init(&sc->sc_sel_mtx, IPL_BIO); - sc->sc_opened = 1; + /* we need to guarantee some ccbs will be available for the iopool */ + rv = pool_prime(&sc->sc_ccb_pool, 8); + if (rv != 0) { + pool_destroy(&sc->sc_ccb_pool); + state = VSCSI_S_CLOSED; + } - return (0); + /* commit changes */ + mtx_enter(&sc->sc_state_mtx); + sc->sc_state = state; + mtx_leave(&sc->sc_state_mtx); + + return (rv); } int @@ -238,6 +285,8 @@ vscsiioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) int read = 0; int err = 0; + rw_enter_write(&sc->sc_ioc_lock); + switch (cmd) { case VSCSI_I2T: err = vscsi_i2t(sc, (struct vscsi_ioc_i2t *)addr); @@ -267,6 +316,8 @@ vscsiioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) break; } + rw_exit_write(&sc->sc_ioc_lock); + return (err); } @@ -369,7 +420,6 @@ vscsi_t2i(struct vscsi_softc *sc, struct vscsi_ioc_t2i *t2i) struct scsi_xfer *xs; struct scsi_link *link; int rv = 0; - int polled; TAILQ_FOREACH(ccb, &sc->sc_ccb_t2i, ccb_entry) { if (ccb->ccb_tag == t2i->tag) @@ -400,15 +450,7 @@ vscsi_t2i(struct vscsi_softc *sc, struct vscsi_ioc_t2i *t2i) break; } - polled = ISSET(xs->flags, SCSI_POLL); - - scsi_done(xs); - - if (polled) { - ccb->ccb_xs = NULL; - wakeup(ccb); - } else - vscsi_ccb_put(sc, ccb); + vscsi_done(sc, ccb); return (rv); } @@ -487,60 +529,77 @@ vscsiclose(dev_t dev, int flags, int mode, struct proc *p) { struct vscsi_softc *sc = DEV2SC(dev); struct vscsi_ccb *ccb; - int polled; int i; - sc->sc_opened = 0; + mtx_enter(&sc->sc_state_mtx); + KASSERT(sc->sc_state == VSCSI_S_RUNNING); + sc->sc_state = VSCSI_S_CONFIG; + mtx_leave(&sc->sc_state_mtx); while ((ccb = TAILQ_FIRST(&sc->sc_ccb_t2i)) != NULL) { TAILQ_REMOVE(&sc->sc_ccb_t2i, ccb, ccb_entry); - polled = ISSET(ccb->ccb_xs->flags, SCSI_POLL); - - vscsi_xs_stuffup(ccb->ccb_xs); - - if (polled) { - ccb->ccb_xs = NULL; - wakeup(ccb); - } else - vscsi_ccb_put(sc, ccb); + ccb->ccb_xs->error = XS_DRIVER_STUFFUP; + vscsi_done(sc, ccb); } while ((ccb = TAILQ_FIRST(&sc->sc_ccb_i2t)) != NULL) { TAILQ_REMOVE(&sc->sc_ccb_i2t, ccb, ccb_entry); - polled = ISSET(ccb->ccb_xs->flags, SCSI_POLL); - - vscsi_xs_stuffup(ccb->ccb_xs); + ccb->ccb_xs->error = XS_DRIVER_STUFFUP; + vscsi_done(sc, ccb); + } - if (polled) { - ccb->ccb_xs = NULL; - wakeup(ccb); - } else - vscsi_ccb_put(sc, ccb); + mtx_enter(&sc->sc_state_mtx); + while (sc->sc_ccb_count > 0) { + msleep(&sc->sc_ccb_count, &sc->sc_state_mtx, + PRIBIO, "vscsiccb", 0); } + mtx_leave(&sc->sc_state_mtx); - rw_enter_write(&sc->sc_ccb_polling); pool_destroy(&sc->sc_ccb_pool); - rw_exit_write(&sc->sc_ccb_polling); for (i = 0; i < sc->sc_link.adapter_buswidth; i++) scsi_detach_target(sc->sc_scsibus, i, DETACH_FORCE); - rw_exit(&sc->sc_open); + mtx_enter(&sc->sc_state_mtx); + sc->sc_state = VSCSI_S_CLOSED; + mtx_leave(&sc->sc_state_mtx); return (0); } -struct vscsi_ccb * -vscsi_ccb_get(struct vscsi_softc *sc, int waitok) +void * +vscsi_ccb_get(void *cookie) { - struct vscsi_ccb *ccb; + struct vscsi_softc *sc = cookie; + struct vscsi_ccb *ccb = NULL; - ccb = pool_get(&sc->sc_ccb_pool, waitok ? PR_WAITOK : PR_NOWAIT); - if (ccb == NULL) - return (NULL); + mtx_enter(&sc->sc_state_mtx); + if (sc->sc_state == VSCSI_S_RUNNING && + (ccb = pool_get(&sc->sc_ccb_pool, PR_NOWAIT)) != NULL) { + ccb->ccb_tag = sc->sc_ccb_tag++; + ccb->ccb_datalen = 0; - ccb->ccb_tag = sc->sc_ccb_tag++; - ccb->ccb_datalen = 0; + sc->sc_ccb_count++; + } + mtx_leave(&sc->sc_state_mtx); return (ccb); } + +void +vscsi_ccb_put(void *cookie, void *io) +{ + struct vscsi_softc *sc = cookie; + struct vscsi_ccb *ccb = io; + + mtx_enter(&sc->sc_state_mtx); + + pool_put(&sc->sc_ccb_pool, ccb); + sc->sc_ccb_count--; + + if (sc->sc_state != VSCSI_S_RUNNING && + sc->sc_ccb_count == 0) + wakeup(&sc->sc_ccb_count); + + mtx_leave(&sc->sc_state_mtx); +} -- 2.20.1