| Message ID | 20200117103434.1363985-1-david.edmondson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: Add --target-is-zero to convert | expand |
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.
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.
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
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;