Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters

[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.

The offset != 0 check ends up redundant when you introduce the switch,
so in theory it could as well be removed.

Kevin