[Qemu-devel] [QEMU PATCH 0/2] virtio-blk: writeback cache enable improvements

[Qemu-devel] [QEMU PATCH 0/2] virtio-blk: writeback cache enable improvements

[Paolo Bonzini]

These patches let virtio-blk use the new support for toggling the cache
mode between writethrough and writeback.

The first patch introduces a new feature bit and configuration field to
do this.  The second patch disables writeback caching for guests that do
not negotiate VIRTIO_BLK_F_WCACHE (meaning that they cannot send flush
requests), so that limited or older guests are now safe wrt power losses.
VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and was
backported to RHEL/CentOS 5.6 (in 2010).

The Windows drivers (which work by emulating SCSI on top of virtio-blk)
have bugs in this area, which I reported on the Red Hat Bugzilla as
bugs 837321 and 837324.  With these patches they will suffer a
performance hit but gain correctness.

Paolo Bonzini (2):
  virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE
  virtio-blk: disable write cache if not negotiated

 hw/virtio-blk.c |   30 ++++++++++++++++++++++++++++--
 hw/virtio-blk.h |    4 +++-
 2 files changed, 31 insertions(+), 3 deletions(-)

-- 
1.7.10.2

[Qemu-devel] [PATCH 1/2] virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE

[Paolo Bonzini]

Introduce a new feature bit and configuration field that provide
support for toggling the cache mode between writethrough and writeback.

Also rename VIRTIO_BLK_F_WCACHE to VIRTIO_BLK_F_WCE for consistency with
the spec.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-blk.c |   16 ++++++++++++++--
 hw/virtio-blk.h |    4 +++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index fe07746..280f96d 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -510,9 +510,19 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     blkcfg.size_max = 0;
     blkcfg.physical_block_exp = get_physical_block_exp(s->conf);
     blkcfg.alignment_offset = 0;
+    blkcfg.wce = bdrv_enable_write_cache(s->bs);
     memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
 }
 
+static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
+{
+    VirtIOBlock *s = to_virtio_blk(vdev);
+    struct virtio_blk_config blkcfg;
+
+    memcpy(&blkcfg, config, sizeof(blkcfg));
+    bdrv_set_enable_write_cache(s->bs, blkcfg.wce != 0);
+}
+
 static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
 {
     VirtIOBlock *s = to_virtio_blk(vdev);
@@ -523,9 +533,10 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
     features |= (1 << VIRTIO_BLK_F_BLK_SIZE);
     features |= (1 << VIRTIO_BLK_F_SCSI);
 
+    features |= (1 << VIRTIO_BLK_F_CONFIG_WCE);
     if (bdrv_enable_write_cache(s->bs))
-        features |= (1 << VIRTIO_BLK_F_WCACHE);
-    
+        features |= (1 << VIRTIO_BLK_F_WCE);
+
     if (bdrv_is_read_only(s->bs))
         features |= 1 << VIRTIO_BLK_F_RO;
 
@@ -615,6 +626,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
                                           sizeof(VirtIOBlock));
 
     s->vdev.get_config = virtio_blk_update_config;
+    s->vdev.set_config = virtio_blk_set_config;
     s->vdev.get_features = virtio_blk_get_features;
     s->vdev.reset = virtio_blk_reset;
     s->bs = blk->conf.bs;
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index d785001..afea114 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -31,8 +31,9 @@
 #define VIRTIO_BLK_F_BLK_SIZE   6       /* Block size of disk is available*/
 #define VIRTIO_BLK_F_SCSI       7       /* Supports scsi command passthru */
 /* #define VIRTIO_BLK_F_IDENTIFY   8       ATA IDENTIFY supported, DEPRECATED */
-#define VIRTIO_BLK_F_WCACHE     9       /* write cache enabled */
+#define VIRTIO_BLK_F_WCE        9       /* write cache enabled */
 #define VIRTIO_BLK_F_TOPOLOGY   10      /* Topology information is available */
+#define VIRTIO_BLK_F_CONFIG_WCE 11      /* write cache configurable */
 
 #define VIRTIO_BLK_ID_BYTES     20      /* ID string length */
 
@@ -49,6 +50,7 @@ struct virtio_blk_config
     uint8_t alignment_offset;
     uint16_t min_io_size;
     uint32_t opt_io_size;
+    uint8_t wce;
 } QEMU_PACKED;
 
 /* These two define direction. */
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH 1/2] virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE

[Kevin Wolf]

Am 03.07.2012 15:20, schrieb Paolo Bonzini:
> Introduce a new feature bit and configuration field that provide
> support for toggling the cache mode between writethrough and writeback.
> 
> Also rename VIRTIO_BLK_F_WCACHE to VIRTIO_BLK_F_WCE for consistency with
> the spec.

