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

[Qemu-devel] [RFC PATCH 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 patches:
        real    9m25.057s user  12m11.091s sys  3m48.281s
        real    9m23.429s user  11m58.628s sys  3m47.125s
        real    9m23.524s user  12m2.458s sys   3m44.722s

        with patches:
        real    9m25.808s user  12m16.543s sys  3m50.648s
        real    9m22.711s user  12m12.172s sys  3m49.426s
        real    9m21.516s user  12m18.127s sys  3m50.762s

So 1%-2% more CPU usage was measured in the guest, but that doesn't make
much sense for virtio with ioeventfd, so I assume it is all within noise.

Any opinions?  Should I run any more tests, perhaps with cache=directsync?
Does performance of cache=writethrough matter much, especially if we flip
the default?

Thanks,

Paolo

Paolo Bonzini (7):
  block: flush in writethrough mode after writes
  block: flush in writethrough mode after snapshot operations
  savevm: flush after saving vm state
  block: do not pass BDRV_O_CACHE_WB to the protocol
  block: copy enable_write_cache in bdrv_append
  block: add bdrv_set_enable_write_cache
  ide: support enable/disable write cache
  block: do not handle writethrough in qcow2 caches

 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 +-
 9 files changed, 39 insertions(+), 51 deletions(-)

-- 
1.7.10.1

[Qemu-devel] [RFC PATCH 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 file 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.10.1

[Qemu-devel] [RFC PATCH 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>
---
        Kind of unrelated, found by inspection.

 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] [RFC PATCH 3/7] block: do not pass BDRV_O_CACHE_WB to the protocol

[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 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 3db7150..b3d0054 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.10.1

Re: [Qemu-devel] [RFC PATCH 3/7] block: do not pass BDRV_O_CACHE_WB to the protocol

[Stefan Hajnoczi]

On Fri, May 18, 2012 at 3:18 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 3db7150..b3d0054 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);

Do you really want to open image files with O_DSYNC?  For example,
when I try these patches with -drive
if=virtio,file=test.img,cache=none I find that the image file file
descriptor has O_DSYNC set!  That would make cache=none equivalent to
cache=directsync.

Stefan

Re: [Qemu-devel] [RFC PATCH 3/7] block: do not pass BDRV_O_CACHE_WB to the protocol

[Paolo Bonzini]

Il 23/05/2012 14:06, Stefan Hajnoczi ha scritto:
>> > diff --git a/block.c b/block.c
>> > index 3db7150..b3d0054 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);
> Do you really want to open image files with O_DSYNC?  For example,
> when I try these patches with -drive
> if=virtio,file=test.img,cache=none I find that the image file file
> descriptor has O_DSYNC set!  That would make cache=none equivalent to
> cache=directsync.

See the revised version I posted yesterday, this patch is bonkers. :)

What I wanted is "| BDRV_O_CACHE_WB".

Paolo

[Qemu-devel] [RFC PATCH 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 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index b3d0054..e89b35a 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.10.1

[Qemu-devel] [RFC PATCH 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(+)

diff --git a/block.c b/block.c
index e89b35a..47b4a43 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 37a3369..fbb4609 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.10.1

[Qemu-devel] [RFC PATCH 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 file 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.10.1

[Qemu-devel] [RFC PATCH 7/7] block: do not handle writethrough in qcow2 caches

[Paolo Bonzini]

Let qcow2 caches operate always in writeback mode.  The block layer
adds flushes after every guest-initiated data write, and these will
also flush the qcow2 caches to disk.

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