[PATCH v3 0/1] qemu-img: Add --target-is-zero to indicate that a target is blank

[PATCH v3 0/1] qemu-img: Add --target-is-zero to indicate that a target is blank

[David Edmondson]

qemu-img: Add --target-is-zero to indicate that a target is blank

v3:
- Merge with the rST docs.
- No more need to fix @var -> @code.

v2:
- Remove target_is_zero, preferring to set has_zero_init
  directly (Mark Kanda).
- Disallow --target-is-zero in the presence of a backing file (Max
  Reitz).
- Add relevant documentation (Max Reitz).
- @var -> @code for options in qemu-img.texi.


David Edmondson (1):
  qemu-img: Add --target-is-zero to convert

 docs/interop/qemu-img.rst |  8 +++++++-
 qemu-img-cmds.hx          |  4 ++--
 qemu-img.c                | 25 ++++++++++++++++++++++---
 3 files changed, 31 insertions(+), 6 deletions(-)

-- 
2.24.1

[PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[David Edmondson]

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>
---
 docs/interop/qemu-img.rst |  8 +++++++-
 qemu-img-cmds.hx          |  4 ++--
 qemu-img.c                | 25 ++++++++++++++++++++++---
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst
index fa27e5c7b453..99bdbe4740ee 100644
--- a/docs/interop/qemu-img.rst
+++ b/docs/interop/qemu-img.rst
@@ -214,6 +214,12 @@ Parameters to convert subcommand:
   will still be printed.  Areas that cannot be read from the source will be
   treated as containing only zeroes.
 
+.. option:: --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 ``-n`` parameter to skip image creation.
+
 Parameters to dd subcommand:
 
 .. program:: qemu-img-dd
@@ -366,7 +372,7 @@ Command description:
   4
     Error on reading data
 
-.. option:: 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] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. option:: 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] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
 
   Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
   to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 3fd836ca9090..e6f98b75473f 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -39,9 +39,9 @@ SRST
 ERST
 
 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")
 SRST
-.. option:: 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
+.. option:: 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
 ERST
 
 DEF("create", img_create,
diff --git a/qemu-img.c b/qemu-img.c
index 2b4562b9d9f2..e0bfc33ef4f6 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");
-- 
2.24.1

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Eric Blake]

On 2/4/20 3:52 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

'filled with zeroes' is accurate for a preallocated image, but reads 
awkwardly for a sparse image; it might be better to state 'reads as zero'.

> 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>
> ---
>   docs/interop/qemu-img.rst |  8 +++++++-
>   qemu-img-cmds.hx          |  4 ++--
>   qemu-img.c                | 25 ++++++++++++++++++++++---
>   3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst
> index fa27e5c7b453..99bdbe4740ee 100644
> --- a/docs/interop/qemu-img.rst
> +++ b/docs/interop/qemu-img.rst
> @@ -214,6 +214,12 @@ Parameters to convert subcommand:
>     will still be printed.  Areas that cannot be read from the source will be
>     treated as containing only zeroes.
>   
> +.. option:: --target-is-zero
> +
> +  Assume that the destination is filled with zeros. This parameter is

Spelled 'zeroes' just three lines earlier.

> +  mutually exclusive with the use of a backing file. It is required to
> +  also use the ``-n`` parameter to skip image creation.

I understand that it is safer to have restrictions now and lift them 
later, than to allow use of the option at any time and leave room for 
the user to shoot themselves in the foot with no way to add safety 
later.  The argument against no backing file is somewhat understandable 
(technically, as long as the backing file also reads as all zeroes, then 
the overall image reads as all zeroes - but why have a backing file that 
has no content?); the argument requiring -n is a bit weaker (if I'm 
creating an image, I _know_ it reads as all zeroes, so the 
--target-is-zero argument is redundant, but it shouldn't hurt to allow it).

> +++ b/qemu-img.c

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

So I think we could drop this hunk with no change in behavior.

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

while keeping this one makes sense.  At any rate, typo fixes are minor, 
and whether or not we drop the hunk I claim is a needless restriction 
against redundancy,

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

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

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Vladimir Sementsov-Ogievskiy]

04.02.2020 16:31, Eric Blake wrote:
> On 2/4/20 3:52 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
> 
> 'filled with zeroes' is accurate for a preallocated image, but reads awkwardly for a sparse image; it might be better to state 'reads as zero'.
> 
>> 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>
>> ---
>>   docs/interop/qemu-img.rst |  8 +++++++-
>>   qemu-img-cmds.hx          |  4 ++--
>>   qemu-img.c                | 25 ++++++++++++++++++++++---
>>   3 files changed, 31 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst
>> index fa27e5c7b453..99bdbe4740ee 100644
>> --- a/docs/interop/qemu-img.rst
>> +++ b/docs/interop/qemu-img.rst
>> @@ -214,6 +214,12 @@ Parameters to convert subcommand:
>>     will still be printed.  Areas that cannot be read from the source will be
>>     treated as containing only zeroes.
>> +.. option:: --target-is-zero
>> +
>> +  Assume that the destination is filled with zeros. This parameter is
> 
> Spelled 'zeroes' just three lines earlier.
> 
>> +  mutually exclusive with the use of a backing file. It is required to
>> +  also use the ``-n`` parameter to skip image creation.
> 
> I understand that it is safer to have restrictions now and lift them later, than to allow use of the option at any time and leave room for the user to shoot themselves in the foot with no way to add safety later.  The argument against no backing file is somewhat understandable (technically, as long as the backing file also reads as all zeroes, then the overall image reads as all zeroes - but why have a backing file that has no content?); the argument requiring -n is a bit weaker (if I'm creating an image, I _know_ it reads as all zeroes, so the --target-is-zero argument is redundant, but it shouldn't hurt to allow it).

