| Message ID | 20200204095246.1974117-2-david.edmondson@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu-img: Add --target-is-zero to indicate that a target is blank | expand |
On 2/4/20 3:52 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 'filled with zeroes' is accurate for a preallocated image, but reads awkwardly for a sparse image; it might be better to state 'reads as zero'. > 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 <david.edmondson@oracle.com> > --- > 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 Spelled 'zeroes' just three lines earlier. > + mutually exclusive with the use of a backing file. It is required to > + also use the ``-n`` parameter to skip image creation. I understand that it is safer to have restrictions now and lift them later, than to allow use of the option at any time and leave room for the user to shoot themselves in the foot with no way to add safety later. The argument against no backing file is somewhat understandable (technically, as long as the backing file also reads as all zeroes, then the overall image reads as all zeroes - but why have a backing file that has no content?); the argument requiring -n is a bit weaker (if I'm creating an image, I _know_ it reads as all zeroes, so the --target-is-zero argument is redundant, but it shouldn't hurt to allow it). > +++ b/qemu-img.c > @@ -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; > + } So I think we could drop this hunk with no change in behavior. > + > 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; > + } > + while keeping this one makes sense. At any rate, typo fixes are minor, and whether or not we drop the hunk I claim is a needless restriction against redundancy, Reviewed-by: Eric Blake <eblake@redhat.com>
04.02.2020 16:31, Eric Blake wrote: > On 2/4/20 3:52 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 > > 'filled with zeroes' is accurate for a preallocated image, but reads awkwardly for a sparse image; it might be better to state 'reads as zero'. > >> 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 <david.edmondson@oracle.com> >> --- >> 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 > > Spelled 'zeroes' just three lines earlier. > >> + mutually exclusive with the use of a backing file. It is required to >> + also use the ``-n`` parameter to skip image creation. > > I understand that it is safer to have restrictions now and lift them later, than to allow use of the option at any time and leave room for the user to shoot themselves in the foot with no way to add safety later. The argument against no backing file is somewhat understandable (technically, as long as the backing file also reads as all zeroes, then the overall image reads as all zeroes - but why have a backing file that has no content?); the argument requiring -n is a bit weaker (if I'm creating an image, I _know_ it reads as all zeroes, so the --target-is-zero argument is redundant, but it shouldn't hurt to allow it). I know that it reads as all zeroes, only if this format provides zero initialization.. > >> +++ b/qemu-img.c > >> @@ -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; >> + } > > So I think we could drop this hunk with no change in behavior. I think, no we can't. If we allow target-is-zero, with -n, we'd better to check that what we are creating is zero-initialized (format has zero-init), and if not we should report error. > >> + >> 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; >> + } >> + > > while keeping this one makes sense. At any rate, typo fixes are minor, and whether or not we drop the hunk I claim is a needless restriction against redundancy, > > Reviewed-by: Eric Blake <eblake@redhat.com> >
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 <david.edmondson@oracle.com> > --- > 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? > + 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 <vsementsov@virtuozzo.com>
On Tuesday, 2020-02-04 at 07:31:52 -06, Eric Blake wrote: > On 2/4/20 3:52 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 > > 'filled with zeroes' is accurate for a preallocated image, but reads > awkwardly for a sparse image; it might be better to state 'reads as zero'. Okay. >> 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 <david.edmondson@oracle.com> >> --- >> 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 > > Spelled 'zeroes' just three lines earlier. My understanding is that "zeros" is the correct plural of "zero" (and that "zeroes" relates to the use of "zero" as a verb), but perhaps that's another British English thing? I don't care enough to fight about it. >> + mutually exclusive with the use of a backing file. It is required to >> + also use the ``-n`` parameter to skip image creation. > > I understand that it is safer to have restrictions now and lift them > later, than to allow use of the option at any time and leave room for > the user to shoot themselves in the foot with no way to add safety > later. The argument against no backing file is somewhat understandable > (technically, as long as the backing file also reads as all zeroes, then > the overall image reads as all zeroes - but why have a backing file that > has no content?); the argument requiring -n is a bit weaker (if I'm > creating an image, I _know_ it reads as all zeroes, so the > --target-is-zero argument is redundant, but it shouldn't hurt to allow it). Given your patchset that improves the general behaviour of zero detection, I'm definitely inclined to leave the backing file case alone in this patch. Convincing myself that a newly created image can only ever read as zero will take a little more time, which I'm happy to spend if it's considered significant. >> +++ b/qemu-img.c > >> @@ -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; >> + } > > So I think we could drop this hunk with no change in behavior. > >> + >> 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; >> + } >> + > > while keeping this one makes sense. At any rate, typo fixes are minor, > and whether or not we drop the hunk I claim is a needless restriction > against redundancy, > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks. dme.
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 <david.edmondson@oracle.com> >> --- >> 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 <vsementsov@virtuozzo.com> > > > -- > Best regards, > Vladimir dme.
On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: >> I understand that it is safer to have restrictions now and lift them >> later, than to allow use of the option at any time and leave room for >> the user to shoot themselves in the foot with no way to add safety >> later. The argument against no backing file is somewhat >> understandable (technically, as long as the backing file also reads as >> all zeroes, then the overall image reads as all zeroes - but why have >> a backing file that has no content?); the argument requiring -n is a >> bit weaker (if I'm creating an image, I _know_ it reads as all zeroes, >> so the --target-is-zero argument is redundant, but it shouldn't hurt >> to allow it). > > I know that it reads as all zeroes, only if this format provides zero > initialization.. > >> >>> +++ b/qemu-img.c >> >>> @@ -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; >>> + } >> >> So I think we could drop this hunk with no change in behavior. > > I think, no we can't. If we allow target-is-zero, with -n, we'd better > to check that what we are creating is zero-initialized (format has > zero-init), and if not we should report error. > Good call. Yes, if we allow --target-is-zero without -n, we MUST insist that bdrv_has_zero_init() returns 1 (or, after my followup series, bdrv_known_zeroes() includes BDRV_ZERO_CREATE). I can work that into v2 of my series, if desired. >> >>> + >>> 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; >>> + } >>> + >> >> while keeping this one makes sense. At any rate, typo fixes are >> minor, and whether or not we drop the hunk I claim is a needless >> restriction against redundancy, >> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> > >
On 2/4/20 8:21 AM, David Edmondson wrote: > On Tuesday, 2020-02-04 at 07:31:52 -06, Eric Blake wrote: > >> On 2/4/20 3:52 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 >> >> 'filled with zeroes' is accurate for a preallocated image, but reads >> awkwardly for a sparse image; it might be better to state 'reads as zero'. > >>> +++ 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 >> >> Spelled 'zeroes' just three lines earlier. > > My understanding is that "zeros" is the correct plural of "zero" (and > that "zeroes" relates to the use of "zero" as a verb), but perhaps > that's another British English thing? > > I don't care enough to fight about it. > https://english.stackexchange.com/questions/3824/what-is-the-plural-form-of-zero says both zeros and zeroes are fine for the noun (with UK leaning more to zeros), but only zeroes for the verb; but also concedes that zeroes is gaining in popularity over time. I likewise won't bother tweaking your patch (I see in v4 that you left it unchanged).
On 04.02.20 15:23, Eric Blake wrote: > On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: > >>> I understand that it is safer to have restrictions now and lift them >>> later, than to allow use of the option at any time and leave room for >>> the user to shoot themselves in the foot with no way to add safety >>> later. The argument against no backing file is somewhat >>> understandable (technically, as long as the backing file also reads >>> as all zeroes, then the overall image reads as all zeroes - but why >>> have a backing file that has no content?); the argument requiring -n >>> is a bit weaker (if I'm creating an image, I _know_ it reads as all >>> zeroes, so the --target-is-zero argument is redundant, but it >>> shouldn't hurt to allow it). >> >> I know that it reads as all zeroes, only if this format provides zero >> initialization.. >> >>> >>>> +++ b/qemu-img.c >>> >>>> @@ -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; >>>> + } >>> >>> So I think we could drop this hunk with no change in behavior. >> >> I think, no we can't. If we allow target-is-zero, with -n, we'd better >> to check that what we are creating is zero-initialized (format has >> zero-init), and if not we should report error. >> > > Good call. Yes, if we allow --target-is-zero without -n, we MUST insist > that bdrv_has_zero_init() returns 1 (or, after my followup series, > bdrv_known_zeroes() includes BDRV_ZERO_CREATE). Why? I could imagine a user creating a qcow2 image on some block device with preallocation where we cannot verify that the result will be zero. But they want qemu not to zero the device, so they would specify --target-is-zero. OTOH, we can always choose to allow that behavior at a later point. Max
07.02.2020 13:33, Max Reitz wrote: > On 04.02.20 15:23, Eric Blake wrote: >> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: >> >>>> I understand that it is safer to have restrictions now and lift them >>>> later, than to allow use of the option at any time and leave room for >>>> the user to shoot themselves in the foot with no way to add safety >>>> later. The argument against no backing file is somewhat >>>> understandable (technically, as long as the backing file also reads >>>> as all zeroes, then the overall image reads as all zeroes - but why >>>> have a backing file that has no content?); the argument requiring -n >>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all >>>> zeroes, so the --target-is-zero argument is redundant, but it >>>> shouldn't hurt to allow it). >>> >>> I know that it reads as all zeroes, only if this format provides zero >>> initialization.. >>> >>>> >>>>> +++ b/qemu-img.c >>>> >>>>> @@ -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; >>>>> + } >>>> >>>> So I think we could drop this hunk with no change in behavior. >>> >>> I think, no we can't. If we allow target-is-zero, with -n, we'd better >>> to check that what we are creating is zero-initialized (format has >>> zero-init), and if not we should report error. >>> >> >> Good call. Yes, if we allow --target-is-zero without -n, we MUST insist >> that bdrv_has_zero_init() returns 1 (or, after my followup series, >> bdrv_known_zeroes() includes BDRV_ZERO_CREATE). > > Why? > > I could imagine a user creating a qcow2 image on some block device with > preallocation where we cannot verify that the result will be zero. But > they want qemu not to zero the device, so they would specify > --target-is-zero. If user create image, setting --target-is-zero is always valid. But if we in same operation create the image automatically, having --target-is-zero, when we know that what we are creating is not zero is misleading and should fail.. If we want to add a behavior to skip zeros unconditionally, we should call new option --skip-zeroes, to clearly specify what we want. > > OTOH, we can always choose to allow that behavior at a later point. > > Max >
On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote: > 07.02.2020 13:33, Max Reitz wrote: >> On 04.02.20 15:23, Eric Blake wrote: >>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: >>> >>>>> I understand that it is safer to have restrictions now and lift them >>>>> later, than to allow use of the option at any time and leave room for >>>>> the user to shoot themselves in the foot with no way to add safety >>>>> later. The argument against no backing file is somewhat >>>>> understandable (technically, as long as the backing file also reads >>>>> as all zeroes, then the overall image reads as all zeroes - but why >>>>> have a backing file that has no content?); the argument requiring -n >>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all >>>>> zeroes, so the --target-is-zero argument is redundant, but it >>>>> shouldn't hurt to allow it). >>>> >>>> I know that it reads as all zeroes, only if this format provides zero >>>> initialization.. >>>> >>>>> >>>>>> +++ b/qemu-img.c >>>>> >>>>>> @@ -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; >>>>>> + } >>>>> >>>>> So I think we could drop this hunk with no change in behavior. >>>> >>>> I think, no we can't. If we allow target-is-zero, with -n, we'd better >>>> to check that what we are creating is zero-initialized (format has >>>> zero-init), and if not we should report error. >>>> >>> >>> Good call. Yes, if we allow --target-is-zero without -n, we MUST insist >>> that bdrv_has_zero_init() returns 1 (or, after my followup series, >>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE). >> >> Why? >> >> I could imagine a user creating a qcow2 image on some block device with >> preallocation where we cannot verify that the result will be zero. But >> they want qemu not to zero the device, so they would specify >> --target-is-zero. > > If user create image, setting --target-is-zero is always valid. But if > we in > same operation create the image automatically, having --target-is-zero, > when > we know that what we are creating is not zero is misleading and should > fail.. bdrv_has_zero_init() doesn’t return false only for images that we know are not zero. It returns true for images where we know they are. But if we don’t know, then it returns false also. > If we want to add a behavior to skip zeros unconditionally, we should > call new > option --skip-zeroes, to clearly specify what we want. It was my impression that this was exactly what --target-is-zero means and implies. Max
07.02.2020 17:41, Max Reitz wrote: > On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote: >> 07.02.2020 13:33, Max Reitz wrote: >>> On 04.02.20 15:23, Eric Blake wrote: >>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> >>>>>> I understand that it is safer to have restrictions now and lift them >>>>>> later, than to allow use of the option at any time and leave room for >>>>>> the user to shoot themselves in the foot with no way to add safety >>>>>> later. The argument against no backing file is somewhat >>>>>> understandable (technically, as long as the backing file also reads >>>>>> as all zeroes, then the overall image reads as all zeroes - but why >>>>>> have a backing file that has no content?); the argument requiring -n >>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all >>>>>> zeroes, so the --target-is-zero argument is redundant, but it >>>>>> shouldn't hurt to allow it). >>>>> >>>>> I know that it reads as all zeroes, only if this format provides zero >>>>> initialization.. >>>>> >>>>>> >>>>>>> +++ b/qemu-img.c >>>>>> >>>>>>> @@ -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; >>>>>>> + } >>>>>> >>>>>> So I think we could drop this hunk with no change in behavior. >>>>> >>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd better >>>>> to check that what we are creating is zero-initialized (format has >>>>> zero-init), and if not we should report error. >>>>> >>>> >>>> Good call. Yes, if we allow --target-is-zero without -n, we MUST insist >>>> that bdrv_has_zero_init() returns 1 (or, after my followup series, >>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE). >>> >>> Why? >>> >>> I could imagine a user creating a qcow2 image on some block device with >>> preallocation where we cannot verify that the result will be zero. But >>> they want qemu not to zero the device, so they would specify >>> --target-is-zero. >> >> If user create image, setting --target-is-zero is always valid. But if >> we in >> same operation create the image automatically, having --target-is-zero, >> when >> we know that what we are creating is not zero is misleading and should >> fail.. > > bdrv_has_zero_init() doesn’t return false only for images that we know > are not zero. It returns true for images where we know they are. But > if we don’t know, then it returns false also. yes, but we don't have better check. > >> If we want to add a behavior to skip zeros unconditionally, we should >> call new >> option --skip-zeroes, to clearly specify what we want. > > It was my impression that this was exactly what --target-is-zero means > and implies. > For me it sounds strange that user has better knowledge about what Qemu creates than Qemu itself. And if it so - it should be fixed in Qemu, rather than creating user interface to hint Qemu what it does.
On 2/7/20 8:41 AM, Max Reitz wrote: >>> I could imagine a user creating a qcow2 image on some block device with >>> preallocation where we cannot verify that the result will be zero. But >>> they want qemu not to zero the device, so they would specify >>> --target-is-zero. >> >> If user create image, setting --target-is-zero is always valid. But if >> we in >> same operation create the image automatically, having --target-is-zero, >> when >> we know that what we are creating is not zero is misleading and should >> fail.. > > bdrv_has_zero_init() doesn’t return false only for images that we know > are not zero. It returns true for images where we know they are. But > if we don’t know, then it returns false also. Huh? bdrv_has_zero_init() currently returns 1 if a driver knows that creating an image results in that image reading as 0. That means it can return 1 even for non-zero images that were not just created. Thus, that interface has both false positives (returning 1 for a non-zero image if the driver hard-codes it to 1) and false negatives (returning 0 for an image that happens to read as zero). The false negatives are less important (if we don't know if the image is already zero, then zeroing it again is a waste of time but not semantically wrong) than the false positives (but as long as you don't rely on bdrv_has_zero_init() unless you also know the image was just created, you are safely avoiding the false positives). And that's the whole point of my series to add a qcow2 persistent bit to track whether an image has known-zero contents: qemu-img should not be calling bdrv_has_zero_init(), since it is so finicky on what it means. > >> If we want to add a behavior to skip zeros unconditionally, we should >> call new >> option --skip-zeroes, to clearly specify what we want. > > It was my impression that this was exactly what --target-is-zero means > and implies. --target-is-zero turns into the behavior of 'skip a pre-zeroing pass'. If the destination is already zero, then copying zeroes from the source is a waste of time. If the destination is not already zero, then zeroes from the source are not copied, and the destination will not be identical to the source. We then have a choice of whether --target-is-zero is merely a way to tell qemu something that it couldn't learn otherwise but still be as safe as possible (if we can quickly prove the target has non-zero data, the user lied about it being already zero, so fail the command, so add yet another option to bypass the safety check), or whether it really is synonymous with 'only copy the non-zero portions of the source, and if the destination was not all zero the copy is not faithful but I meant for it to be that way'.
On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote: > 07.02.2020 17:41, Max Reitz wrote: >> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote: >>> 07.02.2020 13:33, Max Reitz wrote: >>>> On 04.02.20 15:23, Eric Blake wrote: >>>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>> >>>>>>> I understand that it is safer to have restrictions now and lift them >>>>>>> later, than to allow use of the option at any time and leave room >>>>>>> for >>>>>>> the user to shoot themselves in the foot with no way to add safety >>>>>>> later. The argument against no backing file is somewhat >>>>>>> understandable (technically, as long as the backing file also reads >>>>>>> as all zeroes, then the overall image reads as all zeroes - but why >>>>>>> have a backing file that has no content?); the argument requiring -n >>>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all >>>>>>> zeroes, so the --target-is-zero argument is redundant, but it >>>>>>> shouldn't hurt to allow it). >>>>>> >>>>>> I know that it reads as all zeroes, only if this format provides zero >>>>>> initialization.. >>>>>> >>>>>>> >>>>>>>> +++ b/qemu-img.c >>>>>>> >>>>>>>> @@ -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; >>>>>>>> + } >>>>>>> >>>>>>> So I think we could drop this hunk with no change in behavior. >>>>>> >>>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd >>>>>> better >>>>>> to check that what we are creating is zero-initialized (format has >>>>>> zero-init), and if not we should report error. >>>>>> >>>>> >>>>> Good call. Yes, if we allow --target-is-zero without -n, we MUST >>>>> insist >>>>> that bdrv_has_zero_init() returns 1 (or, after my followup series, >>>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE). >>>> >>>> Why? >>>> >>>> I could imagine a user creating a qcow2 image on some block device with >>>> preallocation where we cannot verify that the result will be zero. But >>>> they want qemu not to zero the device, so they would specify >>>> --target-is-zero. >>> >>> If user create image, setting --target-is-zero is always valid. But if >>> we in >>> same operation create the image automatically, having --target-is-zero, >>> when >>> we know that what we are creating is not zero is misleading and should >>> fail.. >> >> bdrv_has_zero_init() doesn’t return false only for images that we know >> are not zero. It returns true for images where we know they are. But >> if we don’t know, then it returns false also. > > yes, but we don't have better check. Correct, but maybe the user knows more, hence why it may make sense for them to provide us with some information we don’t have. >>> If we want to add a behavior to skip zeros unconditionally, we should >>> call new >>> option --skip-zeroes, to clearly specify what we want. >> >> It was my impression that this was exactly what --target-is-zero means >> and implies. >> > > For me it sounds strange that user has better knowledge about what Qemu > creates than Qemu itself. And if it so - it should be fixed in Qemu, > rather than creating user interface to hint Qemu what it does. I brought an example where qemu cannot know whether the image is zero (preallocation on a block device), but the user / management layer might know. Max
On 07.02.20 15:57, Eric Blake wrote: > On 2/7/20 8:41 AM, Max Reitz wrote: > >>>> I could imagine a user creating a qcow2 image on some block device with >>>> preallocation where we cannot verify that the result will be zero. But >>>> they want qemu not to zero the device, so they would specify >>>> --target-is-zero. >>> >>> If user create image, setting --target-is-zero is always valid. But if >>> we in >>> same operation create the image automatically, having --target-is-zero, >>> when >>> we know that what we are creating is not zero is misleading and should >>> fail.. >> >> bdrv_has_zero_init() doesn’t return false only for images that we know >> are not zero. It returns true for images where we know they are. But >> if we don’t know, then it returns false also. > > Huh? > > bdrv_has_zero_init() currently returns 1 if a driver knows that creating > an image results in that image reading as 0. That means it can return 1 > even for non-zero images that were not just created. Thus, that > interface has both false positives (returning 1 for a non-zero image if > the driver hard-codes it to 1) and false negatives (returning 0 for an > image that happens to read as zero). The false negatives are less > important (if we don't know if the image is already zero, then zeroing > it again is a waste of time but not semantically wrong) than the false > positives (but as long as you don't rely on bdrv_has_zero_init() unless > you also know the image was just created, you are safely avoiding the > false positives). > > And that's the whole point of my series to add a qcow2 persistent bit to > track whether an image has known-zero contents: qemu-img should not be > calling bdrv_has_zero_init(), since it is so finicky on what it means. Sorry, I was unclear. I meant “that we know are not zero immediately after creation”. My point that it may return false even for (newly created) images that are zero stands. One could also say it returns only “yes” (is zero) or “maybe”, and not “yes” or “no”. >>> If we want to add a behavior to skip zeros unconditionally, we should >>> call new >>> option --skip-zeroes, to clearly specify what we want. >> >> It was my impression that this was exactly what --target-is-zero means >> and implies. > > --target-is-zero turns into the behavior of 'skip a pre-zeroing pass'. > If the destination is already zero, then copying zeroes from the source > is a waste of time. If the destination is not already zero, then zeroes > from the source are not copied, and the destination will not be > identical to the source. We then have a choice of whether > --target-is-zero is merely a way to tell qemu something that it couldn't > learn otherwise but still be as safe as possible (if we can quickly > prove the target has non-zero data, the user lied about it being already > zero, so fail the command, so add yet another option to bypass the > safety check), or whether it really is synonymous with 'only copy the > non-zero portions of the source, and if the destination was not all zero > the copy is not faithful but I meant for it to be that way'. If you claim that it isn’t supposed to be an unsafe override, and if we plan to take your series in some form or another, it follows that we will have to drop this patch here. Because after your series, qemu can have some insight into existing images (either in the driver’s implementation of make_zero, or in qemu-img itself by virtue of some is_zero function). Therefore, we could do the same “safety check” and see whether our insight agrees on what the user told us. This would make the flag completely superfluous, though, because when qemu knows the image to be zero, it does the right thing anyway. Therefore, I see this flag to be for cases where qemu doesn’t know. And that makes it an unsafe override. Max
07.02.2020 18:03, Max Reitz wrote: > On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote: >> 07.02.2020 17:41, Max Reitz wrote: >>> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote: >>>> 07.02.2020 13:33, Max Reitz wrote: >>>>> On 04.02.20 15:23, Eric Blake wrote: >>>>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>>> >>>>>>>> I understand that it is safer to have restrictions now and lift them >>>>>>>> later, than to allow use of the option at any time and leave room >>>>>>>> for >>>>>>>> the user to shoot themselves in the foot with no way to add safety >>>>>>>> later. The argument against no backing file is somewhat >>>>>>>> understandable (technically, as long as the backing file also reads >>>>>>>> as all zeroes, then the overall image reads as all zeroes - but why >>>>>>>> have a backing file that has no content?); the argument requiring -n >>>>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as all >>>>>>>> zeroes, so the --target-is-zero argument is redundant, but it >>>>>>>> shouldn't hurt to allow it). >>>>>>> >>>>>>> I know that it reads as all zeroes, only if this format provides zero >>>>>>> initialization.. >>>>>>> >>>>>>>> >>>>>>>>> +++ b/qemu-img.c >>>>>>>> >>>>>>>>> @@ -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; >>>>>>>>> + } >>>>>>>> >>>>>>>> So I think we could drop this hunk with no change in behavior. >>>>>>> >>>>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd >>>>>>> better >>>>>>> to check that what we are creating is zero-initialized (format has >>>>>>> zero-init), and if not we should report error. >>>>>>> >>>>>> >>>>>> Good call. Yes, if we allow --target-is-zero without -n, we MUST >>>>>> insist >>>>>> that bdrv_has_zero_init() returns 1 (or, after my followup series, >>>>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE). >>>>> >>>>> Why? >>>>> >>>>> I could imagine a user creating a qcow2 image on some block device with >>>>> preallocation where we cannot verify that the result will be zero. But >>>>> they want qemu not to zero the device, so they would specify >>>>> --target-is-zero. >>>> >>>> If user create image, setting --target-is-zero is always valid. But if >>>> we in >>>> same operation create the image automatically, having --target-is-zero, >>>> when >>>> we know that what we are creating is not zero is misleading and should >>>> fail.. >>> >>> bdrv_has_zero_init() doesn’t return false only for images that we know >>> are not zero. It returns true for images where we know they are. But >>> if we don’t know, then it returns false also. >> >> yes, but we don't have better check. > > Correct, but maybe the user knows more, hence why it may make sense for > them to provide us with some information we don’t have. > >>>> If we want to add a behavior to skip zeros unconditionally, we should >>>> call new >>>> option --skip-zeroes, to clearly specify what we want. >>> >>> It was my impression that this was exactly what --target-is-zero means >>> and implies. >>> >> >> For me it sounds strange that user has better knowledge about what Qemu >> creates than Qemu itself. And if it so - it should be fixed in Qemu, >> rather than creating user interface to hint Qemu what it does. > > I brought an example where qemu cannot know whether the image is zero > (preallocation on a block device), but the user / management layer might > know. > It sounds unsafe for me. User can't know how exactly Qemu do preallocation, which syscalls it calls, etc. How can user be sure, that Qemu produces all-zero image, if even Qemu doesn't sure in it? Otherwise, we should document, how exactly (up to syscalls, their parameters, flags, the whole logic and algorithm) preallocation is done, so user can analyze it and be sure that produced image would be all-zero (when Qemu can't determine it because some specific block device, for which Qemu doesn't know that its preallocation algorithm produces all-zero, but user is sure in it)...
07.02.2020 18:12, Max Reitz wrote: > On 07.02.20 15:57, Eric Blake wrote: >> On 2/7/20 8:41 AM, Max Reitz wrote: >> >>>>> I could imagine a user creating a qcow2 image on some block device with >>>>> preallocation where we cannot verify that the result will be zero. But >>>>> they want qemu not to zero the device, so they would specify >>>>> --target-is-zero. >>>> >>>> If user create image, setting --target-is-zero is always valid. But if >>>> we in >>>> same operation create the image automatically, having --target-is-zero, >>>> when >>>> we know that what we are creating is not zero is misleading and should >>>> fail.. >>> >>> bdrv_has_zero_init() doesn’t return false only for images that we know >>> are not zero. It returns true for images where we know they are. But >>> if we don’t know, then it returns false also. >> >> Huh? >> >> bdrv_has_zero_init() currently returns 1 if a driver knows that creating >> an image results in that image reading as 0. That means it can return 1 >> even for non-zero images that were not just created. Thus, that >> interface has both false positives (returning 1 for a non-zero image if >> the driver hard-codes it to 1) and false negatives (returning 0 for an >> image that happens to read as zero). The false negatives are less >> important (if we don't know if the image is already zero, then zeroing >> it again is a waste of time but not semantically wrong) than the false >> positives (but as long as you don't rely on bdrv_has_zero_init() unless >> you also know the image was just created, you are safely avoiding the >> false positives). >> >> And that's the whole point of my series to add a qcow2 persistent bit to >> track whether an image has known-zero contents: qemu-img should not be >> calling bdrv_has_zero_init(), since it is so finicky on what it means. > > Sorry, I was unclear. I meant “that we know are not zero immediately > after creation”. > > My point that it may return false even for (newly created) images that > are zero stands. One could also say it returns only “yes” (is zero) or > “maybe”, and not “yes” or “no”. > >>>> If we want to add a behavior to skip zeros unconditionally, we should >>>> call new >>>> option --skip-zeroes, to clearly specify what we want. >>> >>> It was my impression that this was exactly what --target-is-zero means >>> and implies. >> >> --target-is-zero turns into the behavior of 'skip a pre-zeroing pass'. >> If the destination is already zero, then copying zeroes from the source >> is a waste of time. If the destination is not already zero, then zeroes >> from the source are not copied, and the destination will not be >> identical to the source. We then have a choice of whether >> --target-is-zero is merely a way to tell qemu something that it couldn't >> learn otherwise but still be as safe as possible (if we can quickly >> prove the target has non-zero data, the user lied about it being already >> zero, so fail the command, so add yet another option to bypass the >> safety check), or whether it really is synonymous with 'only copy the >> non-zero portions of the source, and if the destination was not all zero >> the copy is not faithful but I meant for it to be that way'. > > If you claim that it isn’t supposed to be an unsafe override, and if we > plan to take your series in some form or another, it follows that we > will have to drop this patch here. > > Because after your series, qemu can have some insight into existing > images (either in the driver’s implementation of make_zero, or in > qemu-img itself by virtue of some is_zero function). Therefore, we > could do the same “safety check” and see whether our insight agrees on > what the user told us. > > This would make the flag completely superfluous, though, because when > qemu knows the image to be zero, it does the right thing anyway. > > Therefore, I see this flag to be for cases where qemu doesn’t know. And > that makes it an unsafe override. > IMHO, the flag just sounds wrong for images created by Qemu, and sounds OK for images provided by user (still, it can never be safe, as user may mistake). Still, as I understand, in most of real cases, Qemu actually can determine is-zero by itself, we just didn't have this coded (Eric's series does). And may be we really don't need this arguable flag.. So, really, may be rename it to --skip-zeroes, to make obvious that it is unsafe and user is responsible for the result? Or even to --x-skip-zeroes, and drop it when all needed cases are covered by auto detection?
On 07.02.20 16:16, Vladimir Sementsov-Ogievskiy wrote: > 07.02.2020 18:03, Max Reitz wrote: >> On 07.02.20 15:53, Vladimir Sementsov-Ogievskiy wrote: >>> 07.02.2020 17:41, Max Reitz wrote: >>>> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote: >>>>> 07.02.2020 13:33, Max Reitz wrote: >>>>>> On 04.02.20 15:23, Eric Blake wrote: >>>>>>> On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> >>>>>>>>> I understand that it is safer to have restrictions now and lift >>>>>>>>> them >>>>>>>>> later, than to allow use of the option at any time and leave room >>>>>>>>> for >>>>>>>>> the user to shoot themselves in the foot with no way to add safety >>>>>>>>> later. The argument against no backing file is somewhat >>>>>>>>> understandable (technically, as long as the backing file also >>>>>>>>> reads >>>>>>>>> as all zeroes, then the overall image reads as all zeroes - but >>>>>>>>> why >>>>>>>>> have a backing file that has no content?); the argument >>>>>>>>> requiring -n >>>>>>>>> is a bit weaker (if I'm creating an image, I _know_ it reads as >>>>>>>>> all >>>>>>>>> zeroes, so the --target-is-zero argument is redundant, but it >>>>>>>>> shouldn't hurt to allow it). >>>>>>>> >>>>>>>> I know that it reads as all zeroes, only if this format provides >>>>>>>> zero >>>>>>>> initialization.. >>>>>>>> >>>>>>>>> >>>>>>>>>> +++ b/qemu-img.c >>>>>>>>> >>>>>>>>>> @@ -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; >>>>>>>>>> + } >>>>>>>>> >>>>>>>>> So I think we could drop this hunk with no change in behavior. >>>>>>>> >>>>>>>> I think, no we can't. If we allow target-is-zero, with -n, we'd >>>>>>>> better >>>>>>>> to check that what we are creating is zero-initialized (format has >>>>>>>> zero-init), and if not we should report error. >>>>>>>> >>>>>>> >>>>>>> Good call. Yes, if we allow --target-is-zero without -n, we MUST >>>>>>> insist >>>>>>> that bdrv_has_zero_init() returns 1 (or, after my followup series, >>>>>>> bdrv_known_zeroes() includes BDRV_ZERO_CREATE). >>>>>> >>>>>> Why? >>>>>> >>>>>> I could imagine a user creating a qcow2 image on some block device >>>>>> with >>>>>> preallocation where we cannot verify that the result will be >>>>>> zero. But >>>>>> they want qemu not to zero the device, so they would specify >>>>>> --target-is-zero. >>>>> >>>>> If user create image, setting --target-is-zero is always valid. But if >>>>> we in >>>>> same operation create the image automatically, having >>>>> --target-is-zero, >>>>> when >>>>> we know that what we are creating is not zero is misleading and should >>>>> fail.. >>>> >>>> bdrv_has_zero_init() doesn’t return false only for images that we know >>>> are not zero. It returns true for images where we know they are. But >>>> if we don’t know, then it returns false also. >>> >>> yes, but we don't have better check. >> >> Correct, but maybe the user knows more, hence why it may make sense for >> them to provide us with some information we don’t have. >> >>>>> If we want to add a behavior to skip zeros unconditionally, we should >>>>> call new >>>>> option --skip-zeroes, to clearly specify what we want. >>>> >>>> It was my impression that this was exactly what --target-is-zero means >>>> and implies. >>>> >>> >>> For me it sounds strange that user has better knowledge about what Qemu >>> creates than Qemu itself. And if it so - it should be fixed in Qemu, >>> rather than creating user interface to hint Qemu what it does. >> >> I brought an example where qemu cannot know whether the image is zero >> (preallocation on a block device), but the user / management layer might >> know. >> > > It sounds unsafe for me. User can't know how exactly Qemu do preallocation, > which syscalls it calls, etc. How can user be sure, that Qemu produces > all-zero image, if even Qemu doesn't sure in it? For example, qcow2 images are always zero if the underlying storage is zero. Then again, it isn’t like we need --target-is-zero without -n anyway. If users want that, they can always use qemu-img create + qemu-img convert -n --target-is-zero. So seeing that you’re uncomfortable with the idea of --target-is-zero with -n, I can’t say there is any actual reason why we’d have to allow it. Max > Otherwise, we should document, how exactly (up to syscalls, their > parameters, flags, the whole logic and algorithm) preallocation is done, > so user can analyze it and be sure that produced image would be all-zero > (when Qemu can't determine it because some specific block device, for which > Qemu doesn't know that its preallocation algorithm produces all-zero, but > user is sure in it)..
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 + 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, 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"); + 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");
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 <david.edmondson@oracle.com> --- docs/interop/qemu-img.rst | 8 +++++++- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 25 ++++++++++++++++++++++--- 3 files changed, 31 insertions(+), 6 deletions(-)