* [PATCH] qemu-img: Add --target-is-zero to convert
[not found] <id:m21rryz8al.fsf@dme.org>
@ 2020-01-17 10:34 ` David Edmondson
2020-01-17 19:44 ` no-reply
` (2 more replies)
2020-01-17 10:41 ` David Edmondson
1 sibling, 3 replies; 9+ messages in thread
From: David Edmondson @ 2020-01-17 10:34 UTC (permalink / raw)
To: qemu-devel; +Cc: David Edmondson
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;
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;
+
+ 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 &&
@@ -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");
+ goto fail_getopt;
+ }
+
s.src_num = argc - optind - 1;
out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
--
2.24.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] qemu-img: Add --target-is-zero to convert
[not found] <id:m21rryz8al.fsf@dme.org>
2020-01-17 10:34 ` [PATCH] qemu-img: Add --target-is-zero to convert David Edmondson
@ 2020-01-17 10:41 ` David Edmondson
2020-01-17 16:12 ` no-reply
1 sibling, 1 reply; 9+ messages in thread
From: David Edmondson @ 2020-01-17 10:41 UTC (permalink / raw)
To: qemu-devel; +Cc: David Edmondson
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.
---
Apologies if this arrives twice - this From address wasn't subscribed
the first time around.
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;
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;
+
+ 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 &&
@@ -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");
+ goto fail_getopt;
+ }
+
s.src_num = argc - optind - 1;
out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
--
2.24.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] qemu-img: Add --target-is-zero to convert
2020-01-17 10:41 ` David Edmondson
@ 2020-01-17 16:12 ` no-reply
0 siblings, 0 replies; 9+ messages in thread
From: no-reply @ 2020-01-17 16:12 UTC (permalink / raw)
To: david.edmondson; +Cc: david.edmondson, qemu-devel
Patchew URL: https://patchew.org/QEMU/20200117104156.1364069-1-david.edmondson@oracle.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20200117104156.1364069-1-david.edmondson@oracle.com
Type: series
Subject: [PATCH] qemu-img: Add --target-is-zero to convert
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c181366 qemu-img: Add --target-is-zero to convert
=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 0 warnings, 62 lines checked
Commit c181366f92d4 (qemu-img: Add --target-is-zero to convert) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200117104156.1364069-1-david.edmondson@oracle.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qemu-img: Add --target-is-zero to convert
2020-01-17 10:34 ` [PATCH] qemu-img: Add --target-is-zero to convert David Edmondson
@ 2020-01-17 19:44 ` no-reply
2020-01-21 0:33 ` John Snow
2020-01-21 15:02 ` Max Reitz
2 siblings, 0 replies; 9+ messages in thread
From: no-reply @ 2020-01-17 19:44 UTC (permalink / raw)
To: david.edmondson; +Cc: david.edmondson, qemu-devel
Patchew URL: https://patchew.org/QEMU/20200117103434.1363985-1-david.edmondson@oracle.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20200117103434.1363985-1-david.edmondson@oracle.com
Type: series
Subject: [PATCH] qemu-img: Add --target-is-zero to convert
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
From https://github.com/patchew-project/qemu
* [new tag] patchew/20200117103434.1363985-1-david.edmondson@oracle.com -> patchew/20200117103434.1363985-1-david.edmondson@oracle.com
Switched to a new branch 'test'
71f2a28 qemu-img: Add --target-is-zero to convert
=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 0 warnings, 62 lines checked
Commit 71f2a28bebbe (qemu-img: Add --target-is-zero to convert) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200117103434.1363985-1-david.edmondson@oracle.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qemu-img: Add --target-is-zero to convert
2020-01-17 10:34 ` [PATCH] qemu-img: Add --target-is-zero to convert David Edmondson
2020-01-17 19:44 ` no-reply
@ 2020-01-21 0:33 ` John Snow
2020-01-21 13:06 ` David Edmondson
2020-01-21 15:02 ` Max Reitz
2 siblings, 1 reply; 9+ messages in thread
From: John Snow @ 2020-01-21 0:33 UTC (permalink / raw)
To: David Edmondson, qemu-devel; +Cc: Kevin Wolf, Qemu-block, Max Reitz
CC qemu-block and block maintainers
On 1/17/20 5: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.
>
Is there no way to convince bdrv_has_zero_init to return what we want
already in this case? I cannot recall off hand, but wonder if there's an
advanced syntax method of specifying the target image that can set this
flag already.
> 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;
> 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;
> +
> + 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 &&
> @@ -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");
> + goto fail_getopt;
> + }
> +
> s.src_num = argc - optind - 1;
> out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qemu-img: Add --target-is-zero to convert
2020-01-21 0:33 ` John Snow
@ 2020-01-21 13:06 ` David Edmondson
0 siblings, 0 replies; 9+ messages in thread
From: David Edmondson @ 2020-01-21 13:06 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, Qemu-block, Max Reitz
On Monday, 2020-01-20 at 19:33:43 -05, John Snow wrote:
> CC qemu-block and block maintainers
>
> On 1/17/20 5: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.
>>
>
> Is there no way to convince bdrv_has_zero_init to return what we want
> already in this case?
In the current HEAD code, bdrv_has_zero_init will never be called for
“convert -n” (skip target volume creation).
If -n is not specified the host_device block driver doesn't provide a
bdrv_has_zero_init function, so it's assumed not supported.
> I cannot recall off hand, but wonder if there's an advanced syntax
> method of specifying the target image that can set this flag already.
I couldn't see one, but I'd be happy to learn of its existence. My first
approach was to add a raw specific option and add it using
--target-image-opts, resulting in something like:
qemu-img convert -n --target-image-opts sparse.qcow2 \
driver=raw,file.filename=/dev/sdg,assume-blank=on
(assume-blank=on is the new bit). This worked, but is only useful for
raw targets.
The discussion here led me to switch to --target-is-zero.
Mark Kanda sent me some comments suggesting that I get rid of the new
target_is_zero boolean and simply set has_zero_init, which I will do
before sending out another patch if this overall approach is considered
appropriate.
>> 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;
>> 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;
>> +
>> + 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 &&
>> @@ -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");
>> + goto fail_getopt;
>> + }
>> +
>> s.src_num = argc - optind - 1;
>> out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
>>
>>
dme.
--
Please forgive me if I act a little strange, for I know not what I do.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qemu-img: Add --target-is-zero to convert
2020-01-17 10:34 ` [PATCH] qemu-img: Add --target-is-zero to convert David Edmondson
2020-01-17 19:44 ` no-reply
2020-01-21 0:33 ` John Snow
@ 2020-01-21 15:02 ` Max Reitz
2020-01-23 12:17 ` David Edmondson
2 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2020-01-21 15:02 UTC (permalink / raw)
To: David Edmondson, qemu-devel, Qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 4923 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qemu-img: Add --target-is-zero to convert
2020-01-21 15:02 ` Max Reitz
@ 2020-01-23 12:17 ` David Edmondson
2020-01-24 10:11 ` Max Reitz
0 siblings, 1 reply; 9+ messages in thread
From: David Edmondson @ 2020-01-23 12:17 UTC (permalink / raw)
To: Max Reitz, qemu-devel, Qemu-block
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] qemu-img: Add --target-is-zero to convert
2020-01-23 12:17 ` David Edmondson
@ 2020-01-24 10:11 ` Max Reitz
0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2020-01-24 10:11 UTC (permalink / raw)
To: David Edmondson, qemu-devel, Qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 681 bytes --]
On 23.01.20 13:17, David Edmondson wrote:
> On Tuesday, 2020-01-21 at 16:02:16 +01, Max Reitz wrote:
>
>> Hi,
>>
>> On 17.01.20 11:34, David Edmondson wrote:
[...]
>>> +
>>> + 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.
Just the fact that for some reason I read “target_has_backing” as
“target_is_zero”. Sorry for the false alarm. O:-)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-01-24 10:12 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <id:m21rryz8al.fsf@dme.org>
2020-01-17 10:34 ` [PATCH] qemu-img: Add --target-is-zero to convert David Edmondson
2020-01-17 19:44 ` no-reply
2020-01-21 0:33 ` John Snow
2020-01-21 13:06 ` David Edmondson
2020-01-21 15:02 ` Max Reitz
2020-01-23 12:17 ` David Edmondson
2020-01-24 10:11 ` Max Reitz
2020-01-17 10:41 ` David Edmondson
2020-01-17 16:12 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.