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?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
