snapshot: use local variable to bdrv_pwrite_sync L1 table

PATCH In review Zhang Haoyu 3 participants 5 messages Oct 22, 2014 – Oct 23, 2014 Block layer

Share & tools

Zhang Haoyu

author

2014-10-22 12:40 UTC #0 lore raw

PATCH v5 1/1 snapshot: use local variable to bdrv_pwrite_sync L1 table

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 <[email protected]>

Reviewed-by Max Reitz <[email protected]>

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

block/qcow2-refcount.c +12 ~~-16~~

index 2bcaaf9..4cf6639 100644
@@ -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;
 }

Signature · 1 line

1.7.12.4

Max Reitz

2014-10-22 13:51 UTC #1 lore raw

Quoted 78 lines · 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 <[email protected]>

Reviewed-by Max Reitz <[email protected]>

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

  block/qcow2-refcount.c | 28 ++++++++++++----------------
  1 file changed, 12 insertions(+), 16 deletions(-)

block/qcow2-refcount.c +6 ~~-11~~

index 2bcaaf9..4cf6639 100644
@@ -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:
          }

block/qcow2-refcount.c fail: · line 1057

Quoted 10 lines

block/qcow2-refcount.c +5 -3

 
          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 <[email protected]>", though. Also, please don't include a Reviewed-by if the reviewer did not explicitly state "Reviewed-by: Foo Bar <[email protected]>". As far as I remember, I never explicitly stated "Reviewed-by: Max Reitz <[email protected]>" 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

Quoted 8 lines

block/qcow2-refcount.c +1 -2

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

Michael Tokarev

2014-10-23 06:59 UTC #2 lore raw

In reply to Zhang Haoyu

Quoted 3 lines · 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.

...

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

Max Reitz

2014-10-23 17:43 UTC #3 lore raw

In reply to Zhang Haoyu

Quoted 20 lines · 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 <[email protected]>
Reviewed-by Max Reitz <[email protected]>
---
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

Quoted 7 lines

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

Max Reitz

2014-10-23 17:57 UTC #4 lore raw

In reply to Zhang Haoyu

Quoted 77 lines · 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 <[email protected]>

Reviewed-by Max Reitz <[email protected]>

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

  block/qcow2-refcount.c | 28 ++++++++++++----------------
  1 file changed, 12 insertions(+), 16 deletions(-)

block/qcow2-refcount.c +6 ~~-11~~

index 2bcaaf9..4cf6639 100644
@@ -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++) {

block/qcow2-refcount.c fail: · line 1053

Quoted 7 lines

block/qcow2-refcount.c +1 -3

@@ -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

Quoted 12 lines

block/qcow2-refcount.c +5 -2

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