Re: [Qemu-devel] [PATCH 2/2] virtio-blk: do not relay a previous driver's WCE configuration to the current

[Paolo Bonzini]

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