Am 29.08.2013 14:53, schrieb Kevin Wolf:
Am 29.08.2013 um 14:38 hat Max Reitz geschrieben:
Am 29.08.2013 14:33, schrieb Kevin Wolf:
Am 29.08.2013 um 12:16 hat Max Reitz geschrieben:
Do not try to update the refcount for zero clusters in
qcow2_update_snapshot_refcount.
Signed-off-by: Max Reitz <address@hidden>
---
block/qcow2-refcount.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
Please don't forget to add a test case for v2.
Okay.
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 1244693..7555242 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
for(j = 0; j < s->l2_size; j++) {
offset = be64_to_cpu(l2_table[j]);
if (offset != 0) {
+ uint64_t cluster_index;
+
old_offset = offset;
offset &= ~QCOW_OFLAG_COPIED;
- if (offset & QCOW_OFLAG_COMPRESSED) {
+
+ switch (qcow2_get_cluster_type(offset)) {
+ case QCOW2_CLUSTER_COMPRESSED:
nb_csectors = ((offset >> s->csize_shift) &
s->csize_mask) + 1;
if (addend != 0) {
@@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
}
/* compressed clusters are never modified */
refcount = 2;
- } else {
- uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >>
s->cluster_bits;
+ break;
+
+ case QCOW2_CLUSTER_NORMAL:
+ cluster_index = (offset & L2E_OFFSET_MASK) >>
s->cluster_bits;
if (addend != 0) {
refcount = update_cluster_refcount(bs,
cluster_index, addend,
QCOW2_DISCARD_SNAPSHOT);
@@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
ret = refcount;
goto fail;
}
+ break;
I think this part isn't quite right. Even zero clusters can have an
allocated offset, so I think what's really needed is that the same code
runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0)
case for the unallocated case.
So then it would be enough to change:
- if (offset != 0) {
+ if ((offset & L2E_OFFSET_MASK) != 0) {
and just execute the same code for QCOW2_CLUSTER_NORMAL and
QCOW2_CLUSTER_ZERO?
Doing only this change in the original code might happen to work, but
it's not really clean as the L2 entry format is different for compressed
clusters (L2E_OFFSET_MASK isn't valid for it). So having the switch for
the cluster type is a good thing, but the implementation for normal and
zero clusters should be shared indeed.
Yes, of course I meant to include the switch as well.
The offset != 0 check ends up redundant when you introduce the switch,
so in theory it could as well be removed.
Well, I don't know; for zero clusters, the if statement is required
anyway (since qcow2_get_cluster_type does not distinguish between
allocated and non-allocated zero clusters), so I though we might as
well just leave it where it is (just slightly adjusted in the way I
proposed).