snapshot: use local variable to bdrv_pwrite_sync L1 table

PATCH Superseded Zhang Haoyu 5 participants 8 messages Oct 21, 2014 – Oct 23, 2014 Block layer

Share & tools

Zhang Haoyu

author

2014-10-21 08:15 UTC #0 lore raw

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

---
 block/qcow2-refcount.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

block/qcow2-refcount.c +7 ~~-15~~

index 2bcaaf9..8b318e8 100644
@@ -881,7 +881,6 @@ 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;

@@ -889,6 +888,11 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     l2_table = NULL;
     l1_table = NULL;
     l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
+    if (l1_size2 && l1_table == NULL) {
+        ret = -ENOMEM;
+        goto fail;
+    }
 
     s->cache_discards = true;

@@ -896,13 +900,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 +909,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,12 +1051,8 @@ 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 (l1_allocated)
+    if (l1_table)
         g_free(l1_table);
     return ret;
 }

Signature · 1 line

1.7.12.4

Max Reitz

2014-10-21 09:25 UTC #1 lore raw

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

---
  block/qcow2-refcount.c | 22 +++++++---------------
  1 file changed, 7 insertions(+), 15 deletions(-)

block/qcow2-refcount.c +0 -1

index 2bcaaf9..8b318e8 100644
@@ -881,7 +881,6 @@ 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 889

Quoted 2 lines

block/qcow2-refcount.c +0 -0

@@ -889,6 +888,11 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
      l2_table = NULL;
      l1_table = NULL;

Please remove this assignment; thanks to this hunk we don't need it anymore.

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

Quoted 2 lines

block/qcow2-refcount.c +1 -0

      l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = g_try_malloc0(align_offset(l1_size2, 512));

I wanted to propose using qemu_try_blockalign(), but since it'd require a memset() afterwards, it gets rather ugly.

Could you at least replace 512 by BDRV_SECTOR_SIZE, and maybe even align_offset() by ROUND_UP()? We should probably do the latter in all of the qcow2 code, though, I think it's just there because it has been around since before there was a ROUND_UP()...

block/qcow2-refcount.c fail:

Quoted 32 lines

block/qcow2-refcount.c +5 -9

+    if (l1_size2 && l1_table == NULL) {
+        ret = -ENOMEM;
+        goto fail;
+    }
 
      s->cache_discards = true;

@@ -896,13 +900,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 +909,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,12 +1051,8 @@ fail:

I don't think it will change a lot, but could you wrap the "s->cache_discards = false; qcow2_process_discards(bs, ret);" in an "if (s->cache_discards)"? You have introduced a case where s->cache_discards was still false, so we don't need to call qcow2_process_discards() then (which will hopefully return immediately, but well...).

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

Quoted 11 lines

block/qcow2-refcount.c +1 -5

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

Just drop the condition. g_free(l1_table); is enough.

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

Quoted 2 lines

block/qcow2-refcount.c +0 -0
      return ret;
  }

The change itself is good, it just needs some polishing.

Max

Zhang Haoyu

author

2014-10-21 10:49 UTC #2 lore raw

Re: [Qemu-devel] [PATCH] snapshot: use local variable to bdrv_pwrite_syncL1 table

Quoted 18 lines

Quoted 18 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]>
---
  block/qcow2-refcount.c | 22 +++++++---------------
  1 file changed, 7 insertions(+), 15 deletions(-)
block/qcow2-refcount.c +0 -1
index 2bcaaf9..8b318e8 100644
@@ -881,7 +881,6 @@ 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;

Quoted 8 lines

block/qcow2-refcount.c int qcow2_update_snapshot_refcount(BlockDriverState *bs, · line 889
Quoted 6 lines
block/qcow2-refcount.c +0 -0
      int64_t old_offset, old_l2_offset;
      int i, j, l1_modified = 0, nb_csectors, refcount;
      int ret;
@@ -889,6 +888,11 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
      l2_table = NULL;
      l1_table = NULL;
Please remove this assignment; thanks to this hunk we don't need it anymore.

OK.

Quoted 4 lines

block/qcow2-refcount.c int qcow2_update_snapshot_refcount(BlockDriverState *bs, · line 891
Quoted 3 lines
block/qcow2-refcount.c +1 -0
      l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = g_try_malloc0(align_offset(l1_size2, 512));

Quoted 8 lines

I wanted to propose using qemu_try_blockalign(), but since it'd require a memset() afterwards, it gets rather ugly.
Could you at least replace 512 by BDRV_SECTOR_SIZE, and maybe even align_offset() by ROUND_UP()? We should probably do the latter in all of the qcow2 code, though, I think it's just there because it has been around since before there was a ROUND_UP()...

Good, I will replace 512 with BDRV_SECTOR_SIZE, and replace align_offset with ROUND_UP.

Quoted 30 lines

block/qcow2-refcount.c +5 -9

+    if (l1_size2 && l1_table == NULL) {
+        ret = -ENOMEM;
+        goto fail;
+    }
 
      s->cache_discards = true;

@@ -896,13 +900,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 +909,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);
      }

