[Qemu-devel] [PATCH] qcow2: Cache refcount blocks during snapshot creation

[Qemu-devel] [PATCH] qcow2: Cache refcount blocks during snapshot creation

[Kevin Wolf]

The really time consuming part of snapshotting is to adjust the reference count
of all clusters. Currently after each adjusted cluster the refcount block is
written to disk.

Don't write each single byte immediately to disk but cache all writes to the
refcount block and write them out once we're done with the block.

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

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index dd6e293..d42c6e6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -31,6 +31,26 @@ static int update_refcount(BlockDriverState *bs,
                             int64_t offset, int64_t length,
                             int addend);
 
+
+static int cache_refcount_updates = 0;
+
+static int write_refcount_block(BDRVQcowState *s)
+{
+    size_t size = s->cluster_size;
+
+    if (s->refcount_block_cache_offset == 0) {
+        return 0;
+    }
+
+    if (bdrv_pwrite(s->hd, s->refcount_block_cache_offset,
+            s->refcount_block_cache, size) != size)
+    {
+        return -EIO;
+    }
+
+    return 0;
+}
+
 /*********************************************************/
 /* refcount handling */
 
@@ -68,6 +88,11 @@ static int load_refcount_block(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     int ret;
+
+    if (cache_refcount_updates) {
+        write_refcount_block(s);
+    }
+
     ret = bdrv_pread(s->hd, refcount_block_offset, s->refcount_block_cache,
                      s->cluster_size);
     if (ret != s->cluster_size)
@@ -169,6 +194,7 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
     int64_t offset, refcount_block_offset;
     int ret, refcount_table_index;
     uint64_t data64;
+    int cache = cache_refcount_updates;
 
     /* Find L1 index and grow refcount table if needed */
     refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
@@ -181,6 +207,10 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
     /* Load or allocate the refcount block */
     refcount_block_offset = s->refcount_table[refcount_table_index];
     if (!refcount_block_offset) {
+        if (cache_refcount_updates) {
+            write_refcount_block(s);
+            cache_refcount_updates = 0;
+        }
         /* create a new refcount block */
         /* Note: we cannot update the refcount now to avoid recursion */
         offset = alloc_clusters_noref(bs, s->cluster_size);
@@ -199,6 +229,7 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
         refcount_block_offset = offset;
         s->refcount_block_cache_offset = offset;
         update_refcount(bs, offset, s->cluster_size, 1);
+        cache_refcount_updates = cache;
     } else {
         if (refcount_block_offset != s->refcount_block_cache_offset) {
             if (load_refcount_block(bs, refcount_block_offset) < 0)
@@ -215,6 +246,10 @@ static int write_refcount_block_entries(BDRVQcowState *s,
 {
     size_t size;
 
+    if (cache_refcount_updates) {
+        return 0;
+    }
+
     first_index &= ~(REFCOUNTS_PER_SECTOR - 1);
     last_index = (last_index + REFCOUNTS_PER_SECTOR)
         & ~(REFCOUNTS_PER_SECTOR - 1);
@@ -471,6 +506,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     int l2_size, i, j, l1_modified, l2_modified, nb_csectors, refcount;
 
     qcow2_l2_cache_reset(bs);
+    cache_refcount_updates = 1;
 
     l2_table = NULL;
     l1_table = NULL;
@@ -563,11 +599,15 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     if (l1_allocated)
         qemu_free(l1_table);
     qemu_free(l2_table);
+    cache_refcount_updates = 0;
+    write_refcount_block(s);
     return 0;
  fail:
     if (l1_allocated)
         qemu_free(l1_table);
     qemu_free(l2_table);
+    cache_refcount_updates = 0;
+    write_refcount_block(s);
     return -EIO;
 }
 
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH] qcow2: Cache refcount blocks during snapshot creation

[Nathan Froyd]

On Fri, Jun 26, 2009 at 08:19:38PM +0200, Kevin Wolf wrote:
> +    if (s->refcount_block_cache_offset == 0) {
> +        return 0;
> +    }
> +
> +    if (bdrv_pwrite(s->hd, s->refcount_block_cache_offset,
> +            s->refcount_block_cache, size) != size)
> +    {

Nit: bad formatting for opening brace here.

-Nathan

Re: [Qemu-devel] [PATCH] qcow2: Cache refcount blocks during snapshot creation

[Kevin Wolf]

Nathan Froyd schrieb:
> On Fri, Jun 26, 2009 at 08:19:38PM +0200, Kevin Wolf wrote:
>> +    if (s->refcount_block_cache_offset == 0) {
>> +        return 0;
>> +    }
>> +
>> +    if (bdrv_pwrite(s->hd, s->refcount_block_cache_offset,
>> +            s->refcount_block_cache, size) != size)
>> +    {
> 
> Nit: bad formatting for opening brace here.

Oh, I like nitpicking. The coding style says: "The opening brace is on
the line that contains the control flow statement that introduces the
new block". No way to conform here without breaking the 80 characters
limit, so I did what I think is most reasonable. ;-)

I guess you mean something like the following:

    if (bdrv_pwrite(s->hd, s->refcount_block_cache_offset,
            s->refcount_block_cache, size) != size) {
            do_it();
    }

I don't like this very much because you can't see from the indentation
where the condition ends and the "then" branch begins (which is
different from a single-line condition). Maybe something to clarify in
the coding style and I'll happily follow it then.

Kevin

Re: [Qemu-devel] [PATCH] qcow2: Cache refcount blocks during snapshot creation

[Stefan Weil]

Kevin Wolf schrieb:
> Nathan Froyd schrieb:
>> On Fri, Jun 26, 2009 at 08:19:38PM +0200, Kevin Wolf wrote:
>>> + if (s->refcount_block_cache_offset == 0) {
>>> + return 0;
>>> + }
>>> +
>>> + if (bdrv_pwrite(s->hd, s->refcount_block_cache_offset,
>>> + s->refcount_block_cache, size) != size)
>>> + {
>> Nit: bad formatting for opening brace here.
>
> Oh, I like nitpicking. The coding style says: "The opening brace is on
> the line that contains the control flow statement that introduces the
> new block". No way to conform here without breaking the 80 characters
> limit, so I did what I think is most reasonable. ;-)
>
> I guess you mean something like the following:
>
> if (bdrv_pwrite(s->hd, s->refcount_block_cache_offset,
> s->refcount_block_cache, size) != size) {
> do_it();
> }
>
> I don't like this very much because you can't see from the indentation
> where the condition ends and the "then" branch begins (which is
> different from a single-line condition). Maybe something to clarify in
> the coding style and I'll happily follow it then.
>
> Kevin
>
>
>

Or like this?

    if (bdrv_pwrite(s->hd, s->refcount_block_cache_offset,
                    s->refcount_block_cache, size) != size) {
        do_it();
    }


Stefan :-)