My spec (and my kernel as well) call it VIRTIO_BLK_F_FLUSH.

What's the status of the kernel and spec side of the change?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio-blk.c |   16 ++++++++++++++--
>  hw/virtio-blk.h |    4 +++-
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index fe07746..280f96d 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -510,9 +510,19 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      blkcfg.size_max = 0;
>      blkcfg.physical_block_exp = get_physical_block_exp(s->conf);
>      blkcfg.alignment_offset = 0;
> +    blkcfg.wce = bdrv_enable_write_cache(s->bs);
>      memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
>  }
>  
> +static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> +{
> +    VirtIOBlock *s = to_virtio_blk(vdev);
> +    struct virtio_blk_config blkcfg;
> +
> +    memcpy(&blkcfg, config, sizeof(blkcfg));
> +    bdrv_set_enable_write_cache(s->bs, blkcfg.wce != 0);
> +}

We need to call bdrv_flush() here when turning WCE off. And it seems we
don't have a way to signal failure, or may we just leave the bit unchanged?

> @@ -49,6 +50,7 @@ struct virtio_blk_config
>      uint8_t alignment_offset;
>      uint16_t min_io_size;
>      uint32_t opt_io_size;
> +    uint8_t wce;
>  } QEMU_PACKED;

If the spec isn't set in stone yet, we could make it a flags field
instead of using a whole byte for a single flag.

Kevin

Re: [Qemu-devel] [PATCH 1/2] virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE

[Paolo Bonzini]

Il 03/07/2012 15:46, Kevin Wolf ha scritto:
>> > Introduce a new feature bit and configuration field that provide
>> > support for toggling the cache mode between writethrough and writeback.
>> > 
>> > Also rename VIRTIO_BLK_F_WCACHE to VIRTIO_BLK_F_WCE for consistency with
>> > the spec.
> My spec (and my kernel as well) call it VIRTIO_BLK_F_FLUSH.
> 
> What's the status of the kernel and spec side of the change?

Both posted.  The spec patch that introduces VIRTIO_BLK_F_CONFIG_WCE
also renames it to VIRTIO_BLK_F_WCE, since that's really what it does.
See this old comment in the kernel (not in the latest git anymore):

       /*
        * If the FLUSH feature is supported we do have support for
        * flushing a volatile write cache on the host.  [...]
        * otherwise, we must assume that the host does not
        * perform any kind of volatile write caching.
        */

Paolo

[Qemu-devel] [PATCH 2/2] virtio-blk: disable write cache if not negotiated

[Paolo Bonzini]

If the guest does not support flushes, we should run in writethrough mode.
The setting is temporary until the next reset, so that for example the
BIOS will run in writethrough mode while Linux will run with a writeback
cache.

VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and
was backported to RHEL/CentOS 5.6 (in 2010).  The Windows drivers have
two bugs, which I reported on the Red Hat Bugzilla as bugs 837321 and
837324.  With these patches they will suffer a performance hit but
gain correctness.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-blk.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 280f96d..500e026 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -543,6 +543,19 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
     return features;
 }
 
