| Message ID | 20200124103458.1525982-2-david.edmondson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: Add --target-is-zero to indicate that a target is blank | expand |
On 1/24/20 4: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. > > 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> > --- > qemu-img-cmds.hx | 4 ++-- > qemu-img.c | 25 ++++++++++++++++++++++--- > qemu-img.texi | 4 ++++ > 3 files changed, 28 insertions(+), 5 deletions(-) I'm working up a patch series that tries to auto-set this flag without user interaction where possible (for example, if lseek(fd, 0, SEEK_DATA) returns EOF, or if fstat() reports 0 blocks allocated, or if qcow2 sees no L2 tables allocated, or a proposed extension to NBD passes on the same...). I may rebase my series on top of your patch and tweak things in yours accordingly. But as it stands, the idea makes sense to me; even if we add ways for some images to efficiently report initial state (and our existing bdrv_has_zero_init() is NOT such a method), there are enough other scenarios where the knob will be the only way to let qemu-img know the intent. > + case OPTION_TARGET_IS_ZERO: > + /* > + * The user asserting that the target is blank has the > + * same effect as the target driver supporting zero > + * initialisation. Hmm. A git grep shows that 'initialization' has 200 hits, 'initialisation' has only 29. But I think it's a US vs. UK thing, so I don't care which spelling you use. > @@ -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; > + } > + Makes sense, although we could perhaps relax it to also work even when the -n flag is supplied IF the destination image supports my proposal for a new status bit set when an image is known to be opened with all zero content. > 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; > + } > + Makes sense, although we could perhaps relax it to also work even when there is a backing file IF the backing file supports my proposal for a new status bit set when an image is known to be opened with all zero content. As my patch proposal is still not submitted, I'm fine if yours lands as-is: Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 1/24/20 4: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. >> >> 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> >> --- >> qemu-img-cmds.hx | 4 ++-- >> qemu-img.c | 25 ++++++++++++++++++++++--- >> qemu-img.texi | 4 ++++ >> 3 files changed, 28 insertions(+), 5 deletions(-) > > I'm working up a patch series that tries to auto-set this flag without > user interaction where possible (for example, if lseek(fd, 0, SEEK_DATA) > returns EOF, or if fstat() reports 0 blocks allocated, or if qcow2 sees > no L2 tables allocated, or a proposed extension to NBD passes on the > same...). I may rebase my series on top of your patch and tweak things > in yours accordingly. > > But as it stands, the idea makes sense to me; even if we add ways for > some images to efficiently report initial state (and our existing > bdrv_has_zero_init() is NOT such a method), there are enough other > scenarios where the knob will be the only way to let qemu-img know the > intent. Having qemu-img figure things out on its own is obviously desirable, but I agree that there are enough cases where this won't be possible and, given the resulting performance improvement, it will still be useful to allow the caller to force things. >> + case OPTION_TARGET_IS_ZERO: >> + /* >> + * The user asserting that the target is blank has the >> + * same effect as the target driver supporting zero >> + * initialisation. > > Hmm. A git grep shows that 'initialization' has 200 hits, > 'initialisation' has only 29. But I think it's a US vs. UK thing, so I > don't care which spelling you use. Yes, it's British English spelling. It was unconscious - I'll switch if there is an existing policy. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks. If the conversion of the documentation to rST is imminent then I'll wait for that before submitting a followup with corresponding changes applied to the new docs. dme.
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 <david.edmondson@oracle.com> > --- > 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: >
On 2/3/20 12:20 PM, Vladimir Sementsov-Ogievskiy wrote: > 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. Correct. Patch 2/2 is now obsolete and no longer necessary, and patch 1 needs some tweaks now that we don't have qemu-img.texi. > > I like this thing, and I'd like to make similar option for backup job. My followup patches to add an all-zero bit to qcow2 are also useful in this regard.
On Monday, 2020-02-03 at 21:20:16 +03, Vladimir Sementsov-Ogievskiy wrote: > 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. Updated v3 just sent. dme.
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:
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> --- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 25 ++++++++++++++++++++++--- qemu-img.texi | 4 ++++ 3 files changed, 28 insertions(+), 5 deletions(-)