[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 18/21] qcow2: Add function for refcount order
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 18/21] qcow2: Add function for refcount order amendment |
Date: |
Tue, 18 Nov 2014 10:55:04 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v2 11/21] iotests: Prepare for refcount_width option, (continued)
- [Qemu-devel] [PATCH v2 12/21] qcow2: Allow creation with refcount order != 4, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 10/21] qcow2: refcount_order parameter for qcow2_create2, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 13/21] block: Add opaque value to the amend CB, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 15/21] qcow2: Use abort() instead of assert(false), Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 16/21] qcow2: Split upgrade/downgrade paths for amend, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 14/21] qcow2: Use error_report() in qcow2_amend_options(), Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 17/21] qcow2: Use intermediate helper CB for amend, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 18/21] qcow2: Add function for refcount order amendment, Max Reitz, 2014/11/14
- Re: [Qemu-devel] [PATCH v2 18/21] qcow2: Add function for refcount order amendment,
Eric Blake <=
- [Qemu-devel] [PATCH v2 19/21] qcow2: Invoke refcount order amendment function, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 20/21] qcow2: Point to amend function in check, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 21/21] iotests: Add test for different refcount widths, Max Reitz, 2014/11/14
- Re: [Qemu-devel] [PATCH v2 21/21] iotests: Add test for different refcount widths, Max Reitz, 2014/11/17