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

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

index 2bcaaf9..3e4050a 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;

block/qcow2-refcount.c +10 -14

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

Quoted 37 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]>

---
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 +0 -1

index 2bcaaf9..3e4050a 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;
 

block/qcow2-refcount.c int qcow2_update_snapshot_refcount(BlockDriverState *bs, · line 891

Quoted 5 lines

block/qcow2-refcount.c +2 -1

      l2_table = NULL;
-    l1_table = NULL;
      l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = qemu_try_blockalign(bs->file, l1_size2);
+    if (l1_size2 && l1_table == NULL) {

I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL, What will happen?

Best regards,
-Gonglei

Quoted 50 lines

block/qcow2-refcount.c +10 -14

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

Quoted 38 lines · Gonglei

Quoted 36 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]>

---
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 +0 -1

index 2bcaaf9..3e4050a 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;

Quoted 8 lines

block/qcow2-refcount.c int qcow2_update_snapshot_refcount(BlockDriverState *bs, · line 891

Quoted 6 lines

block/qcow2-refcount.c +2 -1

      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_size2 && l1_table == NULL) {

I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL, What will happen?

Then this condition is met and you return with -ENOMEM...?

Quoted 49 lines · Max Reitz

Quoted 47 lines · Gonglei

Quoted 32 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]>

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

index 2bcaaf9..3e4050a 100644
@@ -881,14 +881,17 @@ int 
 qcow2_update_snapshot_refcount(BlockDriverState *bs,
  {
      BDRVQcowState *s = bs->opaque;

block/qcow2-refcount.c int · line 891

Quoted 10 lines

block/qcow2-refcount.c +2 -2

      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_size2 && l1_table == NULL) {

I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL,
What will happen?

Quoted 1 line

Then this condition is met and you return with -ENOMEM...?

Oh, but I see something different: qemu_try_blockalign() never returns NULL, not even when you request 0 bytes.

Therefore, just "if (l1_table == NULL)" is sufficient.

Max

Quoted 49 lines · Max Reitz

Quoted 48 lines · Max Reitz

Quoted 47 lines · Gonglei

Quoted 32 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]>

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

index 2bcaaf9..3e4050a 100644
@@ -881,14 +881,17 @@ int
 qcow2_update_snapshot_refcount(BlockDriverState *bs,
   {
       BDRVQcowState *s = bs->opaque;

block/qcow2-refcount.c int · line 891

Quoted 10 lines

block/qcow2-refcount.c +2 -2

       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_size2 && l1_table == NULL) {

I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL,
What will happen?

Quoted 3 lines

Quoted 1 line

Then this condition is met and you return with -ENOMEM...?

Oh, but I see something different: qemu_try_blockalign() never returns NULL, not even when you request 0 bytes.

Yes. But if l1_size2 is zero, will waste memory or some other problems. Please see below:

the original code:
l1_table = g_try_malloc0(align_offset(0, 512));
-> l1_table = g_try_malloc0(0)
so l1_table == NULL.

after this patch:
l1_table = qemu_try_blockalign(bs->file, 0)
l1_table will not be NULL.

I don't know whether l1_size2 can be zero or not.

Quoted 63 lines · Gonglei

Quoted 49 lines · Max Reitz

Quoted 48 lines · Max Reitz

Quoted 47 lines · Gonglei

Quoted 32 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]>

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

index 2bcaaf9..3e4050a 100644
@@ -881,14 +881,17 @@ int
 qcow2_update_snapshot_refcount(BlockDriverState *bs,
   {
       BDRVQcowState *s = bs->opaque;

block/qcow2-refcount.c int · line 891

Quoted 10 lines

block/qcow2-refcount.c +2 -2

       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_size2 && l1_table == NULL) {

I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL,
What will happen?

Quoted 3 lines

Quoted 1 line

Then this condition is met and you return with -ENOMEM...?

Oh, but I see something different: qemu_try_blockalign() never returns NULL, not even when you request 0 bytes.

Yes. But if l1_size2 is zero, will waste memory or some other problems. Please see below:

the original code:
l1_table = g_try_malloc0(align_offset(0, 512));
-> l1_table = g_try_malloc0(0)
so l1_table == NULL.

Quoted 3 lines

after this patch:
l1_table = qemu_try_blockalign(bs->file, 0)
l1_table will not be NULL.

Okay, so you meant "if l1_size2 == 0 and l1_table != NULL".

Quoted 1 line

I don't know whether l1_size2 can be zero or not.

Probably not, but if it cannot be zero, testing for that case makes even less sense.

Quoted 63 lines · Max Reitz

Quoted 62 lines · Gonglei

Quoted 49 lines · Max Reitz

Quoted 48 lines · Max Reitz

Quoted 47 lines · Gonglei

Quoted 32 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]>

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

index 2bcaaf9..3e4050a 100644
@@ -881,14 +881,17 @@ int
 qcow2_update_snapshot_refcount(BlockDriverState *bs,
    {
        BDRVQcowState *s = bs->opaque;

block/qcow2-refcount.c int · line 891

Quoted 10 lines

block/qcow2-refcount.c +2 -2

        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_size2 && l1_table == NULL) {

I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL,
What will happen?

Quoted 3 lines

Quoted 1 line

Then this condition is met and you return with -ENOMEM...?

Oh, but I see something different: qemu_try_blockalign() never returns NULL, not even when you request 0 bytes.

Yes. But if l1_size2 is zero, will waste memory or some other problems. Please see below:

the original code:

l1_table = g_try_malloc0(align_offset(0, 512));
   -> l1_table = g_try_malloc0(0)

so l1_table == NULL.

Quoted 5 lines

Quoted 3 lines

after this patch:
l1_table = qemu_try_blockalign(bs->file, 0)
l1_table will not be NULL.

Okay, so you meant "if l1_size2 == 0 and l1_table != NULL".

Hum, sorry for my typo ;)

Quoted 4 lines

Quoted 1 line

I don't know whether l1_size2 can be zero or not.

Probably not, but if it cannot be zero, testing for that case makes even less sense.

So, can we add a check at the begin of this function?

Quoted 72 lines · Gonglei

Quoted 63 lines · Max Reitz

Quoted 62 lines · Gonglei

Quoted 49 lines · Max Reitz

Quoted 48 lines · Max Reitz

Quoted 47 lines · Gonglei

Quoted 32 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]>

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

index 2bcaaf9..3e4050a 100644
@@ -881,14 +881,17 @@ int
 qcow2_update_snapshot_refcount(BlockDriverState *bs,
    {
        BDRVQcowState *s = bs->opaque;

block/qcow2-refcount.c int · line 891

Quoted 10 lines

block/qcow2-refcount.c +2 -2

        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_size2 && l1_table == NULL) {

I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL,
What will happen?

Quoted 3 lines

Quoted 1 line

Then this condition is met and you return with -ENOMEM...?

Oh, but I see something different: qemu_try_blockalign() never returns NULL, not even when you request 0 bytes.

Yes. But if l1_size2 is zero, will waste memory or some other problems. Please see below:

the original code:

l1_table = g_try_malloc0(align_offset(0, 512));
   -> l1_table = g_try_malloc0(0)

so l1_table == NULL.

Quoted 5 lines

Quoted 3 lines

after this patch:
l1_table = qemu_try_blockalign(bs->file, 0)
l1_table will not be NULL.

Okay, so you meant "if l1_size2 == 0 and l1_table != NULL".

Hum, sorry for my typo ;)

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

I don't know whether l1_size2 can be zero or not.

Probably not, but if it cannot be zero, testing for that case makes even less sense.

So, can we add a check at the begin of this function?

We can just omit the "l1_size2 && " from the "if (l1_size2 && l1_table == NULL)".

Quoted 37 lines

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

---
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 +0 -1

index 2bcaaf9..3e4050a 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;
 

block/qcow2-refcount.c int qcow2_update_snapshot_refcount(BlockDriverState *bs, · line 891

Quoted 5 lines

block/qcow2-refcount.c +2 -1

     l2_table = NULL;
-    l1_table = NULL;
     l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = qemu_try_blockalign(bs->file, l1_size2);
+    if (l1_size2 && l1_table == NULL) {

I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL, What will happen?

Best regards,
-Gonglei

Quoted 50 lines

block/qcow2-refcount.c +10 -14

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