* [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps
@ 2021-07-08 1:29 Eric Blake
2021-07-08 1:30 ` [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap Eric Blake
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Eric Blake @ 2021-07-08 1:29 UTC (permalink / raw)
To: qemu-devel; +Cc: nsoffer, vsementsov, qemu-block
This is mostly a convenience factor as one could already use 'qemu-img
info' to learn which bitmaps are broken and then 'qemu-img bitmap
--remove' to nuke them before calling 'qemu-img convert --bitmaps',
but it does have the advantage that the copied file is usable without
extra efforts and the broken bitmap is not deleted from the source
file.
Eric Blake (2):
iotests: Improve and rename test 291 to qemu-img-bitmap
qemu-img: Add --skip-broken for 'convert --bitmaps'
docs/tools/qemu-img.rst | 8 +++-
block/dirty-bitmap.c | 2 +-
qemu-img.c | 20 +++++++-
.../{291 => tests/qemu-img-bitmaps} | 17 ++++++-
.../{291.out => tests/qemu-img-bitmaps.out} | 46 ++++++++++++++++++-
5 files changed, 87 insertions(+), 6 deletions(-)
rename tests/qemu-iotests/{291 => tests/qemu-img-bitmaps} (88%)
rename tests/qemu-iotests/{291.out => tests/qemu-img-bitmaps.out} (76%)
--
2.31.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap 2021-07-08 1:29 [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps Eric Blake @ 2021-07-08 1:30 ` Eric Blake 2021-07-09 6:33 ` Vladimir Sementsov-Ogievskiy 2021-07-08 1:30 ` [PATCH 2/2] qemu-img: Add --skip-broken for 'convert --bitmaps' Eric Blake 2021-07-09 9:50 ` [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps Vladimir Sementsov-Ogievskiy 2 siblings, 1 reply; 9+ messages in thread From: Eric Blake @ 2021-07-08 1:30 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, vsementsov, qemu-block, Max Reitz, nsoffer, John Snow Enhance the test to demonstrate behavior of qemu-img with a qcow2 image containing an inconsistent bitmap, and rename it now that we support useful iotest names. While at it, fix a missing newline in the error message thus exposed. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/dirty-bitmap.c | 2 +- .../{291 => tests/qemu-img-bitmaps} | 13 +++++++- .../{291.out => tests/qemu-img-bitmaps.out} | 32 ++++++++++++++++++- 3 files changed, 44 insertions(+), 3 deletions(-) rename tests/qemu-iotests/{291 => tests/qemu-img-bitmaps} (92%) rename tests/qemu-iotests/{291.out => tests/qemu-img-bitmaps.out} (82%) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 68d295d6e3ed..0ef46163e3ea 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -193,7 +193,7 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used", bitmap->name); error_append_hint(errp, "Try block-dirty-bitmap-remove to delete" - " this bitmap from disk"); + " this bitmap from disk\n"); return -1; } diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/tests/qemu-img-bitmaps similarity index 92% rename from tests/qemu-iotests/291 rename to tests/qemu-iotests/tests/qemu-img-bitmaps index 20efb080a6c0..76cd9e31e850 100755 --- a/tests/qemu-iotests/291 +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps @@ -3,7 +3,7 @@ # # Test qemu-img bitmap handling # -# Copyright (C) 2018-2020 Red Hat, Inc. +# Copyright (C) 2018-2021 Red Hat, Inc. # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -32,6 +32,7 @@ _cleanup() trap "_cleanup; exit \$status" 0 1 2 3 15 # get standard environment, filters and checks +cd .. . ./common.rc . ./common.filter . ./common.nbd @@ -129,6 +130,16 @@ $QEMU_IMG map --output=json --image-opts \ nbd_server_stop +echo +echo "=== Check handling of inconsistent bitmap ===" +echo + +$QEMU_IO -c abort "$TEST_IMG" 2>/dev/null +$QEMU_IMG bitmap --add "$TEST_IMG" b4 +$QEMU_IMG bitmap --remove "$TEST_IMG" b1 +_img_info --format-specific | _filter_irrelevant_img_info +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" + # success, all done echo '*** done' rm -f $seq.full diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out similarity index 82% rename from tests/qemu-iotests/291.out rename to tests/qemu-iotests/tests/qemu-img-bitmaps.out index 018d6b103f87..17b34eaed30f 100644 --- a/tests/qemu-iotests/291.out +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out @@ -1,4 +1,4 @@ -QA output created by 291 +QA output created by qemu-img-bitmaps === Initial image setup === @@ -115,4 +115,34 @@ Format specific information: [{ "start": 0, "length": 2097152, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, { "start": 2097152, "length": 1048576, "depth": 0, "present": false, "zero": false, "data": false}, { "start": 3145728, "length": 7340032, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}] + +=== Check handling of inconsistent bitmap === + +image: TEST_DIR/t.IMGFMT +file format: IMGFMT +virtual size: 10 MiB (10485760 bytes) +cluster_size: 65536 +backing file: TEST_DIR/t.IMGFMT.base +backing file format: IMGFMT +Format specific information: + bitmaps: + [0]: + flags: + [0]: in-use + [1]: auto + name: b2 + granularity: 65536 + [1]: + flags: + [0]: in-use + name: b0 + granularity: 65536 + [2]: + flags: + [0]: auto + name: b4 + granularity: 65536 + corrupt: false +qemu-img: Failed to populate bitmap b0: Bitmap 'b0' is inconsistent and cannot be used +Try block-dirty-bitmap-remove to delete this bitmap from disk *** done -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap 2021-07-08 1:30 ` [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap Eric Blake @ 2021-07-09 6:33 ` Vladimir Sementsov-Ogievskiy 2021-07-09 13:16 ` Eric Blake 0 siblings, 1 reply; 9+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-07-09 6:33 UTC (permalink / raw) To: Eric Blake, qemu-devel Cc: nsoffer, qemu-block, John Snow, Kevin Wolf, Max Reitz 08.07.2021 04:30, Eric Blake wrote: > Enhance the test to demonstrate behavior of qemu-img with a qcow2 > image containing an inconsistent bitmap, and rename it now that we > support useful iotest names. > > While at it, fix a missing newline in the error message thus exposed. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/dirty-bitmap.c | 2 +- > .../{291 => tests/qemu-img-bitmaps} | 13 +++++++- > .../{291.out => tests/qemu-img-bitmaps.out} | 32 ++++++++++++++++++- > 3 files changed, 44 insertions(+), 3 deletions(-) > rename tests/qemu-iotests/{291 => tests/qemu-img-bitmaps} (92%) > rename tests/qemu-iotests/{291.out => tests/qemu-img-bitmaps.out} (82%) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 68d295d6e3ed..0ef46163e3ea 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -193,7 +193,7 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, > error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used", > bitmap->name); > error_append_hint(errp, "Try block-dirty-bitmap-remove to delete" > - " this bitmap from disk"); > + " this bitmap from disk\n"); > return -1; > } > > diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/tests/qemu-img-bitmaps > similarity index 92% > rename from tests/qemu-iotests/291 > rename to tests/qemu-iotests/tests/qemu-img-bitmaps > index 20efb080a6c0..76cd9e31e850 100755 > --- a/tests/qemu-iotests/291 > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > @@ -3,7 +3,7 @@ > # > # Test qemu-img bitmap handling > # > -# Copyright (C) 2018-2020 Red Hat, Inc. > +# Copyright (C) 2018-2021 Red Hat, Inc. > # > # This program is free software; you can redistribute it and/or modify > # it under the terms of the GNU General Public License as published by > @@ -32,6 +32,7 @@ _cleanup() > trap "_cleanup; exit \$status" 0 1 2 3 15 > > # get standard environment, filters and checks > +cd .. > . ./common.rc > . ./common.filter > . ./common.nbd > @@ -129,6 +130,16 @@ $QEMU_IMG map --output=json --image-opts \ > > nbd_server_stop > > +echo > +echo "=== Check handling of inconsistent bitmap ===" > +echo > + > +$QEMU_IO -c abort "$TEST_IMG" 2>/dev/null > +$QEMU_IMG bitmap --add "$TEST_IMG" b4 > +$QEMU_IMG bitmap --remove "$TEST_IMG" b1 > +_img_info --format-specific | _filter_irrelevant_img_info > +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" Worth then removing remaining inconsistent bitmaps and try again? I think you should now remove $TEST_IMG.copy in _cleanup with squashed in --- a/tests/qemu-iotests/tests/qemu-img-bitmaps +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps @@ -27,6 +27,7 @@ status=1 # failure is the default! _cleanup() { _cleanup_test_img + _rm_test_img "$TEST_IMG.copy" nbd_server_stop } trap "_cl Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > + > # success, all done > echo '*** done' > rm -f $seq.full > diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out > similarity index 82% > rename from tests/qemu-iotests/291.out > rename to tests/qemu-iotests/tests/qemu-img-bitmaps.out > index 018d6b103f87..17b34eaed30f 100644 > --- a/tests/qemu-iotests/291.out > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out > @@ -1,4 +1,4 @@ > -QA output created by 291 > +QA output created by qemu-img-bitmaps > > === Initial image setup === > > @@ -115,4 +115,34 @@ Format specific information: > [{ "start": 0, "length": 2097152, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}, > { "start": 2097152, "length": 1048576, "depth": 0, "present": false, "zero": false, "data": false}, > { "start": 3145728, "length": 7340032, "depth": 0, "present": true, "zero": false, "data": true, "offset": OFFSET}] > + > +=== Check handling of inconsistent bitmap === > + > +image: TEST_DIR/t.IMGFMT > +file format: IMGFMT > +virtual size: 10 MiB (10485760 bytes) > +cluster_size: 65536 > +backing file: TEST_DIR/t.IMGFMT.base > +backing file format: IMGFMT > +Format specific information: > + bitmaps: > + [0]: > + flags: > + [0]: in-use > + [1]: auto > + name: b2 > + granularity: 65536 > + [1]: > + flags: > + [0]: in-use > + name: b0 > + granularity: 65536 > + [2]: > + flags: > + [0]: auto > + name: b4 > + granularity: 65536 > + corrupt: false > +qemu-img: Failed to populate bitmap b0: Bitmap 'b0' is inconsistent and cannot be used > +Try block-dirty-bitmap-remove to delete this bitmap from disk > *** done > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap 2021-07-09 6:33 ` Vladimir Sementsov-Ogievskiy @ 2021-07-09 13:16 ` Eric Blake 2021-07-09 13:49 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 9+ messages in thread From: Eric Blake @ 2021-07-09 13:16 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy Cc: Kevin Wolf, qemu-block, qemu-devel, Max Reitz, nsoffer, John Snow On Fri, Jul 09, 2021 at 09:33:50AM +0300, Vladimir Sementsov-Ogievskiy wrote: > > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > > +echo > > +echo "=== Check handling of inconsistent bitmap ===" > > +echo > > + > > +$QEMU_IO -c abort "$TEST_IMG" 2>/dev/null > > +$QEMU_IMG bitmap --add "$TEST_IMG" b4 > > +$QEMU_IMG bitmap --remove "$TEST_IMG" b1 > > +_img_info --format-specific | _filter_irrelevant_img_info > > +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" > > Worth then removing remaining inconsistent bitmaps and try again? > > I think you should now remove $TEST_IMG.copy in _cleanup $TEST_IMG.copy isn't created on failure (or if it is, that in itself is a problem we should be avoiding), so as written, there was nothing that should have needed cleaning up until patch 2. But your idea (here and in patch 2) of demonstrating manual cleanup for recovery (in addition to the goal of patch 2 of skipping broken bitmaps in the first place) is reasonable, so I'll incorporate that into v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap 2021-07-09 13:16 ` Eric Blake @ 2021-07-09 13:49 ` Vladimir Sementsov-Ogievskiy 2021-07-09 14:17 ` Eric Blake 0 siblings, 1 reply; 9+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-07-09 13:49 UTC (permalink / raw) To: Eric Blake Cc: qemu-devel, nsoffer, qemu-block, John Snow, Kevin Wolf, Max Reitz 09.07.2021 16:16, Eric Blake wrote: > On Fri, Jul 09, 2021 at 09:33:50AM +0300, Vladimir Sementsov-Ogievskiy wrote: >>> +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > >>> +echo >>> +echo "=== Check handling of inconsistent bitmap ===" >>> +echo >>> + >>> +$QEMU_IO -c abort "$TEST_IMG" 2>/dev/null >>> +$QEMU_IMG bitmap --add "$TEST_IMG" b4 >>> +$QEMU_IMG bitmap --remove "$TEST_IMG" b1 >>> +_img_info --format-specific | _filter_irrelevant_img_info >>> +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" >> >> Worth then removing remaining inconsistent bitmaps and try again? >> >> I think you should now remove $TEST_IMG.copy in _cleanup > > $TEST_IMG.copy isn't created on failure (or if it is, that in itself > is a problem we should be avoiding), Seems that's the case: ./build/qemu-img create -f qcow2 x 1M ./build/qemu-img bitmap --add x b1 ./build/qemu-io x qemu-io> abort Aborted (core dumped) ./build/qemu-img info x image: x file format: qcow2 virtual size: 1 MiB (1048576 bytes) disk size: 204 KiB cluster_size: 65536 Format specific information: compat: 1.1 compression type: zlib lazy refcounts: false bitmaps: [0]: flags: [0]: in-use [1]: auto name: b1 granularity: 65536 refcount bits: 16 corrupt: false extended l2: false ls y ls: cannot access 'y': No such file or directory ./build/qemu-img convert --bitmaps -O qcow2 x y qemu-img: Failed to populate bitmap b1: Bitmap 'b1' is inconsistent and cannot be used Try block-dirty-bitmap-remove to delete this bitmap from disk[root@kvm master]# # ls y y ./build/qemu-img info y image: y file format: qcow2 virtual size: 1 MiB (1048576 bytes) disk size: 204 KiB cluster_size: 65536 Format specific information: compat: 1.1 compression type: zlib lazy refcounts: false bitmaps: [0]: flags: name: b1 granularity: 65536 refcount bits: 16 corrupt: false extended l2: false WOW! It even contains the bitmap not marked in-use. That's a real bug. > so as written, there was nothing > that should have needed cleaning up until patch 2. But your idea > (here and in patch 2) of demonstrating manual cleanup for recovery (in > addition to the goal of patch 2 of skipping broken bitmaps in the > first place) is reasonable, so I'll incorporate that into v2. > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap 2021-07-09 13:49 ` Vladimir Sementsov-Ogievskiy @ 2021-07-09 14:17 ` Eric Blake 0 siblings, 0 replies; 9+ messages in thread From: Eric Blake @ 2021-07-09 14:17 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy Cc: Kevin Wolf, qemu-block, qemu-devel, Max Reitz, nsoffer, John Snow On Fri, Jul 09, 2021 at 04:49:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 09.07.2021 16:16, Eric Blake wrote: > > On Fri, Jul 09, 2021 at 09:33:50AM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > > > > > > +echo > > > > +echo "=== Check handling of inconsistent bitmap ===" > > > > +echo > > > > + > > > > +$QEMU_IO -c abort "$TEST_IMG" 2>/dev/null > > > > +$QEMU_IMG bitmap --add "$TEST_IMG" b4 > > > > +$QEMU_IMG bitmap --remove "$TEST_IMG" b1 > > > > +_img_info --format-specific | _filter_irrelevant_img_info > > > > +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" > > > > > > Worth then removing remaining inconsistent bitmaps and try again? > > > > > > I think you should now remove $TEST_IMG.copy in _cleanup > > > > $TEST_IMG.copy isn't created on failure (or if it is, that in itself > > is a problem we should be avoiding), > > Seems that's the case: Oh. Thanks for checking. Yeah, re-reading the code, we don't attempt to copy the bitmaps until after the rest of the convert operation is done. Which means a fix to pre-validate bitmaps before starting the convert requires more code, but may be the right thing (failing at the very end of a long convert attempt, but leaving the file around, is not ideal compared to failing fast). Looks like my v2 will be a bit more beefy. > ./build/qemu-img convert --bitmaps -O qcow2 x y > qemu-img: Failed to populate bitmap b1: Bitmap 'b1' is inconsistent and cannot be used > Try block-dirty-bitmap-remove to delete this bitmap from disk[root@kvm master]# > # ls y > y > ./build/qemu-img info y > image: y > file format: qcow2 > virtual size: 1 MiB (1048576 bytes) > disk size: 204 KiB > cluster_size: 65536 > Format specific information: > compat: 1.1 > compression type: zlib > lazy refcounts: false > bitmaps: > [0]: > flags: > name: b1 > granularity: 65536 > refcount bits: 16 > corrupt: false > extended l2: false > > > WOW! It even contains the bitmap not marked in-use. That's a real bug. So we left things dangling with a bitmap that has completely different contents than in the original file. Yeah, that looks like an issue worth improving. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] qemu-img: Add --skip-broken for 'convert --bitmaps' 2021-07-08 1:29 [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps Eric Blake 2021-07-08 1:30 ` [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap Eric Blake @ 2021-07-08 1:30 ` Eric Blake 2021-07-09 6:54 ` Vladimir Sementsov-Ogievskiy 2021-07-09 9:50 ` [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps Vladimir Sementsov-Ogievskiy 2 siblings, 1 reply; 9+ messages in thread From: Eric Blake @ 2021-07-08 1:30 UTC (permalink / raw) To: qemu-devel; +Cc: nsoffer, Kevin Wolf, vsementsov, qemu-block, Max Reitz 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' which opts in to skipping the broken bitmaps. 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 | 20 +++++++++++++++++-- tests/qemu-iotests/tests/qemu-img-bitmaps | 4 ++++ tests/qemu-iotests/tests/qemu-img-bitmaps.out | 14 +++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index 1d8470eada0e..5cf1c764597b 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]] [-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`` 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 68a4d298098f..e8b012f39c0c 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 { @@ -2101,7 +2102,8 @@ static int convert_do_copy(ImgConvertState *s) return s->ret; } -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; @@ -2113,6 +2115,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, @@ -2167,6 +2173,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) { @@ -2188,6 +2195,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", 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:", @@ -2316,6 +2324,9 @@ static int img_convert(int argc, char **argv) case OPTION_BITMAPS: bitmaps = true; break; + case OPTION_SKIP_BROKEN: + skip_broken = true; + break; } } @@ -2323,6 +2334,11 @@ static int img_convert(int argc, char **argv) out_fmt = "raw"; } + if (skip_broken && !bitmaps) { + error_report("Use of --skip-broken requires --bitmaps"); + goto fail_getopt; + } + if (s.compressed && s.copy_range) { error_report("Cannot enable copy offloading when -c is used"); goto fail_getopt; @@ -2678,7 +2694,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 76cd9e31e850..3e1a39bc81e4 100755 --- a/tests/qemu-iotests/tests/qemu-img-bitmaps +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps @@ -28,6 +28,7 @@ _cleanup() { _cleanup_test_img nbd_server_stop + _rm_test_img "$TEST_DIR/t.$IMGFMT.copy" } trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -139,6 +140,9 @@ $QEMU_IMG bitmap --add "$TEST_IMG" b4 $QEMU_IMG bitmap --remove "$TEST_IMG" b1 _img_info --format-specific | _filter_irrelevant_img_info $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" +$QEMU_IMG convert --bitmaps --skip-broken -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 17b34eaed30f..685bde6d1d63 100644 --- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out @@ -145,4 +145,18 @@ Format specific information: corrupt: false qemu-img: Failed to populate bitmap b0: Bitmap 'b0' is inconsistent and cannot be used Try block-dirty-bitmap-remove to delete this bitmap from disk +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 *** done -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] qemu-img: Add --skip-broken for 'convert --bitmaps' 2021-07-08 1:30 ` [PATCH 2/2] qemu-img: Add --skip-broken for 'convert --bitmaps' Eric Blake @ 2021-07-09 6:54 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 9+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-07-09 6:54 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: nsoffer, qemu-block, Kevin Wolf, Max Reitz 08.07.2021 04:30, 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' which opts in > to skipping the broken bitmaps. > > 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 | 20 +++++++++++++++++-- > tests/qemu-iotests/tests/qemu-img-bitmaps | 4 ++++ > tests/qemu-iotests/tests/qemu-img-bitmaps.out | 14 +++++++++++++ > 4 files changed, 43 insertions(+), 3 deletions(-) > > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst > index 1d8470eada0e..5cf1c764597b 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]] [-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 Of course, [--bitmaps [--skip-broken]] looks like --skip-broken is a suboption.. But actually it's not so. So, shouldn't it be named more explicit, like --skip-broken-bitmaps ? To be sure that we will not interfere in future with some other broken things we want to skip? And to avoid strange but correct command lines like qemu-img convert --skip-broken <someother options> --bitmaps <some other options> src dst > > 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`` 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 68a4d298098f..e8b012f39c0c 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 { > @@ -2101,7 +2102,8 @@ static int convert_do_copy(ImgConvertState *s) > return s->ret; > } > > -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; > @@ -2113,6 +2115,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, > @@ -2167,6 +2173,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) { > @@ -2188,6 +2195,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", 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:", > @@ -2316,6 +2324,9 @@ static int img_convert(int argc, char **argv) > case OPTION_BITMAPS: > bitmaps = true; > break; > + case OPTION_SKIP_BROKEN: > + skip_broken = true; > + break; > } > } > > @@ -2323,6 +2334,11 @@ static int img_convert(int argc, char **argv) > out_fmt = "raw"; > } > > + if (skip_broken && !bitmaps) { > + error_report("Use of --skip-broken requires --bitmaps"); > + goto fail_getopt; > + } > + > if (s.compressed && s.copy_range) { > error_report("Cannot enable copy offloading when -c is used"); > goto fail_getopt; > @@ -2678,7 +2694,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 76cd9e31e850..3e1a39bc81e4 100755 > --- a/tests/qemu-iotests/tests/qemu-img-bitmaps > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps > @@ -28,6 +28,7 @@ _cleanup() > { > _cleanup_test_img > nbd_server_stop > + _rm_test_img "$TEST_DIR/t.$IMGFMT.copy" Aha here it is. It should appear in a previous patch.. Also, it may be simply _rm_test_img "$TEST_IMG.copy" , like in 110. > } > trap "_cleanup; exit \$status" 0 1 2 3 15 > > @@ -139,6 +140,9 @@ $QEMU_IMG bitmap --add "$TEST_IMG" b4 > $QEMU_IMG bitmap --remove "$TEST_IMG" b1 > _img_info --format-specific | _filter_irrelevant_img_info > $QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" > +$QEMU_IMG convert --bitmaps --skip-broken -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" > +TEST_IMG="$TEST_IMG.copy" _img_info --format-specific \ > + | _filter_irrelevant_img_info We still can now remove remaining inconsistent bitmaps and retry convert without --skip-broken, just to cover more nearby cases. > > # 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 17b34eaed30f..685bde6d1d63 100644 > --- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out > +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out > @@ -145,4 +145,18 @@ Format specific information: > corrupt: false > qemu-img: Failed to populate bitmap b0: Bitmap 'b0' is inconsistent and cannot be used > Try block-dirty-bitmap-remove to delete this bitmap from disk > +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 > *** done > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps 2021-07-08 1:29 [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps Eric Blake 2021-07-08 1:30 ` [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap Eric Blake 2021-07-08 1:30 ` [PATCH 2/2] qemu-img: Add --skip-broken for 'convert --bitmaps' Eric Blake @ 2021-07-09 9:50 ` Vladimir Sementsov-Ogievskiy 2 siblings, 0 replies; 9+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-07-09 9:50 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: nsoffer, qemu-block 08.07.2021 04:29, Eric Blake wrote: > This is mostly a convenience factor as one could already use 'qemu-img > info' to learn which bitmaps are broken and then 'qemu-img bitmap > --remove' to nuke them before calling 'qemu-img convert --bitmaps', > but it does have the advantage that the copied file is usable without > extra efforts and the broken bitmap is not deleted from the source > file. > > Eric Blake (2): > iotests: Improve and rename test 291 to qemu-img-bitmap > qemu-img: Add --skip-broken for 'convert --bitmaps' > > docs/tools/qemu-img.rst | 8 +++- > block/dirty-bitmap.c | 2 +- > qemu-img.c | 20 +++++++- > .../{291 => tests/qemu-img-bitmaps} | 17 ++++++- > .../{291.out => tests/qemu-img-bitmaps.out} | 46 ++++++++++++++++++- > 5 files changed, 87 insertions(+), 6 deletions(-) > rename tests/qemu-iotests/{291 => tests/qemu-img-bitmaps} (88%) > rename tests/qemu-iotests/{291.out => tests/qemu-img-bitmaps.out} (76%) > I've applied this onto your nbd branch. This doesn't apply on master. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-09 14:18 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-08 1:29 [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps Eric Blake 2021-07-08 1:30 ` [PATCH 1/2] iotests: Improve and rename test 291 to qemu-img-bitmap Eric Blake 2021-07-09 6:33 ` Vladimir Sementsov-Ogievskiy 2021-07-09 13:16 ` Eric Blake 2021-07-09 13:49 ` Vladimir Sementsov-Ogievskiy 2021-07-09 14:17 ` Eric Blake 2021-07-08 1:30 ` [PATCH 2/2] qemu-img: Add --skip-broken for 'convert --bitmaps' Eric Blake 2021-07-09 6:54 ` Vladimir Sementsov-Ogievskiy 2021-07-09 9:50 ` [PATCH 0/2] Let 'qemu-img convert --bitmaps' skip inconsistent bitmaps Vladimir Sementsov-Ogievskiy
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.