[Qemu-devel] [PATCH 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto

[Qemu-devel] [PATCH 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto

[Kevin Wolf]

When loading an internal snapshot whose L1 table is smaller than the current L1
table, the size of the current L1 would be shrunk to the snapshot's L1 size in
memory, but not on disk. This lead to incorrect refcount updates and eventuelly
to image corruption.

Instead of writing the new L1 size to disk, this simply retains the bigger L1
size that is currently in use and makes sure that the unused part is zeroed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---

Philipp, I think this should fix your corruption. Please give it a try.

Anthony, this must go into 0.15. Given the short time until -rc2, do you prefer
to pick it up directly or should I send a pull request tomorrow? The patch
looks obvious, is tested with the given testcase and survives a basic
qemu-iotests run (though qemu-iotests doesn't exercise snapshots a lot)

Stefan, please review :-)

 block/qcow2-snapshot.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 74823a5..4a8868a 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -330,8 +330,9 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0)
         goto fail;
 
-    s->l1_size = sn->l1_size;
+    memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size);
     l1_size2 = s->l1_size * sizeof(uint64_t);
+
     /* copy the snapshot l1 table to the current l1 table */
     if (bdrv_pread(bs->file, sn->l1_table_offset,
                    s->l1_table, l1_size2) != l1_size2)
-- 
1.7.6

[Qemu-devel] [PATCH v2 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto

[Kevin Wolf]

When loading an internal snapshot whose L1 table is smaller than the current L1
table, the size of the current L1 would be shrunk to the snapshot's L1 size in
memory, but not on disk. This lead to incorrect refcount updates and eventuelly
to image corruption.

Instead of writing the new L1 size to disk, this simply retains the bigger L1
size that is currently in use and makes sure that the unused part is zeroed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---

And the moment you send it out, you notice that it's wrong... *sigh*

v2:
- Check for s->l1_size > sn->l1_size in order to avoid disasters...

Philipp, I think this should fix your corruption. Please give it a try.

Anthony, this must go into 0.15. Given the short time until -rc2, do you prefer
to pick it up directly or should I send a pull request tomorrow? The patch
looks obvious, is tested with the given testcase and survives a basic
qemu-iotests run (though qemu-iotests doesn't exercise snapshots a lot)

Stefan, please review :-)

 block/qcow2-snapshot.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 74823a5..6972e66 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -330,8 +330,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0)
         goto fail;
 
-    s->l1_size = sn->l1_size;
+    if (s->l1_size > sn->l1_size) {
+        memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size);
+    }
     l1_size2 = s->l1_size * sizeof(uint64_t);
+
     /* copy the snapshot l1 table to the current l1 table */
     if (bdrv_pread(bs->file, sn->l1_table_offset,
                    s->l1_table, l1_size2) != l1_size2)
-- 
1.7.6

Re: [Qemu-devel] [PATCH v2 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto

[Philipp Hahn]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

Hello,

On Thursday 04 August 2011 18:24:59 Kevin Wolf wrote:
> When loading an internal snapshot whose L1 table is smaller than the
> current L1 table, the size of the current L1 would be shrunk to the
> snapshot's L1 size in memory, but not on disk. This lead to incorrect
> refcount updates and eventuelly to image corruption.
>
> Instead of writing the new L1 size to disk, this simply retains the bigger
> L1 size that is currently in use and makes sure that the unused part is
> zeroed.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Philipp Hahn <hahn@univention.de>

> Philipp, I think this should fix your corruption. Please give it a try.
Yes, the patch looks conceptually right and fixes the observed problem.

> Anthony, this must go into 0.15.
That bug is also found in 0.12.4 and 0.14.1, so if there ever shoudl be an 
update to those branches, that fix should be applied there as well.

Thanks for your fast support.

Sincerely
Philipp Hahn
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Qemu-devel] [PATCH v2 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto

[Frediano Ziglio]

2011/8/4 Kevin Wolf <kwolf@redhat.com>:
> When loading an internal snapshot whose L1 table is smaller than the current L1
> table, the size of the current L1 would be shrunk to the snapshot's L1 size in
> memory, but not on disk. This lead to incorrect refcount updates and eventuelly
> to image corruption.
>
> Instead of writing the new L1 size to disk, this simply retains the bigger L1
> size that is currently in use and makes sure that the unused part is zeroed.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>
> And the moment you send it out, you notice that it's wrong... *sigh*
>
> v2:
> - Check for s->l1_size > sn->l1_size in order to avoid disasters...
>
> Philipp, I think this should fix your corruption. Please give it a try.
>
> Anthony, this must go into 0.15. Given the short time until -rc2, do you prefer
> to pick it up directly or should I send a pull request tomorrow? The patch
> looks obvious, is tested with the given testcase and survives a basic
> qemu-iotests run (though qemu-iotests doesn't exercise snapshots a lot)
>
> Stefan, please review :-)
>
>  block/qcow2-snapshot.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 74823a5..6972e66 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -330,8 +330,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>     if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0)
>         goto fail;
>
> -    s->l1_size = sn->l1_size;
> +    if (s->l1_size > sn->l1_size) {
> +        memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size);
> +    }
>     l1_size2 = s->l1_size * sizeof(uint64_t);
> +
>     /* copy the snapshot l1 table to the current l1 table */
>     if (bdrv_pread(bs->file, sn->l1_table_offset,
>                    s->l1_table, l1_size2) != l1_size2)
> --
> 1.7.6
>

