[Qemu-devel] [PATCH v3 0/7] Manual writethrough cache and cache mode toggle

[Qemu-devel] [PATCH v3 0/7] Manual writethrough cache and cache mode toggle

[Paolo Bonzini]

This is v3 of the alternative implementation of writethrough caching
for QEMU 1.2.  By always opening drivers in writethrough mode and
doing flushes manually after every write, it achieves three objectives:
1) it makes flipping the cache mode extremely easy; 2) it lets formats
control flushes during metadata updates even in writethrough mode,
which makes the updates more efficient; 3) it makes cache=writethrough
automatically flush metadata without needing extra work in the formats.

The last point should also make implementation of "QED mode" a little
bit simpler.

v2->v3: patch 3 changed again to always add the flag.  Patches reordered,
    the new order is better now that BDRV_O_CACHE_WB is added to all
    BlockDriverStates now.

v1->v2: only patch 3 changed, was completely backwards in v1


Paolo Bonzini (7):
  block: flush in writethrough mode after writes
  savevm: flush after saving vm state
  block: copy enable_write_cache in bdrv_append
  block: add bdrv_set_enable_write_cache
  block: always open drivers in writeback mode
  ide: support enable/disable write cache
  qcow2: always operate caches in writeback mode

 block.c                |   29 +++++++++++++++++++++++++----
 block.h                |    1 +
 block/qcow2-cache.c    |   25 ++-----------------------
 block/qcow2-refcount.c |   12 ------------
 block/qcow2.c          |    7 ++-----
 block/qcow2.h          |    5 +----
 hw/ide/core.c          |   21 ++++++++++++++++++---
 savevm.c               |    2 +-
 8 files changed, 50 insertions(+), 52 deletions(-)

-- 
1.7.10.1

[Qemu-devel] [PATCH v3 1/7] block: flush in writethrough mode after writes

[Paolo Bonzini]

We want to make the formats handle their own flushes
autonomously, while keeping for guests the ability to use a writethrough
cache.  Since formats will write metadata via bs->file, bdrv_co_do_writev
is the only place where we need to add a flush.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 85ef6af..7538112 100644
--- a/block.c
+++ b/block.c
@@ -1758,8 +1758,8 @@ int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
         return ret;
     }
 
-    /* No flush needed for cache modes that use O_DSYNC */
-    if ((bs->open_flags & BDRV_O_CACHE_WB) != 0) {
+    /* No flush needed for cache modes that already do it */
+    if (bs->enable_write_cache) {
         bdrv_flush(bs);
     }
 
@@ -1808,6 +1808,9 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs,
         ret = bdrv_co_do_write_zeroes(bs, cluster_sector_num,
                                       cluster_nb_sectors);
     } else {
+        /* This does not change the data on the disk, it is not necessary
+         * to flush even in cache=writethrough mode.
+         */
         ret = drv->bdrv_co_writev(bs, cluster_sector_num, cluster_nb_sectors,
                                   &bounce_qiov);
     }
@@ -1977,6 +1980,10 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
         ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
     }
 
+    if (ret == 0 && !bs->enable_write_cache) {
+        ret = bdrv_co_flush(bs);
+    }
+
     if (bs->dirty_bitmap) {
         set_dirty_bitmap(bs, sector_num, nb_sectors, 1);
     }
-- 
1.7.10.1

[Qemu-devel] [PATCH v3 2/7] savevm: flush after saving vm state

[Paolo Bonzini]

Writing vm state uses bdrv_pwrite, so it will automatically get flushes
in writethrough mode.  But doing a flush at the end in writeback mode
is probably a good idea anyway.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 savevm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/savevm.c b/savevm.c
index 2d18bab..2b6833d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -400,7 +400,7 @@ static int block_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
 
 static int bdrv_fclose(void *opaque)
 {
-    return 0;
+    return bdrv_flush(opaque);
 }
 
 static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
-- 
1.7.10.1

[Qemu-devel] [PATCH v3 3/7] block: copy enable_write_cache in bdrv_append

[Paolo Bonzini]

Because the guest will be able to flip enable_write_cache, the actual
state may not match what is used to open the new snapshot.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index 7538112..9bff401 100644
--- a/block.c
+++ b/block.c
@@ -1000,6 +1000,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
     tmp.buffer_alignment  = bs_top->buffer_alignment;
     tmp.copy_on_read      = bs_top->copy_on_read;
 
+    tmp.enable_write_cache = bs_top->enable_write_cache;
+
     /* i/o timing parameters */
     tmp.slice_time        = bs_top->slice_time;
     tmp.slice_start       = bs_top->slice_start;
