In many cases the target of a convert operation is a newly provisioned
target that the user knows is blank (filled with zeroes). In this
situation there is no requirement for qemu-img to wastefully zero out
the entire device.

Add a new option, --target-is-zero, allowing the user to indicate that
an existing target device is already zero filled.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 qemu-img-cmds.hx |  4 ++--
 qemu-img.c       | 25 ++++++++++++++++++++++---
 qemu-img.texi    |  4 ++++
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1c93e6d185..6f958a0a48 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -44,9 +44,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] [--salvage] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] [--salvage] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("create", img_create,
diff --git a/qemu-img.c b/qemu-img.c
index 6233b8ca56..46db72dbe0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -70,6 +70,7 @@ enum {
     OPTION_PREALLOCATION = 265,
     OPTION_SHRINK = 266,
     OPTION_SALVAGE = 267,
+    OPTION_TARGET_IS_ZERO = 268,
 };
 
 typedef enum OutputFormat {
@@ -1984,10 +1985,9 @@ static int convert_do_copy(ImgConvertState *s)
     int64_t sector_num = 0;
 
     /* Check whether we have zero initialisation or can get it efficiently */
-    if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
+    if (!s->has_zero_init && 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 &&
@@ -2086,6 +2086,7 @@ static int img_convert(int argc, char **argv)
             {"force-share", no_argument, 0, 'U'},
             {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
             {"salvage", no_argument, 0, OPTION_SALVAGE},
+            {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
             {0, 0, 0, 0}
         };
         c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
@@ -2209,6 +2210,14 @@ static int img_convert(int argc, char **argv)
         case OPTION_TARGET_IMAGE_OPTS:
             tgt_image_opts = true;
             break;
+        case OPTION_TARGET_IS_ZERO:
+            /*
+             * The user asserting that the target is blank has the
+             * same effect as the target driver supporting zero
+             * initialisation.
+             */
+            s.has_zero_init = true;
+            break;
         }
     }
 
@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
         warn_report("This will become an error in future QEMU versions.");
     }
 
+    if (s.has_zero_init && !skip_create) {
+        error_report("--target-is-zero requires use of -n flag");
+        goto fail_getopt;
+    }
+
     s.src_num = argc - optind - 1;
     out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
 
@@ -2380,6 +2394,11 @@ static int img_convert(int argc, char **argv)
     }
     s.target_has_backing = (bool) out_baseimg;
 
