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

[Max Reitz <mreitz@redhat.com>]

On 2015-02-04 at 11:06, Kevin Wolf wrote:
> 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().

Will remove.

>> @@ -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 :-)).

You're right. Now I'm wondering why it worked, though. Maybe  the tests 
only covered the refblock_index == 0 case...

Good catch, thanks!

Max

>>           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