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).
+ 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?