Am 05.08.2011 08:35, schrieb Frediano Ziglio:
> 2011/8/4 Kevin Wolf <kw...@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 <kw...@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
