[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero cluste
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters |
Date: |
Thu, 29 Aug 2013 15:04:37 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 29.08.2013 um 14:56 hat Max Reitz geschrieben:
> 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).
You do need a zero check, but it's only for normal/zero clusters, not
for compressed ones; and it must check offset & L2E_OFFSET_MASK (or
cluster_index) rather than offset itself.
So I think you end up removing the outer if (offset != 0) and adding a
new inner check in case QCOW2_CLUSTER_NORMAL/ZERO.
Kevin