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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> ---
>   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