Re: [Qemu-devel] [RFC V6 33/33] qcow2: Add qcow2_dedup_init and qcow2_dedup_close.

[Stefan Hajnoczi]

On Wed, Feb 06, 2013 at 01:32:06PM +0100, Benoît Canet wrote:
> +static int qcow2_dedup_alloc(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int ret;
> +
> +    ret = qcow2_do_table_init(bs,
> +                              &s->dedup_table,
> +                              s->dedup_table_offset,
> +                              s->dedup_table_size,
> +                              false);
> +
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    s->dedup_tree_by_hash = g_tree_new_full(qcow2_dedup_compare_by_hash, 
> NULL,
> +                                            NULL,
> +                                            
> qcow2_dedup_destroy_qcow_hash_node);
> +    s->dedup_tree_by_sect = g_tree_new_full(qcow2_dedup_compare_by_offset,
> +                                              NULL, NULL, NULL);
> +
> +    s->dedup_cluster_cache = qcow2_cache_create(bs, DEDUP_CACHE_SIZE,
> +                                                s->hash_block_size);

I don't remember any patches that set a dependency on other caches.
This suggests that the write ordering for metadata updates hasn't been
considered yet.

The cache dependencies allow ordering to be expressed between caches.
This way, the refcounts will always be written before the L2 table
updates, for example.

> +
> +    return 0;
> +}
> +
> +static void qcow2_dedup_free(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    g_free(s->dedup_table);
> +
> +    qcow2_cache_flush(bs, s->dedup_cluster_cache);
> +    qcow2_cache_destroy(bs, s->dedup_cluster_cache);
> +    g_tree_destroy(s->dedup_tree_by_sect);
> +    g_tree_destroy(s->dedup_tree_by_hash);
> +}
> +
> +int qcow2_dedup_init(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int ret = 0;
> +
> +    s->has_dedup = true;
> +
> +    ret = qcow2_dedup_alloc(bs);
> +
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* if we are read-only we don't load the deduplication table */
> +    if (bs->read_only) {
> +        return 0;

Might be worth mentioning that has_dedup remains set to true here so
that .bdrv_check() still looks into the dedup tables for read-only
images (qemu-img check is read-only by default).