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

[John Snow <jsnow@redhat.com>]

CC qemu-block and block maintainers

On 1/17/20 5: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.
> 

Is there no way to convince bdrv_has_zero_init to return what we want
already in this case? I cannot recall off hand, but wonder if there's an
advanced syntax method of specifying the target image that can set this
flag already.

> Add a new option, --target-is-zero, allowing the user to indicate that
> an existing target device is already zero filled.
> ---
>  qemu-img.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 95a24b9762..56ca727e8c 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 {
> @@ -1593,6 +1594,7 @@ typedef struct ImgConvertState {
>      bool copy_range;
>      bool salvage;
>      bool quiet;
> +    bool target_is_zero;
>      int min_sparse;
>      int alignment;
>      size_t cluster_sectors;
> @@ -1984,10 +1986,11 @@ 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) {
> +    s->has_zero_init = s->target_is_zero;
> +
> +    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 &&
> @@ -2076,6 +2079,7 @@ static int img_convert(int argc, char **argv)
>          .buf_sectors        = IO_BUF_SIZE / BDRV_SECTOR_SIZE,
>          .wr_in_order        = true,
>          .num_coroutines     = 8,
> +        .target_is_zero     = false,
>      };
>  
>      for(;;) {
> @@ -2086,6 +2090,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 +2214,9 @@ static int img_convert(int argc, char **argv)
>          case OPTION_TARGET_IMAGE_OPTS:
>              tgt_image_opts = true;
>              break;
> +        case OPTION_TARGET_IS_ZERO:
> +            s.target_is_zero = true;
> +            break;
>          }
>      }
>  
> @@ -2247,6 +2255,11 @@ static int img_convert(int argc, char **argv)
>          warn_report("This will become an error in future QEMU versions.");
>      }
>  
> +    if (s.target_is_zero && !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;
>  
>