This patch looked odd at first sight. First a qcow2_grow_l1_table is
called to shrink L1 so perhaps should be qcow2_resize_l1_table.
Perhaps also it would be better to clean entries in
qcow2_grow_l1_table instead of  qcow2_snapshot_goto to avoid same
problem in different calls to qcow2_grow_l1_table. The other oddity
(still to understand) is: why does some code use l1_table above
l1_size ??

Frediano

Re: [Qemu-devel] [PATCH v2 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto

[Kevin Wolf]

Am 05.08.2011 08:35, schrieb Frediano Ziglio:
> 2011/8/4 Kevin Wolf <kwolf@redhat.com>:
>> When loading an internal snapshot whose L1 table is smaller than the current L1
>> table, the size of the current L1 would be shrunk to the snapshot's L1 size in
>> memory, but not on disk. This lead to incorrect refcount updates and eventuelly
>> to image corruption.
>>
>> Instead of writing the new L1 size to disk, this simply retains the bigger L1
>> size that is currently in use and makes sure that the unused part is zeroed.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>
>> And the moment you send it out, you notice that it's wrong... *sigh*
>>
>> v2:
>> - Check for s->l1_size > sn->l1_size in order to avoid disasters...
>>
>> Philipp, I think this should fix your corruption. Please give it a try.
>>
>> Anthony, this must go into 0.15. Given the short time until -rc2, do you prefer
>> to pick it up directly or should I send a pull request tomorrow? The patch
>> looks obvious, is tested with the given testcase and survives a basic
>> qemu-iotests run (though qemu-iotests doesn't exercise snapshots a lot)
>>
>> Stefan, please review :-)
>>
>>  block/qcow2-snapshot.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 74823a5..6972e66 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -330,8 +330,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>>     if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0)
>>         goto fail;
>>
>> -    s->l1_size = sn->l1_size;
>> +    if (s->l1_size > sn->l1_size) {
>> +        memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size);
>> +    }
>>     l1_size2 = s->l1_size * sizeof(uint64_t);
>> +
>>     /* copy the snapshot l1 table to the current l1 table */
>>     if (bdrv_pread(bs->file, sn->l1_table_offset,
>>                    s->l1_table, l1_size2) != l1_size2)
>> --
>> 1.7.6
>>
> 
> This patch looked odd at first sight. First a qcow2_grow_l1_table is
> called to shrink L1 so perhaps should be qcow2_resize_l1_table.

No, it doesn't shrink the table:

    if (min_size <= s->l1_size)
        return 0;

> Perhaps also it would be better to clean entries in
> qcow2_grow_l1_table instead of  qcow2_snapshot_goto to avoid same
> problem in different calls to qcow2_grow_l1_table. The other oddity
> (still to understand) is: why does some code use l1_table above
> l1_size ??

Which code do you mean specifically?

Kevin

