Re: [Qemu-devel] [PATCH v5 10/26] qcow2: Helper function for refcount modification

[Kevin Wolf <kwolf@redhat.com>]

Am 15.12.2014 um 13:50 hat Max Reitz geschrieben:
> Since refcounts do not always have to be a uint16_t, all refcount blocks
> and arrays in memory should not have a specific type (thus they become
> pointers to void) and for accessing them, two helper functions are used
> (a getter and a setter). Those functions are called indirectly through
> function pointers in the BDRVQcowState so they may later be exchanged
> for different refcount orders.
> 
> With the check and repair functions using this function, the refcount
> array they are creating will be in big endian byte order; additionally,
> using realloc_refcount_array() makes the size of this refcount array
> always cluster-aligned. Both combined allow rebuild_refcount_structure()
> to drop the bounce buffer which was used to convert parts of the
> refcount array to big endian byte order and store them on disk. Instead,
> those parts can now be written directly.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/qcow2-refcount.c | 122 ++++++++++++++++++++++++++++---------------------
>  block/qcow2.h          |   8 ++++
>  2 files changed, 79 insertions(+), 51 deletions(-)

> @@ -541,7 +561,7 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int64_t start, last, cluster_offset;
> -    uint16_t *refcount_block = NULL;
> +    void *refcount_block = NULL;
>      int64_t old_table_index = -1;
>      int ret;
>  
> @@ -593,7 +613,7 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
>          /* we can update the count and save it */
>          block_index = cluster_index & (s->refcount_block_size - 1);
>  
> -        refcount = be16_to_cpu(refcount_block[block_index]);
> +        refcount = s->get_refcount(refcount_block, block_index);
>          if (decrease ? (refcount - addend > refcount)
>                       : (refcount + addend < refcount ||
>                          refcount + addend > s->refcount_max))
> @@ -609,7 +629,7 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
>          if (refcount == 0 && cluster_index < s->free_cluster_index) {
>              s->free_cluster_index = cluster_index;
>          }
> -        refcount_block[block_index] = cpu_to_be16(refcount);
> +        s->set_refcount(refcount_block, block_index, refcount);
>  
>          if (refcount == 0 && s->discard_passthrough[type]) {
>              update_refcount_discard(bs, cluster_offset, s->cluster_size);
> @@ -625,8 +645,7 @@ fail:
>      /* Write last changed block to disk */
>      if (refcount_block) {
>          int wret;
> -        wret = qcow2_cache_put(bs, s->refcount_block_cache,
> -            (void**) &refcount_block);
> +        wret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>          if (wret < 0) {
>              return ret < 0 ? ret : wret;
>          }

There is a (void**) cast left in the other qcow2_cache_put() call in
update_refcount().

> @@ -1883,7 +1907,7 @@ static int64_t alloc_clusters_imrt(BlockDriverState *bs,
>   */
>  static int rebuild_refcount_structure(BlockDriverState *bs,
>                                        BdrvCheckResult *res,
> -                                      uint16_t **refcount_table,
> +                                      void **refcount_table,
>                                        int64_t *nb_clusters)
>  {
>      BDRVQcowState *s = bs->opaque;
> @@ -1891,8 +1915,8 @@ static int rebuild_refcount_structure(BlockDriverState *bs,
>      int64_t refblock_offset, refblock_start, refblock_index;
>      uint32_t reftable_size = 0;
>      uint64_t *on_disk_reftable = NULL;
> -    uint16_t *on_disk_refblock;
> -    int i, ret = 0;
> +    void *on_disk_refblock;
> +    int ret = 0;
>      struct {
>          uint64_t reftable_offset;
>          uint32_t reftable_clusters;
> @@ -1902,7 +1926,7 @@ static int rebuild_refcount_structure(BlockDriverState *bs,
>  
>  write_refblocks:
>      for (; cluster < *nb_clusters; cluster++) {
> -        if (!(*refcount_table)[cluster]) {
> +        if (!s->get_refcount(*refcount_table, cluster)) {
>              continue;
>          }
>  
> @@ -1975,17 +1999,13 @@ write_refblocks:
>              goto fail;
>          }
>  
> -        on_disk_refblock = qemu_blockalign0(bs->file, s->cluster_size);
> -        for (i = 0; i < s->refcount_block_size &&
> -                    refblock_start + i < *nb_clusters; i++)
> -        {
> -            on_disk_refblock[i] =
> -                cpu_to_be16((*refcount_table)[refblock_start + i]);
> -        }
> +        /* The size of *refcount_table is always cluster-aligned, therefore the
> +         * write operation will not overflow */
> +        on_disk_refblock = (void *)((uintptr_t)*refcount_table +
> +                                    (refblock_index << s->refcount_block_bits));

Are you sure about s->refcount_block_bits? You're now calculating in
bytes and not in entries any more like before with uint16_t*. So I think
this should be s->cluster_bits now (or refblock_index * s->cluster_size
if you don't want to confuse people more than necessary :-)).

>          ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE,
> -                         (void *)on_disk_refblock, s->cluster_sectors);
> -        qemu_vfree(on_disk_refblock);
> +                         on_disk_refblock, s->cluster_sectors);
>          if (ret < 0) {
>              fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
>              goto fail;

Kevin