Date: Thu, 12 Nov 2009 20:15:27 GMT From: Alexander Motin <mav@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 170571 for review Message-ID: <200911122015.nACKFRKu008763@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://p4web.freebsd.org/chv.cgi?CH=170571 Change 170571 by mav@mav_mavbook on 2009/11/12 20:14:54 Fix several device freeze counting bugs. Assert correct reference counting instead of blindly ignoring problems. Affected files ... .. //depot/projects/scottl-camlock/src/sys/cam/cam_periph.c#43 edit .. //depot/projects/scottl-camlock/src/sys/cam/cam_xpt.c#128 edit .. //depot/projects/scottl-camlock/src/sys/cam/scsi/scsi_cd.c#29 edit .. //depot/projects/scottl-camlock/src/sys/cam/scsi/scsi_ch.c#20 edit Differences ... ==== //depot/projects/scottl-camlock/src/sys/cam/cam_periph.c#43 (text+ko) ==== @@ -981,15 +981,21 @@ { union ccb *saved_ccb; cam_status status; - int frozen; + int frozen = 0; int sense; struct scsi_start_stop_unit *scsi_cmd; u_int32_t relsim_flags, timeout; - int xpt_done_ccb; + int xpt_done_ccb = FALSE; - xpt_done_ccb = FALSE; status = done_ccb->ccb_h.status; - frozen = (status & CAM_DEV_QFRZN) != 0; + if (status & CAM_DEV_QFRZN) { + frozen = 1; + /* + * Clear freeze flag now for case of retry, + * freeze will be dropped later. + */ + done_ccb->ccb_h.status &= ~CAM_DEV_QFRZN; + } sense = (status & CAM_AUTOSNS_VALID) != 0; status &= CAM_STATUS_MASK; @@ -997,17 +1003,6 @@ relsim_flags = 0; saved_ccb = (union ccb *)done_ccb->ccb_h.saved_ccb_ptr; - /* - * Unfreeze the queue once if it is already frozen.. - */ - if (frozen != 0) { - cam_release_devq(done_ccb->ccb_h.path, - /*relsim_flags*/0, - /*openings*/0, - /*timeout*/0, - /*getcount_only*/0); - } - switch (status) { case CAM_REQ_CMP: { @@ -1184,14 +1179,33 @@ */ if (done_ccb->ccb_h.retry_count > 0) done_ccb->ccb_h.retry_count--; - + /* + * Drop freeze taken due to CAM_DEV_QFREEZE flag set on recovery + * request. + */ cam_release_devq(done_ccb->ccb_h.path, /*relsim_flags*/relsim_flags, /*openings*/0, /*timeout*/timeout, /*getcount_only*/0); - if (xpt_done_ccb == TRUE) + if (xpt_done_ccb == TRUE) { + /* + * Copy frozen flag from recovery request if it is set there + * for some reason. + */ + if (frozen != 0) + done_ccb->ccb_h.status |= CAM_DEV_QFRZN; (*done_ccb->ccb_h.cbfcnp)(periph, done_ccb); + } else { + /* Drop freeze taken, if this recovery request got error. */ + if (frozen != 0) { + cam_release_devq(done_ccb->ccb_h.path, + /*relsim_flags*/0, + /*openings*/0, + /*timeout*/0, + /*getcount_only*/0); + } + } } /* @@ -1451,6 +1465,11 @@ action_string = "No recovery CCB supplied"; goto sense_error_done; } + /* + * Clear freeze flag for original request here, as + * this freeze will be dropped as part of ERESTART. + */ + ccb->ccb_h.status &= ~CAM_DEV_QFRZN; bcopy(ccb, save_ccb, sizeof(*save_ccb)); print_ccb = save_ccb; periph->flags |= CAM_PERIPH_RECOVERY_INPROG; ==== //depot/projects/scottl-camlock/src/sys/cam/cam_xpt.c#128 (text+ko) ==== @@ -4131,45 +4131,36 @@ static void xpt_release_devq_device(struct cam_ed *dev, u_int count, int run_queue) { - int rundevq; - rundevq = 0; - if (dev->ccbq.queue.qfrozen_cnt > 0) { - - count = (count > dev->ccbq.queue.qfrozen_cnt) ? - dev->ccbq.queue.qfrozen_cnt : count; - dev->ccbq.queue.qfrozen_cnt -= count; - if (dev->ccbq.queue.qfrozen_cnt == 0) { - - /* - * No longer need to wait for a successful - * command completion. - */ - dev->flags &= ~CAM_DEV_REL_ON_COMPLETE; - - /* - * Remove any timeouts that might be scheduled - * to release this queue. - */ - if ((dev->flags & CAM_DEV_REL_TIMEOUT_PENDING) != 0) { - callout_stop(&dev->callout); - dev->flags &= ~CAM_DEV_REL_TIMEOUT_PENDING; - } - - /* - * Now that we are unfrozen schedule the - * device so any pending transactions are - * run. - */ - if ((dev->ccbq.queue.entries > 0) - && (xpt_schedule_dev_sendq(dev->target->bus, dev)) - && (run_queue != 0)) { - rundevq = 1; - } + KASSERT(count <= dev->ccbq.queue.qfrozen_cnt, + ("xpt_release_devq: requested %u > present %u\n", + count, dev->ccbq.queue.qfrozen_cnt)); + dev->ccbq.queue.qfrozen_cnt -= count; + if (dev->ccbq.queue.qfrozen_cnt == 0) { + /* + * No longer need to wait for a successful + * command completion. + */ + dev->flags &= ~CAM_DEV_REL_ON_COMPLETE; + /* + * Remove any timeouts that might be scheduled + * to release this queue. + */ + if ((dev->flags & CAM_DEV_REL_TIMEOUT_PENDING) != 0) { + callout_stop(&dev->callout); + dev->flags &= ~CAM_DEV_REL_TIMEOUT_PENDING; + } + /* + * Now that we are unfrozen schedule the + * device so any pending transactions are + * run. + */ + if ((dev->ccbq.queue.entries > 0) + && (xpt_schedule_dev_sendq(dev->target->bus, dev)) + && (run_queue != 0)) { + xpt_run_dev_sendq(dev->target->bus); } } - if (rundevq != 0) - xpt_run_dev_sendq(dev->target->bus); } void @@ -4178,32 +4169,30 @@ struct camq *sendq; mtx_assert(sim->mtx, MA_OWNED); - sendq = &(sim->devq->send_queue); - if (sendq->qfrozen_cnt > 0) { + KASSERT(sendq->qfrozen_cnt > 0, + ("xpt_release_simq: requested 1 > present %u\n", + sendq->qfrozen_cnt)); + sendq->qfrozen_cnt--; + if (sendq->qfrozen_cnt == 0) { + /* + * If there is a timeout scheduled to release this + * sim queue, remove it. The queue frozen count is + * already at 0. + */ + if ((sim->flags & CAM_SIM_REL_TIMEOUT_PENDING) != 0){ + callout_stop(&sim->callout); + sim->flags &= ~CAM_SIM_REL_TIMEOUT_PENDING; + } + if (run_queue) { + struct cam_eb *bus; - sendq->qfrozen_cnt--; - if (sendq->qfrozen_cnt == 0) { /* - * If there is a timeout scheduled to release this - * sim queue, remove it. The queue frozen count is - * already at 0. + * Now that we are unfrozen run the send queue. */ - if ((sim->flags & CAM_SIM_REL_TIMEOUT_PENDING) != 0){ - callout_stop(&sim->callout); - sim->flags &= ~CAM_SIM_REL_TIMEOUT_PENDING; - } - - if (run_queue) { - struct cam_eb *bus; - - /* - * Now that we are unfrozen run the send queue. - */ - bus = xpt_find_bus(sim->path_id); - xpt_run_dev_sendq(bus); - xpt_release_bus(bus); - } + bus = xpt_find_bus(sim->path_id); + xpt_run_dev_sendq(bus); + xpt_release_bus(bus); } } } ==== //depot/projects/scottl-camlock/src/sys/cam/scsi/scsi_cd.c#29 (text+ko) ==== @@ -1570,7 +1570,8 @@ bp->bio_resid = bp->bio_bcount; bp->bio_error = error; bp->bio_flags |= BIO_ERROR; - cam_release_devq(done_ccb->ccb_h.path, + if ((done_ccb->ccb_h.status & CAM_DEV_QFRZN) != 0) + cam_release_devq(done_ccb->ccb_h.path, /*relsim_flags*/0, /*reduction*/0, /*timeout*/0, @@ -1658,7 +1659,8 @@ struct ccb_getdev cgd; /* Don't wedge this device's queue */ - cam_release_devq(done_ccb->ccb_h.path, + if ((done_ccb->ccb_h.status & CAM_DEV_QFRZN) != 0) + cam_release_devq(done_ccb->ccb_h.path, /*relsim_flags*/0, /*reduction*/0, /*timeout*/0, ==== //depot/projects/scottl-camlock/src/sys/cam/scsi/scsi_ch.c#20 (text+ko) ==== @@ -606,7 +606,8 @@ retry_scheduled = 0; /* Don't wedge this device's queue */ - cam_release_devq(done_ccb->ccb_h.path, + if ((done_ccb->ccb_h.status & CAM_DEV_QFRZN) != 0) + cam_release_devq(done_ccb->ccb_h.path, /*relsim_flags*/0, /*reduction*/0, /*timeout*/0,
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911122015.nACKFRKu008763>