Patchwork qcow2-refcount: Snapshot update for zero clusters

login
register
mail settings
Submitter Max Reitz
Date Aug. 29, 2013, 10:16 a.m.
Message ID <1377771363-5198-1-git-send-email-mreitz@redhat.com>
Download mbox | patch
Permalink /patch/270756/
State New
Headers show

Comments

Max Reitz - Aug. 29, 2013, 10:16 a.m.
Do not try to update the refcount for zero clusters in
qcow2_update_snapshot_refcount.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
Eric Blake - Aug. 29, 2013, 12:30 p.m.
On 08/29/2013 04:16 AM, Max Reitz wrote:
> Do not try to update the refcount for zero clusters in
> qcow2_update_snapshot_refcount.

Why? What does this fix?

(Hint - your commit message could use more details, and you should add a
testsuite addition that exposes the case that you are fixing)
Kevin Wolf - Aug. 29, 2013, 12:33 p.m.
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 <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)

Please don't forget to add a test case for v2.

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

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

>                      }
>  
>                      if (refcount == 1) {

Kevin
Max Reitz - Aug. 29, 2013, 12:35 p.m.
Am 29.08.2013 14:30, schrieb Eric Blake:
> On 08/29/2013 04:16 AM, Max Reitz wrote:
>> Do not try to update the refcount for zero clusters in
>> qcow2_update_snapshot_refcount.
> Why? What does this fix?
>
> (Hint - your commit message could use more details, and you should add a
> testsuite addition that exposes the case that you are fixing)
>
Okay, I'll put more detail into the message.

The test case is actually kind of contained in my series "block/qcow2: 
Image file option amendment", but, of course, you're right, a dedicated 
test case won't hurt.


Max
Max Reitz - Aug. 29, 2013, 12:38 p.m.
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 <mreitz@redhat.com>
>> ---
>>   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
Kevin Wolf - Aug. 29, 2013, 12:53 p.m.
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 <mreitz@redhat.com>
> >>---
> >>  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
Max Reitz - Aug. 29, 2013, 12:56 p.m.
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 <mreitz@redhat.com>
>>>> ---
>>>>   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).


Max
Kevin Wolf - Aug. 29, 2013, 1:04 p.m.
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 <mreitz@redhat.com>
> >>>>---
> >>>>  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
Max Reitz - Aug. 29, 2013, 1:06 p.m.
Am 29.08.2013 15:04, schrieb Kevin Wolf:
> 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 <mreitz@redhat.com>
>>>>>> ---
>>>>>>   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
Ah, yes, you're right.

Max

Patch

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;
+
+                    default:
+                        refcount = 0;
                     }
 
                     if (refcount == 1) {