+static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
+{
+    VirtIOBlock *s = to_virtio_blk(vdev);
+    uint32_t features;
+
+    if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return;
+    }
+
+    features = vdev->guest_features;
+    bdrv_set_enable_write_cache(s->bs, !!(features & (1 << VIRTIO_BLK_F_WCE)));
+}
+
 static void virtio_blk_save(QEMUFile *f, void *opaque)
 {
     VirtIOBlock *s = opaque;
@@ -628,6 +641,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.set_config = virtio_blk_set_config;
     s->vdev.get_features = virtio_blk_get_features;
+    s->vdev.set_status = virtio_blk_set_status;
     s->vdev.reset = virtio_blk_reset;
     s->bs = blk->conf.bs;
     s->conf = &blk->conf;
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: disable write cache if not negotiated

[Kevin Wolf]

Am 03.07.2012 15:20, schrieb Paolo Bonzini:
> If the guest does not support flushes, we should run in writethrough mode.
> The setting is temporary until the next reset, so that for example the
> BIOS will run in writethrough mode while Linux will run with a writeback
> cache.
> 
> VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and
> was backported to RHEL/CentOS 5.6 (in 2010).  The Windows drivers have
> two bugs, which I reported on the Red Hat Bugzilla as bugs 837321 and
> 837324.  With these patches they will suffer a performance hit but
> gain correctness.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I generally like the idea for a default, but doesn't this override even
an explicit cache=writeback? Are we sure that we want this?

Kevin

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: disable write cache if not negotiated

[Paolo Bonzini]

Il 03/07/2012 15:49, Kevin Wolf ha scritto:
>> If the guest does not support flushes, we should run in writethrough mode.
>> > The setting is temporary until the next reset, so that for example the
>> > BIOS will run in writethrough mode while Linux will run with a writeback
>> > cache.
>> > 
>> > VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and
>> > was backported to RHEL/CentOS 5.6 (in 2010).  The Windows drivers have
>> > two bugs, which I reported on the Red Hat Bugzilla as bugs 837321 and
>> > 837324.  With these patches they will suffer a performance hit but
>> > gain correctness.
>> > 
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> I generally like the idea for a default, but doesn't this override even
> an explicit cache=writeback?

Yes.  It doesn't override cache=unsafe though.

> Are we sure that we want this?

The idea is that this change will overcome Anthony's objections to
switching the default to writeback...

Paolo

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: disable write cache if not negotiated

[Kevin Wolf]

Am 03.07.2012 15:51, schrieb Paolo Bonzini:
> Il 03/07/2012 15:49, Kevin Wolf ha scritto:
>>> If the guest does not support flushes, we should run in writethrough mode.
>>>> The setting is temporary until the next reset, so that for example the
>>>> BIOS will run in writethrough mode while Linux will run with a writeback
>>>> cache.
>>>>
>>>> VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and
>>>> was backported to RHEL/CentOS 5.6 (in 2010).  The Windows drivers have
>>>> two bugs, which I reported on the Red Hat Bugzilla as bugs 837321 and
>>>> 837324.  With these patches they will suffer a performance hit but
>>>> gain correctness.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> I generally like the idea for a default, but doesn't this override even
>> an explicit cache=writeback?
> 
> Yes.  It doesn't override cache=unsafe though.

When the guest doesn't support flushes, cache=writeback is equivalent to
cache=unsafe, so if you want the old behaviour back you can switch to
cache=unsafe without additional risks.

We don't have a cache=directunsafe, though, so if you want to get the
old behaviour of cache=none back, you're out of luck. Not sure how
acceptable this is.

Irrespective of this concern I've come to the conclusion that I agree
and we actually must enforce this for non-unsafe mode, and not doing it
is a bug.

Kevin

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: disable write cache if not negotiated

[Paolo Bonzini]

Il 04/07/2012 12:16, Kevin Wolf ha scritto:
>> > Yes.  It doesn't override cache=unsafe though.
> When the guest doesn't support flushes, cache=writeback is equivalent to
> cache=unsafe, so if you want the old behaviour back you can switch to
> cache=unsafe without additional risks.
> 
> We don't have a cache=directunsafe, though, so if you want to get the
> old behaviour of cache=none back, you're out of luck. Not sure how
> acceptable this is.

If we want to fix this, let's take the occasion to split the parameters
into cache=on/off (well, we have that already), flush=on/off, and a
device-side wce=on/off.

> Irrespective of this concern I've come to the conclusion that I agree
> and we actually must enforce this for non-unsafe mode, and not doing it
> is a bug.

Thanks!  Is that an Acked-by/Reviewed-by? :)

Paolo

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: disable write cache if not negotiated

[Kevin Wolf]

Am 04.07.2012 14:21, schrieb Paolo Bonzini:
> Il 04/07/2012 12:16, Kevin Wolf ha scritto:
>>>> Yes.  It doesn't override cache=unsafe though.
>> When the guest doesn't support flushes, cache=writeback is equivalent to
>> cache=unsafe, so if you want the old behaviour back you can switch to
>> cache=unsafe without additional risks.
>>
>> We don't have a cache=directunsafe, though, so if you want to get the
>> old behaviour of cache=none back, you're out of luck. Not sure how
>> acceptable this is.
> 
> If we want to fix this, let's take the occasion to split the parameters
> into cache=on/off (well, we have that already), flush=on/off, and a
> device-side wce=on/off.

You're volunteering? Great! ;-)

>> Irrespective of this concern I've come to the conclusion that I agree
>> and we actually must enforce this for non-unsafe mode, and not doing it
>> is a bug.
> 
> Thanks!  Is that an Acked-by/Reviewed-by? :)

Before merging the patches (or actually this patch, I think patch 1 is
fairly independent), I'd like to hear more opinions on whether we need
the cache parameter split first. But as far as the hardware is
concerned, sure, take it as an Acked-by and go forward with the spec and
kernel side of things.

Kevin

Re: [Qemu-devel] [PATCH 2/2] virtio-blk: disable write cache if not negotiated

