[Qemu-devel] [PATCH 0/2] Serious bugs in the handling of writeback caching

[Qemu-devel] [PATCH 0/2] Serious bugs in the handling of writeback caching

[Paolo Bonzini]

The first bug is simply that in 1.6.0 the default was changed by mistake
to cache=unsafe.  This affects all block drivers.  It is fixed in the
first patch.

The second is that Linux guests that support the CONFIG_WCE option were
mistaken into thinking that caching was writethrough, and thus did not
send any flush.  The bug is in QEMU, and is fixed by the second patch.

Paolo Bonzini (2):
  blockdev: do not default cache.no-flush to true
  virtio-blk: do not relay a previous driver's WCE configuration to the
    current

 blockdev.c                     | 2 +-
 hw/block/virtio-blk.c          | 8 ++++++--
 include/hw/virtio/virtio-blk.h | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] blockdev: do not default cache.no-flush to true

[Paolo Bonzini]

That's why all my VMs were so fast lately. :)

This changed in 1.6.0 by mistake in patch 29c4e2b (blockdev: Split up
'cache' option, 2013-07-18).

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 07dac05..76bfe30 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -443,7 +443,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if (qemu_opt_get_bool(opts, "cache.direct", false)) {
         bdrv_flags |= BDRV_O_NOCACHE;
     }
-    if (qemu_opt_get_bool(opts, "cache.no-flush", true)) {
+    if (qemu_opt_get_bool(opts, "cache.no-flush", false)) {
         bdrv_flags |= BDRV_O_NO_FLUSH;
     }
 
-- 
1.8.3.1

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

[Paolo Bonzini]

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.

Hence, whenever the guest is reset, the cache mode of the disk should
be reset to whatever was specified in the "-drive" option.  With this
change, the Linux virtio-blk driver finds that writeback caching is
enabled, and tells the block layer to send cache flush commands
appropriately.

Reported-by: Rusty Russell <rusty@au1.ibm.com
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/virtio-blk.c          | 8 ++++++--
 include/hw/virtio/virtio-blk.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e2f55cc..6ed9666 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -460,9 +460,9 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running,
 
 static void virtio_blk_reset(VirtIODevice *vdev)
 {
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     if (s->dataplane) {
         virtio_blk_data_plane_stop(s->dataplane);
     }
@@ -473,6 +473,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
      * are per-device request lists.
      */
     bdrv_drain_all();
+    bdrv_set_enable_write_cache(s->bs, s->original_wce);
 }
 
 /* coalesce internal state, copy to pci i/o region 0
@@ -564,7 +565,10 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
     }
 
     features = vdev->guest_features;
-    bdrv_set_enable_write_cache(s->bs, !!(features & (1 << VIRTIO_BLK_F_WCE)));
+    if (!(features & (1 << VIRTIO_BLK_F_CONFIG_WCE))) {
+        bdrv_set_enable_write_cache(s->bs,
+                                    !!(features & (1 << VIRTIO_BLK_F_WCE)));
+    }
 }
 
 static void virtio_blk_save(QEMUFile *f, void *opaque)
@@ -674,6 +678,7 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
     }
 
     blkconf_serial(&blk->conf, &blk->serial);
+    s->original_wce = bdrv_enable_write_cache(blk->conf.bs);
     if (blkconf_geometry(&blk->conf, NULL, 65535, 255, 255) < 0) {
         return -1;
     }
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index b87cf49..41885da 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -123,6 +123,7 @@ typedef struct VirtIOBlock {
     BlockConf *conf;
     VirtIOBlkConf blk;
     unsigned short sector_mask;
+    bool original_wce;
     VMChangeStateEntry *change;
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     Notifier migration_state_notifier;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] blockdev: do not default cache.no-flush to true

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On 09/19/2013 10:48 AM, Paolo Bonzini wrote:
> That's why all my VMs were so fast lately. :)
> 
> This changed in 1.6.0 by mistake in patch 29c4e2b (blockdev: Split up
> 'cache' option, 2013-07-18).

And my name is tied to that one as a reviewer.  Sorry for letting it
through in the first place.

> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/blockdev.c b/blockdev.c
> index 07dac05..76bfe30 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -443,7 +443,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
>      if (qemu_opt_get_bool(opts, "cache.direct", false)) {
>          bdrv_flags |= BDRV_O_NOCACHE;
>      }
> -    if (qemu_opt_get_bool(opts, "cache.no-flush", true)) {
> +    if (qemu_opt_get_bool(opts, "cache.no-flush", false)) {
>          bdrv_flags |= BDRV_O_NO_FLUSH;
>      }
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

On 09/19/2013 10:48 AM, Paolo Bonzini wrote:
> The following sequence happens:

> Hence, whenever the guest is reset, the cache mode of the disk should
> be reset to whatever was specified in the "-drive" option.  With this
> change, the Linux virtio-blk driver finds that writeback caching is
> enabled, and tells the block layer to send cache flush commands
> appropriately.
> 
> Reported-by: Rusty Russell <rusty@au1.ibm.com
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/virtio-blk.c          | 8 ++++++--
>  include/hw/virtio/virtio-blk.h | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

>  
>      features = vdev->guest_features;
> -    bdrv_set_enable_write_cache(s->bs, !!(features & (1 << VIRTIO_BLK_F_WCE)));
> +    if (!(features & (1 << VIRTIO_BLK_F_CONFIG_WCE))) {
> +        bdrv_set_enable_write_cache(s->bs,
> +                                    !!(features & (1 << VIRTIO_BLK_F_WCE)));
> +    }
>  }
>  
>  static void virtio_blk_save(QEMUFile *f, void *opaque)
> @@ -674,6 +678,7 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
>      }
>  
>      blkconf_serial(&blk->conf, &blk->serial);
> +    s->original_wce = bdrv_enable_write_cache(blk->conf.bs);

At first, I was worried that this does 'bool = int', and whether that
was correct in all cases.  But looking further, bdrv_enable_write_cache
merely returns bs->enable_write_cache (also an int), but that all
assignments to bs->enable_write_cache are careful to only assign 0 or 1.
 A followup patch that changes the types to bool might be in order, but
doesn't invalidate this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Kevin Wolf]

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? I guess making a host feature flag dynamic was
a bad idea to start with.

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.

Kevin

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

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

[Paolo Bonzini]

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.

Hence, whenever the guest is reset, the cache mode of the disk should
be reset to whatever was specified in the "-drive" option.  With this
change, the Linux virtio-blk driver finds that writeback caching is
enabled, and tells the block layer to send cache flush commands
appropriately.

Reported-by: Rusty Russell <rusty@au1.ibm.com
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/virtio-blk.c          | 24 ++++++++++++++++++++++--
 include/hw/virtio/virtio-blk.h |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e2f55cc..49a23c3 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -460,9 +460,9 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running,
 
 static void virtio_blk_reset(VirtIODevice *vdev)
 {
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     if (s->dataplane) {
         virtio_blk_data_plane_stop(s->dataplane);
     }
@@ -473,6 +473,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
      * are per-device request lists.
      */
     bdrv_drain_all();
+    bdrv_set_enable_write_cache(s->bs, s->original_wce);
 }
 
 /* coalesce internal state, copy to pci i/o region 0
@@ -564,7 +565,25 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
     }
 
     features = vdev->guest_features;
-    bdrv_set_enable_write_cache(s->bs, !!(features & (1 << VIRTIO_BLK_F_WCE)));
+
+    /* A guest that supports VIRTIO_BLK_F_CONFIG_WCE must be able to send
+     * cache flushes.  Thus, the "auto writethrough" behavior is never
+     * necessary for guests that support the VIRTIO_BLK_F_CONFIG_WCE feature.
+     * Leaving it enabled would break the following sequence:
+     *
+     *     Guest started with "-drive cache=writethrough"
+     *     Guest sets status to 0
+     *     Guest sets DRIVER bit in status field
+     *     Guest reads host features (WCE=0, CONFIG_WCE=1)
+     *     Guest writes guest features (WCE=0, CONFIG_WCE=1)
+     *     Guest writes 1 to the WCE configuration field (writeback mode)
+     *     Guest sets DRIVER_OK bit in status field
+     *
+     * s->bs would erroneously be placed in writethrough mode.
+     */
+    if (!(features & (1 << VIRTIO_BLK_F_CONFIG_WCE))) {
+        bdrv_set_enable_write_cache(s->bs, !!(features & (1 << VIRTIO_BLK_F_WCE)));
+    }
 }
 
 static void virtio_blk_save(QEMUFile *f, void *opaque)
