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

[Max Reitz]

Hi,

On 17.01.20 11: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.
> ---
>  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;

As you already said, we probably don’t need this and it’d be sufficient
to set the has_zero_init value directly.

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

We cannot has_zero_init to true if the target has a backing file,
because convert_co_write() asserts that the target must not have a
backing file if has_zero_init is true.  (It’s impossible for a file to
be initialized to zeroes if it has a backing file; or at least it
doesn’t make sense then to have a backing file.)

Case in point:

$ ./qemu-img create -f qcow2 src.qcow2 64M
Formatting 'src.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 dst.qcow2 64M

Formatting 'dst.qcow2', fmt=qcow2 size=67108864
backing_file=backing.qcow2 cluster_size=65536 lazy_refcounts=off
refcount_bits=16
$ ./qemu-img convert -n -B backing.qcow2 -f qcow2 -O qcow2
--target-is-zero src.qcow2 dst.qcow2
qemu-img: qemu-img.c:1812: convert_co_write: Assertion
`!s->target_has_backing' failed.
[1]    80813 abort (core dumped)  ./qemu-img convert -n -B backing.qcow2
-f qcow2 -O qcow2 --target-is-zero

> +
> +    if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
> +        !s->target_has_backing) {

(This will be irrelevant after target_has_backing is gone, but because
has_zero_init and target_has_backing are equivalent here, there is no
need to check both.)

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

Hm, I could imagine it being useful even without -n, but maybe it’s
safer to forbid this case for now and reconsider if someone were to ask.

> +        goto fail_getopt;
> +    }
> +
>      s.src_num = argc - optind - 1;
>      out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;

This patch should also add some documentation for the new option (in
qemu-img-cmds.hx and in qemu-img.texi for the man page).

Max

signature.asc
Description: OpenPGP digital signature