On Apr 23, 2013, at 12:02 AM, Alexander Motin <m...@freebsd.org> wrote:
> On 23.04.2013 05:35, Scott Long wrote: >> What problem does this solve, other than to unintentionally break the MPT >> driver? This needs to be backed out of HEAD and stable/9 until a better >> analysis is done. > > It depends on definition of "problem". I don't think that polling in a tight > loop is a right way to do anything while system is operational. > Breaking a common storage controller for nothing more than aesthetic reasons is a problem. I'd like you to discuss and review this stuff more. Please. Pretty, pretty please. > Search through the sources shows that the only two drivers doing nasty things > on shutdown_post_sync are mpt and hptmv. While hptmv really shuts worker > thread down there, mpt just disables interrupts breaking normal operation. > That situation looks quite dirty to me. From one side if controller really > needs shutdown (to flush some caches, etc), then it should not receive any > (or reject all) commands after that point (either polled or not). If it > doesn't need shutdown, then what for are these shaman dances? > You'll need to talk to Gibbs about MPT. I committed the code as a proxy for him; I don't recall why he specifically wanted to shut down interrupts there. Talking to him would be a good pre-commit action. > Instead of revert, I propose this trivial non-invasive patch to fix the > problem by moving hptmv and mpt shutdown a bit later in shutdown order: > http://people.freebsd.org/~mav/post_sync.patch > I'm fine with the concept, so long as it works. Have you tested it? (btw, sorry I missed this response earlier) Thanks, Scott >> On Apr 18, 2013, at 3:44 AM, Alexander Motin <m...@freebsd.org> wrote: >> >>> Author: mav >>> Date: Thu Apr 18 09:44:00 2013 >>> New Revision: 249611 >>> URL: http://svnweb.freebsd.org/changeset/base/249611 >>> >>> Log: >>> MFC r248872, r249048: >>> Make pre-shutdown flush and spindown routines to not use >>> xpt_polled_action(), >>> but execute the commands in regular way. There is no any reason to cook >>> CPU >>> while the system is still fully operational. After this change polling in >>> CAM is used only for kernel dumping. >>> >>> Modified: >>> stable/9/sys/cam/ata/ata_da.c >>> stable/9/sys/cam/scsi/scsi_da.c >>> Directory Properties: >>> stable/9/sys/ (props changed) >>> >>> Modified: stable/9/sys/cam/ata/ata_da.c >>> ============================================================================== >>> --- stable/9/sys/cam/ata/ata_da.c Thu Apr 18 09:40:34 2013 >>> (r249610) >>> +++ stable/9/sys/cam/ata/ata_da.c Thu Apr 18 09:44:00 2013 >>> (r249611) >>> @@ -1825,11 +1825,10 @@ adaflush(void) >>> { >>> struct cam_periph *periph; >>> struct ada_softc *softc; >>> + union ccb *ccb; >>> int error; >>> >>> CAM_PERIPH_FOREACH(periph, &adadriver) { >>> - union ccb ccb; >>> - >>> /* If we paniced with lock held - not recurse here. */ >>> if (cam_periph_owned(periph)) >>> continue; >>> @@ -1845,10 +1844,8 @@ adaflush(void) >>> continue; >>> } >>> >>> - xpt_setup_ccb(&ccb.ccb_h, periph->path, CAM_PRIORITY_NORMAL); >>> - >>> - ccb.ccb_h.ccb_state = ADA_CCB_DUMP; >>> - cam_fill_ataio(&ccb.ataio, >>> + ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL); >>> + cam_fill_ataio(&ccb->ataio, >>> 0, >>> adadone, >>> CAM_DIR_NONE, >>> @@ -1856,20 +1853,17 @@ adaflush(void) >>> NULL, >>> 0, >>> ada_default_timeout*1000); >>> - >>> if (softc->flags & ADA_FLAG_CAN_48BIT) >>> - ata_48bit_cmd(&ccb.ataio, ATA_FLUSHCACHE48, 0, 0, 0); >>> + ata_48bit_cmd(&ccb->ataio, ATA_FLUSHCACHE48, 0, 0, 0); >>> else >>> - ata_28bit_cmd(&ccb.ataio, ATA_FLUSHCACHE, 0, 0, 0); >>> - xpt_polled_action(&ccb); >>> + ata_28bit_cmd(&ccb->ataio, ATA_FLUSHCACHE, 0, 0, 0); >>> >>> - error = cam_periph_error(&ccb, >>> - 0, SF_NO_RECOVERY | SF_NO_RETRY, NULL); >>> - if ((ccb.ccb_h.status & CAM_DEV_QFRZN) != 0) >>> - cam_release_devq(ccb.ccb_h.path, /*relsim_flags*/0, >>> - /*reduction*/0, /*timeout*/0, /*getcount_only*/0); >>> + error = cam_periph_runccb(ccb, adaerror, /*cam_flags*/0, >>> + /*sense_flags*/ SF_NO_RECOVERY | SF_NO_RETRY, >>> + softc->disk->d_devstat); >>> if (error != 0) >>> xpt_print(periph->path, "Synchronize cache failed\n"); >>> + xpt_release_ccb(ccb); >>> cam_periph_unlock(periph); >>> } >>> } >>> @@ -1879,11 +1873,10 @@ adaspindown(uint8_t cmd, int flags) >>> { >>> struct cam_periph *periph; >>> struct ada_softc *softc; >>> + union ccb *ccb; >>> int error; >>> >>> CAM_PERIPH_FOREACH(periph, &adadriver) { >>> - union ccb ccb; >>> - >>> /* If we paniced with lock held - not recurse here. */ >>> if (cam_periph_owned(periph)) >>> continue; >>> @@ -1900,10 +1893,8 @@ adaspindown(uint8_t cmd, int flags) >>> if (bootverbose) >>> xpt_print(periph->path, "spin-down\n"); >>> >>> - xpt_setup_ccb(&ccb.ccb_h, periph->path, CAM_PRIORITY_NORMAL); >>> - >>> - ccb.ccb_h.ccb_state = ADA_CCB_DUMP; >>> - cam_fill_ataio(&ccb.ataio, >>> + ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL); >>> + cam_fill_ataio(&ccb->ataio, >>> 0, >>> adadone, >>> CAM_DIR_NONE | flags, >>> @@ -1911,17 +1902,14 @@ adaspindown(uint8_t cmd, int flags) >>> NULL, >>> 0, >>> ada_default_timeout*1000); >>> + ata_28bit_cmd(&ccb->ataio, cmd, 0, 0, 0); >>> >>> - ata_28bit_cmd(&ccb.ataio, cmd, 0, 0, 0); >>> - xpt_polled_action(&ccb); >>> - >>> - error = cam_periph_error(&ccb, >>> - 0, SF_NO_RECOVERY | SF_NO_RETRY, NULL); >>> - if ((ccb.ccb_h.status & CAM_DEV_QFRZN) != 0) >>> - cam_release_devq(ccb.ccb_h.path, /*relsim_flags*/0, >>> - /*reduction*/0, /*timeout*/0, /*getcount_only*/0); >>> + error = cam_periph_runccb(ccb, adaerror, /*cam_flags*/0, >>> + /*sense_flags*/ SF_NO_RECOVERY | SF_NO_RETRY, >>> + softc->disk->d_devstat); >>> if (error != 0) >>> xpt_print(periph->path, "Spin-down disk failed\n"); >>> + xpt_release_ccb(ccb); >>> cam_periph_unlock(periph); >>> } >>> } >>> >>> Modified: stable/9/sys/cam/scsi/scsi_da.c >>> ============================================================================== >>> --- stable/9/sys/cam/scsi/scsi_da.c Thu Apr 18 09:40:34 2013 >>> (r249610) >>> +++ stable/9/sys/cam/scsi/scsi_da.c Thu Apr 18 09:44:00 2013 >>> (r249611) >>> @@ -2834,11 +2834,10 @@ dashutdown(void * arg, int howto) >>> { >>> struct cam_periph *periph; >>> struct da_softc *softc; >>> + union ccb *ccb; >>> int error; >>> >>> CAM_PERIPH_FOREACH(periph, &dadriver) { >>> - union ccb ccb; >>> - >>> cam_periph_lock(periph); >>> softc = (struct da_softc *)periph->softc; >>> >>> @@ -2852,10 +2851,8 @@ dashutdown(void * arg, int howto) >>> continue; >>> } >>> >>> - xpt_setup_ccb(&ccb.ccb_h, periph->path, CAM_PRIORITY_NORMAL); >>> - >>> - ccb.ccb_h.ccb_state = DA_CCB_DUMP; >>> - scsi_synchronize_cache(&ccb.csio, >>> + ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL); >>> + scsi_synchronize_cache(&ccb->csio, >>> /*retries*/0, >>> /*cbfcnp*/dadone, >>> MSG_SIMPLE_Q_TAG, >>> @@ -2864,15 +2861,12 @@ dashutdown(void * arg, int howto) >>> SSD_FULL_SIZE, >>> 60 * 60 * 1000); >>> >>> - xpt_polled_action(&ccb); >>> - >>> - error = cam_periph_error(&ccb, >>> - 0, SF_NO_RECOVERY | SF_NO_RETRY | SF_QUIET_IR, NULL); >>> - if ((ccb.ccb_h.status & CAM_DEV_QFRZN) != 0) >>> - cam_release_devq(ccb.ccb_h.path, /*relsim_flags*/0, >>> - /*reduction*/0, /*timeout*/0, /*getcount_only*/0); >>> + error = cam_periph_runccb(ccb, daerror, /*cam_flags*/0, >>> + /*sense_flags*/ SF_NO_RECOVERY | SF_NO_RETRY | SF_QUIET_IR, >>> + softc->disk->d_devstat); >>> if (error != 0) >>> xpt_print(periph->path, "Synchronize cache failed\n"); >>> + xpt_release_ccb(ccb); >>> cam_periph_unlock(periph); >>> } >>> } >> > > > -- > Alexander Motin _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