-- 
1.7.10.1

[Qemu-devel] [PATCH v3 4/7] block: add bdrv_set_enable_write_cache

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |    5 +++++
 block.h |    1 +
 2 files changed, 6 insertions(+)

diff --git a/block.c b/block.c
index 9bff401..e4396a6 100644
--- a/block.c
+++ b/block.c
@@ -2380,6 +2380,11 @@ int bdrv_enable_write_cache(BlockDriverState *bs)
     return bs->enable_write_cache;
 }
 
+void bdrv_set_enable_write_cache(BlockDriverState *bs, bool wce)
+{
+    bs->enable_write_cache = wce;
+}
+
 int bdrv_is_encrypted(BlockDriverState *bs)
 {
     if (bs->backing_hd && bs->backing_hd->encrypted)
diff --git a/block.h b/block.h
index 574981d..43bfd99 100644
--- a/block.h
+++ b/block.h
@@ -291,6 +291,7 @@ BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
 int bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_is_sg(BlockDriverState *bs);
 int bdrv_enable_write_cache(BlockDriverState *bs);
+void bdrv_set_enable_write_cache(BlockDriverState *bs, bool wce);
 int bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
 void bdrv_lock_medium(BlockDriverState *bs, bool locked);
-- 
1.7.10.1

[Qemu-devel] [PATCH v3 5/7] block: always open drivers in writeback mode

[Paolo Bonzini]

Formats are entirely in charge of flushes for metadata writes.  For
guest-initiated writes, a writethrough cache is faked in the block layer.
So we can always open in writeback mode.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index e4396a6..48528fd 100644
--- a/block.c
+++ b/block.c
@@ -649,12 +649,13 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
     bs->opaque = g_malloc0(drv->instance_size);
 
     bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
+    open_flags = flags | BDRV_O_CACHE_WB;
 
     /*
      * Clear flags that are internal to the block layer before opening the
      * image.
      */
-    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
 
     /*
      * Snapshots should be writable.
-- 
1.7.10.1

[Qemu-devel] [PATCH v3 6/7] ide: support enable/disable write cache

[Paolo Bonzini]

Enabling or disabling the write cache is done with the SET FEATURES
command.  The command can be issued with sg_sat_set_features from
sg3-utils.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 9785d5f..f28229a 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1047,6 +1047,7 @@ static bool ide_cmd_permitted(IDEState *s, uint32_t cmd)
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val)
 {
+    uint16_t *identify_data;
     IDEState *s;
     int n;
     int lba48 = 0;
@@ -1231,10 +1232,21 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
             goto abort_cmd;
         /* XXX: valid for CDROM ? */
         switch(s->feature) {
-        case 0xcc: /* reverting to power-on defaults enable */
-        case 0x66: /* reverting to power-on defaults disable */
         case 0x02: /* write cache enable */
+            bdrv_set_enable_write_cache(s->bs, true);
+            identify_data = (uint16_t *)s->identify_data;
+            put_le16(identify_data + 85, (1 << 14) | (1 << 5) | 1);
+            s->status = READY_STAT | SEEK_STAT;
+            ide_set_irq(s->bus);
+            break;
         case 0x82: /* write cache disable */
+            bdrv_set_enable_write_cache(s->bs, false);
+            identify_data = (uint16_t *)s->identify_data;
+            put_le16(identify_data + 85, (1 << 14) | 1);
+            ide_flush_cache(s);
+            break;
+        case 0xcc: /* reverting to power-on defaults enable */
+        case 0x66: /* reverting to power-on defaults disable */
         case 0xaa: /* read look-ahead enable */
         case 0x55: /* read look-ahead disable */
         case 0x05: /* set advanced power management mode */
@@ -1250,7 +1262,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
             break;
         case 0x03: { /* set transfer mode */
 		uint8_t val = s->nsector & 0x07;
-            uint16_t *identify_data = (uint16_t *)s->identify_data;
+		identify_data = (uint16_t *)s->identify_data;
 
 		switch (s->nsector >> 3) {
 		case 0x00: /* pio default */
@@ -2146,6 +2158,9 @@ static int ide_drive_post_load(void *opaque, int version_id)
             s->cdrom_changed = 1;
         }
     }
+    if (s->identify_set) {
+        bdrv_set_enable_write_cache(s->bs, !!(s->identify_data[85] & (1 << 5)));
+    }
     return 0;
 }
 
-- 
1.7.10.1

[Qemu-devel] [PATCH v3 7/7] qcow2: always operate caches in writeback mode

[Paolo Bonzini]

Writethrough does not need special-casing anymore in the qcow2 caches.
The block layer adds flushes after every guest-initiated data write,
and these will also flush the qcow2 caches to the OS.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qcow2-cache.c    |   25 ++-----------------------
 block/qcow2-refcount.c |   12 ------------
 block/qcow2.c          |    7 ++-----
 block/qcow2.h          |    5 +----
 4 files changed, 5 insertions(+), 44 deletions(-)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 710d4b1..2d4322a 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -40,11 +40,9 @@ struct Qcow2Cache {
     struct Qcow2Cache*      depends;
     int                     size;
     bool                    depends_on_flush;
-    bool                    writethrough;
 };
 
-Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
-    bool writethrough)
+Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables)
 {
     BDRVQcowState *s = bs->opaque;
     Qcow2Cache *c;
@@ -53,7 +51,6 @@ Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
     c = g_malloc0(sizeof(*c));
     c->size = num_tables;
     c->entries = g_malloc0(sizeof(*c->entries) * num_tables);
-    c->writethrough = writethrough;
 
     for (i = 0; i < c->size; i++) {
         c->entries[i].table = qemu_blockalign(bs, s->cluster_size);
@@ -307,12 +304,7 @@ found:
     *table = NULL;
 
     assert(c->entries[i].ref >= 0);
-
-    if (c->writethrough) {
-        return qcow2_cache_entry_flush(bs, c, i);
-    } else {
-        return 0;
-    }
+    return 0;
 }
 
 void qcow2_cache_entry_mark_dirty(Qcow2Cache *c, void *table)
@@ -329,16 +321,3 @@ void qcow2_cache_entry_mark_dirty(Qcow2Cache *c, void *table)
 found:
     c->entries[i].dirty = true;
 }
