Hi,
On 17.01.20 11:34, David Edmondson wrote:
> In many cases the target of a convert operation is a newly provisioned
> target that the user knows is blank (filled with zeroes). In this
> situation there is no requirement for qemu-img to wastefully zero out
> the entire device.
>
> Add a new option, --target-is-zero, allowing the user to indicate that
> an existing target device is already zero filled.
> ---
> qemu-img.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 95a24b9762..56ca727e8c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -70,6 +70,7 @@ enum {
> OPTION_PREALLOCATION = 265,
> OPTION_SHRINK = 266,
> OPTION_SALVAGE = 267,
> + OPTION_TARGET_IS_ZERO = 268,
> };
>
> typedef enum OutputFormat {
> @@ -1593,6 +1594,7 @@ typedef struct ImgConvertState {
> bool copy_range;
> bool salvage;
> bool quiet;
> + bool target_is_zero;
As you already said, we probably don’t need this and it’d be sufficient
to set the has_zero_init value directly.
> int min_sparse;
> int alignment;
> size_t cluster_sectors;
> @@ -1984,10 +1986,11 @@ static int convert_do_copy(ImgConvertState *s)
> int64_t sector_num = 0;
>
> /* Check whether we have zero initialisation or can get it efficiently */
> - if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
> + s->has_zero_init = s->target_is_zero;
We cannot has_zero_init to true if the target has a backing file,
because convert_co_write() asserts that the target must not have a
backing file if has_zero_init is true. (It’s impossible for a file to
be initialized to zeroes if it has a backing file; or at least it
doesn’t make sense then to have a backing file.)
Case in point:
$ ./qemu-img create -f qcow2 src.qcow2 64M
Formatting 'src.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 dst.qcow2 64M
Formatting 'dst.qcow2', fmt=qcow2 size=67108864
backing_file=backing.qcow2 cluster_size=65536 lazy_refcounts=off
refcount_bits=16
$ ./qemu-img convert -n -B backing.qcow2 -f qcow2 -O qcow2
--target-is-zero src.qcow2 dst.qcow2
qemu-img: qemu-img.c:1812: convert_co_write: Assertion
`!s->target_has_backing' failed.
[1] 80813 abort (core dumped) ./qemu-img convert -n -B backing.qcow2
-f qcow2 -O qcow2 --target-is-zero
> +
> + if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
> + !s->target_has_backing) {
(This will be irrelevant after target_has_backing is gone, but because
has_zero_init and target_has_backing are equivalent here, there is no
need to check both.)
> s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
> - } else {
> - s->has_zero_init = false;
> }
>
> if (!s->has_zero_init && !s->target_has_backing &&
> @@ -2076,6 +2079,7 @@ static int img_convert(int argc, char **argv)
> .buf_sectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE,
> .wr_in_order = true,
> .num_coroutines = 8,
> + .target_is_zero = false,
> };
>
> for(;;) {
> @@ -2086,6 +2090,7 @@ static int img_convert(int argc, char **argv)
> {"force-share", no_argument, 0, 'U'},
> {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
> {"salvage", no_argument, 0, OPTION_SALVAGE},
> + {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
> {0, 0, 0, 0}
> };
> c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
> @@ -2209,6 +2214,9 @@ static int img_convert(int argc, char **argv)
> case OPTION_TARGET_IMAGE_OPTS:
> tgt_image_opts = true;
> break;
> + case OPTION_TARGET_IS_ZERO:
> + s.target_is_zero = true;
> + break;
> }
> }
>
> @@ -2247,6 +2255,11 @@ static int img_convert(int argc, char **argv)
> warn_report("This will become an error in future QEMU versions.");
> }
>
> + if (s.target_is_zero && !skip_create) {
> + error_report("--target-is-zero requires use of -n flag");
Hm, I could imagine it being useful even without -n, but maybe it’s
safer to forbid this case for now and reconsider if someone were to ask.
> + goto fail_getopt;
> + }
> +
> s.src_num = argc - optind - 1;
> out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
This patch should also add some documentation for the new option (in
qemu-img-cmds.hx and in qemu-img.texi for the man page).
Max
signature.asc
Description: OpenPGP digital signature
