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

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

[Zhang Haoyu]

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);
         }
     }
-    if (l1_allocated)
-        g_free(l1_table);
+    g_free(l1_table);
     return ret;
 }
 
-- 
1.7.12.4

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

[Max Reitz]

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;
>   }
>   

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

[Michael Tokarev]

On 10/22/2014 04:39 PM, 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.
...

Okay, so, why do you think that a patch which prompted such a hot
discussion and is at v5 already is applicable to -trivial to start
with?  Yes I know Max already took it, this is a rhetorical
question ;)

Thanks,

/mjt

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

[Max Reitz]

On 22.10.2014 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(-)

Sorry, but this patch breaks the qemu-iotests 060 and 061. I'll have a 
look into it myself, but for now I had to drop it from my block tree.

Max

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

[Max Reitz]

On 22.10.2014 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) {

This needs to be "if (ret == 0 && l1_table_offset == s->l1_table_offset) 
{". We don't want to overwrite the active L1 table in memory when we're 
working on an inactive snapshot L1 table.

With that change, you can keep my R-b. I'll leave this patch out of the 
queue for Kevin this week though, because it's still a non-trivial 
change so I don't want to do it without your permission (and because 
Kevin is merging my queue right now, I won't get your permission in 
time), sorry.

Max

> +            for (i = 0; i < l1_size; i++) {
> +                be64_to_cpus(&l1_table[i]);
> +            }
> +            memcpy(s->l1_table, l1_table, l1_size2);
>           }
>       }
> -    if (l1_allocated)
> -        g_free(l1_table);
> +    g_free(l1_table);
>       return ret;
>   }
>