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

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

[Paolo Bonzini]

This is an alternative implementation of writethrough caching.  By always
opening protocols in writethrough mode and doing flushes manually after
every write, it achieves two results: 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.

In practice, the performance result is a wash.  I measured "make -j3
vmlinux" on a 2-core guest/4-core host, with 2GB memory in the guest
and 8GB in the host.

Performance was measured starting qemu-kvm with an empty qcow2 image,
a virtio disk and cache=writethrough (F16 installation + exploded kernel
tarball in the backing file), and the results are as follows:

    without patch:
    real    9m22.416s user    12m8.955s sys     3m46.331s
    real    9m21.602s user    12m20.124s sys     3m51.126s
    real    9m22.258s user    12m14.900s sys     3m48.595s

    with patch:
    real    9m17.617s user    12m16.837s sys     3m48.637s
    real    9m18.926s user    12m20.083s sys     3m50.458s
    real    9m15.751s user    12m41.944s sys     3m56.447s

Unlike the RFC, this is tested exactly with these seven patches.  The RFC
was tested with follow-up cleanups that hid the problem in patch 3.

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: always open protocol in writeback mode
  block: copy enable_write_cache in bdrv_append
  block: add bdrv_set_enable_write_cache
  ide: support enable/disable write cache
  qcow2: always operate caches in writeback mode

 block.c                |   20 +++++++++++++++++---
 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          |   18 +++++++++++++++---
 savevm.c               |    2 +-
 8 files changed, 39 insertions(+), 51 deletions(-)

[Qemu-devel] [PATCH 1.2 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>
---
 block.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index af2ab4f..7add33c 100644
--- a/block.c
+++ b/block.c
@@ -1747,8 +1747,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);
     }
 
@@ -1797,6 +1797,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);
     }
@@ -1966,6 +1969,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.1

[Qemu-devel] [PATCH 1.2 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>
---
 savevm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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.1

[Qemu-devel] [PATCH 1.2 3/7] block: always open protocol 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.

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

diff --git a/block.c b/block.c
index 7add33c..43b3de6 100644
--- a/block.c
+++ b/block.c
@@ -661,7 +661,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
     if (drv->bdrv_file_open) {
         ret = drv->bdrv_file_open(bs, filename, open_flags);
     } else {
-        ret = bdrv_file_open(&bs->file, filename, open_flags);
+        ret = bdrv_file_open(&bs->file, filename, open_flags | BDRV_O_CACHE_WB);
         if (ret >= 0) {
             ret = drv->bdrv_open(bs, open_flags);
         }
-- 
1.7.1

[Qemu-devel] [PATCH 1.2 4/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>
---
 block.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 43b3de6..b682764 100644
--- a/block.c
+++ b/block.c
@@ -989,6 +989,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.1

[Qemu-devel] [PATCH 1.2 5/7] block: add bdrv_set_enable_write_cache

[Paolo Bonzini]

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

diff --git a/block.c b/block.c
index b682764..bb709c2 100644
--- a/block.c
+++ b/block.c
@@ -2369,6 +2369,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 7408acc..eacdb7b 100644
--- a/block.h
+++ b/block.h
@@ -280,6 +280,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.1

[Qemu-devel] [PATCH 1.2 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 |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 9785d5f..7c50567 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 */
-- 
1.7.1

[Qemu-devel] [PATCH 1.2 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 812c93c..8035fe9 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 655799c..a369c4b 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 93567f6..8a47405 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -296,11 +296,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.1

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

[Kevin Wolf]

Am 22.05.2012 18:17, schrieb Paolo Bonzini:
> This is an alternative implementation of writethrough caching.  By always
> opening protocols in writethrough mode and doing flushes manually after
> every write, it achieves two results: 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.
> 
> In practice, the performance result is a wash.  I measured "make -j3
> vmlinux" on a 2-core guest/4-core host, with 2GB memory in the guest
> and 8GB in the host.
> 
> Performance was measured starting qemu-kvm with an empty qcow2 image,
> a virtio disk and cache=writethrough (F16 installation + exploded kernel
> tarball in the backing file), and the results are as follows:
> 
>     without patch:
>     real    9m22.416s user    12m8.955s sys     3m46.331s
>     real    9m21.602s user    12m20.124s sys     3m51.126s
>     real    9m22.258s user    12m14.900s sys     3m48.595s
> 
>     with patch:
>     real    9m17.617s user    12m16.837s sys     3m48.637s
>     real    9m18.926s user    12m20.083s sys     3m50.458s
>     real    9m15.751s user    12m41.944s sys     3m56.447s
> 
> Unlike the RFC, this is tested exactly with these seven patches.  The RFC
> was tested with follow-up cleanups that hid the problem in patch 3.
> 
> v1->v2: only patch 3 changed, was completely backwards in v1

Are you going to send a v3 for patches 3 and possibly 5/6?

Kevin