I know that it reads as all zeroes, only if this format provides zero initialization..

> 
>> +++ b/qemu-img.c
> 
>> @@ -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;
>> +    }
> 
> So I think we could drop this hunk with no change in behavior.

I think, no we can't. If we allow target-is-zero, with -n, we'd better to check that what we are creating is zero-initialized (format has zero-init), and if not we should report error.

> 
>> +
>>       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;
>> +    }
>> +
> 
> while keeping this one makes sense.  At any rate, typo fixes are minor, and whether or not we drop the hunk I claim is a needless restriction against redundancy,
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Eric Blake]

On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:

>> I understand that it is safer to have restrictions now and lift them 
>> later, than to allow use of the option at any time and leave room for 
>> the user to shoot themselves in the foot with no way to add safety 
>> later.  The argument against no backing file is somewhat 
>> understandable (technically, as long as the backing file also reads as 
>> all zeroes, then the overall image reads as all zeroes - but why have 
>> a backing file that has no content?); the argument requiring -n is a 
>> bit weaker (if I'm creating an image, I _know_ it reads as all zeroes, 
>> so the --target-is-zero argument is redundant, but it shouldn't hurt 
>> to allow it).
> 
> I know that it reads as all zeroes, only if this format provides zero 
> initialization..
> 
>>
>>> +++ b/qemu-img.c
>>
>>> @@ -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;
>>> +    }
>>
>> So I think we could drop this hunk with no change in behavior.
> 
> I think, no we can't. If we allow target-is-zero, with -n, we'd better 
> to check that what we are creating is zero-initialized (format has 
> zero-init), and if not we should report error.
> 

Good call.  Yes, if we allow --target-is-zero without -n, we MUST insist 
that bdrv_has_zero_init() returns 1 (or, after my followup series, 
bdrv_known_zeroes() includes BDRV_ZERO_CREATE).  I can work that into v2 
of my series, if desired.

>>
>>> +
>>>       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;
>>> +    }
>>> +
>>
>> while keeping this one makes sense.  At any rate, typo fixes are 
>> minor, and whether or not we drop the hunk I claim is a needless 
>> restriction against redundancy,
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
> 
> 

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

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2092 bytes --]

On 04.02.20 15:23, Eric Blake wrote:
> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>> I understand that it is safer to have restrictions now and lift them
>>> later, than to allow use of the option at any time and leave room for
>>> the user to shoot themselves in the foot with no way to add safety
>>> later.  The argument against no backing file is somewhat
>>> understandable (technically, as long as the backing file also reads
>>> as all zeroes, then the overall image reads as all zeroes - but why
>>> have a backing file that has no content?); the argument requiring -n
>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all
>>> zeroes, so the --target-is-zero argument is redundant, but it
>>> shouldn't hurt to allow it).
>>
>> I know that it reads as all zeroes, only if this format provides zero
>> initialization..
>>
>>>
>>>> +++ b/qemu-img.c
>>>
>>>> @@ -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;
>>>> +    }
>>>
>>> So I think we could drop this hunk with no change in behavior.
>>
>> I think, no we can't. If we allow target-is-zero, with -n, we'd better
>> to check that what we are creating is zero-initialized (format has
>> zero-init), and if not we should report error.
>>
> 
> Good call.  Yes, if we allow --target-is-zero without -n, we MUST insist
> that bdrv_has_zero_init() returns 1 (or, after my followup series,
> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).

Why?

I could imagine a user creating a qcow2 image on some block device with
preallocation where we cannot verify that the result will be zero.  But
they want qemu not to zero the device, so they would specify
--target-is-zero.

OTOH, we can always choose to allow that behavior at a later point.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Vladimir Sementsov-Ogievskiy]

07.02.2020 13:33, Max Reitz wrote:
> On 04.02.20 15:23, Eric Blake wrote:
>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>>> I understand that it is safer to have restrictions now and lift them
>>>> later, than to allow use of the option at any time and leave room for
>>>> the user to shoot themselves in the foot with no way to add safety
>>>> later.  The argument against no backing file is somewhat
>>>> understandable (technically, as long as the backing file also reads
>>>> as all zeroes, then the overall image reads as all zeroes - but why
>>>> have a backing file that has no content?); the argument requiring -n
>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all
>>>> zeroes, so the --target-is-zero argument is redundant, but it
>>>> shouldn't hurt to allow it).
>>>
>>> I know that it reads as all zeroes, only if this format provides zero
>>> initialization..
>>>
>>>>
>>>>> +++ b/qemu-img.c
>>>>
>>>>> @@ -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;
>>>>> +    }
>>>>
>>>> So I think we could drop this hunk with no change in behavior.
>>>
>>> I think, no we can't. If we allow target-is-zero, with -n, we'd better
>>> to check that what we are creating is zero-initialized (format has
>>> zero-init), and if not we should report error.
>>>
>>
>> Good call.  Yes, if we allow --target-is-zero without -n, we MUST insist
>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
> 
> Why?
> 
> I could imagine a user creating a qcow2 image on some block device with
> preallocation where we cannot verify that the result will be zero.  But
> they want qemu not to zero the device, so they would specify
> --target-is-zero.

If user create image, setting --target-is-zero is always valid. But if we in
same operation create the image automatically, having --target-is-zero, when
we know that what we are creating is not zero is misleading and should
fail..

If we want to add a behavior to skip zeros unconditionally, we should call new
option --skip-zeroes, to clearly specify what we want.

