qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
Date: Thu, 29 Aug 2013 14:38:43 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130805 Thunderbird/17.0.8

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?

+
+                    default:
+                        refcount = 0;
Other places enumerate the enum values explicitly and have a default
label that simply abort()s. This will help catching forgotten places
when a new value is added.
Okay.

                      }
if (refcount == 1) {
Kevin
Max



reply via email to

[Prev in Thread] Current Thread [Next in Thread]