qemu-img.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
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
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
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; > >
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.
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
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.
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
© 2016 - 2024 Red Hat, Inc.