-
-bool qcow2_cache_set_writethrough(BlockDriverState *bs, Qcow2Cache *c,
-    bool enable)
-{
-    bool old = c->writethrough;
-
-    if (!old && enable) {
-        qcow2_cache_flush(bs, c);
-    }
-
-    c->writethrough = enable;
-    return old;
-}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 5d6ea72..66f3915 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -726,13 +726,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     int64_t old_offset, old_l2_offset;
     int i, j, l1_modified = 0, nb_csectors, refcount;
     int ret;
-    bool old_l2_writethrough, old_refcount_writethrough;
-
-    /* Switch caches to writeback mode during update */
-    old_l2_writethrough =
-        qcow2_cache_set_writethrough(bs, s->l2_table_cache, false);
-    old_refcount_writethrough =
-        qcow2_cache_set_writethrough(bs, s->refcount_block_cache, false);
 
     l2_table = NULL;
     l1_table = NULL;
@@ -856,11 +849,6 @@ fail:
         qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
     }
 
-    /* Enable writethrough cache mode again */
-    qcow2_cache_set_writethrough(bs, s->l2_table_cache, old_l2_writethrough);
-    qcow2_cache_set_writethrough(bs, s->refcount_block_cache,
-        old_refcount_writethrough);
-
     /* Update L1 only if it isn't deleted anyway (addend = -1) */
     if (addend >= 0 && l1_modified) {
         for(i = 0; i < l1_size; i++)
diff --git a/block/qcow2.c b/block/qcow2.c
index d66de58..57fd43d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -220,7 +220,6 @@ static int qcow2_open(BlockDriverState *bs, int flags)
     int len, i, ret = 0;
     QCowHeader header;
     uint64_t ext_end;
-    bool writethrough;
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
@@ -367,10 +366,8 @@ static int qcow2_open(BlockDriverState *bs, int flags)
     }
 
     /* alloc L2 table/refcount block cache */
-    writethrough = ((flags & BDRV_O_CACHE_WB) == 0);
-    s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE, writethrough);
-    s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE,
-        writethrough);
+    s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE);
+    s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE);
 
     s->cluster_cache = g_malloc(s->cluster_size);
     /* one more sector for decompressed data alignment */
diff --git a/block/qcow2.h b/block/qcow2.h
index c6e7237..455b6d7 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -297,11 +297,8 @@ void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs);
 
 /* qcow2-cache.c functions */
-Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
-    bool writethrough);
+Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
 int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
-bool qcow2_cache_set_writethrough(BlockDriverState *bs, Qcow2Cache *c,
-    bool enable);
 
 void qcow2_cache_entry_mark_dirty(Qcow2Cache *c, void *table);
 int qcow2_cache_flush(BlockDriverState *bs, Qcow2Cache *c);