> 
> OTOH, we can always choose to allow that behavior at a later point.
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2900 bytes --]

On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 13:33, Max Reitz wrote:
>> On 04.02.20 15:23, Eric Blake wrote:
>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>>> I understand that it is safer to have restrictions now and lift them
>>>>> later, than to allow use of the option at any time and leave room for
>>>>> the user to shoot themselves in the foot with no way to add safety
>>>>> later.  The argument against no backing file is somewhat
>>>>> understandable (technically, as long as the backing file also reads
>>>>> as all zeroes, then the overall image reads as all zeroes - but why
>>>>> have a backing file that has no content?); the argument requiring -n
>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all
>>>>> zeroes, so the --target-is-zero argument is redundant, but it
>>>>> shouldn't hurt to allow it).
>>>>
>>>> I know that it reads as all zeroes, only if this format provides zero
>>>> initialization..
>>>>
>>>>>
>>>>>> +++ b/qemu-img.c
>>>>>
>>>>>> @@ -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;
>>>>>> +    }
>>>>>
>>>>> So I think we could drop this hunk with no change in behavior.
>>>>
>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd better
>>>> to check that what we are creating is zero-initialized (format has
>>>> zero-init), and if not we should report error.
>>>>
>>>
>>> Good call.  Yes, if we allow --target-is-zero without -n, we MUST insist
>>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
>>
>> Why?
>>
>> I could imagine a user creating a qcow2 image on some block device with
>> preallocation where we cannot verify that the result will be zero.  But
>> they want qemu not to zero the device, so they would specify
>> --target-is-zero.
> 
> If user create image, setting --target-is-zero is always valid. But if
> we in
> same operation create the image automatically, having --target-is-zero,
> when
> we know that what we are creating is not zero is misleading and should
> fail..

bdrv_has_zero_init() doesn’t return false only for images that we know
are not zero.  It returns true for images where we know they are.  But
if we don’t know, then it returns false also.

> If we want to add a behavior to skip zeros unconditionally, we should
> call new
> option --skip-zeroes, to clearly specify what we want.

It was my impression that this was exactly what --target-is-zero means
and implies.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Vladimir Sementsov-Ogievskiy]

07.02.2020 17:41, Max Reitz wrote:
> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
>> 07.02.2020 13:33, Max Reitz wrote:
>>> On 04.02.20 15:23, Eric Blake wrote:
>>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>
>>>>>> I understand that it is safer to have restrictions now and lift them
>>>>>> later, than to allow use of the option at any time and leave room for
>>>>>> the user to shoot themselves in the foot with no way to add safety
>>>>>> later.  The argument against no backing file is somewhat
>>>>>> understandable (technically, as long as the backing file also reads
>>>>>> as all zeroes, then the overall image reads as all zeroes - but why
>>>>>> have a backing file that has no content?); the argument requiring -n
>>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all
>>>>>> zeroes, so the --target-is-zero argument is redundant, but it
>>>>>> shouldn't hurt to allow it).
>>>>>
>>>>> I know that it reads as all zeroes, only if this format provides zero
>>>>> initialization..
>>>>>
>>>>>>
>>>>>>> +++ b/qemu-img.c
>>>>>>
>>>>>>> @@ -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;
>>>>>>> +    }
>>>>>>
>>>>>> So I think we could drop this hunk with no change in behavior.
>>>>>
>>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd better
>>>>> to check that what we are creating is zero-initialized (format has
>>>>> zero-init), and if not we should report error.
>>>>>
>>>>
>>>> Good call.  Yes, if we allow --target-is-zero without -n, we MUST insist
>>>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
>>>
>>> Why?
>>>
>>> I could imagine a user creating a qcow2 image on some block device with
>>> preallocation where we cannot verify that the result will be zero.  But
>>> they want qemu not to zero the device, so they would specify
>>> --target-is-zero.
>>
>> If user create image, setting --target-is-zero is always valid. But if
>> we in
>> same operation create the image automatically, having --target-is-zero,
>> when
>> we know that what we are creating is not zero is misleading and should
>> fail..
> 
> bdrv_has_zero_init() doesn’t return false only for images that we know
> are not zero.  It returns true for images where we know they are.  But
> if we don’t know, then it returns false also.

yes, but we don't have better check.

> 
>> If we want to add a behavior to skip zeros unconditionally, we should
>> call new
>> option --skip-zeroes, to clearly specify what we want.
> 
> It was my impression that this was exactly what --target-is-zero means
> and implies.
> 

For me it sounds strange that user has better knowledge about what Qemu
creates than Qemu itself. And if it so - it should be fixed in Qemu,
rather than creating user interface to hint Qemu what it does.



-- 
Best regards,
Vladimir

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3722 bytes --]

