Re: [PULL 00/28] Block layer patches
On Fri, 9 Jul 2021 at 13:50, Kevin Wolf wrote: > > The following changes since commit 9db3065c62a983286d06c207f4981408cf42184d: > > Merge remote-tracking branch > 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging > (2021-07-08 16:30:18 +0100) > > are available in the Git repository at: > > git://repo.or.cz/qemu/kevin.git tags/for-upstream > > for you to fetch changes up to e60edf69e2f64e818466019313517a2e6d6b63f4: > > block: Make blockdev-reopen stable API (2021-07-09 13:19:11 +0200) > > > Block layer patches > > - Make blockdev-reopen stable > - Remove deprecated qemu-img backing file without format > - rbd: Convert to coroutines and add write zeroes support > - rbd: Updated MAINTAINERS > - export/fuse: Allow other users access to the export > - vhost-user: Fix backends without multiqueue support > - Fix drive-backup transaction endless drained section Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1 for any user-visible changes. -- PMM
Re: [PATCH v2 3/3] qemu-img: Add --skip-broken-bitmaps for 'convert --bitmaps'
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 --- 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
Re: [PATCH v2 2/3] qemu-img: Fail fast on convert --bitmaps with inconsistent bitmap
On 7/9/21 6:39 PM, Eric Blake wrote: Waiting until the end of the convert operation (a potentially time-consuming task) to finally detect that we can't copy a bitmap is bad, comparing to failing fast up front. Furthermore, this prevents us from leaving a file behind with a bitmap that is not marked as inconsistent even though it does not have sane contents. I don't think this is an issue since qemu-img terminate with non-zero exit code, and we cannot ensure that image is complete if we fail in the middle of the operation for all image formats and protocols. For files we could use a temporary file and rename after successful conversion for for raw format on block device we don't have any way to mark the contents as temporary. But failing fast is very important. This fixes the problems exposed in the previous patch to the iotest. Signed-off-by: Eric Blake --- qemu-img.c| 30 +-- tests/qemu-iotests/tests/qemu-img-bitmaps | 2 -- tests/qemu-iotests/tests/qemu-img-bitmaps.out | 20 ++--- 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 7956a8996512..e84b3c530155 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2101,6 +2101,30 @@ static int convert_do_copy(ImgConvertState *s) return s->ret; } +/* Check that bitmaps can be copied, or output an error */ +static int convert_check_bitmaps(BlockDriverState *src) +{ +BdrvDirtyBitmap *bm; + +if (!bdrv_supports_persistent_dirty_bitmap(src)) { +error_report("Source lacks bitmap support"); +return -1; +} +FOR_EACH_DIRTY_BITMAP(src, bm) { +const char *name; + +if (!bdrv_dirty_bitmap_get_persistence(bm)) { +continue; +} +name = bdrv_dirty_bitmap_name(bm); +if (bdrv_dirty_bitmap_inconsistent(bm)) { +error_report("Cannot copy inconsistent bitmap '%s'", name); We can add a useful hint: Try "qemu-img bitmap --remove" to delete this bitmap from disk. +return -1; +} +} +return 0; +} + static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) { BdrvDirtyBitmap *bm; @@ -2127,6 +2151,7 @@ static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst) ); if (err) { error_reportf_err(err, "Failed to populate bitmap %s: ", name); +qmp_block_dirty_bitmap_remove(dst->node_name, name, NULL); This may fail for the same reason populate failed (e.g. storage became inaccessibel in the middle of the copy). Since we fail the convert, I don't think it worth to try to do this kind of cleanup. If we have a way to disable the bitmap before merge, and enable it after successful merge it make more sense, since if the operation fails we are left with disabled bitmap. return -1; } } @@ -2552,9 +2577,8 @@ static int img_convert(int argc, char **argv) ret = -1; goto out; } -if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) { -error_report("Source lacks bitmap support"); -ret = -1; +ret = convert_check_bitmaps(blk_bs(s.src[0])); +if (ret < 0) { goto out; } } diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps b/tests/qemu-iotests/tests/qemu-img-bitmaps index 2f51651d0ce5..3fde95907515 100755 --- a/tests/qemu-iotests/tests/qemu-img-bitmaps +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps @@ -141,8 +141,6 @@ $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" && echo "unexpected success" -# Bug - even though we failed at conversion, we left a file around with -# a bitmap marked as not corrupt TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ | _filter_irrelevant_img_info diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out b/tests/qemu-iotests/tests/qemu-img-bitmaps.out index b762362075d1..546aaa404bba 100644 --- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out +++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out @@ -143,22 +143,6 @@ Format specific information: 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 -image: TEST_DIR/t.IMGFMT.copy -file format: IMGFMT -virtual size: 10 MiB (10485760 bytes) -cluster_size: 65536 -Format specific information: -bitmaps: -[0]: -flags: -name: b0 -granularity: 65536 -[1]: -flags: -[0]: auto -name: b4 -granularity: 65536 -corrupt: false +qemu-img: Cannot copy inconsistent bitmap 'b0' +qemu-img: Could not open
Re: [PATCH v2 1/3] iotests: Improve and rename test 291 to qemu-img-bitmap
On 7/9/21 6:39 PM, Eric Blake wrote: Enhance the test to demonstrate existing less-than-stellar behavior of qemu-img with a qcow2 image containing an inconsistent bitmap: we don't diagnose the problem until after copying the entire image (a potentially long time), and when we do diagnose the failure, we still end up leaving an empty bitmap in the destination. This mess will be cleaned up in the next patch. While at it, rename the test now that we support useful iotest names, and fix a missing newline in the error message thus exposed. Much nicer with a meaningful name! Signed-off-by: Eric Blake --- block/dirty-bitmap.c | 2 +- .../{291 => tests/qemu-img-bitmaps} | 19 +++- .../{291.out => tests/qemu-img-bitmaps.out} | 48 ++- 3 files changed, 66 insertions(+), 3 deletions(-) rename tests/qemu-iotests/{291 => tests/qemu-img-bitmaps} (88%) rename tests/qemu-iotests/{291.out => tests/qemu-img-bitmaps.out} (75%) 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 88% rename from tests/qemu-iotests/291 rename to tests/qemu-iotests/tests/qemu-img-bitmaps index 20efb080a6c0..2f51651d0ce5 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 @@ -27,11 +27,13 @@ status=1 # failure is the default! _cleanup() { _cleanup_test_img +_rm_test_img "$TEST_IMG.copy" nbd_server_stop } trap "_cleanup; exit \$status" 0 1 2 3 15 # get standard environment, filters and checks +cd .. . ./common.rc . ./common.filter . ./common.nbd @@ -129,6 +131,21 @@ $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" && +echo "unexpected success" +# Bug - even though we failed at conversion, we left a file around with +# a bitmap marked as not corrupt +TEST_IMG=$TEST_IMG.copy _img_info --format-specific \ +| _filter_irrelevant_img_info + # 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 75% rename from tests/qemu-iotests/291.out rename to tests/qemu-iotests/tests/qemu-img-bitmaps.out index 23411c0ff4d9..b762362075d1 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,50 @@ Format specific information: [{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET}, { "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false}, { "start": 3145728, "length": 7340032, "depth": 0, "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 In this context a more useful error message would be: Try "qemu-img bitmap --remove" ... but this is not a new issue. +image: TEST_DIR/t.IMGFMT.copy +file format: IMGFMT +virtual size: 10 MiB (10485760 bytes)