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