On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 17:41, Max Reitz wrote:
>> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.02.2020 13:33, Max Reitz wrote:
>>>> On 04.02.20 15:23, Eric Blake wrote:
>>>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>
>>>>>>> I understand that it is safer to have restrictions now and lift them
>>>>>>> later, than to allow use of the option at any time and leave room
>>>>>>> for
>>>>>>> the user to shoot themselves in the foot with no way to add safety
>>>>>>> later.  The argument against no backing file is somewhat
>>>>>>> understandable (technically, as long as the backing file also reads
>>>>>>> as all zeroes, then the overall image reads as all zeroes - but why
>>>>>>> have a backing file that has no content?); the argument requiring -n
>>>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all
>>>>>>> zeroes, so the --target-is-zero argument is redundant, but it
>>>>>>> shouldn't hurt to allow it).
>>>>>>
>>>>>> I know that it reads as all zeroes, only if this format provides zero
>>>>>> initialization..
>>>>>>
>>>>>>>
>>>>>>>> +++ b/qemu-img.c
>>>>>>>
>>>>>>>> @@ -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;
>>>>>>>> +    }
>>>>>>>
>>>>>>> So I think we could drop this hunk with no change in behavior.
>>>>>>
>>>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd
>>>>>> better
>>>>>> to check that what we are creating is zero-initialized (format has
>>>>>> zero-init), and if not we should report error.
>>>>>>
>>>>>
>>>>> Good call.  Yes, if we allow --target-is-zero without -n, we MUST
>>>>> insist
>>>>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>>>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
>>>>
>>>> Why?
>>>>
>>>> I could imagine a user creating a qcow2 image on some block device with
>>>> preallocation where we cannot verify that the result will be zero.  But
>>>> they want qemu not to zero the device, so they would specify
>>>> --target-is-zero.
>>>
>>> If user create image, setting --target-is-zero is always valid. But if
>>> we in
>>> same operation create the image automatically, having --target-is-zero,
>>> when
>>> we know that what we are creating is not zero is misleading and should
>>> fail..
>>
>> bdrv_has_zero_init() doesn’t return false only for images that we know
>> are not zero.  It returns true for images where we know they are.  But
>> if we don’t know, then it returns false also.
> 
> yes, but we don't have better check.

Correct, but maybe the user knows more, hence why it may make sense for
them to provide us with some information we don’t have.

>>> If we want to add a behavior to skip zeros unconditionally, we should
>>> call new
>>> option --skip-zeroes, to clearly specify what we want.
>>
>> It was my impression that this was exactly what --target-is-zero means
>> and implies.
>>
> 
> For me it sounds strange that user has better knowledge about what Qemu
> creates than Qemu itself. And if it so - it should be fixed in Qemu,
> rather than creating user interface to hint Qemu what it does.

I brought an example where qemu cannot know whether the image is zero
(preallocation on a block device), but the user / management layer might
know.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Vladimir Sementsov-Ogievskiy]

07.02.2020 18:03, Max Reitz wrote:
> On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote:
>> 07.02.2020 17:41, Max Reitz wrote:
>>> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>>> 07.02.2020 13:33, Max Reitz wrote:
>>>>> On 04.02.20 15:23, Eric Blake wrote:
>>>>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>
>>>>>>>> I understand that it is safer to have restrictions now and lift them
>>>>>>>> later, than to allow use of the option at any time and leave room
>>>>>>>> for
>>>>>>>> the user to shoot themselves in the foot with no way to add safety
>>>>>>>> later.  The argument against no backing file is somewhat
>>>>>>>> understandable (technically, as long as the backing file also reads
>>>>>>>> as all zeroes, then the overall image reads as all zeroes - but why
>>>>>>>> have a backing file that has no content?); the argument requiring -n
>>>>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all
>>>>>>>> zeroes, so the --target-is-zero argument is redundant, but it
>>>>>>>> shouldn't hurt to allow it).
>>>>>>>
>>>>>>> I know that it reads as all zeroes, only if this format provides zero
>>>>>>> initialization..
>>>>>>>
>>>>>>>>
>>>>>>>>> +++ b/qemu-img.c
>>>>>>>>
>>>>>>>>> @@ -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;
>>>>>>>>> +    }
>>>>>>>>
>>>>>>>> So I think we could drop this hunk with no change in behavior.
>>>>>>>
>>>>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd
>>>>>>> better
>>>>>>> to check that what we are creating is zero-initialized (format has
>>>>>>> zero-init), and if not we should report error.
>>>>>>>
>>>>>>
>>>>>> Good call.  Yes, if we allow --target-is-zero without -n, we MUST
>>>>>> insist
>>>>>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>>>>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
>>>>>
>>>>> Why?
>>>>>
>>>>> I could imagine a user creating a qcow2 image on some block device with
>>>>> preallocation where we cannot verify that the result will be zero.  But
>>>>> they want qemu not to zero the device, so they would specify
>>>>> --target-is-zero.
>>>>
>>>> If user create image, setting --target-is-zero is always valid. But if
>>>> we in
>>>> same operation create the image automatically, having --target-is-zero,
>>>> when
>>>> we know that what we are creating is not zero is misleading and should
>>>> fail..
>>>
>>> bdrv_has_zero_init() doesn’t return false only for images that we know
>>> are not zero.  It returns true for images where we know they are.  But
>>> if we don’t know, then it returns false also.
>>
>> yes, but we don't have better check.
> 
> Correct, but maybe the user knows more, hence why it may make sense for
> them to provide us with some information we don’t have.
> 
>>>> If we want to add a behavior to skip zeros unconditionally, we should
>>>> call new
>>>> option --skip-zeroes, to clearly specify what we want.
>>>
>>> It was my impression that this was exactly what --target-is-zero means
>>> and implies.
>>>
>>
>> For me it sounds strange that user has better knowledge about what Qemu
>> creates than Qemu itself. And if it so - it should be fixed in Qemu,
>> rather than creating user interface to hint Qemu what it does.
> 
> I brought an example where qemu cannot know whether the image is zero
> (preallocation on a block device), but the user / management layer might
> know.
> 

It sounds unsafe for me. User can't know how exactly Qemu do preallocation,
which syscalls it calls, etc. How can user be sure, that Qemu produces
all-zero image, if even Qemu doesn't sure in it?

Otherwise, we should document, how exactly (up to syscalls, their
parameters, flags, the whole logic and algorithm) preallocation is done,
so user can analyze it and be sure that produced image would be all-zero
(when Qemu can't determine it because some specific block device, for which
Qemu doesn't know that its preallocation algorithm produces all-zero, but
user is sure in it)...



