Patchwork [v3,7/7] qcow2: always operate caches in writeback mode

login
register
mail settings
Submitter Paolo Bonzini
Date June 5, 2012, 10:04 p.m.
Message ID <1338933895-20677-8-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/163186/
State New
Headers show

Comments

Paolo Bonzini - June 5, 2012, 10:04 p.m.
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(-)
Kevin Wolf - June 8, 2012, 1:59 p.m.
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
Paolo Bonzini - June 11, 2012, 6:38 a.m.
> Maybe we should add flushes in the right places in
> higher level functions like bdrv_snapshot_create().

Yes, that would be better.

Paolo

Patch

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);