Re: [Qemu-devel] [PATCH v2 18/21] qcow2: Add function for refcount order amendment

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5702 bytes --]

On 11/14/2014 06:06 AM, Max Reitz wrote:
> Add a function qcow2_change_refcount_order() which allows changing the
> refcount order of a qcow2 image.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 457 +++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h          |   4 +
>  2 files changed, 461 insertions(+)
> 

> +static int walk_over_reftable(BlockDriverState *bs, uint64_t **new_reftable,

> +
> +        status_cb(bs, (uint64_t)index * s->refcount_table_size + reftable_index,
> +                  (uint64_t)total * s->refcount_table_size, cb_opaque);

Not sure if the casts are needed (isn't s->refcount_table_size already
uint64_t, and 'int * uint64_t' does the right thing); but I guess it
doesn't hurt to leave them.

> +int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
> +                                BlockDriverAmendStatusCB *status_cb,
> +                                void *cb_opaque, Error **errp)
> +{

> +    do {
> +        int total_walks;
> +
> +        new_allocation = false;
> +
> +        /* At least we have to do this walk and the one which writes the
> +         * refblocks; also, at least we have to do this loop here at least
> +         * twice (normally), first to do the allocations, and second to
> +         * determine that everything is correctly allocated, this then makes
> +         * three walks in total */
> +        total_walks = MIN(walk_index + 2, 3);

This feels wrong...

> +
> +        /* First, allocate the structures so they are present in the refcount
> +         * structures */
> +        ret = walk_over_reftable(bs, &new_reftable, &new_reftable_index,
> +                                 &new_reftable_size, NULL, new_refblock_size,
> +                                 new_refcount_bits, &alloc_refblock,
> +                                 &new_allocation, NULL, status_cb, cb_opaque,
> +                                 walk_index++, total_walks, errp);

...In the common case of just two iterations of the do loop (second
iteration confirms no allocations needed), you call with index 0/2, 1/3,
and then the later non-allocation walk is index 2/3.

In the rare case of three iterations of the do loop, you call with index
0/2, 1/3, 2/3, and then the later non-allocation walk is 3/4.

I highly doubt that it is possible to trigger four iterations of the do
loop, but if it were, you would call with 0/2, 1/3, 2/3, 3/3, and then 4/5.

I think you instead want to have:

total_walks = MAX(walk_index + 2, 3)

then the common case will call with 0/3, 1/3, and the later walk as 2/3

the three-iteration loop will call with 0/3, 1/3, 2/4, and the later
walk as 3/4

the unlikely four-iteration loop will call with 0/3, 1/3, 2/4, 3/5, and
the later walk as 4/5.

> +
> +        new_reftable_index = 0;
> +
> +        if (new_allocation) {
> +            if (new_reftable_offset) {
> +                qcow2_free_clusters(bs, new_reftable_offset,
> +                                    allocated_reftable_size * sizeof(uint64_t),
> +                                    QCOW2_DISCARD_NEVER);

Any reason you picked QCOW2_DISCARD_NEVER instead of some other policy?
 Why not punch holes in the file when throwing out a failed too-small
new table, or when cleaning up the old table once the new table is good?

> +            }
> +
> +            new_reftable_offset = qcow2_alloc_clusters(bs, new_reftable_size *
> +                                                           sizeof(uint64_t));
> +            if (new_reftable_offset < 0) {
> +                error_setg_errno(errp, -new_reftable_offset,
> +                                 "Failed to allocate the new reftable");
> +                ret = new_reftable_offset;
> +                goto done;
> +            }
> +            allocated_reftable_size = new_reftable_size;
> +
> +            new_allocation = true;

This assignment is dead code (it already occurs inside an 'if
(new_allocation)' condition).

> +        }
> +    } while (new_allocation);
> +
> +    /* Second, write the new refblocks */
> +    new_allocation = false;

This assignment is dead code (it can only be reached if the earlier do
loop ended, which is only possible when no allocations are recorded).

> +    ret = walk_over_reftable(bs, &new_reftable, &new_reftable_index,
> +                             &new_reftable_size, new_refblock,
> +                             new_refblock_size, new_refcount_bits,
> +                             &flush_refblock, &new_allocation, new_set_refcount,
> +                             status_cb, cb_opaque, walk_index, walk_index + 1,
> +                             errp);
> +    if (ret < 0) {
> +        goto done;
> +    }
> +    assert(!new_allocation);
> +

Correct.

> +done:
> +    if (new_reftable) {
> +        /* On success, new_reftable actually points to the old reftable (and
> +         * new_reftable_size is the old reftable's size); but that is just
> +         * fine */
> +        for (i = 0; i < new_reftable_size; i++) {
> +            uint64_t offset = new_reftable[i] & REFT_OFFSET_MASK;
> +            if (offset) {
> +                qcow2_free_clusters(bs, offset, s->cluster_size,
> +                                    QCOW2_DISCARD_NEVER);

Again, why the QCOW2_DISCARD_NEVER policy?

Fix the MIN vs. MAX bug, and the two dead assignment statements, and you
can have:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]