Re: [Qemu-devel] [PATCH v2 01/11] qemu-img: Fix bdrv_has_zero_init() use in convert

[Stefano Garzarella]

On Wed, Jul 24, 2019 at 07:12:29PM +0200, Max Reitz wrote:
> bdrv_has_zero_init() only has meaning for newly created images or image
> areas.  If qemu-img convert did not create the image itself, it cannot
> rely on bdrv_has_zero_init()'s result to carry any meaning.
> 
> Signed-off-by: Max Reitz 
> ---
>  qemu-img.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 79983772de..0f4be80c10 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1578,6 +1578,7 @@ typedef struct ImgConvertState {
>  bool has_zero_init;
>  bool compressed;
>  bool unallocated_blocks_are_zero;
> +bool target_is_new;
>  bool target_has_backing;
>  int64_t target_backing_sectors; /* negative if unknown */
>  bool wr_in_order;
> @@ -1975,9 +1976,11 @@ static int convert_do_copy(ImgConvertState *s)
>  int64_t sector_num = 0;
>  
>  /* Check whether we have zero initialisation or can get it efficiently */
> -s->has_zero_init = s->min_sparse && !s->target_has_backing
> - ? bdrv_has_zero_init(blk_bs(s->target))
> - : false;
> +if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
> +s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
> +} else {
> +s->has_zero_init = false;
> +}
>  
>  if (!s->has_zero_init && !s->target_has_backing &&
>  bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))
> @@ -2423,6 +2426,8 @@ static int img_convert(int argc, char **argv)
>  }
>  }
>  
> +s.target_is_new = !skip_create;
> +
>  flags = s.min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
>  ret = bdrv_parse_cache_mode(cache, , );
>  if (ret < 0) {

Make sense!

Reviewed-by: Stefano Garzarella 

Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 01/11] qemu-img: Fix bdrv_has_zero_init() use in convert

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> bdrv_has_zero_init() only has meaning for newly created images or image
> areas.  If qemu-img convert did not create the image itself, it cannot
> rely on bdrv_has_zero_init()'s result to carry any meaning.
> 
> Signed-off-by: Max Reitz 
> ---
>  qemu-img.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 79983772de..0f4be80c10 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1578,6 +1578,7 @@ typedef struct ImgConvertState {
>  bool has_zero_init;
>  bool compressed;
>  bool unallocated_blocks_are_zero;
> +bool target_is_new;
>  bool target_has_backing;
>  int64_t target_backing_sectors; /* negative if unknown */
>  bool wr_in_order;
> @@ -1975,9 +1976,11 @@ static int convert_do_copy(ImgConvertState *s)
>  int64_t sector_num = 0;
>  
>  /* Check whether we have zero initialisation or can get it efficiently */
> -s->has_zero_init = s->min_sparse && !s->target_has_backing
> - ? bdrv_has_zero_init(blk_bs(s->target))
> - : false;
> +if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
> +s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
> +} else {
> +s->has_zero_init = false;
> +}
>  
>  if (!s->has_zero_init && !s->target_has_backing &&
>  bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))
> @@ -2423,6 +2426,8 @@ static int img_convert(int argc, char **argv)
>  }
>  }
>  
> +s.target_is_new = !skip_create;
> +
>  flags = s.min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
>  ret = bdrv_parse_cache_mode(cache, , );
>  if (ret < 0) {


Reviewed-by: Maxim Levitsky 
Best regards,
Maxim Levitsky

[Qemu-devel] [PATCH v2 01/11] qemu-img: Fix bdrv_has_zero_init() use in convert

[Max Reitz]

bdrv_has_zero_init() only has meaning for newly created images or image
areas.  If qemu-img convert did not create the image itself, it cannot
rely on bdrv_has_zero_init()'s result to carry any meaning.

Signed-off-by: Max Reitz 
---
 qemu-img.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 79983772de..0f4be80c10 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1578,6 +1578,7 @@ typedef struct ImgConvertState {
 bool has_zero_init;
 bool compressed;
 bool unallocated_blocks_are_zero;
+bool target_is_new;
 bool target_has_backing;
 int64_t target_backing_sectors; /* negative if unknown */
 bool wr_in_order;
@@ -1975,9 +1976,11 @@ static int convert_do_copy(ImgConvertState *s)
 int64_t sector_num = 0;
 
 /* Check whether we have zero initialisation or can get it efficiently */
-s->has_zero_init = s->min_sparse && !s->target_has_backing
- ? bdrv_has_zero_init(blk_bs(s->target))
- : false;
+if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
+s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
+} else {
+s->has_zero_init = false;
+}
 
 if (!s->has_zero_init && !s->target_has_backing &&
 bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))
@@ -2423,6 +2426,8 @@ static int img_convert(int argc, char **argv)
 }
 }
 
+s.target_is_new = !skip_create;
+
 flags = s.min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
 ret = bdrv_parse_cache_mode(cache, , );
 if (ret < 0) {
-- 
2.21.0