Re: [Qemu-devel] [RFC V6 13/33] qcow2: make the deduplication forget a cluster hash when a cluster is to dedupe

[Stefan Hajnoczi]

On Wed, Feb 06, 2013 at 01:31:46PM +0100, Benoît Canet wrote:
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ef91216..5b1d20d 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -710,6 +710,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
> QCowL2Meta *m)
>  
>      for (i = 0; i < m->nb_clusters; i++) {
>          uint64_t flags = 0;
> +        uint64_t offset = cluster_offset + (i << s->cluster_bits);
>          /* if two concurrent writes happen to the same unallocated cluster
>        * each write allocates separate cluster and writes data concurrently.
>        * The first one to complete updates l2 table with pointer to its
> @@ -722,8 +723,14 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
> QCowL2Meta *m)
>          flags = m->oflag_copied ? QCOW_OFLAG_COPIED : 0;
>          flags |= m->to_deduplicate ? QCOW_OFLAG_TO_DEDUP : 0;
>  
> -        l2_table[l2_index + i] = cpu_to_be64((cluster_offset +
> -                    (i << s->cluster_bits)) | flags);
> +        l2_table[l2_index + i] = cpu_to_be64(offset | flags);
> +
> +        /* make the deduplication forget the cluster to avoid making
> +         * the dedup pointing to a cluster that has changed on it's back.
> +         */
> +        if (m->to_deduplicate) {
> +            qcow2_dedup_forget_cluster_by_sector(bs, offset >> 9);
> +        }

This does not play well with internal snapshots.  Imagine that an
internal snapshot was taken, so refcount == 2.

Now the cluster is overwritten by the guest but we still need to hang on
to the original data since the snapshot refers to it.

If dedup forgets about the cluster then dedup is only effective for the
current disk image, but it ignores snapshots.  Ideally dedup would take
snapshots into account since they share the same data clusters as the
current image.

Stefan