From owner-svn-src-stable@FreeBSD.ORG Tue Apr 23 07:09:21 2013 Return-Path: Delivered-To: svn-src-stable@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 232746A5; Tue, 23 Apr 2013 07:09:21 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-ee0-f53.google.com (mail-ee0-f53.google.com [74.125.83.53]) by mx1.freebsd.org (Postfix) with ESMTP id 5C8D31C20; Tue, 23 Apr 2013 07:09:19 +0000 (UTC) Received: by mail-ee0-f53.google.com with SMTP id d17so90268eek.26 for ; Tue, 23 Apr 2013 00:09:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:sender:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=0uppvLtrGWrwssiS87K25MWNwuBSHWi31pTGBM1ySTw=; b=EDLD/8n6VU/qnTi+fvK2TT+1PGdpU53iNZlT8s+9PyWNUXFqyLqsybTqJwvnuPHIKB 7IJ+xJISYhppor5JuDQ8HrVp3wLKisX0k6LL6K+Zp4TdWgYlzoCbS/ykekQCWtilXl6O ALm6MQZEA6PMKcG68uvd+6iSke22hMeYayramFgQp8jsErVPiGjD21QD/zaPzdym0ZSC YMIbKNSIEM0SH135MwfmUybiR27BTyuPFPtUqeqceDQISq5qln7PSnWKDJnsKGouV2v7 /zJ4/UhrUrQlWppI/fxC7rbbdC41KniohuucgFdHoKtF3E+pNpkGjSMJEfMt2P4R4liu Wz2Q== X-Received: by 10.15.95.74 with SMTP id bc50mr23279475eeb.36.1366700570894; Tue, 23 Apr 2013 00:02:50 -0700 (PDT) Received: from mavbook.mavhome.dp.ua (mavhome.mavhome.dp.ua. [213.227.240.37]) by mx.google.com with ESMTPS id j43sm10325915eep.4.2013.04.23.00.02.48 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 23 Apr 2013 00:02:49 -0700 (PDT) Sender: Alexander Motin Message-ID: <51763216.5000101@FreeBSD.org> Date: Tue, 23 Apr 2013 10:02:46 +0300 From: Alexander Motin User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:17.0) Gecko/20130413 Thunderbird/17.0.5 MIME-Version: 1.0 To: Scott Long Subject: Re: svn commit: r249611 - in stable/9/sys/cam: ata scsi References: <201304180944.r3I9i05t093967@svn.freebsd.org> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: svn-src-stable@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Peter Wemm X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Apr 2013 07:09:21 -0000 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. 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? 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 > On Apr 18, 2013, at 3:44 AM, Alexander Motin 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