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

[Nir Soffer]

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.