Re: [Qemu-devel] [PATCH v2 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto

[Frediano Ziglio]

2011/8/5 Kevin Wolf <kwolf@redhat.com>:
> Am 05.08.2011 08:35, schrieb Frediano Ziglio:
>> 2011/8/4 Kevin Wolf <kwolf@redhat.com>:
>>> When loading an internal snapshot whose L1 table is smaller than the current L1
>>> table, the size of the current L1 would be shrunk to the snapshot's L1 size in
>>> memory, but not on disk. This lead to incorrect refcount updates and eventuelly
>>> to image corruption.
>>>
>>> Instead of writing the new L1 size to disk, this simply retains the bigger L1
>>> size that is currently in use and makes sure that the unused part is zeroed.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>
>>> And the moment you send it out, you notice that it's wrong... *sigh*
>>>
>>> v2:
>>> - Check for s->l1_size > sn->l1_size in order to avoid disasters...
>>>
>>> Philipp, I think this should fix your corruption. Please give it a try.
>>>
>>> Anthony, this must go into 0.15. Given the short time until -rc2, do you prefer
>>> to pick it up directly or should I send a pull request tomorrow? The patch
>>> looks obvious, is tested with the given testcase and survives a basic
>>> qemu-iotests run (though qemu-iotests doesn't exercise snapshots a lot)
>>>
>>> Stefan, please review :-)
>>>
>>>  block/qcow2-snapshot.c |    5 ++++-
>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>>> index 74823a5..6972e66 100644
>>> --- a/block/qcow2-snapshot.c
>>> +++ b/block/qcow2-snapshot.c
>>> @@ -330,8 +330,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>>>     if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0)
>>>         goto fail;
>>>
>>> -    s->l1_size = sn->l1_size;
>>> +    if (s->l1_size > sn->l1_size) {
>>> +        memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size);
>>> +    }
>>>     l1_size2 = s->l1_size * sizeof(uint64_t);
>>> +
>>>     /* copy the snapshot l1 table to the current l1 table */
>>>     if (bdrv_pread(bs->file, sn->l1_table_offset,
>>>                    s->l1_table, l1_size2) != l1_size2)
>>> --
>>> 1.7.6
>>>
>>
>> This patch looked odd at first sight. First a qcow2_grow_l1_table is
>> called to shrink L1 so perhaps should be qcow2_resize_l1_table.
>
> No, it doesn't shrink the table:
>
>    if (min_size <= s->l1_size)
>        return 0;
>

Yes, but perhaps returning success and not clipping anything is not
that correct, perhaps qcow2_snapshot_goto should not call
qcow2_grow_l1_table with a shorter value.

>> Perhaps also it would be better to clean entries in
>> qcow2_grow_l1_table instead of  qcow2_snapshot_goto to avoid same
>> problem in different calls to qcow2_grow_l1_table. The other oddity
>> (still to understand) is: why does some code use l1_table above
>> l1_size ??
>
> Which code do you mean specifically?
>
> Kevin
>

I think this is the issue

# 1204 -> 128 cluster per L2 entry -> 128k per L2 entry
# 128 cluster per L1 entry -> 16M per L1 entry
qemu-img create -f qcow2 /tmp/sn.qcow2 16m -o cluster_size=1024
qemu-img snapshot -c foo /tmp/sn.qcow2
# extend L1
qemu-io -c 'write -b 0 4M' /tmp/sn.qcow2
# shrink
qemu-img snapshot -a foo /tmp/sn.qcow2
qemu-img check /tmp/sn.qcow2

Well... I was trying to get a leak and got a core instead. I removed
your patch and got leaks.

also, should not be

    memset(s->l1_table + sn->l1_size, 0, (s->l1_size - sn->l1_size) *
sizeof(uint64_t));

instead of

    memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size);

Frediano