[Paolo Bonzini]

Il 04/07/2012 14:50, Kevin Wolf ha scritto:
> Before merging the patches (or actually this patch, I think patch 1 is
> fairly independent),

Yes, it is.

> I'd like to hear more opinions on whether we need
> the cache parameter split first

Ok, let's discuss it next week on the KVM/QEMU call.  Getting the cache
parameter split before 1.2 is going to be hard, but who knows.

> But as far as the hardware is
> concerned, sure, take it as an Acked-by and go forward with the spec and
> kernel side of things.

Rusty already committed everything, so it's too late! :)  (Sorry for the
tabs vs. spaces BTW).

Paolo

Re: [Qemu-devel] [QEMU PATCH 0/2] virtio-blk: writeback cache enable improvements

[Paolo Bonzini]

Il 03/07/2012 15:20, Paolo Bonzini ha scritto:
> These patches let virtio-blk use the new support for toggling the cache
> mode between writethrough and writeback.
> 
> The first patch introduces a new feature bit and configuration field to
> do this.  The second patch disables writeback caching for guests that do
> not negotiate VIRTIO_BLK_F_WCACHE (meaning that they cannot send flush
> requests), so that limited or older guests are now safe wrt power losses.
> VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and was
> backported to RHEL/CentOS 5.6 (in 2010).
> 
> The Windows drivers (which work by emulating SCSI on top of virtio-blk)
> have bugs in this area, which I reported on the Red Hat Bugzilla as
> bugs 837321 and 837324.  With these patches they will suffer a
> performance hit but gain correctness.
> 
> Paolo Bonzini (2):
>   virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE
>   virtio-blk: disable write cache if not negotiated

Ping - Anthony, mst?

Paolo

Re: [Qemu-devel] [QEMU PATCH 0/2] virtio-blk: writeback cache enable improvements

[Paolo Bonzini]

Il 23/07/2012 18:32, Paolo Bonzini ha scritto:
> Il 03/07/2012 15:20, Paolo Bonzini ha scritto:
>> > These patches let virtio-blk use the new support for toggling the cache
>> > mode between writethrough and writeback.
>> > 
>> > The first patch introduces a new feature bit and configuration field to
>> > do this.  The second patch disables writeback caching for guests that do
>> > not negotiate VIRTIO_BLK_F_WCACHE (meaning that they cannot send flush
>> > requests), so that limited or older guests are now safe wrt power losses.
>> > VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and was
>> > backported to RHEL/CentOS 5.6 (in 2010).
>> > 
>> > The Windows drivers (which work by emulating SCSI on top of virtio-blk)
>> > have bugs in this area, which I reported on the Red Hat Bugzilla as
>> > bugs 837321 and 837324.  With these patches they will suffer a
>> > performance hit but gain correctness.
>> > 
>> > Paolo Bonzini (2):
>> >   virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE
>> >   virtio-blk: disable write cache if not negotiated
> Ping - Anthony, mst?

Ping^2, so we can switch writethrough->writeback in 1.2.

Paolo

Re: [Qemu-devel] [QEMU PATCH 0/2] virtio-blk: writeback cache enable improvements

[Kevin Wolf]

Am 01.08.2012 17:52, schrieb Paolo Bonzini:
> Il 23/07/2012 18:32, Paolo Bonzini ha scritto:
>> Il 03/07/2012 15:20, Paolo Bonzini ha scritto:
>>>> These patches let virtio-blk use the new support for toggling the cache
>>>> mode between writethrough and writeback.
>>>>
>>>> The first patch introduces a new feature bit and configuration field to
>>>> do this.  The second patch disables writeback caching for guests that do
>>>> not negotiate VIRTIO_BLK_F_WCACHE (meaning that they cannot send flush
>>>> requests), so that limited or older guests are now safe wrt power losses.
>>>> VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and was
>>>> backported to RHEL/CentOS 5.6 (in 2010).
>>>>
>>>> The Windows drivers (which work by emulating SCSI on top of virtio-blk)
>>>> have bugs in this area, which I reported on the Red Hat Bugzilla as
>>>> bugs 837321 and 837324.  With these patches they will suffer a
>>>> performance hit but gain correctness.
>>>>
>>>> Paolo Bonzini (2):
>>>>   virtio-blk: support VIRTIO_BLK_F_CONFIG_WCE
>>>>   virtio-blk: disable write cache if not negotiated
>> Ping - Anthony, mst?
> 
> Ping^2, so we can switch writethrough->writeback in 1.2.

Speak now or forever hold your peace. If there are no more comments
until Friday, I'll just apply it.

Kevin