On Tuesday, 2020-01-21 at 16:02:16 +01, Max Reitz wrote:
> 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.)
I'll add a check causing (has_zero_init && target_has_backing) to throw
an error after the target_has_backing is determined.
> 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.)
I don't understand this comment - I must be missing something.
If both has_zero_init and target_has_backing are false here, we should
go and check bdrv_has_zero_init().
They can't both be true (when the above mentioned test is added) and if
either is true, we don't want to call brv_has_zero_init(), as either the
user has asserted that the target is blank or we have a backing file.
>> 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).
Will do.
dme.
--
You can't hide from the flipside.