Quoted 8 lines

block/qcow2-refcount.c fail:
Quoted 2 lines
block/qcow2-refcount.c +0 -0
      for(i = 0; i < l1_size; i++) {
@@ -1055,12 +1051,8 @@ fail:
I don't think it will change a lot, but could you wrap the "s->cache_discards = false; qcow2_process_discards(bs, ret);" in an "if (s->cache_discards)"? You have introduced a case where s->cache_discards was still false, so we don't need to call qcow2_process_discards() then (which will hopefully return immediately, but well...).

s->cache_discards's initial value is true in qcow2_update_snapshot_refcount(), where s->cache_discards is set to false?
Or you means s->cache_discards should be set to false
after g_try_malloc0(align_offset(l1_size2, 512)) failed.

Quoted 13 lines

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

Quoted 12 lines

block/qcow2-refcount.c +1 -5

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

Quoted 2 lines

Just drop the condition. g_free(l1_table); is enough.

OK.

Quoted 6 lines

block/qcow2-refcount.c fail: · line 1058
Quoted 2 lines
block/qcow2-refcount.c +0 -0
      return ret;
  }
The change itself is good, it just needs some polishing.
Max

Max Reitz

2014-10-21 10:53 UTC #3 lore raw

Re: [Qemu-devel] [PATCH] snapshot: use local variable to bdrv_pwrite_syncL1 table

Quoted 74 lines · Zhang Haoyu

Quoted 9 lines
Quoted 9 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]>
---
   block/qcow2-refcount.c | 22 +++++++---------------
   1 file changed, 7 insertions(+), 15 deletions(-)
Quoted 16 lines
Quoted 13 lines
block/qcow2-refcount.c +0 -1
index 2bcaaf9..8b318e8 100644
@@ -881,7 +881,6 @@ 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 889
Quoted 2 lines
block/qcow2-refcount.c +0 -0
@@ -889,6 +888,11 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
       l2_table = NULL;
       l1_table = NULL;
Please remove this assignment; thanks to this hunk we don't need it anymore.
OK.
Quoted 10 lines
block/qcow2-refcount.c int qcow2_update_snapshot_refcount(BlockDriverState *bs, · line 891
Quoted 2 lines
block/qcow2-refcount.c +1 -0
       l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
I wanted to propose using qemu_try_blockalign(), but since it'd require a memset() afterwards, it gets rather ugly.
Could you at least replace 512 by BDRV_SECTOR_SIZE, and maybe even align_offset() by ROUND_UP()? We should probably do the latter in all of the qcow2 code, though, I think it's just there because it has been around since before there was a ROUND_UP()...
Good, I will replace 512 with BDRV_SECTOR_SIZE, and replace align_offset with ROUND_UP.
Quoted 30 lines
Quoted 30 lines
block/qcow2-refcount.c +5 -9
+    if (l1_size2 && l1_table == NULL) {
+        ret = -ENOMEM;
+        goto fail;
+    }
 
       s->cache_discards = true;
 
@@ -896,13 +900,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 +909,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);
       }
 
Quoted 7 lines
block/qcow2-refcount.c fail:
Quoted 2 lines
block/qcow2-refcount.c +0 -0
       for(i = 0; i < l1_size; i++) {
@@ -1055,12 +1051,8 @@ fail:
I don't think it will change a lot, but could you wrap the "s->cache_discards = false; qcow2_process_discards(bs, ret);" in an "if (s->cache_discards)"? You have introduced a case where s->cache_discards was still false, so we don't need to call qcow2_process_discards() then (which will hopefully return immediately, but well...).

Quoted 2 lines

s->cache_discards's initial value is true in qcow2_update_snapshot_refcount(), where s->cache_discards is set to false?

It is? I only see it set to true after the "if (l1_size2 && l1_table == NULL)" conditional block.

Well, okay, I don't know about the callers of qcow2_process_discards(), so they may have set s->cache_discards to true and then expect this function to always call qcow2_process_discards() and set s->cache_discards to false. Okay, then let's just keep it as it is.

Max

Quoted 22 lines

Or you means s->cache_discards should be set to false after g_try_malloc0(align_offset(l1_size2, 512)) failed.
Quoted 11 lines
block/qcow2-refcount.c fail: · line 1056
Quoted 11 lines
block/qcow2-refcount.c +1 -5
           }
 
           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 (l1_allocated)
