[Qemu-devel] Re: [patch 4/5][v3] Aggregate same type clusters.

[Kevin Wolf <kwolf@suse.de>]

Laurent.Vivier@bull.net schrieb:
> Modify get_cluster_offset(), alloc_cluster_offset() to specify how many clusters
> we want.
> 
> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
> ---
>  block-qcow2.c |  236 ++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 182 insertions(+), 54 deletions(-)
> 
> Index: qemu/block-qcow2.c
> ===================================================================
> --- qemu.orig/block-qcow2.c	2008-08-12 12:49:24.000000000 +0200
> +++ qemu/block-qcow2.c	2008-08-12 15:26:58.000000000 +0200
> @@ -52,6 +52,8 @@
>  #define QCOW_CRYPT_NONE 0
>  #define QCOW_CRYPT_AES  1
>  
> +#define QCOW_MAX_CRYPT_CLUSTERS 32
> +
>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1LL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) */
> @@ -263,7 +265,8 @@ static int qcow_open(BlockDriverState *b
>      if (!s->cluster_cache)
>          goto fail;
>      /* one more sector for decompressed data alignment */
> -    s->cluster_data = qemu_malloc(s->cluster_size + 512);
> +    s->cluster_data = qemu_malloc(QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
> +                                  + 512);
>      if (!s->cluster_data)
>          goto fail;
>      s->cluster_cache_offset = -1;
> @@ -613,43 +616,98 @@ static uint64_t *l2_allocate(BlockDriver
>   * For a given offset of the disk image, return cluster offset in
>   * qcow2 file.
>   *
> + * on entry, *num is the number of contiguous clusters we'd like to
> + * access following offset.
> + *
> + * on exit, *num is the number of contiguous clusters we can read.
> + *
>   * Return 1, if the offset is found
>   * Return 0, otherwise.
>   *
>   */
>  
> -static uint64_t get_cluster_offset(BlockDriverState *bs, uint64_t offset)
> +static uint64_t get_cluster_offset(BlockDriverState *bs,
> +                                   uint64_t offset, int *num)
>  {
>      BDRVQcowState *s = bs->opaque;
>      int l1_index, l2_index;
> -    uint64_t l2_offset, *l2_table, cluster_offset;
> +    uint64_t l2_offset, *l2_table, cluster_offset, next;
> +    int l1_bits;
> +    int index_in_cluster, nb_available, nb_needed;
> +
> +    index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
> +    nb_needed = *num + index_in_cluster;
> +
> +    l1_bits = s->l2_bits + s->cluster_bits;
> +
> +    /* compute how many bytes there are between the offset and
> +     * and the end of the l1 entry
> +     */
> +
> +    nb_available = (1 << l1_bits) - (offset & ((1 << l1_bits) - 1));
> +
> +    /* compute the number of available sectors */
> +
> +    nb_available = (nb_available >> 9) + index_in_cluster;
> +
> +    cluster_offset = 0;
>  
>      /* seek the the l2 offset in the l1 table */
>  
> -    l1_index = offset >> (s->l2_bits + s->cluster_bits);
> +    l1_index = offset >> l1_bits;
>      if (l1_index >= s->l1_size)
> -        return 0;
> +        goto out;
>  
>      l2_offset = s->l1_table[l1_index];
>  
>      /* seek the l2 table of the given l2 offset */
>  
>      if (!l2_offset)
> -        return 0;
> +        goto out;
>  
>      /* load the l2 table in memory */
>  
>      l2_offset &= ~QCOW_OFLAG_COPIED;
>      l2_table = l2_load(bs, l2_offset);
>      if (l2_table == NULL)
> -        return 0;
> +        goto out;

You agreed that return 0 is actually the right thing to do here because
this is a real error.


>  
>      /* find the cluster offset for the given disk offset */
>  
>      l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
>      cluster_offset = be64_to_cpu(l2_table[l2_index]);
> +    nb_available = s->cluster_sectors;
> +    l2_index++;
> +
> +    if (!cluster_offset) {
>  
> -    return cluster_offset & ~QCOW_OFLAG_COPIED;
> +       /* how many empty clusters ? */
> +
> +       while (nb_available < nb_needed && !l2_table[l2_index]) {
> +           l2_index++;
> +           nb_available += s->cluster_sectors;
> +       }
> +    } else {
> +
> +       /* how many allocated clusters ? */
> +
> +       cluster_offset &= ~QCOW_OFLAG_COPIED;
> +       while (nb_available < nb_needed) {
> +           next = be64_to_cpu(l2_table[l2_index]) & ~QCOW_OFLAG_COPIED;
> +           if (next != cluster_offset + (nb_available << 9))
> +               break;
> +           l2_index++;
> +           nb_available += s->cluster_sectors;
> +       }
> +   }
> +
> +out:
> +    if (nb_available > nb_needed)
> +        nb_available = nb_needed;
> +
> +    *num = nb_available - index_in_cluster;
> +
> +    return cluster_offset;
>  }
>  
>  /*
> @@ -660,13 +718,10 @@ static uint64_t get_cluster_offset(Block
>   */
>  
>  static void free_any_clusters(BlockDriverState *bs,
> -                              uint64_t cluster_offset)
> +                              uint64_t cluster_offset, int nb_clusters)
>  {
>      BDRVQcowState *s = bs->opaque;
>  
> -    if (cluster_offset == 0)
> -        return;
> -

I don't think this check hurts. Even if this should never happen anyway.

The rest of it looks good.

Kevin