[Qemu-devel] [PATCH v2 0/6] qcow2: cache flush fixes and performance improvements

[Qemu-devel] [PATCH v2 0/6] qcow2: cache flush fixes and performance improvements

[Stefan Hajnoczi]

Users have reported that qcow2 internal snapshot creation is slow.  I filed a
bug report at https://bugs.launchpad.net/qemu/+bug/1126369.

This patch series reduces the time for qcow2 internal snapshot creation
significantly, from more than 3 minutes to under 1 second.

In the process of removing unnecessary cache flushes, I also stumbled upon
instances of bdrv_flush(bs->file) where we really want to flush metadata
updates.  Since qcow2 caches metadata this actually does not write out the
metadata updates to disk!  The fix is either bdrv_flush(bs) or a more specific
cache flush (e.g. refcount block cache).

This series passes qemu-iotests.

v2:
 * Use qcow2_cache_set_dependency() in qcow2_alloc_bytes() instead of forcing a flush [Kevin]
 * Add bdrv_flush() error code paths [Kevin]

Stefan Hajnoczi (6):
  qcow2: flush refcount cache correctly in alloc_refcount_block()
  qcow2: flush refcount cache correctly in qcow2_write_snapshots()
  qcow2: set L2 cache dependency in qcow2_alloc_bytes()
  qcow2: flush in qcow2_update_snapshot_refcount()
  qcow2: drop flush in update_cluster_refcount()
  qcow2: drop unnecessary flush in qcow2_update_snapshot_refcount()

 block/qcow2-refcount.c | 24 ++++++++++++++----------
 block/qcow2-snapshot.c | 10 ++++------
 2 files changed, 18 insertions(+), 16 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 1/6] qcow2: flush refcount cache correctly in alloc_refcount_block()

[Stefan Hajnoczi]

update_refcount() affects the refcount cache, it does not write to disk.
Therefore bdrv_flush(bs->file) does nothing.  We need to flush the
refcount cache in order to write out the refcount updates!

While we're here also add error returns when qcow2_cache_flush() fails.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-refcount.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 55543ed..e8b5d0a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -201,7 +201,10 @@ static int alloc_refcount_block(BlockDriverState *bs,
     *refcount_block = NULL;
 
     /* We write to the refcount table, so we might depend on L2 tables */
-    qcow2_cache_flush(bs, s->l2_table_cache);
+    ret = qcow2_cache_flush(bs, s->l2_table_cache);
+    if (ret < 0) {
+        return ret;
+    }
 
     /* Allocate the refcount block itself and mark it as used */
     int64_t new_block = alloc_clusters_noref(bs, s->cluster_size);
@@ -237,7 +240,10 @@ static int alloc_refcount_block(BlockDriverState *bs,
             goto fail_block;
         }
 
-        bdrv_flush(bs->file);
+        ret = qcow2_cache_flush(bs, s->refcount_block_cache);
+        if (ret < 0) {
+            goto fail_block;
+        }
 
         /* Initialize the new refcount block only after updating its refcount,
          * update_refcount uses the refcount cache itself */
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 2/6] qcow2: flush refcount cache correctly in qcow2_write_snapshots()

[Stefan Hajnoczi]

Since qcow2 metadata is cached we need to flush the caches, not just the
underlying file.  Use bdrv_flush(bs) instead of bdrv_flush(bs->file).

Also add the error return path when bdrv_flush() fails and move the
flush after checking for qcow2_alloc_clusters() failure so that the
qcow2_alloc_clusters() error return value takes precedence.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-snapshot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index eb8fcd5..f4719d9 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -180,11 +180,14 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 
     /* Allocate space for the new snapshot list */
     snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
-    bdrv_flush(bs->file);
     offset = snapshots_offset;
     if (offset < 0) {
         return offset;
     }
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        return ret;
+    }
 
     /* Write all snapshots to the new list */
     for(i = 0; i < s->nb_snapshots; i++) {
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 3/6] qcow2: set L2 cache dependency in qcow2_alloc_bytes()

[Stefan Hajnoczi]

Compressed writes use qcow2_alloc_bytes() to allocate space with byte
granularity.  The affected clusters' refcounts will be incremented but
we do not need to flush yet.