+    if (l1_table)
           g_free(l1_table);
Quoted 2 lines
Just drop the condition. g_free(l1_table); is enough.
OK.
Quoted 5 lines
block/qcow2-refcount.c fail: · line 1058
Quoted 2 lines
block/qcow2-refcount.c +0 -0
       return ret;
   }
The change itself is good, it just needs some polishing.
Max

Eric Blake

2014-10-22 22:21 UTC #4 lore raw

In reply to Max Reitz

Quoted 10 lines · Max Reitz

Quoted 9 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]>
---
  block/qcow2-refcount.c | 22 +++++++---------------
  1 file changed, 7 insertions(+), 15 deletions(-)

I know we're up to v5 and that Max already took it into his branch, but...

Quoted 5 lines

Quoted 2 lines
      l1_size2 = l1_size * sizeof(uint64_t);
+ l1_table = g_try_malloc0(align_offset(l1_size2, 512));
I wanted to propose using qemu_try_blockalign(), but since it'd require a memset() afterwards, it gets rather ugly.

Not after this recent patch:

https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html

Signature · 2 lines

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Gonglei

2014-10-23 01:02 UTC #5 lore raw

Quoted 18 lines · Eric Blake

Quoted 10 lines · Max Reitz
Quoted 9 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]>
---
  block/qcow2-refcount.c | 22 +++++++---------------
  1 file changed, 7 insertions(+), 15 deletions(-)
I know we're up to v5 and that Max already took it into his branch, but...
Quoted 3 lines
Quoted 2 lines
      l1_size2 = l1_size * sizeof(uint64_t);
+ l1_table = g_try_malloc0(align_offset(l1_size2, 512));

Quoted 7 lines

Quoted 2 lines
I wanted to propose using qemu_try_blockalign(), but since it'd require a memset() afterwards, it gets rather ugly.
Not after this recent patch:
https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html

Good catch :)

Best regards,
-Gonglei

Kevin Wolf

2014-10-23 07:03 UTC #6 lore raw

In reply to Eric Blake

Am 23.10.2014 um 00:18 hat Eric Blake geschrieben:

Quoted 18 lines · Max Reitz

Quoted 10 lines · Zhang Haoyu
Quoted 9 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]>
---
  block/qcow2-refcount.c | 22 +++++++---------------
  1 file changed, 7 insertions(+), 15 deletions(-)
I know we're up to v5 and that Max already took it into his branch, but...
Quoted 3 lines
Quoted 2 lines
      l1_size2 = l1_size * sizeof(uint64_t);
+ l1_table = g_try_malloc0(align_offset(l1_size2, 512));

Quoted 6 lines

Quoted 2 lines
I wanted to propose using qemu_try_blockalign(), but since it'd require a memset() afterwards, it gets rather ugly.
Not after this recent patch:
https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html

We can switch to qemu_try_blockalign0() in a follow-up patch.

But this is far from being a fast path, so there is very little to gain anyway.

Kevin

Max Reitz

2014-10-23 07:13 UTC #7 lore raw

Quoted 19 lines · Kevin Wolf

Am 23.10.2014 um 00:18 hat Eric Blake geschrieben:
Quoted 18 lines · Max Reitz
Quoted 10 lines · Zhang Haoyu
Quoted 9 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]>
---
   block/qcow2-refcount.c | 22 +++++++---------------
   1 file changed, 7 insertions(+), 15 deletions(-)
I know we're up to v5 and that Max already took it into his branch, but...
Quoted 4 lines
Quoted 2 lines
       l1_size2 = l1_size * sizeof(uint64_t);
+ l1_table = g_try_malloc0(align_offset(l1_size2, 512));
I wanted to propose using qemu_try_blockalign(), but since it'd require a memset() afterwards, it gets rather ugly.

Quoted 7 lines

Quoted 3 lines
Not after this recent patch:
https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html
We can switch to qemu_try_blockalign0() in a follow-up patch.
But this is far from being a fast path, so there is very little to gain anyway.

We don't need the 0 variant anyway. In v5, the one I applied, it's just qemu_try_blockalign(). The size does not have to be aligned to BDRV_SECTOR_SIZE, since any access index is below l1_size and only bdrv_pread() and bdrv_pwrite() are used, so we can first omit the align_offset(). Second, all the rest (0 .. l1_size - 1) is overwritten either by memcpy() or bdrv_pread(). Therefore, no 0-ing required and qemu_try_blockalign() was fine.

There are things we can do in follow-up patches to optimize v5 (getting read of the memcpy()s), but as Kevin said (and myself before that in reply to v5), this function is not performance-critical, so there's no real point in doing so (other than "Clearly unoptimized code makes my fingers twitch").

Max