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

[Frediano Ziglio]

2011/8/4 Kevin Wolf <address@hidden>:
> 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 <address@hidden>
> ---
>
> 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