[Qemu-devel] [PATCH 0/2] qcow2: Update multiple refcounts at once

[Qemu-devel] [PATCH 0/2] qcow2: Update multiple refcounts at once

[Kevin Wolf]

When updating the refcount of a range of cluste, qcow2 currently processes
single clusters, so for each cluster there is a separate write to the dis
image. This series changes this behaviour to change the refcount of all cluters
in the same refcount block before writing anything to disk, saving some write
operations.

The influence of this change is highest when lots of large request frequently
allocate multiple clusters at once - growing an empty image during FS creation
or installation. The allocation itself is already optimized for this case,
refcount updates are an important missing piece: I trief mke2fs on an empty
image (cache=off) and it went down from 7:15 min to 1:59 min. Even with
writeback caching we still gain a small percentage (with the right qemu-io test
case even a huge one, but I guess this doesn't matter in practice).

I'm fairly confident that this series is correct, it has passed the test
scripts I have here and some manual testing. However, given the recent qcow2
history I don't feel completely comfortable with touching this part of qcow2,
so please run this with all test cases you all have for it. And give it a
thorough review, of course.

Kevin Wolf (2):
  qcow2: Refactor update_refcount
  qcow2: Update multiple refcounts at once

 block/qcow2.c |  118 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 88 insertions(+), 30 deletions(-)

[Qemu-devel] [PATCH 1/2] qcow2: Refactor update_refcount

[Kevin Wolf]

This is a preparation patch with no functional changes. It moves the allocation
of new refcounts block to a new function and makes update_cluster_refcount (for
one cluster) call update_refcount (for multiple clusters) instead the other way
round.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |   86 +++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 77f433e..28f7ffe 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -173,7 +173,7 @@ static int get_refcount(BlockDriverState *bs, int64_t cluster_index);
 static int update_cluster_refcount(BlockDriverState *bs,
                                    int64_t cluster_index,
                                    int addend);
-static void update_refcount(BlockDriverState *bs,
+static int update_refcount(BlockDriverState *bs,
                             int64_t offset, int64_t length,
                             int addend);
 static int64_t alloc_clusters(BlockDriverState *bs, int64_t size);
@@ -2548,29 +2548,25 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size)
     return -EIO;
 }
 