-- 
Best regards,
Vladimir

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5132 bytes --]

On 07.02.20 16:16, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 18:03, Max Reitz wrote:
>> On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.02.2020 17:41, Max Reitz wrote:
>>>> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 07.02.2020 13:33, Max Reitz wrote:
>>>>>> On 04.02.20 15:23, Eric Blake wrote:
>>>>>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>
>>>>>>>>> I understand that it is safer to have restrictions now and lift
>>>>>>>>> them
>>>>>>>>> later, than to allow use of the option at any time and leave room
>>>>>>>>> for
>>>>>>>>> the user to shoot themselves in the foot with no way to add safety
>>>>>>>>> later.  The argument against no backing file is somewhat
>>>>>>>>> understandable (technically, as long as the backing file also
>>>>>>>>> reads
>>>>>>>>> as all zeroes, then the overall image reads as all zeroes - but
>>>>>>>>> why
>>>>>>>>> have a backing file that has no content?); the argument
>>>>>>>>> requiring -n
>>>>>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as
>>>>>>>>> all
>>>>>>>>> zeroes, so the --target-is-zero argument is redundant, but it
>>>>>>>>> shouldn't hurt to allow it).
>>>>>>>>
>>>>>>>> I know that it reads as all zeroes, only if this format provides
>>>>>>>> zero
>>>>>>>> initialization..
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +++ b/qemu-img.c
>>>>>>>>>
>>>>>>>>>> @@ -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;
>>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>> So I think we could drop this hunk with no change in behavior.
>>>>>>>>
>>>>>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd
>>>>>>>> better
>>>>>>>> to check that what we are creating is zero-initialized (format has
>>>>>>>> zero-init), and if not we should report error.
>>>>>>>>
>>>>>>>
>>>>>>> Good call.  Yes, if we allow --target-is-zero without -n, we MUST
>>>>>>> insist
>>>>>>> that bdrv_has_zero_init() returns 1 (or, after my followup series,
>>>>>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE).
>>>>>>
>>>>>> Why?
>>>>>>
>>>>>> I could imagine a user creating a qcow2 image on some block device
>>>>>> with
>>>>>> preallocation where we cannot verify that the result will be
>>>>>> zero.  But
>>>>>> they want qemu not to zero the device, so they would specify
>>>>>> --target-is-zero.
>>>>>
>>>>> If user create image, setting --target-is-zero is always valid. But if
>>>>> we in
>>>>> same operation create the image automatically, having
>>>>> --target-is-zero,
>>>>> when
>>>>> we know that what we are creating is not zero is misleading and should
>>>>> fail..
>>>>
>>>> bdrv_has_zero_init() doesn’t return false only for images that we know
>>>> are not zero.  It returns true for images where we know they are.  But
>>>> if we don’t know, then it returns false also.
>>>
>>> yes, but we don't have better check.
>>
>> Correct, but maybe the user knows more, hence why it may make sense for
>> them to provide us with some information we don’t have.
>>
>>>>> If we want to add a behavior to skip zeros unconditionally, we should
>>>>> call new
>>>>> option --skip-zeroes, to clearly specify what we want.
>>>>
>>>> It was my impression that this was exactly what --target-is-zero means
>>>> and implies.
>>>>
>>>
>>> For me it sounds strange that user has better knowledge about what Qemu
>>> creates than Qemu itself. And if it so - it should be fixed in Qemu,
>>> rather than creating user interface to hint Qemu what it does.
>>
>> I brought an example where qemu cannot know whether the image is zero
>> (preallocation on a block device), but the user / management layer might
>> know.
>>
> 
> It sounds unsafe for me. User can't know how exactly Qemu do preallocation,
> which syscalls it calls, etc. How can user be sure, that Qemu produces
> all-zero image, if even Qemu doesn't sure in it?

For example, qcow2 images are always zero if the underlying storage is zero.

Then again, it isn’t like we need --target-is-zero without -n anyway.
If users want that, they can always use qemu-img create +
qemu-img convert -n --target-is-zero.  So seeing that you’re
uncomfortable with the idea of --target-is-zero with -n, I can’t say
there is any actual reason why we’d have to allow it.

Max

> Otherwise, we should document, how exactly (up to syscalls, their
> parameters, flags, the whole logic and algorithm) preallocation is done,
> so user can analyze it and be sure that produced image would be all-zero
> (when Qemu can't determine it because some specific block device, for which
> Qemu doesn't know that its preallocation algorithm produces all-zero, but
> user is sure in it)..


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Eric Blake]

On 2/7/20 8:41 AM, Max Reitz wrote:

>>> I could imagine a user creating a qcow2 image on some block device with
>>> preallocation where we cannot verify that the result will be zero.  But
>>> they want qemu not to zero the device, so they would specify
>>> --target-is-zero.
>>
>> If user create image, setting --target-is-zero is always valid. But if
>> we in
>> same operation create the image automatically, having --target-is-zero,
>> when
>> we know that what we are creating is not zero is misleading and should
>> fail..
> 
> bdrv_has_zero_init() doesn’t return false only for images that we know
> are not zero.  It returns true for images where we know they are.  But
> if we don’t know, then it returns false also.

Huh?

bdrv_has_zero_init() currently returns 1 if a driver knows that creating 
an image results in that image reading as 0.  That means it can return 1 
even for non-zero images that were not just created.  Thus, that 
interface has both false positives (returning 1 for a non-zero image if 
the driver hard-codes it to 1) and false negatives (returning 0 for an 
image that happens to read as zero).  The false negatives are less 
important (if we don't know if the image is already zero, then zeroing 
it again is a waste of time but not semantically wrong) than the false 
positives (but as long as you don't rely on bdrv_has_zero_init() unless 
you also know the image was just created, you are safely avoiding the 
false positives).

