From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53846) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VMxwo-000671-Ba for qemu-devel@nongnu.org; Fri, 20 Sep 2013 06:28:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VMxwi-0000Z4-CP for qemu-devel@nongnu.org; Fri, 20 Sep 2013 06:28:26 -0400 Message-ID: <523C234D.2020003@redhat.com> Date: Fri, 20 Sep 2013 12:28:29 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1379609334-20811-1-git-send-email-pbonzini@redhat.com> <1379609334-20811-3-git-send-email-pbonzini@redhat.com> <20130920095424.GD2800@dhcp-200-207.str.redhat.com> In-Reply-To: <20130920095424.GD2800@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] virtio-blk: do not relay a previous driver's WCE configuration to the current List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: rusty@au1.ibm.com, qemu-devel@nongnu.org, stefanha@redhat.com, qemu-stable@nongnu.org Il 20/09/2013 11:54, Kevin Wolf ha scritto: > Am 19.09.2013 um 18:48 hat Paolo Bonzini geschrieben: >> The following sequence happens: >> - the SeaBIOS virtio-blk driver does not support the WCE feature, which >> causes QEMU to disable writeback caching >> >> - the Linux virtio-blk driver resets the device, finds WCE is available >> but writeback caching is disabled; tells block layer to not send cache >> flush commands >> >> - the Linux virtio-blk driver sets the DRIVER_OK bit, which causes >> writeback caching to be re-enabled, but the Linux virtio-blk driver does >> not know of this side effect and cache flushes remain disabled >> >> The bug is at the third step. If the guest does know about CONFIG_WCE, >> QEMU should ignore the WCE feature's state. The guest will control the >> cache mode solely using configuration space. This change makes Linux >> do flushes correctly, but Linux will keep SeaBIOS's writethrough mode. > > This sounds fishy. The solutions happens to make recent Linux kernels do > the right thing, but wouldn't drivers that don't know CONFIG_WCE still > fall into the same trap? No, drivers that don't know CONFIG_WCE will do the following: 1) -drive cache=writethrough case, WCE supported When the driver resets the device, QEMU disables the write cache (virtio_blk_reset). Thus VIRTIO_BLK_F_WCE is not advertised. The Linux virtio-blk driver tells the block layer to not send cache flush commands, which is correct because they are useless. VIRTIO_BLK_F_WCE is obviously not negotiated, and virtio_blk_set_status confirms the disk in writethrough mode. 2) -drive cache=writeback case, WCE supported When the driver resets the device, QEMU disables the write cache (virtio_blk_reset). Thus VIRTIO_BLK_F_WCE is advertised by the device and negotiated by the driver. The Linux virtio-blk driver recognizes that VIRTIO_BLK_F_WCE is negotiated and tells the block layer to send cache flush commands. virtio_blk_set_status confirms the disk in writeback mode. 3) -drive cache=writethrough case, WCE not supported When the driver resets the device, QEMU disables the write cache (virtio_blk_reset). Thus VIRTIO_BLK_F_WCE is not advertised. The virtio-blk driver doesn't do anything. virtio_blk_set_status confirms the disk in writethrough mode. 4) -drive cache=writeback case, WCE not supported When the driver resets the device, QEMU disables the write cache (virtio_blk_reset). Thus VIRTIO_BLK_F_WCE is advertised by the device, but not negotiated by the driver. The virtio-blk driver doesn't do anything. virtio_blk_set_status places the disk in writethrough mode. > I guess making a host feature flag dynamic was > a bad idea to start with. I disagree, it is very useful. The bug was unfortunate indeed, and probably happened due to testing the two patches (CONFIG_WCE and no-WCE-implies-writethrough) independently rather than together. > Perhaps we should restrict the magic to disabling WCE in case the guest > doesn't have VIRTIO_BLK_F_WCE, but never allow it to enable WCE even > though we've already advertised that the host doesn't have WCE. That's already what happens, because (thanks to the new "bdrv_set_enable_write_cache(s->bs, s->original_wce);" at reset time) VIRTIO_BLK_F_WCE is never exposed in writethrough mode. Paolo