-/* addend must be 1 or -1 */
-/* XXX: cache several refcount block clusters ? */
-static int update_cluster_refcount(BlockDriverState *bs,
-                                   int64_t cluster_index,
-                                   int addend)
+
+static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
 {
     BDRVQcowState *s = bs->opaque;
     int64_t offset, refcount_block_offset;
-    int ret, refcount_table_index, block_index, refcount;
+    int ret, refcount_table_index;
     uint64_t data64;
 
+    /* Find L1 index and grow refcount table if needed */
     refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
     if (refcount_table_index >= s->refcount_table_size) {
-        if (addend < 0)
-            return -EINVAL;
         ret = grow_refcount_table(bs, refcount_table_index + 1);
         if (ret < 0)
             return ret;
     }
+
+    /* Load or allocate the refcount block */
     refcount_block_offset = s->refcount_table[refcount_table_index];
     if (!refcount_block_offset) {
-        if (addend < 0)
-            return -EINVAL;
         /* create a new refcount block */
         /* Note: we cannot update the refcount now to avoid recursion */
         offset = alloc_clusters_noref(bs, s->cluster_size);
@@ -2595,25 +2591,28 @@ static int update_cluster_refcount(BlockDriverState *bs,
                 return -EIO;
         }
     }
-    /* we can update the count and save it */
-    block_index = cluster_index &
-        ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
-    refcount = be16_to_cpu(s->refcount_block_cache[block_index]);
-    refcount += addend;
-    if (refcount < 0 || refcount > 0xffff)
-        return -EINVAL;
-    if (refcount == 0 && cluster_index < s->free_cluster_index) {
-        s->free_cluster_index = cluster_index;
+
+    return refcount_block_offset;
+}
+
+/* addend must be 1 or -1 */
+static int update_cluster_refcount(BlockDriverState *bs,
+                                   int64_t cluster_index,
+                                   int addend)
+{
+    BDRVQcowState *s = bs->opaque;
+    int ret;
+
+    ret = update_refcount(bs, cluster_index << s->cluster_bits, 1, addend);
+    if (ret < 0) {
+        return ret;
     }
-    s->refcount_block_cache[block_index] = cpu_to_be16(refcount);
-    if (bdrv_pwrite(s->hd,
-                    refcount_block_offset + (block_index << REFCOUNT_SHIFT),
-                    &s->refcount_block_cache[block_index], 2) != 2)
-        return -EIO;
-    return refcount;
+
+    return get_refcount(bs, cluster_index);
 }
 
-static void update_refcount(BlockDriverState *bs,
+/* XXX: cache several refcount block clusters ? */
+static int update_refcount(BlockDriverState *bs,
                             int64_t offset, int64_t length,
                             int addend)
 {
@@ -2625,13 +2624,40 @@ static void update_refcount(BlockDriverState *bs,
            offset, length, addend);
 #endif
     if (length <= 0)
-        return;
+        return -EINVAL;
     start = offset & ~(s->cluster_size - 1);
     last = (offset + length - 1) & ~(s->cluster_size - 1);
     for(cluster_offset = start; cluster_offset <= last;
-        cluster_offset += s->cluster_size) {
-        update_cluster_refcount(bs, cluster_offset >> s->cluster_bits, addend);
+        cluster_offset += s->cluster_size)
+    {
+        int64_t refcount_block_offset;
+        int block_index, refcount;
+        int64_t cluster_index = cluster_offset >> s->cluster_bits;
+
+        /* Load the refcount block and allocate it if needed */
+        refcount_block_offset = alloc_refcount_block(bs, cluster_index);
+        if (refcount_block_offset < 0) {
+            return refcount_block_offset;
+        }
+
+        /* we can update the count and save it */
+        block_index = cluster_index &
+            ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
+        refcount = be16_to_cpu(s->refcount_block_cache[block_index]);
+        refcount += addend;
+        if (refcount < 0 || refcount > 0xffff)
+            return -EINVAL;
+        if (refcount == 0 && cluster_index < s->free_cluster_index) {
+            s->free_cluster_index = cluster_index;
+        }
+        s->refcount_block_cache[block_index] = cpu_to_be16(refcount);
+        if (bdrv_pwrite(s->hd,
+                        refcount_block_offset + (block_index << REFCOUNT_SHIFT),
+                        &s->refcount_block_cache[block_index], 2) != 2)
+            return -EIO;
+
     }
+    return 0;
 }
 
 /*
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/2] qcow2: Update multiple refcounts at once

[Kevin Wolf]

Don't write each single changed refcount block entry to the disk after it is
written, but update all entries of the block and write all of them at once.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |   40 ++++++++++++++++++++++++++++++++++++----
 1 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 28f7ffe..b6f7003 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2618,6 +2618,9 @@ static int update_refcount(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     int64_t start, last, cluster_offset;
+    int64_t refcount_block_offset = 0;
+    int64_t table_index = -1, old_table_index;
+    int first_index = -1, last_index = -1;
 
 #ifdef DEBUG_ALLOC2
     printf("update_refcount: offset=%lld size=%lld addend=%d\n",
@@ -2630,10 +2633,25 @@ static int update_refcount(BlockDriverState *bs,
     for(cluster_offset = start; cluster_offset <= last;
         cluster_offset += s->cluster_size)
     {
-        int64_t refcount_block_offset;
         int block_index, refcount;
         int64_t cluster_index = cluster_offset >> s->cluster_bits;
 
+        /* Only write refcount block to disk when we are done with it */
+        old_table_index = table_index;
+        table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
+        if ((old_table_index >= 0) && (table_index != old_table_index)) {
+            size_t size = (last_index - first_index + 1) << REFCOUNT_SHIFT;
+            if (bdrv_pwrite(s->hd,
+                refcount_block_offset + (first_index << REFCOUNT_SHIFT),
+                &s->refcount_block_cache[first_index], size) != size)
+            {
+                return -EIO;
+            }
+
+            first_index = -1;
+            last_index = -1;
+        }
+
         /* Load the refcount block and allocate it if needed */
         refcount_block_offset = alloc_refcount_block(bs, cluster_index);
         if (refcount_block_offset < 0) {
@@ -2643,6 +2661,13 @@ static int update_refcount(BlockDriverState *bs,
         /* we can update the count and save it */
         block_index = cluster_index &
             ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
+        if (first_index == -1 || block_index < first_index) {
+            first_index = block_index;
+        }
+        if (block_index > last_index) {
+            last_index = block_index;
+        }
+
         refcount = be16_to_cpu(s->refcount_block_cache[block_index]);
         refcount += addend;
         if (refcount < 0 || refcount > 0xffff)
@@ -2651,12 +2676,19 @@ static int update_refcount(BlockDriverState *bs,
             s->free_cluster_index = cluster_index;
         }
         s->refcount_block_cache[block_index] = cpu_to_be16(refcount);
+    }
+
+    /* Write last changed block to disk */
+    if (refcount_block_offset != 0) {
+        size_t size = (last_index - first_index + 1) << REFCOUNT_SHIFT;
         if (bdrv_pwrite(s->hd,
-                        refcount_block_offset + (block_index << REFCOUNT_SHIFT),
-                        &s->refcount_block_cache[block_index], 2) != 2)
+            refcount_block_offset + (first_index << REFCOUNT_SHIFT),
+            &s->refcount_block_cache[first_index], size) != size)
+        {
             return -EIO;
-
+        }
     }
+
     return 0;
 }
 
-- 
1.6.0.6