qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [PATCH v5] snapshot: use local variable to bdrv_pwrit


From: Max Reitz
Subject: Re: [Qemu-trivial] [PATCH v5] snapshot: use local variable to bdrv_pwrite_sync L1 table
Date: Thu, 23 Oct 2014 19:57:41 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0

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 <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
---
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;
  }




reply via email to

[Prev in Thread] Current Thread [Next in Thread]