Message ID | 20200220160710.533297-7-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
Series | [PULL,01/18] docs: improve qcow2 spec about extending image header | expand |
On 2/20/20 10:06 AM, Max Reitz wrote: > From: David Edmondson <david.edmondson@oracle.com> > > In many cases the target of a convert operation is a newly provisioned > target that the user knows is blank (reads as zero). 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 will return zeros for all reads. > > Signed-off-by: David Edmondson <david.edmondson@oracle.com> > Message-Id: <20200205110248.2009589-2-david.edmondson@oracle.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > docs/interop/qemu-img.rst | 9 ++++++++- > qemu-img-cmds.hx | 4 ++-- > qemu-img.c | 26 +++++++++++++++++++++++--- > 3 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst > index 42e4451db4..5f40137c10 100644 > --- a/docs/interop/qemu-img.rst > +++ b/docs/interop/qemu-img.rst > @@ -214,6 +214,13 @@ 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 reading the destination image will always return > + zeros. This parameter is mutually exclusive with a destination image Late tweak now that this is in a pull request, so we may want a followup patch, but: The image doesn't always return zeros after we write to it, maybe we should tweak this sentence: Assume that reading the destination image will initially return all zeros. Also, my earlier comment about 'zeroes' one line before 'zeros' still applies - although both spellings are valid, we look inconsistent when we can't make up our mind within two adjacent paragraphs.
On Thursday, 2020-02-20 at 10:36:04 -06, Eric Blake wrote: > On 2/20/20 10:06 AM, Max Reitz wrote: >> From: David Edmondson <david.edmondson@oracle.com> >> >> In many cases the target of a convert operation is a newly provisioned >> target that the user knows is blank (reads as zero). 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 will return zeros for all reads. >> >> Signed-off-by: David Edmondson <david.edmondson@oracle.com> >> Message-Id: <20200205110248.2009589-2-david.edmondson@oracle.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> docs/interop/qemu-img.rst | 9 ++++++++- >> qemu-img-cmds.hx | 4 ++-- >> qemu-img.c | 26 +++++++++++++++++++++++--- >> 3 files changed, 33 insertions(+), 6 deletions(-) >> >> diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst >> index 42e4451db4..5f40137c10 100644 >> --- a/docs/interop/qemu-img.rst >> +++ b/docs/interop/qemu-img.rst >> @@ -214,6 +214,13 @@ 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 reading the destination image will always return >> + zeros. This parameter is mutually exclusive with a destination image > > Late tweak now that this is in a pull request, so we may want a followup > patch, but: > > The image doesn't always return zeros after we write to it, maybe we > should tweak this sentence: > > Assume that reading the destination image will initially return all zeros. I will send a patch for this. > Also, my earlier comment about 'zeroes' one line before 'zeros' still > applies - although both spellings are valid, we look inconsistent when > we can't make up our mind within two adjacent paragraphs. If we can agree on one of "zeros" or "zeroes" then I'm happy to send a patch making it consistent everywhere. I think that given there are existing functions with "zeroes" in the name, I'd be inclined to go that way. dme.
diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst index 42e4451db4..5f40137c10 100644 --- a/docs/interop/qemu-img.rst +++ b/docs/interop/qemu-img.rst @@ -214,6 +214,13 @@ 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 reading the destination image will always return + zeros. This parameter is mutually exclusive with a destination image + that has 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 +373,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 d7fbc6b1f4..c9c54de1df 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 2b4562b9d9..0faf2cd2f5 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,12 @@ 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 when the destination " + "image has 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");