Re: [Qemu-devel] [PATCH 5/5] Change order of metadata update to prevent loosing guest data because of unexpected exit.

[Anthony Liguori <anthony@codemonkey.ws>]

Gleb Natapov wrote:
> Currently the order is this (during cow since it's the interesting case):
> 1. Decrement refcount of old clusters
> 2. Increment refcount for newly allocated clusters
> 3. Copy content of old sectors that will not be rewritten
> 4. Update L2 table with pointers to new clusters
> 5. Write guest data into new clusters (asynchronously)
>
> There are several problems with this order. The first one is that if qemu
> crashes (or killed or host reboots) after new clusters are linked into L2
> table but before user data is written there, then on the next reboot guest
> will find neither old data nor new one in those sectors and this is not
> what gust expects even when journaling file system is in use.  The other
> problem is that if qemu is killed between steps 1 and 4 then refcount
> of old cluster will be incorrect and may cause snapshot corruption.
>
> The patch change the order to be like this:
> 1. Increment refcount for newly allocated clusters
> 2. Write guest data into new clusters (asynchronously)
> 3. Copy content of old sectors that were not rewritten
> 4. Update L2 table with pointers to new clusters
> 5. Decrement refcount of old clusters
>
> Unexpected crash may cause cluster leakage, but guest data should be safe.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>
>  block-qcow2.c |  155 +++++++++++++++++++++++++++++++++------------------------
>  1 files changed, 91 insertions(+), 64 deletions(-)
>
> diff --git a/block-qcow2.c b/block-qcow2.c
> index 0771281..c600517 100644
> --- a/block-qcow2.c
> +++ b/block-qcow2.c
> @@ -852,6 +852,69 @@ static uint64_t alloc_compressed_cluster_offset(BlockDriverState *bs,
>      return cluster_offset;
>  }
>  
> +typedef struct QCowL2Meta
> +{
> +    uint64_t offset;
> +    int n_start;
> +    int nb_available;
> +    int nb_clusters;
> +} QCowL2Meta;
> +
> +static int alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
> +        QCowL2Meta *m)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int i, j = 0, l2_index, ret;
> +    uint64_t *old_cluster, start_sect, l2_offset, *l2_table;
> +
> +    if (m->nb_clusters == 0)
> +        return 0;
> +
> +    if (!(old_cluster = qemu_malloc(m->nb_clusters * sizeof(uint64_t))))
> +        return -ENOMEM;
>   

This memory is never freed.

Regards,

Anthony Liguori