Message ID | 1360153926-9492-11-git-send-email-benoit@irqsave.net |
---|---|
State | New |
Headers | show |
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?
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);
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(-)