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

[Max Reitz]

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 <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > ---
> >   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