[Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[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 zhan...@sangfor.com
Reviewed-by: Max Reitz mre...@redhat.com
---
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..3e4050a 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_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,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;
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Gonglei]

On 2014/10/22 19:45, 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 zhan...@sangfor.com
 Reviewed-by: Max Reitz mre...@redhat.com
 ---
 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..3e4050a 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_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

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

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz]

On 2014-10-22 at 13:59, Gonglei wrote:

On 2014/10/22 19:45, 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 zhan...@sangfor.com
Reviewed-by: Max Reitz mre...@redhat.com
---
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..3e4050a 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_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...?

Max


Best regards,
-Gonglei


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

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz]

On 2014-10-22 at 14:01, Max Reitz wrote:

On 2014-10-22 at 13:59, Gonglei wrote:

On 2014/10/22 19:45, 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 zhan...@sangfor.com
Reviewed-by: Max Reitz mre...@redhat.com
---
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..3e4050a 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_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...?


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

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Gonglei]

On 2014/10/22 20:02, Max Reitz wrote:

 On 2014-10-22 at 14:01, Max Reitz wrote:
 On 2014-10-22 at 13:59, Gonglei wrote:
 On 2014/10/22 19:45, 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 zhan...@sangfor.com
 Reviewed-by: Max Reitz mre...@redhat.com
 ---
 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..3e4050a 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_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...?
 
 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.

Best regards,
-Gonglei

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


 Max

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz]

On 2014-10-22 at 14:21, Gonglei wrote:

On 2014/10/22 20:02, Max Reitz wrote:


On 2014-10-22 at 14:01, Max Reitz wrote:

On 2014-10-22 at 13:59, Gonglei wrote:

On 2014/10/22 19:45, 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 zhan...@sangfor.com
Reviewed-by: Max Reitz mre...@redhat.com
---
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..3e4050a 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_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...?

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.


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


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.


Max