+    if (s.has_zero_init && s.target_has_backing) {
+        error_report("Cannot use --target-is-zero with a backing file");
+        goto out;
+    }
+
     if (s.src_num > 1 && out_baseimg) {
         error_report("Having a backing file for the target makes no sense when "
                      "concatenating multiple input images");
diff --git a/qemu-img.texi b/qemu-img.texi
index b5156d6316..3b6dfd8682 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -179,6 +179,10 @@ information.
 Try to ignore I/O errors when reading.  Unless in quiet mode (@code{-q}), errors
 will still be printed.  Areas that cannot be read from the source will be
 treated as containing only zeroes.
+@item --target-is-zero
+Assume that the destination is filled with zeros. This parameter is
+mutually exclusive with the use of a backing file. It is required to
+also use the @code{-n} parameter to skip image creation.
 @end table
 
 Parameters to dd subcommand:
-- 
2.24.1

On 1/24/20 4:34 AM, David Edmondson wrote:
> In many cases the target of a convert operation is a newly provisioned
> target that the user knows is blank (filled with zeroes). In this
> situation there is no requirement for qemu-img to wastefully zero out
> the entire device.
> 
> Add a new option, --target-is-zero, allowing the user to indicate that
> an existing target device is already zero filled.
> 
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
>   qemu-img-cmds.hx |  4 ++--
>   qemu-img.c       | 25 ++++++++++++++++++++++---
>   qemu-img.texi    |  4 ++++
>   3 files changed, 28 insertions(+), 5 deletions(-)

I'm working up a patch series that tries to auto-set this flag without 
user interaction where possible (for example, if lseek(fd, 0, SEEK_DATA) 
returns EOF, or if fstat() reports 0 blocks allocated, or if qcow2 sees 
no L2 tables allocated, or a proposed extension to NBD passes on the 
same...).  I may rebase my series on top of your patch and tweak things 
in yours accordingly.

But as it stands, the idea makes sense to me; even if we add ways for 
some images to efficiently report initial state (and our existing 
bdrv_has_zero_init() is NOT such a method), there are enough other 
scenarios where the knob will be the only way to let qemu-img know the 
intent.


> +        case OPTION_TARGET_IS_ZERO:
> +            /*
> +             * The user asserting that the target is blank has the
> +             * same effect as the target driver supporting zero
> +             * initialisation.

Hmm. A git grep shows that 'initialization' has 200 hits, 
'initialisation' has only 29. But I think it's a US vs. UK thing, so I 
don't care which spelling you use.


> @@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
>           warn_report("This will become an error in future QEMU versions.");
>       }
>   
> +    if (s.has_zero_init && !skip_create) {
> +        error_report("--target-is-zero requires use of -n flag");
> +        goto fail_getopt;
> +    }
> +

Makes sense, although we could perhaps relax it to also work even when 
the -n flag is supplied IF the destination image supports my proposal 
for a new status bit set when an image is known to be opened with all 
zero content.

>       s.src_num = argc - optind - 1;
>       out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
>   
> @@ -2380,6 +2394,11 @@ static int img_convert(int argc, char **argv)
>       }
>       s.target_has_backing = (bool) out_baseimg;
>   
> +    if (s.has_zero_init && s.target_has_backing) {
> +        error_report("Cannot use --target-is-zero with a backing file");
> +        goto out;
> +    }
> +

Makes sense, although we could perhaps relax it to also work even when 
there is a backing file IF the backing file supports my proposal for a 
new status bit set when an image is known to be opened with all zero 
content.

As my patch proposal is still not submitted, I'm fine if yours lands as-is:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 1/24/20 4:34 AM, David Edmondson wrote:
>> In many cases the target of a convert operation is a newly provisioned
>> target that the user knows is blank (filled with zeroes). In this
>> situation there is no requirement for qemu-img to wastefully zero out
>> the entire device.
>> 
>> Add a new option, --target-is-zero, allowing the user to indicate that
>> an existing target device is already zero filled.
>> 
>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>   qemu-img-cmds.hx |  4 ++--
>>   qemu-img.c       | 25 ++++++++++++++++++++++---
>>   qemu-img.texi    |  4 ++++
>>   3 files changed, 28 insertions(+), 5 deletions(-)
>
> I'm working up a patch series that tries to auto-set this flag without 
> user interaction where possible (for example, if lseek(fd, 0, SEEK_DATA) 
> returns EOF, or if fstat() reports 0 blocks allocated, or if qcow2 sees 
> no L2 tables allocated, or a proposed extension to NBD passes on the 
> same...).  I may rebase my series on top of your patch and tweak things 
> in yours accordingly.
>
> But as it stands, the idea makes sense to me; even if we add ways for 
> some images to efficiently report initial state (and our existing 
> bdrv_has_zero_init() is NOT such a method), there are enough other 
> scenarios where the knob will be the only way to let qemu-img know the 
> intent.

Having qemu-img figure things out on its own is obviously desirable, but
I agree that there are enough cases where this won't be possible and,
given the resulting performance improvement, it will still be useful to
allow the caller to force things.

>> +        case OPTION_TARGET_IS_ZERO:
>> +            /*
>> +             * The user asserting that the target is blank has the
>> +             * same effect as the target driver supporting zero
>> +             * initialisation.
>
> Hmm. A git grep shows that 'initialization' has 200 hits, 
> 'initialisation' has only 29. But I think it's a US vs. UK thing, so I 
> don't care which spelling you use.

Yes, it's British English spelling. It was unconscious - I'll switch if
there is an existing policy.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks.

If the conversion of the documentation to rST is imminent then I'll wait
for that before submitting a followup with corresponding changes applied
to the new docs.

dme.
-- 
I'd come on over but I haven't got a raincoat.

24.01.2020 13:34, David Edmondson wrote:
> In many cases the target of a convert operation is a newly provisioned
> target that the user knows is blank (filled with zeroes). In this
> situation there is no requirement for qemu-img to wastefully zero out
> the entire device.
> 
> Add a new option, --target-is-zero, allowing the user to indicate that
> an existing target device is already zero filled.

Hi! qemu-img.c part looks OK for me, but other doesn't apply on master now.

I like this thing, and I'd like to make similar option for backup job.

> 
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
>   qemu-img-cmds.hx |  4 ++--
>   qemu-img.c       | 25 ++++++++++++++++++++++---
>   qemu-img.texi    |  4 ++++
>   3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 1c93e6d185..6f958a0a48 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -44,9 +44,9 @@ STEXI
>   ETEXI
>   
>   DEF("convert", img_convert,
> -    "convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
> +    "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
>   STEXI
> -@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] [--salvage] @var{filename} [@var{filename2} [...]] @var{output_filename}
> +@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] [--salvage] @var{filename} [@var{filename2} [...]] @var{output_filename}
>   ETEXI
>   
>   DEF("create", img_create,
> diff --git a/qemu-img.c b/qemu-img.c
> index 6233b8ca56..46db72dbe0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -70,6 +70,7 @@ enum {
>       OPTION_PREALLOCATION = 265,
>       OPTION_SHRINK = 266,
>       OPTION_SALVAGE = 267,
> +    OPTION_TARGET_IS_ZERO = 268,
>   };
>   
>   typedef enum OutputFormat {
> @@ -1984,10 +1985,9 @@ static int convert_do_copy(ImgConvertState *s)
>       int64_t sector_num = 0;
>   
>       /* Check whether we have zero initialisation or can get it efficiently */
> -    if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
> +    if (!s->has_zero_init && 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 &&
> @@ -2086,6 +2086,7 @@ static int img_convert(int argc, char **argv)
>               {"force-share", no_argument, 0, 'U'},
>               {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
>               {"salvage", no_argument, 0, OPTION_SALVAGE},
> +            {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
>               {0, 0, 0, 0}
>           };
>           c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
> @@ -2209,6 +2210,14 @@ static int img_convert(int argc, char **argv)
>           case OPTION_TARGET_IMAGE_OPTS:
>               tgt_image_opts = true;
>               break;
> +        case OPTION_TARGET_IS_ZERO:
> +            /*
> +             * The user asserting that the target is blank has the
> +             * same effect as the target driver supporting zero
> +             * initialisation.
> +             */
> +            s.has_zero_init = true;
> +            break;
>           }
>       }
>   
> @@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
>           warn_report("This will become an error in future QEMU versions.");
>       }
>   
> +    if (s.has_zero_init && !skip_create) {
> +        error_report("--target-is-zero requires use of -n flag");
> +        goto fail_getopt;
> +    }
> +
>       s.src_num = argc - optind - 1;
>       out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
>   
> @@ -2380,6 +2394,11 @@ static int img_convert(int argc, char **argv)
>       }
>       s.target_has_backing = (bool) out_baseimg;
>   
> +    if (s.has_zero_init && s.target_has_backing) {
> +        error_report("Cannot use --target-is-zero with a backing file");
> +        goto out;
> +    }
> +
>       if (s.src_num > 1 && out_baseimg) {
>           error_report("Having a backing file for the target makes no sense when "
>                        "concatenating multiple input images");
> diff --git a/qemu-img.texi b/qemu-img.texi
> index b5156d6316..3b6dfd8682 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -179,6 +179,10 @@ information.
>   Try to ignore I/O errors when reading.  Unless in quiet mode (@code{-q}), errors
>   will still be printed.  Areas that cannot be read from the source will be
>   treated as containing only zeroes.
> +@item --target-is-zero
> +Assume that the destination is filled with zeros. This parameter is
> +mutually exclusive with the use of a backing file. It is required to
> +also use the @code{-n} parameter to skip image creation.
>   @end table
>   
>   Parameters to dd subcommand:
> 


-- 
Best regards,
Vladimir

On 2/3/20 12:20 PM, Vladimir Sementsov-Ogievskiy wrote:
> 24.01.2020 13:34, David Edmondson wrote:
>> In many cases the target of a convert operation is a newly provisioned
>> target that the user knows is blank (filled with zeroes). In this
>> situation there is no requirement for qemu-img to wastefully zero out
>> the entire device.
>>
>> Add a new option, --target-is-zero, allowing the user to indicate that
>> an existing target device is already zero filled.
> 
> Hi! qemu-img.c part looks OK for me, but other doesn't apply on master now.

Correct. Patch 2/2 is now obsolete and no longer necessary, and patch 1 
needs some tweaks now that we don't have qemu-img.texi.

> 
> I like this thing, and I'd like to make similar option for backup job.

My followup patches to add an all-zero bit to qcow2 are also useful in 
this regard.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Monday, 2020-02-03 at 21:20:16 +03, Vladimir Sementsov-Ogievskiy wrote:

> 24.01.2020 13:34, David Edmondson wrote:
>> In many cases the target of a convert operation is a newly provisioned
>> target that the user knows is blank (filled with zeroes). In this
>> situation there is no requirement for qemu-img to wastefully zero out
>> the entire device.
>> 
>> Add a new option, --target-is-zero, allowing the user to indicate that
>> an existing target device is already zero filled.
>
> Hi! qemu-img.c part looks OK for me, but other doesn't apply on master now.

Updated v3 just sent.

dme.
-- 
But he said, leave me alone, I'm a family man.