And that's the whole point of my series to add a qcow2 persistent bit to 
track whether an image has known-zero contents: qemu-img should not be 
calling bdrv_has_zero_init(), since it is so finicky on what it means.

> 
>> If we want to add a behavior to skip zeros unconditionally, we should
>> call new
>> option --skip-zeroes, to clearly specify what we want.
> 
> It was my impression that this was exactly what --target-is-zero means
> and implies.

--target-is-zero turns into the behavior of 'skip a pre-zeroing pass'. 
If the destination is already zero, then copying zeroes from the source 
is a waste of time. If the destination is not already zero, then zeroes 
from the source are not copied, and the destination will not be 
identical to the source.  We then have a choice of whether 
--target-is-zero is merely a way to tell qemu something that it couldn't 
learn otherwise but still be as safe as possible (if we can quickly 
prove the target has non-zero data, the user lied about it being already 
zero, so fail the command, so add yet another option to bypass the 
safety check), or whether it really is synonymous with 'only copy the 
non-zero portions of the source, and if the destination was not all zero 
the copy is not faithful but I meant for it to be that way'.

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

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3964 bytes --]

On 07.02.20 15:57, Eric Blake wrote:
> On 2/7/20 8:41 AM, Max Reitz wrote:
> 
>>>> I could imagine a user creating a qcow2 image on some block device with
>>>> preallocation where we cannot verify that the result will be zero.  But
>>>> they want qemu not to zero the device, so they would specify
>>>> --target-is-zero.
>>>
>>> If user create image, setting --target-is-zero is always valid. But if
>>> we in
>>> same operation create the image automatically, having --target-is-zero,
>>> when
>>> we know that what we are creating is not zero is misleading and should
>>> fail..
>>
>> bdrv_has_zero_init() doesn’t return false only for images that we know
>> are not zero.  It returns true for images where we know they are.  But
>> if we don’t know, then it returns false also.
> 
> Huh?
> 
> bdrv_has_zero_init() currently returns 1 if a driver knows that creating
> an image results in that image reading as 0.  That means it can return 1
> even for non-zero images that were not just created.  Thus, that
> interface has both false positives (returning 1 for a non-zero image if
> the driver hard-codes it to 1) and false negatives (returning 0 for an
> image that happens to read as zero).  The false negatives are less
> important (if we don't know if the image is already zero, then zeroing
> it again is a waste of time but not semantically wrong) than the false
> positives (but as long as you don't rely on bdrv_has_zero_init() unless
> you also know the image was just created, you are safely avoiding the
> false positives).
> 
> And that's the whole point of my series to add a qcow2 persistent bit to
> track whether an image has known-zero contents: qemu-img should not be
> calling bdrv_has_zero_init(), since it is so finicky on what it means.

Sorry, I was unclear.  I meant “that we know are not zero immediately
after creation”.

My point that it may return false even for (newly created) images that
are zero stands.  One could also say it returns only “yes” (is zero) or
“maybe”, and not “yes” or “no”.

>>> If we want to add a behavior to skip zeros unconditionally, we should
>>> call new
>>> option --skip-zeroes, to clearly specify what we want.
>>
>> It was my impression that this was exactly what --target-is-zero means
>> and implies.
> 
> --target-is-zero turns into the behavior of 'skip a pre-zeroing pass'.
> If the destination is already zero, then copying zeroes from the source
> is a waste of time. If the destination is not already zero, then zeroes
> from the source are not copied, and the destination will not be
> identical to the source.  We then have a choice of whether
> --target-is-zero is merely a way to tell qemu something that it couldn't
> learn otherwise but still be as safe as possible (if we can quickly
> prove the target has non-zero data, the user lied about it being already
> zero, so fail the command, so add yet another option to bypass the
> safety check), or whether it really is synonymous with 'only copy the
> non-zero portions of the source, and if the destination was not all zero
> the copy is not faithful but I meant for it to be that way'.

If you claim that it isn’t supposed to be an unsafe override, and if we
plan to take your series in some form or another, it follows that we
will have to drop this patch here.

Because after your series, qemu can have some insight into existing
images (either in the driver’s implementation of make_zero, or in
qemu-img itself by virtue of some is_zero function).  Therefore, we
could do the same “safety check” and see whether our insight agrees on
what the user told us.

This would make the flag completely superfluous, though, because when
qemu knows the image to be zero, it does the right thing anyway.

Therefore, I see this flag to be for cases where qemu doesn’t know.  And
that makes it an unsafe override.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Vladimir Sementsov-Ogievskiy]

