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 <address@hidden>
>> ---
>>   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 <address@hidden>
>
>
> -- 
> Best regards,
> Vladimir

dme.
-- 
You bring light in.