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

---
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 | 24 +++++++-----------------
  1 file changed, 7 insertions(+), 17 deletions(-)

block/qcow2-refcount.c +0 -0

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

block/qcow2-refcount.c +5 -9

+    if (l1_size2 && 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 +1 -2

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

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

---
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 | 24 +++++++-----------------
  1 file changed, 7 insertions(+), 17 deletions(-)

block/qcow2-refcount.c +0 -1

index 2bcaaf9..29a916a 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 890

Quoted 4 lines

block/qcow2-refcount.c +1 -1

      l2_table = NULL;
-    l1_table = NULL;
      l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE));

I'm sorry, but I just now realized that we don't even need the 0 variant here. We are never accessing any element beyond l1_table[l1_size - 1], and all elements from 0 until l1_size - 1 are overwritten by bdrv_pread() or memcpy(). Therefore we can simply use qemu_try_blockalign(bs->file, l1_size2) here.

(and we should be using qemu_{try_,}blockalign() instead of g_{try_,}malloc{0,}() whenever possible for performance reasons)

Quoted 34 lines

block/qcow2-refcount.c +5 -9

+    if (l1_size2 && 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,8 @@ fail:
          }
 

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

Quoted 5 lines

block/qcow2-refcount.c +0 -4

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

But I realized something even more important as well: If you are modifying the current L1 table, you need to copy the result back to s->l1_table.

Sorry for having missed these in my previous review.

Max

Quoted 7 lines

block/qcow2-refcount.c +1 -2

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

Quoted 27 lines

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

---
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 | 24 +++++++-----------------
   1 file changed, 7 insertions(+), 17 deletions(-)

block/qcow2-refcount.c +0 -1

index 2bcaaf9..29a916a 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;
 

Quoted 10 lines

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

Quoted 4 lines

block/qcow2-refcount.c +1 -1

       l2_table = NULL;
-    l1_table = NULL;
       l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE));

I'm sorry, but I just now realized that we don't even need the 0 variant here. We are never accessing any element beyond l1_table[l1_size - 1], and all elements from 0 until l1_size - 1 are overwritten by bdrv_pread() or memcpy(). Therefore we can simply use qemu_try_blockalign(bs->file, l1_size2) here.

OK, I will replace g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)) with qemu_try_blockalign(bs, ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)).

Quoted 37 lines

(and we should be using qemu_{try_,}blockalign() instead of g_{try_,}malloc{0,}() whenever possible for performance reasons)

Quoted 34 lines

block/qcow2-refcount.c +5 -9

+    if (l1_size2 && 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,8 @@ fail:
           }
 

Quoted 9 lines

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

Quoted 5 lines

block/qcow2-refcount.c +0 -4

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

But I realized something even more important as well: If you are modifying the current L1 table, you need to copy the result back to s->l1_table.

My fault.

Quoted 11 lines

Sorry for having missed these in my previous review.

Max

Quoted 7 lines

block/qcow2-refcount.c +1 -2

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

Quoted 37 lines · Zhang Haoyu

Quoted 37 lines

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

---
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 | 24 +++++++-----------------
   1 file changed, 7 insertions(+), 17 deletions(-)

block/qcow2-refcount.c +0 -0

index 2bcaaf9..29a916a 100644
@@ -881,14 +881,17 @@ int 
 qcow2_update_snapshot_refcount(BlockDriverState *bs,
   {

block/qcow2-refcount.c int · line 890

Quoted 10 lines

block/qcow2-refcount.c +1 -2

       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 = g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE));

I'm sorry, but I just now realized that we don't even need the 0 variant here. We are never accessing any element beyond l1_table[l1_size - 1], and all elements from 0 until l1_size - 1 are overwritten by bdrv_pread() or memcpy(). Therefore we can simply use qemu_try_blockalign(bs->file, l1_size2) here.

Quoted 2 lines

OK, I will replace g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)) with qemu_try_blockalign(bs, ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)).

You can actually omit the ROUND_UP(). There are no accesses beyond l1_table[l1_size - 1], if I'm not mistaken.

Max

Quoted 58 lines

Quoted 38 lines

(and we should be using qemu_{try_,}blockalign() instead of g_{try_,}malloc{0,}() whenever possible for performance reasons)

Quoted 35 lines

block/qcow2-refcount.c +4 -0

+    if (l1_size2 && 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) {

block/qcow2-refcount.c +0 -7

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

block/qcow2-refcount.c +1 -2

@@ -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,8 @@ fail:
           }
              ret = bdrv_pwrite_sync(bs->file, l1_table_offset, 

Quoted 9 lines

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

Quoted 5 lines

block/qcow2-refcount.c +0 -4

 l1_table, l1_size2);
-
-        for (i = 0; i < l1_size; i++) {
-            be64_to_cpus(&l1_table[i]);
-        }

But I realized something even more important as well: If you are modifying the current L1 table, you need to copy the result back to s->l1_table.

My fault.

Quoted 10 lines

Sorry for having missed these in my previous review.

Max

Quoted 6 lines

block/qcow2-refcount.c +1 -2

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