Re: [PATCH v2 3/3] qemu-img: Add --skip-broken-bitmaps for 'convert --bitmaps'

[Eric Blake]

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