07.02.2020 18:12, Max Reitz wrote:
> On 07.02.20 15:57, Eric Blake wrote:
>> On 2/7/20 8:41 AM, Max Reitz wrote:
>>
>>>>> I could imagine a user creating a qcow2 image on some block device with
>>>>> preallocation where we cannot verify that the result will be zero.  But
>>>>> they want qemu not to zero the device, so they would specify
>>>>> --target-is-zero.
>>>>
>>>> If user create image, setting --target-is-zero is always valid. But if
>>>> we in
>>>> same operation create the image automatically, having --target-is-zero,
>>>> when
>>>> we know that what we are creating is not zero is misleading and should
>>>> fail..
>>>
>>> bdrv_has_zero_init() doesn’t return false only for images that we know
>>> are not zero.  It returns true for images where we know they are.  But
>>> if we don’t know, then it returns false also.
>>
>> Huh?
>>
>> bdrv_has_zero_init() currently returns 1 if a driver knows that creating
>> an image results in that image reading as 0.  That means it can return 1
>> even for non-zero images that were not just created.  Thus, that
>> interface has both false positives (returning 1 for a non-zero image if
>> the driver hard-codes it to 1) and false negatives (returning 0 for an
>> image that happens to read as zero).  The false negatives are less
>> important (if we don't know if the image is already zero, then zeroing
>> it again is a waste of time but not semantically wrong) than the false
>> positives (but as long as you don't rely on bdrv_has_zero_init() unless
>> you also know the image was just created, you are safely avoiding the
>> false positives).
>>
>> And that's the whole point of my series to add a qcow2 persistent bit to
>> track whether an image has known-zero contents: qemu-img should not be
>> calling bdrv_has_zero_init(), since it is so finicky on what it means.
> 
> Sorry, I was unclear.  I meant “that we know are not zero immediately
> after creation”.
> 
> My point that it may return false even for (newly created) images that
> are zero stands.  One could also say it returns only “yes” (is zero) or
> “maybe”, and not “yes” or “no”.
> 
>>>> If we want to add a behavior to skip zeros unconditionally, we should
>>>> call new
>>>> option --skip-zeroes, to clearly specify what we want.
>>>
>>> It was my impression that this was exactly what --target-is-zero means
>>> and implies.
>>
>> --target-is-zero turns into the behavior of 'skip a pre-zeroing pass'.
>> If the destination is already zero, then copying zeroes from the source
>> is a waste of time. If the destination is not already zero, then zeroes
>> from the source are not copied, and the destination will not be
>> identical to the source.  We then have a choice of whether
>> --target-is-zero is merely a way to tell qemu something that it couldn't
>> learn otherwise but still be as safe as possible (if we can quickly
>> prove the target has non-zero data, the user lied about it being already
>> zero, so fail the command, so add yet another option to bypass the
>> safety check), or whether it really is synonymous with 'only copy the
>> non-zero portions of the source, and if the destination was not all zero
>> the copy is not faithful but I meant for it to be that way'.
> 
> If you claim that it isn’t supposed to be an unsafe override, and if we
> plan to take your series in some form or another, it follows that we
> will have to drop this patch here.
> 
> Because after your series, qemu can have some insight into existing
> images (either in the driver’s implementation of make_zero, or in
> qemu-img itself by virtue of some is_zero function).  Therefore, we
> could do the same “safety check” and see whether our insight agrees on
> what the user told us.
> 
> This would make the flag completely superfluous, though, because when
> qemu knows the image to be zero, it does the right thing anyway.
> 
> Therefore, I see this flag to be for cases where qemu doesn’t know.  And
> that makes it an unsafe override.
> 


IMHO, the flag just sounds wrong for images created by Qemu, and sounds OK
for images provided by user (still, it can never be safe, as user may mistake).

Still, as I understand, in most of real cases, Qemu actually can determine
is-zero by itself, we just didn't have this coded (Eric's series does). And
may be we really don't need this arguable flag..

So, really, may be rename it to --skip-zeroes, to make obvious that it is unsafe
and user is responsible for the result? Or even to --x-skip-zeroes, and drop it
when all needed cases are covered by auto detection?

-- 
Best regards,
Vladimir

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[David Edmondson]

On Tuesday, 2020-02-04 at 07:31:52 -06, Eric Blake wrote:

> On 2/4/20 3:52 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
>
> 'filled with zeroes' is accurate for a preallocated image, but reads 
> awkwardly for a sparse image; it might be better to state 'reads as zero'.

Okay.

>> 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>
>> ---
>>   docs/interop/qemu-img.rst |  8 +++++++-
>>   qemu-img-cmds.hx          |  4 ++--
>>   qemu-img.c                | 25 ++++++++++++++++++++++---
>>   3 files changed, 31 insertions(+), 6 deletions(-)
>> 
>> diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst
>> index fa27e5c7b453..99bdbe4740ee 100644
>> --- a/docs/interop/qemu-img.rst
>> +++ b/docs/interop/qemu-img.rst
>> @@ -214,6 +214,12 @@ Parameters to convert subcommand:
>>     will still be printed.  Areas that cannot be read from the source will be
>>     treated as containing only zeroes.
>>   
>> +.. option:: --target-is-zero
>> +
>> +  Assume that the destination is filled with zeros. This parameter is
>
> Spelled 'zeroes' just three lines earlier.

My understanding is that "zeros" is the correct plural of "zero" (and
that "zeroes" relates to the use of "zero" as a verb), but perhaps
that's another British English thing?

I don't care enough to fight about it.

>> +  mutually exclusive with the use of a backing file. It is required to
>> +  also use the ``-n`` parameter to skip image creation.
>
> I understand that it is safer to have restrictions now and lift them 
> later, than to allow use of the option at any time and leave room for 
> the user to shoot themselves in the foot with no way to add safety 
> later.  The argument against no backing file is somewhat understandable 
> (technically, as long as the backing file also reads as all zeroes, then 
> the overall image reads as all zeroes - but why have a backing file that 
> has no content?); the argument requiring -n is a bit weaker (if I'm 
> creating an image, I _know_ it reads as all zeroes, so the 
> --target-is-zero argument is redundant, but it shouldn't hurt to allow it).

Given your patchset that improves the general behaviour of zero
detection, I'm definitely inclined to leave the backing file case alone
in this patch.

Convincing myself that a newly created image can only ever read as zero
will take a little more time, which I'm happy to spend if it's
considered significant.

>> +++ b/qemu-img.c
>
>> @@ -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;
>> +    }
>
> So I think we could drop this hunk with no change in behavior.
>
>> +
>>       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;
>> +    }
>> +
>
> while keeping this one makes sense.  At any rate, typo fixes are minor, 
> and whether or not we drop the hunk I claim is a needless restriction 
> against redundancy,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks.

