Date: Wed, 24 Apr 2013 17:26:59 -0700 From: Scott Long <scott4long@yahoo.com> To: Alexander Motin <mav@FreeBSD.org> Cc: svn-src-stable@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Peter Wemm <peter@wemm.org> Subject: Re: svn commit: r249611 - in stable/9/sys/cam: ata scsi Message-ID: <76B6FC68-C395-49A8-AAF6-45B64E4C0A83@yahoo.com> In-Reply-To: <51763216.5000101@FreeBSD.org> References: <201304180944.r3I9i05t093967@svn.freebsd.org> <A1E6FDCB-5BC0-49D8-8DF6-546E7D845279@yahoo.com> <51763216.5000101@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Apr 23, 2013, at 12:02 AM, Alexander Motin <mav@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. >=20 > 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. >=20 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. =46rom 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? >=20 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 >=20 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 <mav@FreeBSD.org> wrote: >>=20 >>> Author: mav >>> Date: Thu Apr 18 09:44:00 2013 >>> New Revision: 249611 >>> URL: http://svnweb.freebsd.org/changeset/base/249611 >>>=20 >>> 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. >>>=20 >>> Modified: >>> stable/9/sys/cam/ata/ata_da.c >>> stable/9/sys/cam/scsi/scsi_da.c >>> Directory Properties: >>> stable/9/sys/ (props changed) >>>=20 >>> Modified: stable/9/sys/cam/ata/ata_da.c >>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>> --- 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; >>>=20 >>> 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; >>> } >>>=20 >>> - xpt_setup_ccb(&ccb.ccb_h, periph->path, = CAM_PRIORITY_NORMAL); >>> - >>> - ccb.ccb_h.ccb_state =3D ADA_CCB_DUMP; >>> - cam_fill_ataio(&ccb.ataio, >>> + ccb =3D 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); >>>=20 >>> - error =3D cam_periph_error(&ccb, >>> - 0, SF_NO_RECOVERY | SF_NO_RETRY, NULL); >>> - if ((ccb.ccb_h.status & CAM_DEV_QFRZN) !=3D 0) >>> - cam_release_devq(ccb.ccb_h.path, = /*relsim_flags*/0, >>> - /*reduction*/0, /*timeout*/0, = /*getcount_only*/0); >>> + error =3D cam_periph_runccb(ccb, adaerror, = /*cam_flags*/0, >>> + /*sense_flags*/ SF_NO_RECOVERY | SF_NO_RETRY, >>> + softc->disk->d_devstat); >>> if (error !=3D 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; >>>=20 >>> 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"); >>>=20 >>> - xpt_setup_ccb(&ccb.ccb_h, periph->path, = CAM_PRIORITY_NORMAL); >>> - >>> - ccb.ccb_h.ccb_state =3D ADA_CCB_DUMP; >>> - cam_fill_ataio(&ccb.ataio, >>> + ccb =3D 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); >>>=20 >>> - ata_28bit_cmd(&ccb.ataio, cmd, 0, 0, 0); >>> - xpt_polled_action(&ccb); >>> - >>> - error =3D cam_periph_error(&ccb, >>> - 0, SF_NO_RECOVERY | SF_NO_RETRY, NULL); >>> - if ((ccb.ccb_h.status & CAM_DEV_QFRZN) !=3D 0) >>> - cam_release_devq(ccb.ccb_h.path, = /*relsim_flags*/0, >>> - /*reduction*/0, /*timeout*/0, = /*getcount_only*/0); >>> + error =3D cam_periph_runccb(ccb, adaerror, = /*cam_flags*/0, >>> + /*sense_flags*/ SF_NO_RECOVERY | SF_NO_RETRY, >>> + softc->disk->d_devstat); >>> if (error !=3D 0) >>> xpt_print(periph->path, "Spin-down disk = failed\n"); >>> + xpt_release_ccb(ccb); >>> cam_periph_unlock(periph); >>> } >>> } >>>=20 >>> Modified: stable/9/sys/cam/scsi/scsi_da.c >>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>> --- 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; >>>=20 >>> CAM_PERIPH_FOREACH(periph, &dadriver) { >>> - union ccb ccb; >>> - >>> cam_periph_lock(periph); >>> softc =3D (struct da_softc *)periph->softc; >>>=20 >>> @@ -2852,10 +2851,8 @@ dashutdown(void * arg, int howto) >>> continue; >>> } >>>=20 >>> - xpt_setup_ccb(&ccb.ccb_h, periph->path, = CAM_PRIORITY_NORMAL); >>> - >>> - ccb.ccb_h.ccb_state =3D DA_CCB_DUMP; >>> - scsi_synchronize_cache(&ccb.csio, >>> + ccb =3D 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); >>>=20 >>> - xpt_polled_action(&ccb); >>> - >>> - error =3D cam_periph_error(&ccb, >>> - 0, SF_NO_RECOVERY | SF_NO_RETRY | SF_QUIET_IR, = NULL); >>> - if ((ccb.ccb_h.status & CAM_DEV_QFRZN) !=3D 0) >>> - cam_release_devq(ccb.ccb_h.path, = /*relsim_flags*/0, >>> - /*reduction*/0, /*timeout*/0, = /*getcount_only*/0); >>> + error =3D 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 !=3D 0) >>> xpt_print(periph->path, "Synchronize cache = failed\n"); >>> + xpt_release_ccb(ccb); >>> cam_periph_unlock(periph); >>> } >>> } >>=20 >=20 >=20 > --=20 > Alexander Motin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?76B6FC68-C395-49A8-AAF6-45B64E4C0A83>