-- 
1.7.10.1

Re: [Qemu-devel] [PATCH v3 7/7] qcow2: always operate caches in writeback mode

[Kevin Wolf]

Am 06.06.2012 00:04, schrieb Paolo Bonzini:
> Writethrough does not need special-casing anymore in the qcow2 caches.
> The block layer adds flushes after every guest-initiated data write,
> and these will also flush the qcow2 caches to the OS.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 5d6ea72..66f3915 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -726,13 +726,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>      int64_t old_offset, old_l2_offset;
>      int i, j, l1_modified = 0, nb_csectors, refcount;
>      int ret;
> -    bool old_l2_writethrough, old_refcount_writethrough;
> -
> -    /* Switch caches to writeback mode during update */
> -    old_l2_writethrough =
> -        qcow2_cache_set_writethrough(bs, s->l2_table_cache, false);
> -    old_refcount_writethrough =
> -        qcow2_cache_set_writethrough(bs, s->refcount_block_cache, false);
>  
>      l2_table = NULL;
>      l1_table = NULL;
> @@ -856,11 +849,6 @@ fail:
>          qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
>      }
>  
> -    /* Enable writethrough cache mode again */
> -    qcow2_cache_set_writethrough(bs, s->l2_table_cache, old_l2_writethrough);
> -    qcow2_cache_set_writethrough(bs, s->refcount_block_cache,
> -        old_refcount_writethrough);
> -

Here you're dropping an implicit cache flush. Not sure if it really
matters, though. Maybe we should add flushes in the right places in
higher level functions like bdrv_snapshot_create().

Kevin

Re: [Qemu-devel] [PATCH v3 0/7] Manual writethrough cache and cache mode toggle

[Stefan Hajnoczi]

On Tue, Jun 5, 2012 at 11:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This is v3 of the alternative implementation of writethrough caching
> for QEMU 1.2.  By always opening drivers in writethrough mode and

s/writethrough/writeback/

> doing flushes manually after every write, it achieves three objectives:

Stefan

Re: [Qemu-devel] [PATCH v3 0/7] Manual writethrough cache and cache mode toggle

[Stefan Hajnoczi]

On Tue, Jun 5, 2012 at 11:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This is v3 of the alternative implementation of writethrough caching
> for QEMU 1.2.  By always opening drivers in writethrough mode and
> doing flushes manually after every write, it achieves three objectives:
> 1) it makes flipping the cache mode extremely easy; 2) it lets formats
> control flushes during metadata updates even in writethrough mode,
> which makes the updates more efficient; 3) it makes cache=writethrough
> automatically flush metadata without needing extra work in the formats.
>
> The last point should also make implementation of "QED mode" a little
> bit simpler.
>
> v2->v3: patch 3 changed again to always add the flag.  Patches reordered,
>    the new order is better now that BDRV_O_CACHE_WB is added to all
>    BlockDriverStates now.
>
> v1->v2: only patch 3 changed, was completely backwards in v1
>
>
> Paolo Bonzini (7):
>  block: flush in writethrough mode after writes
>  savevm: flush after saving vm state
>  block: copy enable_write_cache in bdrv_append
>  block: add bdrv_set_enable_write_cache
>  block: always open drivers in writeback mode
>  ide: support enable/disable write cache
>  qcow2: always operate caches in writeback mode
>
>  block.c                |   29 +++++++++++++++++++++++++----
>  block.h                |    1 +
>  block/qcow2-cache.c    |   25 ++-----------------------
>  block/qcow2-refcount.c |   12 ------------
>  block/qcow2.c          |    7 ++-----
>  block/qcow2.h          |    5 +----
>  hw/ide/core.c          |   21 ++++++++++++++++++---
>  savevm.c               |    2 +-
>  8 files changed, 50 insertions(+), 52 deletions(-)

Could you run sequential and random read and write tests with fio on a
raw image file, LVM volume, or partition?  Those raw cases perform
best and I'm curious if pwritev()+fdatasync() is noticably different
than open(..., O_DSYNC)+pwritev().  I understand why this is a nice
change for image formats, but for raw we need to make sure there is no
regression.

Stefan

Re: [Qemu-devel] [PATCH v3 7/7] qcow2: always operate caches in writeback mode

[Paolo Bonzini]

> Maybe we should add flushes in the right places in
> higher level functions like bdrv_snapshot_create().

Yes, that would be better.

Paolo