Re: [Qemu-devel] [PATCH v2 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto

[Frediano Ziglio]

2011/8/5 Frediano Ziglio <freddy77@gmail.com>:
> 2011/8/5 Kevin Wolf <kwolf@redhat.com>:
>> Am 05.08.2011 08:35, schrieb Frediano Ziglio:
>>> 2011/8/4 Kevin Wolf <kwolf@redhat.com>:
>>>> When loading an internal snapshot whose L1 table is smaller than the current L1
>>>> table, the size of the current L1 would be shrunk to the snapshot's L1 size in
>>>> memory, but not on disk. This lead to incorrect refcount updates and eventuelly
>>>> to image corruption.
>>>>
>>>> Instead of writing the new L1 size to disk, this simply retains the bigger L1
>>>> size that is currently in use and makes sure that the unused part is zeroed.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>> ---
>>>>
>>>> And the moment you send it out, you notice that it's wrong... *sigh*
>>>>
>>>> v2:
>>>> - Check for s->l1_size > sn->l1_size in order to avoid disasters...
>>>>
>>>> Philipp, I think this should fix your corruption. Please give it a try.
>>>>
>>>> Anthony, this must go into 0.15. Given the short time until -rc2, do you prefer
>>>> to pick it up directly or should I send a pull request tomorrow? The patch
>>>> looks obvious, is tested with the given testcase and survives a basic
>>>> qemu-iotests run (though qemu-iotests doesn't exercise snapshots a lot)
>>>>
>>>> Stefan, please review :-)
>>>>
>>>>  block/qcow2-snapshot.c |    5 ++++-
>>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>>>> index 74823a5..6972e66 100644
>>>> --- a/block/qcow2-snapshot.c
>>>> +++ b/block/qcow2-snapshot.c
>>>> @@ -330,8 +330,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>>>>     if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0)
>>>>         goto fail;
>>>>
>>>> -    s->l1_size = sn->l1_size;
>>>> +    if (s->l1_size > sn->l1_size) {
>>>> +        memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size);
>>>> +    }
>>>>     l1_size2 = s->l1_size * sizeof(uint64_t);
>>>> +
>>>>     /* copy the snapshot l1 table to the current l1 table */
>>>>     if (bdrv_pread(bs->file, sn->l1_table_offset,
>>>>                    s->l1_table, l1_size2) != l1_size2)
>>>> --
>>>> 1.7.6
>>>>
>>>
>>> This patch looked odd at first sight. First a qcow2_grow_l1_table is
>>> called to shrink L1 so perhaps should be qcow2_resize_l1_table.
>>
>> No, it doesn't shrink the table:
>>
>>    if (min_size <= s->l1_size)
>>        return 0;
>>
>
> Yes, but perhaps returning success and not clipping anything is not
> that correct, perhaps qcow2_snapshot_goto should not call
> qcow2_grow_l1_table with a shorter value.
>
>>> Perhaps also it would be better to clean entries in
>>> qcow2_grow_l1_table instead of  qcow2_snapshot_goto to avoid same
>>> problem in different calls to qcow2_grow_l1_table. The other oddity
>>> (still to understand) is: why does some code use l1_table above
>>> l1_size ??
>>
>> Which code do you mean specifically?
>>
>> Kevin
>>
>
> I think this is the issue
>
> # 1204 -> 128 cluster per L2 entry -> 128k per L2 entry
> # 128 cluster per L1 entry -> 16M per L1 entry
> qemu-img create -f qcow2 /tmp/sn.qcow2 16m -o cluster_size=1024
> qemu-img snapshot -c foo /tmp/sn.qcow2
> # extend L1
> qemu-io -c 'write -b 0 4M' /tmp/sn.qcow2
> # shrink
> qemu-img snapshot -a foo /tmp/sn.qcow2
> qemu-img check /tmp/sn.qcow2
>
> Well... I was trying to get a leak and got a core instead. I removed
> your patch and got leaks.
>
> also, should not be
>
>    memset(s->l1_table + sn->l1_size, 0, (s->l1_size - sn->l1_size) *
> sizeof(uint64_t));
>
> instead of
>
>    memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size);
>
> Frediano
>

This patch fix Philipp and my problem

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 74823a5..0e5fc13 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -330,14 +330,16 @@ int qcow2_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id)
     if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0)
         goto fail;

-    s->l1_size = sn->l1_size;
-    l1_size2 = s->l1_size * sizeof(uint64_t);
+    if (s->l1_size > sn->l1_size) {
+        memset(s->l1_table + sn->l1_size, 0, (s->l1_size -
sn->l1_size) * sizeof(uint64_t));
+    }
+    l1_size2 = sn->l1_size * sizeof(uint64_t);
     /* copy the snapshot l1 table to the current l1 table */
     if (bdrv_pread(bs->file, sn->l1_table_offset,
                    s->l1_table, l1_size2) != l1_size2)
         goto fail;
     if (bdrv_pwrite_sync(bs->file, s->l1_table_offset,
-                    s->l1_table, l1_size2) < 0)
+                    s->l1_table, s->l1_size * sizeof(uint64_t)) < 0)
         goto fail;
     for(i = 0;i < s->l1_size; i++) {
         be64_to_cpus(&s->l1_table[i]);


Frediano