[Qemu-devel] [PATCH 1/4] qcow2: Clean-up update_cluster_refcount().

[Qemu-devel] [PATCH 1/4] qcow2: Clean-up update_cluster_refcount().

[Laurent Vivier]

pièce jointe document texte brut
(0001-Clean-up-update_cluster_refcount.patch)
Move some parts to alloc_refcount_block() and block cache
 checking to load_refcount_block().

Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
---
 block-qcow2.c |   71 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 28 deletions(-)

Index: qemu/block-qcow2.c
===================================================================
--- qemu.orig/block-qcow2.c	2008-11-06 16:34:46.000000000 +0100
+++ qemu/block-qcow2.c	2008-11-06 16:40:27.000000000 +0100
@@ -2169,6 +2169,10 @@ static int load_refcount_block(BlockDriv
 {
     BDRVQcowState *s = bs->opaque;
     int ret;
+
+    if (refcount_block_offset == s->refcount_block_cache_offset)
+        return 0;
+
     ret = bdrv_pread(s->hd, refcount_block_offset, s->refcount_block_cache,
                      s->cluster_size);
     if (ret != s->cluster_size)
@@ -2189,11 +2193,8 @@ static int get_refcount(BlockDriverState
     refcount_block_offset = s->refcount_table[refcount_table_index];
     if (!refcount_block_offset)
         return 0;
-    if (refcount_block_offset != s->refcount_block_cache_offset) {
-        /* better than nothing: return allocated if read error */
-        if (load_refcount_block(bs, refcount_block_offset) < 0)
-            return 1;
-    }
+    if (load_refcount_block(bs, refcount_block_offset) < 0)
+        return 1;
     block_index = cluster_index &
         ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
     return be16_to_cpu(s->refcount_block_cache[block_index]);
@@ -2227,6 +2228,38 @@ static int64_t alloc_clusters_noref(Bloc
     }
 }
 
+/* create a new refcount block */
+
+static int64_t alloc_refcount_block(BlockDriverState *bs, int refcount_table_index)
+{
+    BDRVQcowState *s = bs->opaque;
+    int64_t offset;
+    int ret;
+    uint64_t data64;
+
+    /* Note: we cannot update the refcount now to avoid recursion */
+
+    offset = alloc_clusters_noref(bs, s->cluster_size);
+    memset(s->refcount_block_cache, 0, s->cluster_size);
+
+    ret = bdrv_pwrite(s->hd, offset, s->refcount_block_cache, s->cluster_size);
+    if (ret != s->cluster_size)
+        return -1;
+
+    s->refcount_table[refcount_table_index] = offset;
+    data64 = cpu_to_be64(offset);
+    ret = bdrv_pwrite(s->hd, s->refcount_table_offset +
+                      refcount_table_index * sizeof(uint64_t),
+                      &data64, sizeof(data64));
+    if (ret != sizeof(data64))
+        return -1;
+
+    s->refcount_block_cache_offset = offset;
+    update_refcount(bs, offset, s->cluster_size, 1);
+
+    return offset;
+}
+
 static int64_t alloc_clusters(BlockDriverState *bs, int64_t size)
 {
     int64_t offset;
@@ -2359,9 +2392,8 @@ static int update_cluster_refcount(Block
                                    int addend)
 {
     BDRVQcowState *s = bs->opaque;
-    int64_t offset, refcount_block_offset;
+    int64_t refcount_block_offset;
     int ret, refcount_table_index, block_index, refcount;
-    uint64_t data64;
 
     refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
     if (refcount_table_index >= s->refcount_table_size) {
@@ -2375,29 +2407,12 @@ static int update_cluster_refcount(Block
     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);
-        memset(s->refcount_block_cache, 0, s->cluster_size);
-        ret = bdrv_pwrite(s->hd, offset, s->refcount_block_cache, s->cluster_size);
-        if (ret != s->cluster_size)
-            return -EINVAL;
-        s->refcount_table[refcount_table_index] = offset;
-        data64 = cpu_to_be64(offset);
-        ret = bdrv_pwrite(s->hd, s->refcount_table_offset +
-                          refcount_table_index * sizeof(uint64_t),
-                          &data64, sizeof(data64));
-        if (ret != sizeof(data64))
+        refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
+        if (refcount_block_offset < 0)
             return -EINVAL;
-
-        refcount_block_offset = offset;
-        s->refcount_block_cache_offset = offset;
-        update_refcount(bs, offset, s->cluster_size, 1);
     } else {
-        if (refcount_block_offset != s->refcount_block_cache_offset) {
-            if (load_refcount_block(bs, refcount_block_offset) < 0)
-                return -EIO;
-        }
+        if (load_refcount_block(bs, refcount_block_offset) < 0)
+            return -EIO;
     }
     /* we can update the count and save it */
     block_index = cluster_index &

-- 

[Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount.

[Laurent Vivier]

pièce jointe document texte brut
(0002-Allow-update_cluster_refcount-to-update-several-cl.patch)
To improve performance when the qcow2 file is empty, this patch
allows update_cluster_refcount() to update refcount of
several clusters.

Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
---
 block-qcow2.c |  107 ++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 68 insertions(+), 39 deletions(-)

Index: qemu/block-qcow2.c
===================================================================
--- qemu.orig/block-qcow2.c	2008-11-06 16:40:44.000000000 +0100
+++ qemu/block-qcow2.c	2008-11-06 16:40:45.000000000 +0100
@@ -159,6 +159,7 @@ static void refcount_close(BlockDriverSt
 static int get_refcount(BlockDriverState *bs, int64_t cluster_index);
 static int update_cluster_refcount(BlockDriverState *bs,
                                    int64_t cluster_index,
+                                   int nb_clusters,
                                    int addend);
 static void update_refcount(BlockDriverState *bs,
                             int64_t offset, int64_t length,
@@ -1711,7 +1712,7 @@ static int update_snapshot_refcount(Bloc
                         refcount = 2;
                     } else {
                         if (addend != 0) {
-                            refcount = update_cluster_refcount(bs, offset >> s->cluster_bits, addend);
+                            refcount = update_cluster_refcount(bs, offset >> s->cluster_bits, 1, addend);
                         } else {
                             refcount = get_refcount(bs, offset >> s->cluster_bits);
                         }
@@ -1733,7 +1734,7 @@ static int update_snapshot_refcount(Bloc
             }
 
             if (addend != 0) {
-                refcount = update_cluster_refcount(bs, l2_offset >> s->cluster_bits, addend);
+                refcount = update_cluster_refcount(bs, l2_offset >> s->cluster_bits, 1, addend);
             } else {
                 refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
             }
@@ -2292,14 +2293,14 @@ static int64_t alloc_bytes(BlockDriverSt
         if (free_in_cluster == 0)
             s->free_byte_offset = 0;
         if ((offset & (s->cluster_size - 1)) != 0)
-            update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
+            update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
     } else {
         offset = alloc_clusters(bs, s->cluster_size);
         cluster_offset = s->free_byte_offset & ~(s->cluster_size - 1);
         if ((cluster_offset + s->cluster_size) == offset) {
             /* we are lucky: contiguous data */
             offset = s->free_byte_offset;
-            update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
+            update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
             s->free_byte_offset += size;
         } else {
             s->free_byte_offset = offset;
@@ -2389,46 +2390,77 @@ static int grow_refcount_table(BlockDriv
 /* XXX: cache several refcount block clusters ? */
 static int update_cluster_refcount(BlockDriverState *bs,
                                    int64_t cluster_index,
+                                   int nb_clusters,
                                    int addend)
 {
     BDRVQcowState *s = bs->opaque;
     int64_t refcount_block_offset;
-    int ret, refcount_table_index, block_index, refcount;
+    int ret, refcount_table_index, refcount_table_last_index, block_index, refcount;
+    int nb_block_index;
+    int refcount_cache_size;
 
-    refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
-    if (refcount_table_index >= s->refcount_table_size) {
+    if (nb_clusters == 0)
+        return 0;
+
+    refcount_table_last_index = (cluster_index + nb_clusters - 1) >>
+                                (s->cluster_bits - REFCOUNT_SHIFT);
+
+    /* grow the refcount table if needed */
+
+    if (refcount_table_last_index >= s->refcount_table_size) {
         if (addend < 0)
             return -EINVAL;
-        ret = grow_refcount_table(bs, refcount_table_index + 1);
+        ret = grow_refcount_table(bs, refcount_table_last_index + 1);
         if (ret < 0)
             return ret;
     }
-    refcount_block_offset = s->refcount_table[refcount_table_index];
-    if (!refcount_block_offset) {
-        if (addend < 0)
-            return -EINVAL;
-        refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
-        if (refcount_block_offset < 0)
-            return -EINVAL;
-    } else {
-        if (load_refcount_block(bs, refcount_block_offset) < 0)
+
+    while (nb_clusters) {
+        refcount_table_index = cluster_index >>
+                               (s->cluster_bits - REFCOUNT_SHIFT);
+        refcount_block_offset = s->refcount_table[refcount_table_index];
+
+        if (!refcount_block_offset) {
+            if (addend < 0)
+                return -EINVAL;
+            refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
+            if (refcount_block_offset < 0)
+		    return -EINVAL;
+        } else {
+            if (load_refcount_block(bs, refcount_block_offset) < 0)
+                return -EIO;
+        }
+
+        /* we can update the count and save it */
+
+        refcount_cache_size = 1 << (s->cluster_bits - REFCOUNT_SHIFT);
+        nb_block_index = 0;
+        block_index = cluster_index & (refcount_cache_size - 1);
+        refcount = 0;
+        while (nb_clusters &&
+               block_index + nb_block_index < refcount_cache_size) {
+
+            refcount = be16_to_cpu(
+                         s->refcount_block_cache[block_index + nb_block_index]);
+            refcount += addend;
+            if (refcount < 0 || refcount > 0xffff)
+                return -EINVAL;
+            if (refcount == 0 &&
+                cluster_index + nb_block_index < s->free_cluster_index) {
+                s->free_cluster_index = cluster_index + nb_block_index;
+            }
+            s->refcount_block_cache[block_index + nb_block_index] =
+                                                          cpu_to_be16(refcount);
+            nb_block_index++;
+            nb_clusters--;
+        }
+        if (bdrv_pwrite(s->hd,
+                        refcount_block_offset + (block_index << REFCOUNT_SHIFT),
+                        s->refcount_block_cache + block_index,
+                        nb_block_index * sizeof(uint16_t)) !=
+                        nb_block_index * sizeof(uint16_t))
             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;
-    }
-    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;
 }
 
@@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS
                             int addend)
 {
     BDRVQcowState *s = bs->opaque;
-    int64_t start, last, cluster_offset;
+    int64_t start, last;
 
 #ifdef DEBUG_ALLOC2
     printf("update_refcount: offset=%lld size=%lld addend=%d\n",
@@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS
 #endif
     if (length <= 0)
         return;
-    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);
-    }
+    start = offset >> s->cluster_bits;
+    last = (offset + length) >> s->cluster_bits;
+    update_cluster_refcount(bs, start, last - start + 1, addend);
 }
 
 #ifdef DEBUG_ALLOC

-- 

[Qemu-devel] [PATCH 3/4] qcow2: Align I/O access to l2 table and refcount block.

[Laurent Vivier]

pièce jointe document texte brut
(0003-Align-I-O-access-to-l2-table-and-refcount-block.patch)
When used with O_DIRECT, to align I/O access (memory and offset)
avoids to have to do a read before doing a write at the block-raw
level (to align access at this level).

Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
---
 block-qcow2.c |   40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

Index: qemu/block-qcow2.c
===================================================================
--- qemu.orig/block-qcow2.c	2008-11-06 16:40:54.000000000 +0100
+++ qemu/block-qcow2.c	2008-11-06 16:41:09.000000000 +0100
@@ -255,7 +255,8 @@ static int qcow_open(BlockDriverState *b
         be64_to_cpus(&s->l1_table[i]);
     }
     /* alloc L2 cache */
-    s->l2_cache = qemu_malloc(s->l2_size * L2_CACHE_SIZE * sizeof(uint64_t));
+    s->l2_cache = qemu_memalign(512,
+                                s->l2_size * L2_CACHE_SIZE * sizeof(uint64_t));
     if (!s->l2_cache)
         goto fail;
     s->cluster_cache = qemu_malloc(s->cluster_size);
@@ -865,7 +866,7 @@ static uint64_t alloc_cluster_offset(Blo
                                      int *num)
 {
     BDRVQcowState *s = bs->opaque;
-    int l2_index, ret;
+    int l2_index, ret, aligned_index, aligned_size;
     uint64_t l2_offset, *l2_table, cluster_offset;
     int nb_available, nb_clusters, i, j;
     uint64_t start_sect, current;
@@ -988,11 +989,18 @@ static uint64_t alloc_cluster_offset(Blo
                                              (i << s->cluster_bits)) |
                                              QCOW_OFLAG_COPIED);
 
+    /* l2 table is part of l2_cache
+     * size of l2_cache is s->l2_size * L2_CACHE_SIZE * sizeof(uint64_t)
+     * and s->l2_bits is (s->cluster_bits - 3) = 12 - 3 = 9;
+     * and s->l2_size = 1 << s->l2_bits, so l2_cache is aligned on 512 boundary.
+     */
+    aligned_index = l2_index & ~63;
+    aligned_size = (l2_index - aligned_index + nb_clusters + 63) & ~63ULL;
+    aligned_size *= sizeof(uint64_t);
     if (bdrv_pwrite(s->hd,
-                    l2_offset + l2_index * sizeof(uint64_t),
-                    l2_table + l2_index,
-                    nb_clusters * sizeof(uint64_t)) !=
-                    nb_clusters * sizeof(uint64_t))
+                    l2_offset + aligned_index * sizeof(uint64_t),
+                    l2_table + aligned_index,
+                    aligned_size) != aligned_size)
         return 0;
 
 out:
@@ -2137,7 +2145,7 @@ static int refcount_init(BlockDriverStat
     BDRVQcowState *s = bs->opaque;
     int ret, refcount_table_size2, i;
 
-    s->refcount_block_cache = qemu_malloc(s->cluster_size);
+    s->refcount_block_cache = qemu_memalign(512, s->cluster_size);
     if (!s->refcount_block_cache)
         goto fail;
     refcount_table_size2 = s->refcount_table_size * sizeof(uint64_t);
@@ -2398,6 +2406,7 @@ static int update_cluster_refcount(Block
     int ret, refcount_table_index, refcount_table_last_index, block_index, refcount;
     int nb_block_index;
     int refcount_cache_size;
+    int aligned_index, aligned_size;
 
     if (nb_clusters == 0)
         return 0;
@@ -2454,11 +2463,20 @@ static int update_cluster_refcount(Block
             nb_block_index++;
             nb_clusters--;
         }
+        /*
+         * size of refcount_block_cache is s->cluster_size (4096)
+         * so we can align access on a 512 boundary
+         */
+        aligned_index = block_index & ~((512 >> REFCOUNT_SHIFT) - 1);
+        aligned_size = (block_index - aligned_index + nb_block_index + 
+                        ((512 >> REFCOUNT_SHIFT) - 1)) &
+                        ~((512 >> REFCOUNT_SHIFT) - 1);
+        aligned_size *= sizeof(uint16_t);
         if (bdrv_pwrite(s->hd,
-                        refcount_block_offset + (block_index << REFCOUNT_SHIFT),
-                        s->refcount_block_cache + block_index,
-                        nb_block_index * sizeof(uint16_t)) !=
-                        nb_block_index * sizeof(uint16_t))
+                        refcount_block_offset +
+                        (aligned_index << REFCOUNT_SHIFT),
+                        s->refcount_block_cache + aligned_index,
+                        aligned_size) != aligned_size)
             return -EIO;
     }
     return refcount;

-- 

[Qemu-devel] [PATCH 4/4] qcow2: detect if no disk cache

[Laurent Vivier]

pièce jointe document texte brut
(0004-qcow2-detect-if-no-disk-cache.patch)
To reduce the number of bytes read and written, disable
the forced I/O alignment when the file is not open
with O_DIRECT.

Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
---
 block-qcow2.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Index: qemu/block-qcow2.c
===================================================================
--- qemu.orig/block-qcow2.c	2008-11-06 16:41:15.000000000 +0100
+++ qemu/block-qcow2.c	2008-11-06 16:41:16.000000000 +0100
@@ -147,6 +147,7 @@ typedef struct BDRVQcowState {
     int snapshots_size;
     int nb_snapshots;
     QCowSnapshot *snapshots;
+    int disk_nocache;
 } BDRVQcowState;
 
 static int decompress_cluster(BDRVQcowState *s, uint64_t cluster_offset);
@@ -193,6 +194,7 @@ static int qcow_open(BlockDriverState *b
     ret = bdrv_file_open(&s->hd, filename, flags);
     if (ret < 0)
         return ret;
+    s->disk_nocache = (flags & BDRV_O_NOCACHE) != 0;
     if (bdrv_pread(s->hd, 0, &header, sizeof(header)) != sizeof(header))
         goto fail;
     be32_to_cpus(&header.magic);
@@ -994,8 +996,13 @@ static uint64_t alloc_cluster_offset(Blo
      * and s->l2_bits is (s->cluster_bits - 3) = 12 - 3 = 9;
      * and s->l2_size = 1 << s->l2_bits, so l2_cache is aligned on 512 boundary.
      */
-    aligned_index = l2_index & ~63;
-    aligned_size = (l2_index - aligned_index + nb_clusters + 63) & ~63ULL;
+    if (s->disk_nocache) {
+        aligned_index = l2_index & ~63;
+        aligned_size = (l2_index - aligned_index + nb_clusters + 63) & ~63ULL;
+    } else {
+        aligned_index = l2_index;
+        aligned_size = nb_clusters;
+    }
     aligned_size *= sizeof(uint64_t);
     if (bdrv_pwrite(s->hd,
                     l2_offset + aligned_index * sizeof(uint64_t),
@@ -2467,10 +2474,15 @@ static int update_cluster_refcount(Block
          * size of refcount_block_cache is s->cluster_size (4096)
          * so we can align access on a 512 boundary
          */
-        aligned_index = block_index & ~((512 >> REFCOUNT_SHIFT) - 1);
-        aligned_size = (block_index - aligned_index + nb_block_index + 
-                        ((512 >> REFCOUNT_SHIFT) - 1)) &
-                        ~((512 >> REFCOUNT_SHIFT) - 1);
+        if (s->disk_nocache) {
+            aligned_index = block_index & ~((512 >> REFCOUNT_SHIFT) - 1);
+            aligned_size = (block_index - aligned_index + nb_block_index + 
+                            ((512 >> REFCOUNT_SHIFT) - 1)) &
+                            ~((512 >> REFCOUNT_SHIFT) - 1);
+        } else {
+            aligned_index = block_index;
+            aligned_size = nb_block_index;
+        }
         aligned_size *= sizeof(uint16_t);
         if (bdrv_pwrite(s->hd,
                         refcount_block_offset +

-- 

Re: [Qemu-devel] [PATCH 1/4] qcow2: Clean-up update_cluster_refcount().

[Kevin Wolf]

Laurent Vivier schrieb:
> pièce jointe document texte brut
> (0001-Clean-up-update_cluster_refcount.patch)
> Move some parts to alloc_refcount_block() and block cache
>  checking to load_refcount_block().
> 
> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
> ---
>  block-qcow2.c |   71 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 43 insertions(+), 28 deletions(-)
> 
> Index: qemu/block-qcow2.c
> ===================================================================
> --- qemu.orig/block-qcow2.c	2008-11-06 16:34:46.000000000 +0100
> +++ qemu/block-qcow2.c	2008-11-06 16:40:27.000000000 +0100
> @@ -2169,6 +2169,10 @@ static int load_refcount_block(BlockDriv
>  {
>      BDRVQcowState *s = bs->opaque;
>      int ret;
> +
> +    if (refcount_block_offset == s->refcount_block_cache_offset)
> +        return 0;
> +
>      ret = bdrv_pread(s->hd, refcount_block_offset, s->refcount_block_cache,
>                       s->cluster_size);
>      if (ret != s->cluster_size)
> @@ -2189,11 +2193,8 @@ static int get_refcount(BlockDriverState
>      refcount_block_offset = s->refcount_table[refcount_table_index];
>      if (!refcount_block_offset)
>          return 0;
> -    if (refcount_block_offset != s->refcount_block_cache_offset) {
> -        /* better than nothing: return allocated if read error */
> -        if (load_refcount_block(bs, refcount_block_offset) < 0)
> -            return 1;
> -    }
> +    if (load_refcount_block(bs, refcount_block_offset) < 0)
> +        return 1;

Please retain the comment saying that 1 is not the correct value but we
default to allocated for safety if the block could not be read. I don't
think this is obvious.

Otherwise the patch looks fine to me.

Kevin

Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount.

[Kevin Wolf]

Laurent Vivier schrieb:
> pièce jointe document texte brut
> (0002-Allow-update_cluster_refcount-to-update-several-cl.patch)
> To improve performance when the qcow2 file is empty, this patch
> allows update_cluster_refcount() to update refcount of
> several clusters.
> 
> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
> ---
>  block-qcow2.c |  107 ++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 68 insertions(+), 39 deletions(-)
> 
> Index: qemu/block-qcow2.c
> ===================================================================
> --- qemu.orig/block-qcow2.c	2008-11-06 16:40:44.000000000 +0100
> +++ qemu/block-qcow2.c	2008-11-06 16:40:45.000000000 +0100
> @@ -159,6 +159,7 @@ static void refcount_close(BlockDriverSt
>  static int get_refcount(BlockDriverState *bs, int64_t cluster_index);
>  static int update_cluster_refcount(BlockDriverState *bs,
>                                     int64_t cluster_index,
> +                                   int nb_clusters,
>                                     int addend);
>  static void update_refcount(BlockDriverState *bs,
>                              int64_t offset, int64_t length,
> @@ -1711,7 +1712,7 @@ static int update_snapshot_refcount(Bloc
>                          refcount = 2;
>                      } else {
>                          if (addend != 0) {
> -                            refcount = update_cluster_refcount(bs, offset >> s->cluster_bits, addend);
> +                            refcount = update_cluster_refcount(bs, offset >> s->cluster_bits, 1, addend);
>                          } else {
>                              refcount = get_refcount(bs, offset >> s->cluster_bits);
>                          }
> @@ -1733,7 +1734,7 @@ static int update_snapshot_refcount(Bloc
>              }
>  
>              if (addend != 0) {
> -                refcount = update_cluster_refcount(bs, l2_offset >> s->cluster_bits, addend);
> +                refcount = update_cluster_refcount(bs, l2_offset >> s->cluster_bits, 1, addend);
>              } else {
>                  refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
>              }
> @@ -2292,14 +2293,14 @@ static int64_t alloc_bytes(BlockDriverSt
>          if (free_in_cluster == 0)
>              s->free_byte_offset = 0;
>          if ((offset & (s->cluster_size - 1)) != 0)
> -            update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
> +            update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
>      } else {
>          offset = alloc_clusters(bs, s->cluster_size);
>          cluster_offset = s->free_byte_offset & ~(s->cluster_size - 1);
>          if ((cluster_offset + s->cluster_size) == offset) {
>              /* we are lucky: contiguous data */
>              offset = s->free_byte_offset;
> -            update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
> +            update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
>              s->free_byte_offset += size;
>          } else {
>              s->free_byte_offset = offset;
> @@ -2389,46 +2390,77 @@ static int grow_refcount_table(BlockDriv
>  /* XXX: cache several refcount block clusters ? */
>  static int update_cluster_refcount(BlockDriverState *bs,
>                                     int64_t cluster_index,
> +                                   int nb_clusters,
>                                     int addend)
>  {
>      BDRVQcowState *s = bs->opaque;
>      int64_t refcount_block_offset;
> -    int ret, refcount_table_index, block_index, refcount;
> +    int ret, refcount_table_index, refcount_table_last_index, block_index, refcount;
> +    int nb_block_index;
> +    int refcount_cache_size;
>  
> -    refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
> -    if (refcount_table_index >= s->refcount_table_size) {
> +    if (nb_clusters == 0)
> +        return 0;
> +
> +    refcount_table_last_index = (cluster_index + nb_clusters - 1) >>
> +                                (s->cluster_bits - REFCOUNT_SHIFT);
> +
> +    /* grow the refcount table if needed */
> +
> +    if (refcount_table_last_index >= s->refcount_table_size) {
>          if (addend < 0)
>              return -EINVAL;
> -        ret = grow_refcount_table(bs, refcount_table_index + 1);
> +        ret = grow_refcount_table(bs, refcount_table_last_index + 1);
>          if (ret < 0)
>              return ret;
>      }
> -    refcount_block_offset = s->refcount_table[refcount_table_index];
> -    if (!refcount_block_offset) {
> -        if (addend < 0)
> -            return -EINVAL;
> -        refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
> -        if (refcount_block_offset < 0)
> -            return -EINVAL;
> -    } else {
> -        if (load_refcount_block(bs, refcount_block_offset) < 0)
> +
> +    while (nb_clusters) {
> +        refcount_table_index = cluster_index >>
> +                               (s->cluster_bits - REFCOUNT_SHIFT);
> +        refcount_block_offset = s->refcount_table[refcount_table_index];

I guess (comment? ;-)) this outer loop is for handling requests which
span multiple refcount blocks? If so, am I missing something or is
refcount_block_offset the same for each iteration because cluster_index
never changes?

> +
> +        if (!refcount_block_offset) {
> +            if (addend < 0)
> +                return -EINVAL;
> +            refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
> +            if (refcount_block_offset < 0)
> +		    return -EINVAL;
> +        } else {
> +            if (load_refcount_block(bs, refcount_block_offset) < 0)
> +                return -EIO;
> +        }
> +
> +        /* we can update the count and save it */
> +
> +        refcount_cache_size = 1 << (s->cluster_bits - REFCOUNT_SHIFT);
> +        nb_block_index = 0;

I have to admit that nb_block_index is a name where I can't image what
the variable might be for. Seems to be a counter for the changed
refcounts in the current refcount block, and block_index seems to be the
first index to be touched in this block. What about first_index and
cur_refcount or something like that? (I don't like these suggestions too
much, either. Maybe someone has better names.)

> +        block_index = cluster_index & (refcount_cache_size - 1);
> +        refcount = 0;
> +        while (nb_clusters &&
> +               block_index + nb_block_index < refcount_cache_size) {
> +
> +            refcount = be16_to_cpu(
> +                         s->refcount_block_cache[block_index + nb_block_index]);
> +            refcount += addend;
> +            if (refcount < 0 || refcount > 0xffff)
> +                return -EINVAL;

Here we possibly abort in the middle of the operation. If it fails
somewhere in the fifth refcount block, what happens with the four
already updated blocks?

> +            if (refcount == 0 &&
> +                cluster_index + nb_block_index < s->free_cluster_index) {
> +                s->free_cluster_index = cluster_index + nb_block_index;
> +            }
> +            s->refcount_block_cache[block_index + nb_block_index] =
> +                                                          cpu_to_be16(refcount);
> +            nb_block_index++;
> +            nb_clusters--;
> +        }
> +        if (bdrv_pwrite(s->hd,
> +                        refcount_block_offset + (block_index << REFCOUNT_SHIFT),
> +                        s->refcount_block_cache + block_index,
> +                        nb_block_index * sizeof(uint16_t)) !=
> +                        nb_block_index * sizeof(uint16_t))
>              return -EIO;

Same here.

>      }
> -    /* 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 refcount;
>  }
>  
> @@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS
>                              int addend)
>  {
>      BDRVQcowState *s = bs->opaque;
> -    int64_t start, last, cluster_offset;
> +    int64_t start, last;
>  
>  #ifdef DEBUG_ALLOC2
>      printf("update_refcount: offset=%lld size=%lld addend=%d\n",
> @@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS
>  #endif
>      if (length <= 0)
>          return;
> -    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);
> -    }
> +    start = offset >> s->cluster_bits;
> +    last = (offset + length) >> s->cluster_bits;

Off by one for length % cluster_size == 0?

> +    update_cluster_refcount(bs, start, last - start + 1, addend);
>  }
>  
>  #ifdef DEBUG_ALLOC
> 

Kevin

Re: [Qemu-devel] [PATCH 1/4] qcow2: Clean-up update_cluster_refcount().

[Laurent Vivier]

Le jeudi 06 novembre 2008 à 18:32 +0100, Kevin Wolf a écrit :
> Laurent Vivier schrieb:
> > pièce jointe document texte brut
> > (0001-Clean-up-update_cluster_refcount.patch)
> > Move some parts to alloc_refcount_block() and block cache
> >  checking to load_refcount_block().
> > 
> > Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
> > ---
> >  block-qcow2.c |   71 +++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 43 insertions(+), 28 deletions(-)
> > 
> > Index: qemu/block-qcow2.c
> > ===================================================================
> > --- qemu.orig/block-qcow2.c	2008-11-06 16:34:46.000000000 +0100
> > +++ qemu/block-qcow2.c	2008-11-06 16:40:27.000000000 +0100
> > @@ -2169,6 +2169,10 @@ static int load_refcount_block(BlockDriv
> >  {
> >      BDRVQcowState *s = bs->opaque;
> >      int ret;
> > +
> > +    if (refcount_block_offset == s->refcount_block_cache_offset)
> > +        return 0;
> > +
> >      ret = bdrv_pread(s->hd, refcount_block_offset, s->refcount_block_cache,
> >                       s->cluster_size);
> >      if (ret != s->cluster_size)
> > @@ -2189,11 +2193,8 @@ static int get_refcount(BlockDriverState
> >      refcount_block_offset = s->refcount_table[refcount_table_index];
> >      if (!refcount_block_offset)
> >          return 0;
> > -    if (refcount_block_offset != s->refcount_block_cache_offset) {
> > -        /* better than nothing: return allocated if read error */
> > -        if (load_refcount_block(bs, refcount_block_offset) < 0)
> > -            return 1;
> > -    }
> > +    if (load_refcount_block(bs, refcount_block_offset) < 0)
> > +        return 1;
> 
> Please retain the comment saying that 1 is not the correct value but we
> default to allocated for safety if the block could not be read. I don't
> think this is obvious.

I totally agree with you, but it's just a code move, I don't want to
change anything except the line numbers.

I think this kind of change must have its own patch.

Regards,
Laurent
-- 
------------------ Laurent.Vivier@bull.net  ------------------
"Tout ce qui est impossible reste à accomplir"    Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard

Re: [Qemu-devel] [PATCH 1/4] qcow2: Clean-up update_cluster_refcount().

[Kevin Wolf]

Laurent Vivier schrieb:
> Le jeudi 06 novembre 2008 à 18:32 +0100, Kevin Wolf a écrit :
>> Laurent Vivier schrieb:
>>> pièce jointe document texte brut
>>> (0001-Clean-up-update_cluster_refcount.patch)
>>> Move some parts to alloc_refcount_block() and block cache
>>>  checking to load_refcount_block().
>>>
>>> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
>>> ---
>>>  block-qcow2.c |   71 +++++++++++++++++++++++++++++++++++-----------------------
>>>  1 file changed, 43 insertions(+), 28 deletions(-)
>>>
>>> Index: qemu/block-qcow2.c
>>> ===================================================================
>>> --- qemu.orig/block-qcow2.c	2008-11-06 16:34:46.000000000 +0100
>>> +++ qemu/block-qcow2.c	2008-11-06 16:40:27.000000000 +0100
>>> @@ -2169,6 +2169,10 @@ static int load_refcount_block(BlockDriv
>>>  {
>>>      BDRVQcowState *s = bs->opaque;
>>>      int ret;
>>> +
>>> +    if (refcount_block_offset == s->refcount_block_cache_offset)
>>> +        return 0;
>>> +
>>>      ret = bdrv_pread(s->hd, refcount_block_offset, s->refcount_block_cache,
>>>                       s->cluster_size);
>>>      if (ret != s->cluster_size)
>>> @@ -2189,11 +2193,8 @@ static int get_refcount(BlockDriverState
>>>      refcount_block_offset = s->refcount_table[refcount_table_index];
>>>      if (!refcount_block_offset)
>>>          return 0;
>>> -    if (refcount_block_offset != s->refcount_block_cache_offset) {
>>> -        /* better than nothing: return allocated if read error */
>>> -        if (load_refcount_block(bs, refcount_block_offset) < 0)
>>> -            return 1;
>>> -    }
>>> +    if (load_refcount_block(bs, refcount_block_offset) < 0)
>>> +        return 1;
>> Please retain the comment saying that 1 is not the correct value but we
>> default to allocated for safety if the block could not be read. I don't
>> think this is obvious.
> 
> I totally agree with you, but it's just a code move, I don't want to
> change anything except the line numbers.
> 
> I think this kind of change must have its own patch.

No, it's a move which drops a comment and it shouldn't do that. ;-)

> -        /* better than nothing: return allocated if read error */

Kevin

Re: [Qemu-devel] [PATCH 1/4] qcow2: Clean-up update_cluster_refcount().

[Laurent Vivier]

Le vendredi 07 novembre 2008 à 09:54 +0100, Kevin Wolf a écrit :
> Laurent Vivier schrieb:
> > Le jeudi 06 novembre 2008 à 18:32 +0100, Kevin Wolf a écrit :
> >> Laurent Vivier schrieb:
> >>> pièce jointe document texte brut
> >>> (0001-Clean-up-update_cluster_refcount.patch)
> >>> Move some parts to alloc_refcount_block() and block cache
> >>>  checking to load_refcount_block().
> >>>
> >>> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
> >>> ---
> >>>  block-qcow2.c |   71 +++++++++++++++++++++++++++++++++++-----------------------
> >>>  1 file changed, 43 insertions(+), 28 deletions(-)
> >>>
> >>> Index: qemu/block-qcow2.c
> >>> ===================================================================
> >>> --- qemu.orig/block-qcow2.c	2008-11-06 16:34:46.000000000 +0100
> >>> +++ qemu/block-qcow2.c	2008-11-06 16:40:27.000000000 +0100
> >>> @@ -2169,6 +2169,10 @@ static int load_refcount_block(BlockDriv
> >>>  {
> >>>      BDRVQcowState *s = bs->opaque;
> >>>      int ret;
> >>> +
> >>> +    if (refcount_block_offset == s->refcount_block_cache_offset)
> >>> +        return 0;
> >>> +
> >>>      ret = bdrv_pread(s->hd, refcount_block_offset, s->refcount_block_cache,
> >>>                       s->cluster_size);
> >>>      if (ret != s->cluster_size)
> >>> @@ -2189,11 +2193,8 @@ static int get_refcount(BlockDriverState
> >>>      refcount_block_offset = s->refcount_table[refcount_table_index];
> >>>      if (!refcount_block_offset)
> >>>          return 0;
> >>> -    if (refcount_block_offset != s->refcount_block_cache_offset) {
> >>> -        /* better than nothing: return allocated if read error */
> >>> -        if (load_refcount_block(bs, refcount_block_offset) < 0)
> >>> -            return 1;
> >>> -    }
> >>> +    if (load_refcount_block(bs, refcount_block_offset) < 0)
> >>> +        return 1;
> >> Please retain the comment saying that 1 is not the correct value but we
> >> default to allocated for safety if the block could not be read. I don't
> >> think this is obvious.
> > 
> > I totally agree with you, but it's just a code move, I don't want to
> > change anything except the line numbers.
> > 
> > I think this kind of change must have its own patch.
> 
> No, it's a move which drops a comment and it shouldn't do that. ;-)
> 
> > -        /* better than nothing: return allocated if read error */

Oops...

Laurent
-- 
------------------ Laurent.Vivier@bull.net  ------------------
"Tout ce qui est impossible reste à accomplir"    Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard

Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount.

[Laurent Vivier]

Le jeudi 06 novembre 2008 à 19:11 +0100, Kevin Wolf a écrit :
> Laurent Vivier schrieb:
> > pièce jointe document texte brut
> > (0002-Allow-update_cluster_refcount-to-update-several-cl.patch)
> > To improve performance when the qcow2 file is empty, this patch
> > allows update_cluster_refcount() to update refcount of
> > several clusters.
> > 
> > Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
> > ---
> >  block-qcow2.c |  107 ++++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 68 insertions(+), 39 deletions(-)
> > 
> > Index: qemu/block-qcow2.c
> > ===================================================================
> > --- qemu.orig/block-qcow2.c	2008-11-06 16:40:44.000000000 +0100
> > +++ qemu/block-qcow2.c	2008-11-06 16:40:45.000000000 +0100
> > @@ -159,6 +159,7 @@ static void refcount_close(BlockDriverSt
> >  static int get_refcount(BlockDriverState *bs, int64_t cluster_index);
> >  static int update_cluster_refcount(BlockDriverState *bs,
> >                                     int64_t cluster_index,
> > +                                   int nb_clusters,
> >                                     int addend);
> >  static void update_refcount(BlockDriverState *bs,
> >                              int64_t offset, int64_t length,
> > @@ -1711,7 +1712,7 @@ static int update_snapshot_refcount(Bloc
> >                          refcount = 2;
> >                      } else {
> >                          if (addend != 0) {
> > -                            refcount = update_cluster_refcount(bs, offset >> s->cluster_bits, addend);
> > +                            refcount = update_cluster_refcount(bs, offset >> s->cluster_bits, 1, addend);
> >                          } else {
> >                              refcount = get_refcount(bs, offset >> s->cluster_bits);
> >                          }
> > @@ -1733,7 +1734,7 @@ static int update_snapshot_refcount(Bloc
> >              }
> >  
> >              if (addend != 0) {
> > -                refcount = update_cluster_refcount(bs, l2_offset >> s->cluster_bits, addend);
> > +                refcount = update_cluster_refcount(bs, l2_offset >> s->cluster_bits, 1, addend);
> >              } else {
> >                  refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
> >              }
> > @@ -2292,14 +2293,14 @@ static int64_t alloc_bytes(BlockDriverSt
> >          if (free_in_cluster == 0)
> >              s->free_byte_offset = 0;
> >          if ((offset & (s->cluster_size - 1)) != 0)
> > -            update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
> > +            update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
> >      } else {
> >          offset = alloc_clusters(bs, s->cluster_size);
> >          cluster_offset = s->free_byte_offset & ~(s->cluster_size - 1);
> >          if ((cluster_offset + s->cluster_size) == offset) {
> >              /* we are lucky: contiguous data */
> >              offset = s->free_byte_offset;
> > -            update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
> > +            update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
> >              s->free_byte_offset += size;
> >          } else {
> >              s->free_byte_offset = offset;
> > @@ -2389,46 +2390,77 @@ static int grow_refcount_table(BlockDriv
> >  /* XXX: cache several refcount block clusters ? */
> >  static int update_cluster_refcount(BlockDriverState *bs,
> >                                     int64_t cluster_index,
> > +                                   int nb_clusters,
> >                                     int addend)
> >  {
> >      BDRVQcowState *s = bs->opaque;
> >      int64_t refcount_block_offset;
> > -    int ret, refcount_table_index, block_index, refcount;
> > +    int ret, refcount_table_index, refcount_table_last_index, block_index, refcount;
> > +    int nb_block_index;
> > +    int refcount_cache_size;
> >  
> > -    refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
> > -    if (refcount_table_index >= s->refcount_table_size) {
> > +    if (nb_clusters == 0)
> > +        return 0;
> > +
> > +    refcount_table_last_index = (cluster_index + nb_clusters - 1) >>
> > +                                (s->cluster_bits - REFCOUNT_SHIFT);
> > +
> > +    /* grow the refcount table if needed */
> > +
> > +    if (refcount_table_last_index >= s->refcount_table_size) {
> >          if (addend < 0)
> >              return -EINVAL;
> > -        ret = grow_refcount_table(bs, refcount_table_index + 1);
> > +        ret = grow_refcount_table(bs, refcount_table_last_index + 1);
> >          if (ret < 0)
> >              return ret;
> >      }
> > -    refcount_block_offset = s->refcount_table[refcount_table_index];
> > -    if (!refcount_block_offset) {
> > -        if (addend < 0)
> > -            return -EINVAL;
> > -        refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
> > -        if (refcount_block_offset < 0)
> > -            return -EINVAL;
> > -    } else {
> > -        if (load_refcount_block(bs, refcount_block_offset) < 0)
> > +
> > +    while (nb_clusters) {
> > +        refcount_table_index = cluster_index >>
> > +                               (s->cluster_bits - REFCOUNT_SHIFT);
> > +        refcount_block_offset = s->refcount_table[refcount_table_index];
> 
> I guess (comment? ;-)) this outer loop is for handling requests which
> span multiple refcount blocks? If so, am I missing something or is

Yes,

> refcount_block_offset the same for each iteration because cluster_index
> never changes?

You're right, there is a missing "cluster_index++" at the end of this
loop.

> > +
> > +        if (!refcount_block_offset) {
> > +            if (addend < 0)
> > +                return -EINVAL;
> > +            refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
> > +            if (refcount_block_offset < 0)
> > +		    return -EINVAL;
> > +        } else {
> > +            if (load_refcount_block(bs, refcount_block_offset) < 0)
> > +                return -EIO;
> > +        }
> > +
> > +        /* we can update the count and save it */
> > +
> > +        refcount_cache_size = 1 << (s->cluster_bits - REFCOUNT_SHIFT);
> > +        nb_block_index = 0;
> 
> I have to admit that nb_block_index is a name where I can't image what
> the variable might be for. Seems to be a counter for the changed
> refcounts in the current refcount block, and block_index seems to be the
> first index to be touched in this block. What about first_index and
> cur_refcount or something like that? (I don't like these suggestions too
> much, either. Maybe someone has better names.)
> 
> > +        block_index = cluster_index & (refcount_cache_size - 1);
> > +        refcount = 0;
> > +        while (nb_clusters &&
> > +               block_index + nb_block_index < refcount_cache_size) {
> > +
> > +            refcount = be16_to_cpu(
> > +                         s->refcount_block_cache[block_index + nb_block_index]);
> > +            refcount += addend;
> > +            if (refcount < 0 || refcount > 0xffff)
> > +                return -EINVAL;
> 
> Here we possibly abort in the middle of the operation. If it fails
> somewhere in the fifth refcount block, what happens with the four
> already updated blocks?

Yes, you're right. I think we have at least to save refcount we have
updated.

> 
> > +            if (refcount == 0 &&
> > +                cluster_index + nb_block_index < s->free_cluster_index) {
> > +                s->free_cluster_index = cluster_index + nb_block_index;
> > +            }
> > +            s->refcount_block_cache[block_index + nb_block_index] =
> > +                                                          cpu_to_be16(refcount);
> > +            nb_block_index++;
> > +            nb_clusters--;
> > +        }
> > +        if (bdrv_pwrite(s->hd,
> > +                        refcount_block_offset + (block_index << REFCOUNT_SHIFT),
> > +                        s->refcount_block_cache + block_index,
> > +                        nb_block_index * sizeof(uint16_t)) !=
> > +                        nb_block_index * sizeof(uint16_t))
> >              return -EIO;
> 
> Same here.

I think we are not worst than the original behavior. I don't know how to
manage this better.

> >      }
> > -    /* 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 refcount;
> >  }
> >  
> > @@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS
> >                              int addend)
> >  {
> >      BDRVQcowState *s = bs->opaque;
> > -    int64_t start, last, cluster_offset;
> > +    int64_t start, last;
> >  
> >  #ifdef DEBUG_ALLOC2
> >      printf("update_refcount: offset=%lld size=%lld addend=%d\n",
> > @@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS
> >  #endif
> >      if (length <= 0)
> >          return;
> > -    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);
> > -    }
> > +    start = offset >> s->cluster_bits;
> > +    last = (offset + length) >> s->cluster_bits;
> 
> Off by one for length % cluster_size == 0?

Explain, please (but notice the "<=" in the "for (...)").

> > +    update_cluster_refcount(bs, start, last - start + 1, addend);
> >  }
> >  
> >  #ifdef DEBUG_ALLOC
> > 
> 
> Kevin
> 
-- 
------------------ Laurent.Vivier@bull.net  ------------------
"Tout ce qui est impossible reste à accomplir"    Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard

Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount.

[Kevin Wolf]

Laurent Vivier schrieb:
>>> +        block_index = cluster_index & (refcount_cache_size - 1);
>>> +        refcount = 0;
>>> +        while (nb_clusters &&
>>> +               block_index + nb_block_index < refcount_cache_size) {
>>> +
>>> +            refcount = be16_to_cpu(
>>> +                         s->refcount_block_cache[block_index + nb_block_index]);
>>> +            refcount += addend;
>>> +            if (refcount < 0 || refcount > 0xffff)
>>> +                return -EINVAL;
>> Here we possibly abort in the middle of the operation. If it fails
>> somewhere in the fifth refcount block, what happens with the four
>> already updated blocks?
> 
> Yes, you're right. I think we have at least to save refcount we have
> updated.
> 
>>> +            if (refcount == 0 &&
>>> +                cluster_index + nb_block_index < s->free_cluster_index) {
>>> +                s->free_cluster_index = cluster_index + nb_block_index;
>>> +            }
>>> +            s->refcount_block_cache[block_index + nb_block_index] =
>>> +                                                          cpu_to_be16(refcount);
>>> +            nb_block_index++;
>>> +            nb_clusters--;
>>> +        }
>>> +        if (bdrv_pwrite(s->hd,
>>> +                        refcount_block_offset + (block_index << REFCOUNT_SHIFT),
>>> +                        s->refcount_block_cache + block_index,
>>> +                        nb_block_index * sizeof(uint16_t)) !=
>>> +                        nb_block_index * sizeof(uint16_t))
>>>              return -EIO;
>> Same here.
> 
> I think we are not worst than the original behavior. I don't know how to
> manage this better.

Right, this hasn't been optimal before either. I noticed later that
update_refcount didn't even check the return value. So the difference is
that you abort while the old implementation tries the next block.

I'm unsure which behaviour is the better one. Actually, both don't feel
quite right. But being not worse in this aspect than the original still
makes this patch an improvement, so...

>>>      }
>>> -    /* 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 refcount;
>>>  }
>>>  
>>> @@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS
>>>                              int addend)
>>>  {
>>>      BDRVQcowState *s = bs->opaque;
>>> -    int64_t start, last, cluster_offset;
>>> +    int64_t start, last;
>>>  
>>>  #ifdef DEBUG_ALLOC2
>>>      printf("update_refcount: offset=%lld size=%lld addend=%d\n",
>>> @@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS
>>>  #endif
>>>      if (length <= 0)
>>>          return;
>>> -    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);
>>> -    }
>>> +    start = offset >> s->cluster_bits;
>>> +    last = (offset + length) >> s->cluster_bits;
>> Off by one for length % cluster_size == 0?
> 
> Explain, please (but notice the "<=" in the "for (...)").

I didn't even look at the old code yesterday, but the difference is that
you dropped the - 1 for last.

Let's do it by example: Assume cluster_size = 0x1000 for simplicity and
we get offset = 0x1000 and length = 0x2000 (i.e. the last affected byte
would be 0x2fff). The old code correctly produces start = 0x1000 and
last = 0x2000 whereas you get start = 1 and last = 3.

Kevin

Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount.

[Laurent Vivier]

Le vendredi 07 novembre 2008 à 11:21 +0100, Kevin Wolf a écrit :
> Laurent Vivier schrieb:
[...]
> >>>  
> >>> @@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS
> >>>                              int addend)
> >>>  {
> >>>      BDRVQcowState *s = bs->opaque;
> >>> -    int64_t start, last, cluster_offset;
> >>> +    int64_t start, last;
> >>>  
> >>>  #ifdef DEBUG_ALLOC2
> >>>      printf("update_refcount: offset=%lld size=%lld addend=%d\n",
> >>> @@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS
> >>>  #endif
> >>>      if (length <= 0)
> >>>          return;
> >>> -    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);
> >>> -    }
> >>> +    start = offset >> s->cluster_bits;
> >>> +    last = (offset + length) >> s->cluster_bits;
> >> Off by one for length % cluster_size == 0?
> > 
> > Explain, please (but notice the "<=" in the "for (...)").
> 
> I didn't even look at the old code yesterday, but the difference is that
> you dropped the - 1 for last.
> 
> Let's do it by example: Assume cluster_size = 0x1000 for simplicity and
> we get offset = 0x1000 and length = 0x2000 (i.e. the last affected byte
> would be 0x2fff). The old code correctly produces start = 0x1000 and
> last = 0x2000 whereas you get start = 1 and last = 3.

Yes, "last" must be "(offset + length - 1) >> s->cluster_bits"

Thank you,
Laurent
-- 
------------------ Laurent.Vivier@bull.net  ------------------
"Tout ce qui est impossible reste à accomplir"    Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard