Patchwork [RFC,V6,10/33] qcow2: Extract qcow2_dedup_grow_table

login
register
mail settings
Submitter Benoît Canet
Date Feb. 6, 2013, 12:31 p.m.
Message ID <1360153926-9492-11-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/218655/
State New
Headers show

Comments

Benoît Canet - Feb. 6, 2013, 12:31 p.m.
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block/qcow2-cluster.c |  102 +++++++++++++++++++++++++++++++------------------
 block/qcow2-dedup.c   |    3 +-
 block/qcow2.h         |    6 +++
 3 files changed, 71 insertions(+), 40 deletions(-)
Stefan Hajnoczi - Feb. 7, 2013, 9:26 a.m.
On Wed, Feb 06, 2013 at 01:31:43PM +0100, Benoît Canet wrote:
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 63a7241..dbcb6d2 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -29,44 +29,48 @@
>  #include "block/qcow2.h"
>  #include "trace.h"
>  
> -int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size)
> +int qcow2_do_grow_table(BlockDriverState *bs, int min_size, bool exact_size,
> +                        uint64_t **table, uint64_t *table_offset,
> +                        int *table_size, qcow2_save_table save_table,
> +                        const char *table_name)

Maybe it's better to duplicate/reimplement the function specifically for
dedup tables.

qcow2_grow_l1_table() was easy to call, it had 3 arguments.  The new
qcow2_do_grow_table() is more like a macro created to avoid code
duplication - the caller provides arguments the specify lots of internal
details.  There's even a function pointer for part of the function which
turned out not to be reusable.  This is bordering on a hack.

qcow2_do_grow_table() is not an abstraction and it makes reading the
code harder because now the reader has to factor in all these arguments
and their actual values (s->l1_table, etc).

>  {
>      BDRVQcowState *s = bs->opaque;
> -    int new_l1_size, new_l1_size2, ret, i;
> -    uint64_t *new_l1_table;
> -    int64_t new_l1_table_offset;
> -    uint8_t data[12];
> +    int new_size, new_size2, ret, i;
> +    uint64_t *new_table;
> +    int64_t new_table_offset;
>  
> -    if (min_size <= s->l1_size)
> +    if (min_size <= *table_size) {
>          return 0;
> +    }
>  
>      if (exact_size) {
> -        new_l1_size = min_size;
> +        new_size = min_size;
>      } else {
>          /* Bump size up to reduce the number of times we have to grow */
> -        new_l1_size = s->l1_size;
> -        if (new_l1_size == 0) {
> -            new_l1_size = 1;
> +        new_size = *table_size;
> +        if (new_size == 0) {
> +            new_size = 1;
>          }
> -        while (min_size > new_l1_size) {
> -            new_l1_size = (new_l1_size * 3 + 1) / 2;
> +        while (min_size > new_size) {
> +            new_size = (new_size * 3 + 1) / 2;
>          }
>      }
>  
>  #ifdef DEBUG_ALLOC2
> -    fprintf(stderr, "grow l1_table from %d to %d\n", s->l1_size, new_l1_size);
> +    fprintf(stderr, "grow %s_table from %d to %d\n",
> +            table_name, *table_size, new_size);
>  #endif
>  
> -    new_l1_size2 = sizeof(uint64_t) * new_l1_size;
> -    new_l1_table = g_malloc0(align_offset(new_l1_size2, 512));
> -    memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
> +    new_size2 = sizeof(uint64_t) * new_size;
> +    new_table = g_malloc0(align_offset(new_size2, 512));
> +    memcpy(new_table, *table, *table_size * sizeof(uint64_t));
>  
>      /* write new table (align to cluster) */
>      BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);

BLKDBG_L1_GROW_ALLOC_TABLE is wrong for dedup tables.  Please introduce
blkdebug constants for dedup metadata.

> -    new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
> -    if (new_l1_table_offset < 0) {
> -        g_free(new_l1_table);
> -        return new_l1_table_offset;
> +    new_table_offset = qcow2_alloc_clusters(bs, new_size2);
> +    if (new_table_offset < 0) {
> +        g_free(new_table);
> +        return new_table_offset;
>      }
>  
>      ret = qcow2_cache_flush(bs, s->refcount_block_cache);
> @@ -75,34 +79,56 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size)
>      }
>  
>      BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_WRITE_TABLE);
> -    for(i = 0; i < s->l1_size; i++)
> -        new_l1_table[i] = cpu_to_be64(new_l1_table[i]);
> -    ret = bdrv_pwrite_sync(bs->file, new_l1_table_offset, new_l1_table, new_l1_size2);
> +    for (i = 0; i < *table_size; i++) {
> +        new_table[i] = cpu_to_be64(new_table[i]);
> +    }
> +    ret = bdrv_pwrite_sync(bs->file, new_table_offset, new_table, new_size2);
>      if (ret < 0)
>          goto fail;
> -    for(i = 0; i < s->l1_size; i++)
> -        new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
> +    for (i = 0; i < *table_size; i++) {
> +        new_table[i] = be64_to_cpu(new_table[i]);
> +    }
> +
> +    g_free(*table);
> +    qcow2_free_clusters(bs, *table_offset, *table_size * sizeof(uint64_t));
> +    *table_offset = new_table_offset;
> +    *table = new_table;
> +    *table_size = new_size;
>  
>      /* set new table */
>      BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ACTIVATE_TABLE);
> -    cpu_to_be32w((uint32_t*)data, new_l1_size);
> -    cpu_to_be64wu((uint64_t*)(data + 4), new_l1_table_offset);
> -    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size), data,sizeof(data));
> -    if (ret < 0) {
> -        goto fail;
> -    }
> -    g_free(s->l1_table);
> -    qcow2_free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t));
> -    s->l1_table_offset = new_l1_table_offset;
> -    s->l1_table = new_l1_table;
> -    s->l1_size = new_l1_size;
> +    save_table(bs, *table_offset, *table_size);

Return value is ignored.

> +
>      return 0;
>   fail:
> -    g_free(new_l1_table);
> -    qcow2_free_clusters(bs, new_l1_table_offset, new_l1_size2);
> +    g_free(new_table);
> +    qcow2_free_clusters(bs, new_table_offset, new_size2);
>      return ret;
>  }
>  
> +static int qcow2_l1_save_table(BlockDriverState *bs,
> +                               int64_t table_offset, int size)
> +{
> +    uint8_t data[12];
> +    cpu_to_be32w((uint32_t *)data, size);
> +    cpu_to_be64wu((uint64_t *)(data + 4), table_offset);

A struct with proper types would have been much nicer and more portable
to platforms that require proper alignment.  But it's not your fault,
the existing code did this.

> +    return bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size),
> +                            data, sizeof(data));
> +}
> +
> +int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    return qcow2_do_grow_table(bs,
> +                               min_size,
> +                               exact_size,
> +                               &s->l1_table,
> +                               &s->l1_table_offset,
> +                               &s->l1_size,
> +                               qcow2_l1_save_table,
> +                               "l1");
> +}
> +
>  /*
>   * l2_load
>   *
> diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
> index 45b2326..de1b366 100644
> --- a/block/qcow2-dedup.c
> +++ b/block/qcow2-dedup.c
> @@ -575,7 +575,6 @@ exit:
>      return deduped_clusters_nr * s->cluster_sectors - begining_index;
>  }
>  
> -
>  /* Create a deduplication table hash block, write it's offset to disk and
>   * reference it in the RAM deduplication table
>   *
> @@ -592,7 +591,7 @@ static uint64_t qcow2_create_block(BlockDriverState *bs,
>      uint64_t data64;
>      int ret = 0;
>  
> -    /* allocate a new dedup table hash block */
> +    /* allocate a new dedup table cluster */
>      offset = qcow2_alloc_clusters(bs, s->hash_block_size);

Squash these hunk into the patch that introduce these mistakes?

Patch

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 63a7241..dbcb6d2 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -29,44 +29,48 @@ 
 #include "block/qcow2.h"
 #include "trace.h"
 
-int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size)
+int qcow2_do_grow_table(BlockDriverState *bs, int min_size, bool exact_size,
+                        uint64_t **table, uint64_t *table_offset,
+                        int *table_size, qcow2_save_table save_table,
+                        const char *table_name)
 {
     BDRVQcowState *s = bs->opaque;
-    int new_l1_size, new_l1_size2, ret, i;
-    uint64_t *new_l1_table;
-    int64_t new_l1_table_offset;
-    uint8_t data[12];
+    int new_size, new_size2, ret, i;
+    uint64_t *new_table;
+    int64_t new_table_offset;
 
-    if (min_size <= s->l1_size)
+    if (min_size <= *table_size) {
         return 0;
+    }
 
     if (exact_size) {
-        new_l1_size = min_size;
+        new_size = min_size;
     } else {
         /* Bump size up to reduce the number of times we have to grow */
-        new_l1_size = s->l1_size;
-        if (new_l1_size == 0) {
-            new_l1_size = 1;
+        new_size = *table_size;
+        if (new_size == 0) {
+            new_size = 1;
         }
-        while (min_size > new_l1_size) {
-            new_l1_size = (new_l1_size * 3 + 1) / 2;
+        while (min_size > new_size) {
+            new_size = (new_size * 3 + 1) / 2;
         }
     }
 
 #ifdef DEBUG_ALLOC2
-    fprintf(stderr, "grow l1_table from %d to %d\n", s->l1_size, new_l1_size);
+    fprintf(stderr, "grow %s_table from %d to %d\n",
+            table_name, *table_size, new_size);
 #endif
 
-    new_l1_size2 = sizeof(uint64_t) * new_l1_size;
-    new_l1_table = g_malloc0(align_offset(new_l1_size2, 512));
-    memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
+    new_size2 = sizeof(uint64_t) * new_size;
+    new_table = g_malloc0(align_offset(new_size2, 512));
+    memcpy(new_table, *table, *table_size * sizeof(uint64_t));
 
     /* write new table (align to cluster) */
     BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
-    new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
-    if (new_l1_table_offset < 0) {
-        g_free(new_l1_table);
-        return new_l1_table_offset;
+    new_table_offset = qcow2_alloc_clusters(bs, new_size2);
+    if (new_table_offset < 0) {
+        g_free(new_table);
+        return new_table_offset;
     }
 
     ret = qcow2_cache_flush(bs, s->refcount_block_cache);
@@ -75,34 +79,56 @@  int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size)
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_WRITE_TABLE);
-    for(i = 0; i < s->l1_size; i++)
-        new_l1_table[i] = cpu_to_be64(new_l1_table[i]);
-    ret = bdrv_pwrite_sync(bs->file, new_l1_table_offset, new_l1_table, new_l1_size2);
+    for (i = 0; i < *table_size; i++) {
+        new_table[i] = cpu_to_be64(new_table[i]);
+    }
+    ret = bdrv_pwrite_sync(bs->file, new_table_offset, new_table, new_size2);
     if (ret < 0)
         goto fail;
-    for(i = 0; i < s->l1_size; i++)
-        new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
+    for (i = 0; i < *table_size; i++) {
+        new_table[i] = be64_to_cpu(new_table[i]);
+    }
+
+    g_free(*table);
+    qcow2_free_clusters(bs, *table_offset, *table_size * sizeof(uint64_t));
+    *table_offset = new_table_offset;
+    *table = new_table;
+    *table_size = new_size;
 
     /* set new table */
     BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ACTIVATE_TABLE);
-    cpu_to_be32w((uint32_t*)data, new_l1_size);
-    cpu_to_be64wu((uint64_t*)(data + 4), new_l1_table_offset);
-    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size), data,sizeof(data));
-    if (ret < 0) {
-        goto fail;
-    }
-    g_free(s->l1_table);
-    qcow2_free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t));
-    s->l1_table_offset = new_l1_table_offset;
-    s->l1_table = new_l1_table;
-    s->l1_size = new_l1_size;
+    save_table(bs, *table_offset, *table_size);
+
     return 0;
  fail:
-    g_free(new_l1_table);
-    qcow2_free_clusters(bs, new_l1_table_offset, new_l1_size2);
+    g_free(new_table);
+    qcow2_free_clusters(bs, new_table_offset, new_size2);
     return ret;
 }
 
+static int qcow2_l1_save_table(BlockDriverState *bs,
+                               int64_t table_offset, int size)
+{
+    uint8_t data[12];
+    cpu_to_be32w((uint32_t *)data, size);
+    cpu_to_be64wu((uint64_t *)(data + 4), table_offset);
+    return bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, l1_size),
+                            data, sizeof(data));
+}
+
+int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size)
+{
+    BDRVQcowState *s = bs->opaque;
+    return qcow2_do_grow_table(bs,
+                               min_size,
+                               exact_size,
+                               &s->l1_table,
+                               &s->l1_table_offset,
+                               &s->l1_size,
+                               qcow2_l1_save_table,
+                               "l1");
+}
+
 /*
  * l2_load
  *
diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index 45b2326..de1b366 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -575,7 +575,6 @@  exit:
     return deduped_clusters_nr * s->cluster_sectors - begining_index;
 }
 
-
 /* Create a deduplication table hash block, write it's offset to disk and
  * reference it in the RAM deduplication table
  *
@@ -592,7 +591,7 @@  static uint64_t qcow2_create_block(BlockDriverState *bs,
     uint64_t data64;
     int ret = 0;
 
-    /* allocate a new dedup table hash block */
+    /* allocate a new dedup table cluster */
     offset = qcow2_alloc_clusters(bs, s->hash_block_size);
 
     if (offset < 0) {
diff --git a/block/qcow2.h b/block/qcow2.h
index 3b076db..fdba995 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -409,6 +409,12 @@  int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
     int64_t offset, int64_t length, int addend);
 
 /* qcow2-cluster.c functions */
+typedef int (*qcow2_save_table)(BlockDriverState *bs,
+                                int64_t table_offset, int size);
+int qcow2_do_grow_table(BlockDriverState *bs, int min_size, bool exact_size,
+                        uint64_t **table, uint64_t *table_offset,
+                        int *table_size, qcow2_save_table save_table,
+                        const char *table_name);
 int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size);
 void qcow2_l2_cache_reset(BlockDriverState *bs);
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);