dme.
-- 
Everyone I know, goes away in the end.

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Eric Blake]

On 2/4/20 8:21 AM, David Edmondson wrote:
> On Tuesday, 2020-02-04 at 07:31:52 -06, Eric Blake wrote:
> 
>> On 2/4/20 3:52 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
>>
>> 'filled with zeroes' is accurate for a preallocated image, but reads
>> awkwardly for a sparse image; it might be better to state 'reads as zero'.
> 

>>> +++ b/docs/interop/qemu-img.rst
>>> @@ -214,6 +214,12 @@ Parameters to convert subcommand:
>>>      will still be printed.  Areas that cannot be read from the source will be
>>>      treated as containing only zeroes.
>>>    
>>> +.. option:: --target-is-zero
>>> +
>>> +  Assume that the destination is filled with zeros. This parameter is
>>
>> Spelled 'zeroes' just three lines earlier.
> 
> My understanding is that "zeros" is the correct plural of "zero" (and
> that "zeroes" relates to the use of "zero" as a verb), but perhaps
> that's another British English thing?
> 
> I don't care enough to fight about it.
> 

https://english.stackexchange.com/questions/3824/what-is-the-plural-form-of-zero 
says both zeros and zeroes are fine for the noun (with UK leaning more 
to zeros), but only zeroes for the verb; but also concedes that zeroes 
is gaining in popularity over time.  I likewise won't bother tweaking 
your patch (I see in v4 that you left it unchanged).

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

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Vladimir Sementsov-Ogievskiy]

04.02.2020 12:52, 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>
> ---
>   docs/interop/qemu-img.rst |  8 +++++++-
>   qemu-img-cmds.hx          |  4 ++--
>   qemu-img.c                | 25 ++++++++++++++++++++++---
>   3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst
> index fa27e5c7b453..99bdbe4740ee 100644
> --- a/docs/interop/qemu-img.rst
> +++ b/docs/interop/qemu-img.rst
> @@ -214,6 +214,12 @@ Parameters to convert subcommand:
>     will still be printed.  Areas that cannot be read from the source will be
>     treated as containing only zeroes.
>   
> +.. option:: --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

Should we mention, that s/backing file/backing file for destination/, to make it clean
that source backing file is unrelated?

> +  also use the ``-n`` parameter to skip image creation.
> +
>   Parameters to dd subcommand:
>   
>   .. program:: qemu-img-dd
> @@ -366,7 +372,7 @@ Command description:
>     4
>       Error on reading data
>   
> -.. option:: 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] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
> +.. option:: 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] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
>   
>     Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
>     to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 3fd836ca9090..e6f98b75473f 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -39,9 +39,9 @@ SRST
>   ERST
>   
>   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")
>   SRST
> -.. option:: 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
> +.. option:: 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
>   ERST
>   
>   DEF("create", img_create,

Side question: is there plan to get rid of this duplication, and generate everything from rst?

> diff --git a/qemu-img.c b/qemu-img.c
> index 2b4562b9d9f2..e0bfc33ef4f6 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");

And here, probably better: "with a backing file on destination"

> +        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");
> 

With or without my suggestions:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[David Edmondson]

On Tuesday, 2020-02-04 at 16:59:39 +03, Vladimir Sementsov-Ogievskiy wrote:

> 04.02.2020 12:52, 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>
>> ---
>>   docs/interop/qemu-img.rst |  8 +++++++-
>>   qemu-img-cmds.hx          |  4 ++--
>>   qemu-img.c                | 25 ++++++++++++++++++++++---
>>   3 files changed, 31 insertions(+), 6 deletions(-)
>> 
>> diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst
>> index fa27e5c7b453..99bdbe4740ee 100644
>> --- a/docs/interop/qemu-img.rst
>> +++ b/docs/interop/qemu-img.rst
>> @@ -214,6 +214,12 @@ Parameters to convert subcommand:
>>     will still be printed.  Areas that cannot be read from the source will be
>>     treated as containing only zeroes.
>>   
>> +.. option:: --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
>
> Should we mention, that s/backing file/backing file for destination/, to make it clean
> that source backing file is unrelated?

Yes, that makes sense, similarly in the error message.

>> +  also use the ``-n`` parameter to skip image creation.
>> +
>>   Parameters to dd subcommand:
>>   
>>   .. program:: qemu-img-dd
>> @@ -366,7 +372,7 @@ Command description:
>>     4
>>       Error on reading data
>>   
>> -.. option:: 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] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
>> +.. option:: 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] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
>>   
>>     Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
>>     to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
>> index 3fd836ca9090..e6f98b75473f 100644
>> --- a/qemu-img-cmds.hx
>> +++ b/qemu-img-cmds.hx
>> @@ -39,9 +39,9 @@ SRST
>>   ERST
>>   
>>   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")
>>   SRST
>> -.. option:: 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
>> +.. option:: 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
>>   ERST
>>   
>>   DEF("create", img_create,
>
> Side question: is there plan to get rid of this duplication, and generate everything from rst?
>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 2b4562b9d9f2..e0bfc33ef4f6 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");
>
> And here, probably better: "with a backing file on destination"
>
>> +        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");
>> 
>
> With or without my suggestions:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>
> -- 
> Best regards,
> Vladimir

dme.
-- 
You bring light in.