Re: [Qemu-trivial] [PATCH v5] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz <mreitz@redhat.com>]

On 2014-10-22 at 14:39, Zhang Haoyu wrote:
> Use local variable to bdrv_pwrite_sync L1 table,
> needless to make conversion of cached L1 table between
> big-endian and host style.
>
> Signed-off-by: Zhang Haoyu <zhanghy@sangfor.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> v4 -> v5:
> - delete superfluous check of "l1_size2 != 0"
>    after qemu_try_blockalign(l1_size2)
>
> v3 -> v4:
>   - convert local L1 table to host-style before copy it
>     back to s->l1_table
>
> v2 -> v3:
>   - replace g_try_malloc0 with qemu_try_blockalign
>   - copy the latest local L1 table back to s->l1_table
>     after successfully bdrv_pwrite_sync L1 table
>
> v1 -> v2:
>   - remove the superflous assignment, l1_table = NULL;
>   - replace 512 with BDRV_SECTOR_SIZE, and align_offset with ROUND_UP
>   - remove needless check of if (l1_table) before g_free(l1_table)
>
>   block/qcow2-refcount.c | 28 ++++++++++++----------------
>   1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 2bcaaf9..4cf6639 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -881,14 +881,17 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>   {
>       BDRVQcowState *s = bs->opaque;
>       uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
> -    bool l1_allocated = false;
>       int64_t old_offset, old_l2_offset;
>       int i, j, l1_modified = 0, nb_csectors, refcount;
>       int ret;
>   
>       l2_table = NULL;
> -    l1_table = NULL;
>       l1_size2 = l1_size * sizeof(uint64_t);
> +    l1_table = qemu_try_blockalign(bs->file, l1_size2);
> +    if (l1_table == NULL) {
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
>   
>       s->cache_discards = true;
>   
> @@ -896,13 +899,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>        * l1_table_offset when it is the current s->l1_table_offset! Be careful
>        * when changing this! */
>       if (l1_table_offset != s->l1_table_offset) {
> -        l1_table = g_try_malloc0(align_offset(l1_size2, 512));
> -        if (l1_size2 && l1_table == NULL) {
> -            ret = -ENOMEM;
> -            goto fail;
> -        }
> -        l1_allocated = true;
> -
>           ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
>           if (ret < 0) {
>               goto fail;
> @@ -912,8 +908,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>               be64_to_cpus(&l1_table[i]);
>       } else {
>           assert(l1_size == s->l1_size);
> -        l1_table = s->l1_table;
> -        l1_allocated = false;
> +        memcpy(l1_table, s->l1_table, l1_size2);
>       }
>   
>       for(i = 0; i < l1_size; i++) {
> @@ -1055,13 +1050,14 @@ fail:
>           }
>   
>           ret = bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table, l1_size2);
> -
> -        for (i = 0; i < l1_size; i++) {
> -            be64_to_cpus(&l1_table[i]);
> +        if (ret == 0) {
> +            for (i = 0; i < l1_size; i++) {
> +                be64_to_cpus(&l1_table[i]);
> +            }
> +            memcpy(s->l1_table, l1_table, l1_size2);

Well, "for (i = 0; i < l1_size; i++) { s->l1_table[i] = 
be64_to_cpu(l1_table[i]; }" would have saved us this memcpy(). But this 
function is not critical for performance, so it doesn't really matter 
(if it was about performance, we could get rid of the first memcpy() as 
well by the same means).

Oh, and by the way: For your future patches, if you create a new version 
which has non-trivial changes (I normally consider only whitespace and 
comment changes trivial, and not even all of those), please drop the 
Reviewed-by. There are exceptions to this where a reviewer explicitly 
states "If you do $foo, then: Reviewed-by: Foo Bar <fbar@example.com>", 
though. Also, please don't include a Reviewed-by if the reviewer did not 
explicitly state "Reviewed-by: Foo Bar <fbar@example.com>". As far as I 
remember, I never explicitly stated "Reviewed-by: Max Reitz 
<mreitz@redhat.com>" in regards to this series and that was intended.

Anyway, thanks for your patch, I applied it to my block branch:
https://github.com/XanClic/qemu/commits/block

Max

>           }
>       }
> -    if (l1_allocated)
> -        g_free(l1_table);
> +    g_free(l1_table);
>       return ret;
>   }
>