Set a L2 cache dependency on the refcount block cache, so that the
refcounts get written out before the L2 updates.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-refcount.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e8b5d0a..4d9df5f 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -669,7 +669,11 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
         }
     }
 
-    bdrv_flush(bs->file);
+    /* The cluster refcount was incremented, either by qcow2_alloc_clusters()
+     * or explicitly by update_cluster_refcount().  Refcount blocks must be
+     * flushed before the caller's L2 table updates.
+     */
+    qcow2_cache_set_dependency(bs, s->l2_table_cache, s->refcount_block_cache);
     return offset;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 4/6] qcow2: flush in qcow2_update_snapshot_refcount()

[Stefan Hajnoczi]

Users of qcow2_update_snapshot_refcount() do not flush consistently.
qcow2_snapshot_create() flushes but qcow2_snapshot_goto() and
qcow2_snapshot_delete() do not.

Solve this by moving the bdrv_flush() into
qcow2_update_snapshot_refcount().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-refcount.c | 2 +-
 block/qcow2-snapshot.c | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 4d9df5f..3d29d30 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -851,7 +851,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
         }
     }
 
-    ret = 0;
+    ret = bdrv_flush(bs);
 fail:
     if (l2_table) {
         qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index f4719d9..992a5c8 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -381,11 +381,6 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         goto fail;
     }
 
-    ret = bdrv_flush(bs);
-    if (ret < 0) {
-        goto fail;
-    }
-
     /* Append the new snapshot to the snapshot list */
     new_snapshot_list = g_malloc((s->nb_snapshots + 1) * sizeof(QCowSnapshot));
     if (s->snapshots) {
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 5/6] qcow2: drop flush in update_cluster_refcount()

[Stefan Hajnoczi]

The update_cluster_refcount() function increments/decrements a cluster's
refcount and then returns the new refcount value.

There is no need to flush since both update_cluster_refcount() callers
already take care of this:

1. qcow2_alloc_bytes() calls update_cluster_refcount() when compressed
   sectors will be appended to an existing cluster with enough free
   space.  qcow2_alloc_bytes() already flushes so there is no need to do
   so in update_cluster_refcount().

2. qcow2_update_snapshot_refcount() sets a cache dependency on refcounts
   if it needs to update L2 entries.  It also flushes before completing.

Removing this flush significantly speeds up qcow2 snapshot creation:

  $ qemu-img create -f qcow2 test.qcow2 -o size=50G,preallocation=metadata
  $ time qemu-img snapshot -c new test.qcow2

Time drops from more than 3 minutes to under 1 second.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-refcount.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3d29d30..92519ea 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -532,8 +532,6 @@ static int update_cluster_refcount(BlockDriverState *bs,
         return ret;
     }
 
-    bdrv_flush(bs->file);
-
     return get_refcount(bs, cluster_index);
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 6/6] qcow2: drop unnecessary flush in qcow2_update_snapshot_refcount()

[Stefan Hajnoczi]

We already flush when the function completes.  There is no need to flush
after every compressed cluster.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-refcount.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 92519ea..9bfb390 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -790,10 +790,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                             if (ret < 0) {
                                 goto fail;
                             }
-
-                            /* TODO Flushing once for the whole function should
-                             * be enough */
-                            bdrv_flush(bs->file);
                         }
                         /* compressed clusters are never modified */
                         refcount = 2;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 0/6] qcow2: cache flush fixes and performance improvements

[Kevin Wolf]

Am 04.03.2013 um 15:02 hat Stefan Hajnoczi geschrieben:
> Users have reported that qcow2 internal snapshot creation is slow.  I filed a
> bug report at https://bugs.launchpad.net/qemu/+bug/1126369.
> 
> This patch series reduces the time for qcow2 internal snapshot creation
> significantly, from more than 3 minutes to under 1 second.
> 
> In the process of removing unnecessary cache flushes, I also stumbled upon
> instances of bdrv_flush(bs->file) where we really want to flush metadata
> updates.  Since qcow2 caches metadata this actually does not write out the
> metadata updates to disk!  The fix is either bdrv_flush(bs) or a more specific
> cache flush (e.g. refcount block cache).
> 
> This series passes qemu-iotests.

Thanks, applied all to the block branch.

Kevin