| Message ID | 20210709153951.2801666-4-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps | expand |
On 7/9/21 6:39 PM, Eric Blake wrote: > The point of 'qemu-img convert --bitmaps' is to be a convenience for > actions that are already possible through a string of smaller > 'qemu-img bitmap' sub-commands. One situation not accounted for > already is that if a source image contains an inconsistent bitmap (for > example, because a qemu process died abruptly before flushing bitmap > state), the user MUST delete those inconsistent bitmaps before > anything else useful can be done with the image. The only thing affected by inconsistent bitmap is creating incremental backup, and taking some space on storage. Anything else should not be affected by having such bitmap so the user does not need to remove it. In oVirt we don't check or repair images after unclean guest shutdown. Maybe this is a good idea for future version. Inconsistent bitmaps are removed only when the user ask to remove the related checkpoint. > We don't want to delete inconsistent bitmaps by default: although a > corrupt bitmap is only a loss of optimization rather than a corruption > of user-visible data, it is still nice to require the user to opt in > to the fact that they are aware of the loss of the bitmap. Still, > requiring the user to check 'qemu-img info' to see whether bitmaps are > consistent, then use 'qemu-img bitmap --remove' to remove offenders, > all before using 'qemu-img convert', is a lot more work than just > adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which > opts in to skipping the broken bitmaps. I think this is more than convenience. During live storage migration in oVirt, we mirror the top layer to the destination using libvirt blockCopy, and copy the rest of the chain using qemu-img convert with the --bitmaps option. If we have to remove inconsistent bitmaps at this point we need to modify images opened for reading by qemu, which is likely not possible and even if it is possible, sounds like a bad idea. > > After testing the new option, also demonstrate the way to manually fix > things (either deleting bad bitmaps, or re-creating them as empty) so > that it is possible to convert without the option. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1946084 > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > docs/tools/qemu-img.rst | 8 ++++- > qemu-img.c | 26 +++++++++++++--- > tests/qemu-iotests/tests/qemu-img-bitmaps | 10 ++++++ > tests/qemu-iotests/tests/qemu-img-bitmaps.out | 31 +++++++++++++++++++ > 4 files changed, 69 insertions(+), 6 deletions(-) > > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst > index cfe11478791f..4d407b180450 100644 > --- a/docs/tools/qemu-img.rst > +++ b/docs/tools/qemu-img.rst > @@ -414,7 +414,7 @@ Command description: > 4 > Error on reading data > > -.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME > +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME I liked --skip-broken more, but Vladimir is right that this is not really a sub-option. > > Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM* > to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can > @@ -456,6 +456,12 @@ Command description: > *NUM_COROUTINES* specifies how many coroutines work in parallel during > the convert process (defaults to 8). > > + Use of ``--bitmaps`` requests that any persistent bitmaps present in > + the original are also copied to the destination. If any bitmap is > + inconsistent in the source, the conversion will fail unless > + ``--skip-broken-bitmaps`` is also specified to copy only the > + consistent bitmaps. > + > .. option:: create [--object OBJECTDEF] [-q] [-f FMT] [-b BACKING_FILE] [-F BACKING_FMT] [-u] [-o OPTIONS] FILENAME [SIZE] > > Create the new disk image *FILENAME* of size *SIZE* and format > diff --git a/qemu-img.c b/qemu-img.c > index e84b3c530155..661538edd785 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -82,6 +82,7 @@ enum { > OPTION_MERGE = 274, > OPTION_BITMAPS = 275, > OPTION_FORCE = 276, > + OPTION_SKIP_BROKEN = 277, > }; > > typedef enum OutputFormat { > @@ -2102,7 +2103,7 @@ static int convert_do_copy(ImgConvertState *s) > } > > /* Check that bitmaps can be copied, or output an error */ > -static int convert_check_bitmaps(BlockDriverState *src) > +static int convert_check_bitmaps(BlockDriverState *src, bool skip_broken) > { > BdrvDirtyBitmap *bm; > > @@ -2117,7 +2118,7 @@ static int convert_check_bitmaps(BlockDriverState *src) > continue; > } > name = bdrv_dirty_bitmap_name(bm); > - if (bdrv_dirty_bitmap_inconsistent(bm)) { > + if (!skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) { > error_report("Cannot copy inconsistent bitmap '%s'", name); We can add another hint: Try --skip-brocken-bitmaps to skip this bitmap or "qemu-img bitmap --remove" to delete it from disk. > return -1; > } > @@ -2125,7 +2126,8 @@ static int convert_check_bitmaps(BlockDriverState *src) > return 0; > } > > -static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) > +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst, > + bool skip_broken) > { > BdrvDirtyBitmap *bm; > Error *err = NULL; > @@ -2137,6 +2139,10 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) > continue; > } > name = bdrv_dirty_bitmap_name(bm); > + if (skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) { > + warn_report("Skipping inconsistent bitmap %s", name); In other logs we quote the bitmap name:'%s' > + continue; > + } > qmp_block_dirty_bitmap_add(dst->node_name, name, > true, bdrv_dirty_bitmap_granularity(bm), > true, true, > @@ -2192,6 +2198,7 @@ static int img_convert(int argc, char **argv) > bool force_share = false; > bool explict_min_sparse = false; > bool bitmaps = false; > + bool skip_broken = false; > int64_t rate_limit = 0; > > ImgConvertState s = (ImgConvertState) { > @@ -2213,6 +2220,7 @@ static int img_convert(int argc, char **argv) > {"salvage", no_argument, 0, OPTION_SALVAGE}, > {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO}, > {"bitmaps", no_argument, 0, OPTION_BITMAPS}, > + {"skip-broken-bitmaps", no_argument, 0, OPTION_SKIP_BROKEN}, > {0, 0, 0, 0} > }; > c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WUr:", > @@ -2341,6 +2349,9 @@ static int img_convert(int argc, char **argv) > case OPTION_BITMAPS: > bitmaps = true; > break; > + case OPTION_SKIP_BROKEN: > + skip_broken = true; > + break; > } > } > > @@ -2348,6 +2359,11 @@ static int img_convert(int argc, char **argv) > out_fmt = "raw"; > } > > + if (skip_broken && !bitmaps) { > + error_report("Use of --skip-broken-bitmaps requires --bitmaps"); > + goto fail_getopt; > + } > + > if (s.compressed && s.copy_range) { > error_report("Cannot enable copy offloading when -c is used"); > goto fail_getopt; > @@ -2577,7 +2593,7 @@ static int img_convert(int argc, char **argv) > ret = -1; > goto out; > } > - ret = convert_check_bitmaps(blk_bs(s.src[0])); > + ret = convert_check_bitmaps(blk_bs(s.src[0]), skip_broken); > if (ret < 0) { > goto out; > } > @@ -2702,7 +2718,7 @@ static int img_convert(int argc, char **argv) > > /* Now copy the bitmaps */ > if (bitmaps && ret == 0) { > - ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs); > + ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs, skip_broken); > } > > out: > diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps b/tests/qemu-iotests/tests/qemu-img-bitmaps > index 3fde95907515..20f3dffa8e5e 100755 > --- a/tests/qemu-iotests/tests/qemu-img-bitmaps > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" && > echo "unexpected success" > TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ > | _filter_irrelevant_img_info A new title here will make the test output much more clear. > +$QEMU_IMG convert --bitmaps --skip-broken-bitmaps \ > + -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" > +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ > + | _filter_irrelevant_img_info New title will make both the test and output more clear. Maybe also using more descriptive names for the bitmaps (e.g bad, good). > +_rm_test_img "$TEST_IMG.copy" > +$QEMU_IMG bitmap --remove "$TEST_IMG" b0 > +$QEMU_IMG bitmap --remove --add "$TEST_IMG" b2 > +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" > +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ > + | _filter_irrelevant_img_info > > # success, all done > echo '*** done' > diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out > index 546aaa404bba..4d196e24d0fb 100644 > --- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out > @@ -145,4 +145,35 @@ Format specific information: > corrupt: false > qemu-img: Cannot copy inconsistent bitmap 'b0' > qemu-img: Could not open 'TEST_DIR/t.IMGFMT.copy': Could not open 'TEST_DIR/t.IMGFMT.copy': No such file or directory Why to we get this error? I guess it is part of the first copy that should fail? > +qemu-img: warning: Skipping inconsistent bitmap b0 > +qemu-img: warning: Skipping inconsistent bitmap b2 Looks useful, I need to check that we log such warnings. > +image: TEST_DIR/t.IMGFMT.copy > +file format: IMGFMT > +virtual size: 10 MiB (10485760 bytes) > +cluster_size: 65536 > +Format specific information: > + bitmaps: > + [0]: > + flags: > + [0]: auto > + name: b4 > + granularity: 65536 > + corrupt: false > +image: TEST_DIR/t.IMGFMT.copy > +file format: IMGFMT > +virtual size: 10 MiB (10485760 bytes) > +cluster_size: 65536 > +Format specific information: > + bitmaps: > + [0]: > + flags: > + [0]: auto > + name: b4 > + granularity: 65536 > + [1]: > + flags: > + [0]: auto > + name: b2 > + granularity: 65536 > + corrupt: false > *** done >
On Sat, Jul 10, 2021 at 09:37:35PM +0300, Nir Soffer wrote: > > We don't want to delete inconsistent bitmaps by default: although a > > corrupt bitmap is only a loss of optimization rather than a corruption > > of user-visible data, it is still nice to require the user to opt in > > to the fact that they are aware of the loss of the bitmap. Still, > > requiring the user to check 'qemu-img info' to see whether bitmaps are > > consistent, then use 'qemu-img bitmap --remove' to remove offenders, > > all before using 'qemu-img convert', is a lot more work than just > > adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which > > opts in to skipping the broken bitmaps. > > I think this is more than convenience. During live storage migration in > oVirt, we mirror the top layer to the destination using libvirt blockCopy, > and copy the rest of the chain using qemu-img convert with the --bitmaps > option. Still, this feels like enough of a feature that I'd really like R-b in time to prepare a pull request for inclusion in soft freeze; the justification for it being a bug fix is a tough sell. > > +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME > > I liked --skip-broken more, but Vladimir is right that this is not really a > sub-option. getopt_long() lets you abbreviate; '--sk' and '--skip-broken' are both unambiguous prefixes of '--skip-broken-bitmaps'. > > @@ -2117,7 +2118,7 @@ static int convert_check_bitmaps(BlockDriverState *src) > > continue; > > } > > name = bdrv_dirty_bitmap_name(bm); > > - if (bdrv_dirty_bitmap_inconsistent(bm)) { > > + if (!skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) { > > error_report("Cannot copy inconsistent bitmap '%s'", name); > > We can add another hint: > > Try --skip-brocken-bitmaps to skip this bitmap or "qemu-img bitmap > --remove" to delete it from disk. Sure, I can see about adding that. > > > return -1; > > } > > @@ -2125,7 +2126,8 @@ static int convert_check_bitmaps(BlockDriverState *src) > > return 0; > > } > > > > -static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) > > +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst, > > + bool skip_broken) > > { > > BdrvDirtyBitmap *bm; > > Error *err = NULL; > > @@ -2137,6 +2139,10 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) > > continue; > > } > > name = bdrv_dirty_bitmap_name(bm); > > + if (skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) { > > + warn_report("Skipping inconsistent bitmap %s", name); > > In other logs we quote the bitmap name:'%s' Yes, will fix. > > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > > @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" && > > echo "unexpected success" > > TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ > > | _filter_irrelevant_img_info > > A new title here will make the test output much more clear. Or even just a bare 'echo' to separate things with blank lines. Will improve. > > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out > > @@ -145,4 +145,35 @@ Format specific information: > > corrupt: false > > qemu-img: Cannot copy inconsistent bitmap 'b0' > > qemu-img: Could not open 'TEST_DIR/t.IMGFMT.copy': Could not open 'TEST_DIR/t.IMGFMT.copy': No such file or directory > > Why to we get this error? I guess it is part of the first copy that should > fail? Yes - proof that we no longer leave a broken file around, but instead failed fast (in fact, that's part of the previous patch). > > > +qemu-img: warning: Skipping inconsistent bitmap b0 > > +qemu-img: warning: Skipping inconsistent bitmap b2 > > Looks useful, I need to check that we log such warnings. > Anything else I should improve before sending a v2?
On Tue, Jul 13, 2021 at 8:53 PM Eric Blake <eblake@redhat.com> wrote: > > On Sat, Jul 10, 2021 at 09:37:35PM +0300, Nir Soffer wrote: > > > We don't want to delete inconsistent bitmaps by default: although a > > > corrupt bitmap is only a loss of optimization rather than a corruption > > > of user-visible data, it is still nice to require the user to opt in > > > to the fact that they are aware of the loss of the bitmap. Still, > > > requiring the user to check 'qemu-img info' to see whether bitmaps are > > > consistent, then use 'qemu-img bitmap --remove' to remove offenders, > > > all before using 'qemu-img convert', is a lot more work than just > > > adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which > > > opts in to skipping the broken bitmaps. > > > > I think this is more than convenience. During live storage migration in > > oVirt, we mirror the top layer to the destination using libvirt blockCopy, > > and copy the rest of the chain using qemu-img convert with the --bitmaps > > option. > > Still, this feels like enough of a feature that I'd really like R-b in > time to prepare a pull request for inclusion in soft freeze; the > justification for it being a bug fix is a tough sell. This is not a bug in the current code, more like missing handling of important use case. Without this we cannot copy images in some cases, or we must require downtime to check and repair images before copying disks. > > > +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME > > > > I liked --skip-broken more, but Vladimir is right that this is not really a > > sub-option. > > getopt_long() lets you abbreviate; '--sk' and '--skip-broken' are both > unambiguous prefixes of '--skip-broken-bitmaps'. Nice to learn that > > > @@ -2117,7 +2118,7 @@ static int convert_check_bitmaps(BlockDriverState *src) > > > continue; > > > } > > > name = bdrv_dirty_bitmap_name(bm); > > > - if (bdrv_dirty_bitmap_inconsistent(bm)) { > > > + if (!skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) { > > > error_report("Cannot copy inconsistent bitmap '%s'", name); > > > > We can add another hint: > > > > Try --skip-brocken-bitmaps to skip this bitmap or "qemu-img bitmap > > --remove" to delete it from disk. > > Sure, I can see about adding that. > > > > > > > return -1; > > > } > > > @@ -2125,7 +2126,8 @@ static int convert_check_bitmaps(BlockDriverState *src) > > > return 0; > > > } > > > > > > -static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) > > > +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst, > > > + bool skip_broken) > > > { > > > BdrvDirtyBitmap *bm; > > > Error *err = NULL; > > > @@ -2137,6 +2139,10 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) > > > continue; > > > } > > > name = bdrv_dirty_bitmap_name(bm); > > > + if (skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) { > > > + warn_report("Skipping inconsistent bitmap %s", name); > > > > In other logs we quote the bitmap name:'%s' > > Yes, will fix. > > > > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > > > @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" && > > > echo "unexpected success" > > > TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ > > > | _filter_irrelevant_img_info > > > > A new title here will make the test output much more clear. > > Or even just a bare 'echo' to separate things with blank lines. Will > improve. > > > > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out > > > @@ -145,4 +145,35 @@ Format specific information: > > > corrupt: false > > > qemu-img: Cannot copy inconsistent bitmap 'b0' > > > qemu-img: Could not open 'TEST_DIR/t.IMGFMT.copy': Could not open 'TEST_DIR/t.IMGFMT.copy': No such file or directory > > > > Why to we get this error? I guess it is part of the first copy that should > > fail? > > Yes - proof that we no longer leave a broken file around, but instead > failed fast (in fact, that's part of the previous patch). > > > > > > +qemu-img: warning: Skipping inconsistent bitmap b0 > > > +qemu-img: warning: Skipping inconsistent bitmap b2 > > > > Looks useful, I need to check that we log such warnings. > > > > Anything else I should improve before sending a v2? I think we covered everything, but Vladimir may want to comment.
09.07.2021 18:39, Eric Blake wrote: > The point of 'qemu-img convert --bitmaps' is to be a convenience for > actions that are already possible through a string of smaller > 'qemu-img bitmap' sub-commands. One situation not accounted for > already is that if a source image contains an inconsistent bitmap (for > example, because a qemu process died abruptly before flushing bitmap > state), the user MUST delete those inconsistent bitmaps before > anything else useful can be done with the image. > > We don't want to delete inconsistent bitmaps by default: although a > corrupt bitmap is only a loss of optimization rather than a corruption > of user-visible data, it is still nice to require the user to opt in > to the fact that they are aware of the loss of the bitmap. Still, > requiring the user to check 'qemu-img info' to see whether bitmaps are > consistent, then use 'qemu-img bitmap --remove' to remove offenders, > all before using 'qemu-img convert', is a lot more work than just > adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which > opts in to skipping the broken bitmaps. > > After testing the new option, also demonstrate the way to manually fix > things (either deleting bad bitmaps, or re-creating them as empty) so > that it is possible to convert without the option. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1946084 > Signed-off-by: Eric Blake <eblake@redhat.com> [..] > --- a/tests/qemu-iotests/tests/qemu-img-bitmaps > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" && > echo "unexpected success" > TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ > | _filter_irrelevant_img_info > +$QEMU_IMG convert --bitmaps --skip-broken-bitmaps \ > + -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" Nitpicking: sometimes you quote filename variables, sometimes not.. > +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ > + | _filter_irrelevant_img_info > +_rm_test_img "$TEST_IMG.copy" > +$QEMU_IMG bitmap --remove "$TEST_IMG" b0 > +$QEMU_IMG bitmap --remove --add "$TEST_IMG" b2 > +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" > +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ > + | _filter_irrelevant_img_info > With or without adjustments you discussed: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index cfe11478791f..4d407b180450 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -414,7 +414,7 @@ Command description: 4 Error on reading data -.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM* to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can @@ -456,6 +456,12 @@ Command description: *NUM_COROUTINES* specifies how many coroutines work in parallel during the convert process (defaults to 8). + Use of ``--bitmaps`` requests that any persistent bitmaps present in + the original are also copied to the destination. If any bitmap is + inconsistent in the source, the conversion will fail unless + ``--skip-broken-bitmaps`` is also specified to copy only the + consistent bitmaps. + .. option:: create [--object OBJECTDEF] [-q] [-f FMT] [-b BACKING_FILE] [-F BACKING_FMT] [-u] [-o OPTIONS] FILENAME [SIZE] Create the new disk image *FILENAME* of size *SIZE* and format diff --git a/qemu-img.c b/qemu-img.c index e84b3c530155..661538edd785 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -82,6 +82,7 @@ enum { OPTION_MERGE = 274, OPTION_BITMAPS = 275, OPTION_FORCE = 276, + OPTION_SKIP_BROKEN = 277, }; typedef enum OutputFormat { @@ -2102,7 +2103,7 @@ static int convert_do_copy(ImgConvertState *s) } /* Check that bitmaps can be copied, or output an error */ -static int convert_check_bitmaps(BlockDriverState *src) +static int convert_check_bitmaps(BlockDriverState *src, bool skip_broken) { BdrvDirtyBitmap *bm; @@ -2117,7 +2118,7 @@ static int convert_check_bitmaps(BlockDriverState *src) continue; } name = bdrv_dirty_bitmap_name(bm); - if (bdrv_dirty_bitmap_inconsistent(bm)) { + if (!skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) { error_report("Cannot copy inconsistent bitmap '%s'", name); return -1; } @@ -2125,7 +2126,8 @@ static int convert_check_bitmaps(BlockDriverState *src) return 0; } -static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst, + bool skip_broken) { BdrvDirtyBitmap *bm; Error *err = NULL; @@ -2137,6 +2139,10 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) continue; } name = bdrv_dirty_bitmap_name(bm); + if (skip_broken && bdrv_dirty_bitmap_inconsistent(bm)) { + warn_report("Skipping inconsistent bitmap %s", name); + continue; + } qmp_block_dirty_bitmap_add(dst->node_name, name, true, bdrv_dirty_bitmap_granularity(bm), true, true, @@ -2192,6 +2198,7 @@ static int img_convert(int argc, char **argv) bool force_share = false; bool explict_min_sparse = false; bool bitmaps = false; + bool skip_broken = false; int64_t rate_limit = 0; ImgConvertState s = (ImgConvertState) { @@ -2213,6 +2220,7 @@ static int img_convert(int argc, char **argv) {"salvage", no_argument, 0, OPTION_SALVAGE}, {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO}, {"bitmaps", no_argument, 0, OPTION_BITMAPS}, + {"skip-broken-bitmaps", no_argument, 0, OPTION_SKIP_BROKEN}, {0, 0, 0, 0} }; c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WUr:", @@ -2341,6 +2349,9 @@ static int img_convert(int argc, char **argv) case OPTION_BITMAPS: bitmaps = true; break; + case OPTION_SKIP_BROKEN: + skip_broken = true; + break; } } @@ -2348,6 +2359,11 @@ static int img_convert(int argc, char **argv) out_fmt = "raw"; } + if (skip_broken && !bitmaps) { + error_report("Use of --skip-broken-bitmaps requires --bitmaps"); + goto fail_getopt; + } + if (s.compressed && s.copy_range) { error_report("Cannot enable copy offloading when -c is used"); goto fail_getopt; @@ -2577,7 +2593,7 @@ static int img_convert(int argc, char **argv) ret = -1; goto out; } - ret = convert_check_bitmaps(blk_bs(s.src[0])); + ret = convert_check_bitmaps(blk_bs(s.src[0]), skip_broken); if (ret < 0) { goto out; } @@ -2702,7 +2718,7 @@ static int img_convert(int argc, char **argv) /* Now copy the bitmaps */ if (bitmaps && ret == 0) { - ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs); + ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs, skip_broken); } out: diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps b/tests/qemu-iotests/tests/qemu-img-bitmaps index 3fde95907515..20f3dffa8e5e 100755 --- a/tests/qemu-iotests/tests/qemu-img-bitmaps +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps @@ -143,6 +143,16 @@ $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" && echo "unexpected success" TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ | _filter_irrelevant_img_info +$QEMU_IMG convert --bitmaps --skip-broken-bitmaps \ + -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ + | _filter_irrelevant_img_info +_rm_test_img "$TEST_IMG.copy" +$QEMU_IMG bitmap --remove "$TEST_IMG" b0 +$QEMU_IMG bitmap --remove --add "$TEST_IMG" b2 +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ + | _filter_irrelevant_img_info # success, all done echo '*** done' diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out index 546aaa404bba..4d196e24d0fb 100644 --- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out @@ -145,4 +145,35 @@ Format specific information: corrupt: false qemu-img: Cannot copy inconsistent bitmap 'b0' qemu-img: Could not open 'TEST_DIR/t.IMGFMT.copy': Could not open 'TEST_DIR/t.IMGFMT.copy': No such file or directory +qemu-img: warning: Skipping inconsistent bitmap b0 +qemu-img: warning: Skipping inconsistent bitmap b2 +image: TEST_DIR/t.IMGFMT.copy +file format: IMGFMT +virtual size: 10 MiB (10485760 bytes) +cluster_size: 65536 +Format specific information: + bitmaps: + [0]: + flags: + [0]: auto + name: b4 + granularity: 65536 + corrupt: false +image: TEST_DIR/t.IMGFMT.copy +file format: IMGFMT +virtual size: 10 MiB (10485760 bytes) +cluster_size: 65536 +Format specific information: + bitmaps: + [0]: + flags: + [0]: auto + name: b4 + granularity: 65536 + [1]: + flags: + [0]: auto + name: b2 + granularity: 65536 + corrupt: false *** done
The point of 'qemu-img convert --bitmaps' is to be a convenience for actions that are already possible through a string of smaller 'qemu-img bitmap' sub-commands. One situation not accounted for already is that if a source image contains an inconsistent bitmap (for example, because a qemu process died abruptly before flushing bitmap state), the user MUST delete those inconsistent bitmaps before anything else useful can be done with the image. We don't want to delete inconsistent bitmaps by default: although a corrupt bitmap is only a loss of optimization rather than a corruption of user-visible data, it is still nice to require the user to opt in to the fact that they are aware of the loss of the bitmap. Still, requiring the user to check 'qemu-img info' to see whether bitmaps are consistent, then use 'qemu-img bitmap --remove' to remove offenders, all before using 'qemu-img convert', is a lot more work than just adding a knob 'qemu-img convert --bitmaps --skip-broken-bitmaps' which opts in to skipping the broken bitmaps. After testing the new option, also demonstrate the way to manually fix things (either deleting bad bitmaps, or re-creating them as empty) so that it is possible to convert without the option. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1946084 Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/tools/qemu-img.rst | 8 ++++- qemu-img.c | 26 +++++++++++++--- tests/qemu-iotests/tests/qemu-img-bitmaps | 10 ++++++ tests/qemu-iotests/tests/qemu-img-bitmaps.out | 31 +++++++++++++++++++ 4 files changed, 69 insertions(+), 6 deletions(-)