[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC V6 10/33] qcow2: Extract qcow2_dedup_grow_table
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [RFC V6 10/33] qcow2: Extract qcow2_dedup_grow_table |
Date: |
Thu, 7 Feb 2013 10:26:22 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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?
- Re: [Qemu-devel] [RFC V6 26/33] qcow2: Add verification of dedup table., (continued)
- [Qemu-devel] [RFC V6 23/33] qcow2: Add qcow2_dedup_is_running to probe if dedup is running., Benoît Canet, 2013/02/06
- [Qemu-devel] [RFC V6 24/33] qcow2: Integrate deduplication in qcow2_co_writev loop., Benoît Canet, 2013/02/06
- [Qemu-devel] [RFC V6 14/33] qcow2: Create qcow2_is_cluster_to_dedup., Benoît Canet, 2013/02/06
- [Qemu-devel] [RFC V6 28/33] qcow2: Add check_dedup_l2 in order to check l2 of dedup table., Benoît Canet, 2013/02/06
- [Qemu-devel] [RFC V6 09/33] qcow2: Implement qcow2_compute_cluster_hash., Benoît Canet, 2013/02/06
- [Qemu-devel] [RFC V6 10/33] qcow2: Extract qcow2_dedup_grow_table, Benoît Canet, 2013/02/06
- Re: [Qemu-devel] [RFC V6 10/33] qcow2: Extract qcow2_dedup_grow_table,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [RFC V6 00/33] QCOW2 deduplication core functionality, Stefan Hajnoczi, 2013/02/08