Re: [Qemu-devel] [PATCH 3/5] Write table offset and size in one syscall.

[Anthony Liguori <anthony@codemonkey.ws>]

Gleb Natapov wrote:
> Otherwise if VM is killed between two writes data may be lost.
> But if offset and size fields are at the same disk block one
> write should update them both simultaneously.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>
>  block-qcow2.c |   27 ++++++++++-----------------
>  1 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/block-qcow2.c b/block-qcow2.c
> index 69f6414..7f99921 100644
> --- a/block-qcow2.c
> +++ b/block-qcow2.c
> @@ -429,8 +429,7 @@ static int grow_l1_table(BlockDriverState *bs, int min_size)
>      int new_l1_size, new_l1_size2, ret, i;
>      uint64_t *new_l1_table;
>      uint64_t new_l1_table_offset;
> -    uint64_t data64;
> -    uint32_t data32;
> +    uint8_t data[12];
>   

This assumes packing will happen correctly.

>  
>      new_l1_size = s->l1_size;
>      if (min_size <= new_l1_size)
> @@ -460,14 +459,12 @@ static int grow_l1_table(BlockDriverState *bs, int min_size)
>          new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
>  
>      /* set new table */
> -    data64 = cpu_to_be64(new_l1_table_offset);
> -    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_table_offset),
> -                    &data64, sizeof(data64)) != sizeof(data64))
> -        goto fail;
> -    data32 = cpu_to_be32(new_l1_size);
> -    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size),
> -                    &data32, sizeof(data32)) != sizeof(data32))
> +    *(uint32_t*)data = cpu_to_be32(new_l1_size);
> +    *(uint64_t*)&data[4] = cpu_to_be64(new_l1_table_offset);
> +    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data,
> +                sizeof(data)) != sizeof(data))
>          goto fail;
>
>   

Why not just introduces a uint8_t data[12] in this function, memcpy to 
the right offsets, and do one brdv_pwrite?  Then you don't need to do 
weird things with packing.

>      qemu_free(s->l1_table);
>      free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t));
>      s->l1_table_offset = new_l1_table_offset;
> @@ -2278,8 +2275,7 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size)
>      int new_table_size, new_table_size2, refcount_table_clusters, i, ret;
>      uint64_t *new_table;
>      int64_t table_offset;
> -    uint64_t data64;
> -    uint32_t data32;
> +    uint8_t data[12];
>      int old_table_size;
>      int64_t old_table_offset;
>  
> @@ -2318,13 +2314,10 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size)
>      for(i = 0; i < s->refcount_table_size; i++)
>          be64_to_cpus(&new_table[i]);
>  
> -    data64 = cpu_to_be64(table_offset);
> +    *(uint64_t*)data = cpu_to_be64(table_offset);
> +    *(uint32_t*)&data[8] = cpu_to_be32(refcount_table_clusters);
>      if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset),
> -                    &data64, sizeof(data64)) != sizeof(data64))
> -        goto fail;
> -    data32 = cpu_to_be32(refcount_table_clusters);
> -    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_clusters),
> -                    &data32, sizeof(data32)) != sizeof(data32))
> +                    data, sizeof(data)) != sizeof(data))
>          goto fail;
>   

Same here.  Alternatively, you could use the cpu_to_beXXs() variants and 
just pass in pointer offsets.

Regards,

Anthony Liguori

>      qemu_free(s->refcount_table);
>      old_table_offset = s->refcount_table_offset;
>
>
>
>