* [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
@ 2013-08-29 10:16 Max Reitz
2013-08-29 12:30 ` Eric Blake
2013-08-29 12:33 ` Kevin Wolf
0 siblings, 2 replies; 9+ messages in thread
From: Max Reitz @ 2013-08-29 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
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(-)
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) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
2013-08-29 10:16 [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters Max Reitz
@ 2013-08-29 12:30 ` Eric Blake
2013-08-29 12:35 ` Max Reitz
2013-08-29 12:33 ` Kevin Wolf
1 sibling, 1 reply; 9+ messages in thread
From: Eric Blake @ 2013-08-29 12:30 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 413 bytes --]
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)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
2013-08-29 10:16 [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters Max Reitz
2013-08-29 12:30 ` Eric Blake
@ 2013-08-29 12:33 ` Kevin Wolf
2013-08-29 12:38 ` Max Reitz
1 sibling, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2013-08-29 12:33 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
2013-08-29 12:30 ` Eric Blake
@ 2013-08-29 12:35 ` Max Reitz
0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2013-08-29 12:35 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
2013-08-29 12:33 ` Kevin Wolf
@ 2013-08-29 12:38 ` Max Reitz
2013-08-29 12:53 ` Kevin Wolf
0 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2013-08-29 12:38 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
2013-08-29 12:38 ` Max Reitz
@ 2013-08-29 12:53 ` Kevin Wolf
2013-08-29 12:56 ` Max Reitz
0 siblings, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2013-08-29 12:53 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
2013-08-29 12:53 ` Kevin Wolf
@ 2013-08-29 12:56 ` Max Reitz
2013-08-29 13:04 ` Kevin Wolf
0 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2013-08-29 12:56 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
2013-08-29 12:56 ` Max Reitz
@ 2013-08-29 13:04 ` Kevin Wolf
2013-08-29 13:06 ` Max Reitz
0 siblings, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2013-08-29 13:04 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters
2013-08-29 13:04 ` Kevin Wolf
@ 2013-08-29 13:06 ` Max Reitz
0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2013-08-29 13:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-08-29 13:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-29 10:16 [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters Max Reitz
2013-08-29 12:30 ` Eric Blake
2013-08-29 12:35 ` Max Reitz
2013-08-29 12:33 ` Kevin Wolf
2013-08-29 12:38 ` Max Reitz
2013-08-29 12:53 ` Kevin Wolf
2013-08-29 12:56 ` Max Reitz
2013-08-29 13:04 ` Kevin Wolf
2013-08-29 13:06 ` Max Reitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).