@@ -674,6 +693,7 @@ static int virtio_blk_device_init(VirtIODevice *vdev)
     }
 
     blkconf_serial(&blk->conf, &blk->serial);
+    s->original_wce = bdrv_enable_write_cache(blk->conf.bs);
     if (blkconf_geometry(&blk->conf, NULL, 65535, 255, 255) < 0) {
         return -1;
     }
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index b87cf49..41885da 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -123,6 +123,7 @@ typedef struct VirtIOBlock {
     BlockConf *conf;
     VirtIOBlkConf blk;
     unsigned short sector_mask;
+    bool original_wce;
     VMChangeStateEntry *change;
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     Notifier migration_state_notifier;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] Serious bugs in the handling of writeback caching

[Stefan Hajnoczi]

On Thu, Sep 19, 2013 at 06:48:52PM +0200, Paolo Bonzini wrote:
> The first bug is simply that in 1.6.0 the default was changed by mistake
> to cache=unsafe.  This affects all block drivers.  It is fixed in the
> first patch.
> 
> The second is that Linux guests that support the CONFIG_WCE option were
> mistaken into thinking that caching was writethrough, and thus did not
> send any flush.  The bug is in QEMU, and is fixed by the second patch.
> 
> Paolo Bonzini (2):
>   blockdev: do not default cache.no-flush to true
>   virtio-blk: do not relay a previous driver's WCE configuration to the
>     current
> 
>  blockdev.c                     | 2 +-
>  hw/block/virtio-blk.c          | 8 ++++++--
>  include/hw/virtio/virtio-blk.h | 1 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan