Re: [Qemu-devel] [RFC V6 19/33] block: Add qcow2_dedup format and image creation code.

[Stefan Hajnoczi]

On Wed, Feb 06, 2013 at 01:31:52PM +0100, Benoît Canet wrote:
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ad202fa..9cbb2f0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -277,6 +277,11 @@ int qcow2_mark_dirty(BlockDriverState *bs)
>      return qcow2_add_feature(bs, QCOW2_INCOMPAT_DIRTY);
>  }
>  
> +static int qcow2_activate_dedup(BlockDriverState *bs)
> +{
> +    return qcow2_add_feature(bs, QCOW2_INCOMPAT_DEDUP);
> +}

I suggest dropping this wrapper function, what "activiate dedup" means
is not clear.  It turns out it simply sets the feature bit in the file
header, nothing else.

Best to set the feature bit directly so save readers from having to jump
to the definition of qcow2_activate_dedup().

> @@ -1371,11 +1381,29 @@ static int qcow2_create2(const char *filename, 
> int64_t total_size,
>      }
>  
>      /* Okay, now that we have a valid image, let's give it the right size */
> +    s = bs->opaque;
>      ret = bdrv_truncate(bs, total_size * BDRV_SECTOR_SIZE);
>      if (ret < 0) {
>          goto out;
>      }
>  
> +    if (dedup) {

BDRVQcowState *s = bs->opaque;

A local variable here would be nicer than at function scope.

> @@ -1447,24 +1501,42 @@ static int qcow2_create(const char *filename, 
> QEMUOptionParameter *options)
>              }
>          } else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
>              flags |= options->value.n ? BLOCK_FLAG_LAZY_REFCOUNTS : 0;
> +        } else if (!strcmp(options->name, BLOCK_OPT_DEDUP)) {
> +            hash_algo = qcow2_get_dedup_hash_algo(options->value.s);
> +            if (hash_algo < 0) {
> +                return hash_algo;
> +            }
> +            dedup = true;
>          }
>          options++;
>      }
>  
> +    if (dedup) {
> +        version = 3;
> +    }

Lazy refcounts don't force the version.  It would be consistent to
refrain from forcing the version too.

> @@ -1809,9 +1931,49 @@ static BlockDriver bdrv_qcow2 = {
>      .bdrv_check = qcow2_check,
>  };
>  
> +static BlockDriver bdrv_qcow2_dedup = {

Missing comment explaining the need to duplicate the BlockDriver for
dedup.