Re: [libvirt] [PATCH v2 20/25] qemu: Implement backup job APIs and qemu handling

[Eric Blake]

[adding some qemu visibility]

On 12/5/19 7:34 AM, Peter Krempa wrote:

> > > +    if (!(mergebitmapsstore = virJSONValueCopy(mergebitmaps)))
> > > +        return -1;
> > > +
> > > +    if (qemuMonitorTransactionBitmapAdd(actions,
> > > +                                        dd->domdisk->src->nodeformat,
> > > +                                        dd->incrementalBitmap,
> > > +                                        false,
> > > +                                        true) < 0)
> > > +        return -1;
> > > +
> > > +    if (qemuMonitorTransactionBitmapMerge(actions,
> > > +                                          dd->domdisk->src->nodeformat,
> > > +                                          dd->incrementalBitmap,
> > > +                                          &mergebitmaps) < 0)
> > > +        return -1;
> > > +
> > > +    if (qemuMonitorTransactionBitmapAdd(actions,
> > > +                                        dd->store->nodeformat,
> > > +                                        dd->incrementalBitmap,
> > > +                                        false,
> > > +                                        true) < 0)
> > > +        return -1;
> >
> > Why do we need two of these calls?
> > /me reads on
> >
> > > +
> > > +    if (qemuMonitorTransactionBitmapMerge(actions,
> > > +                                          dd->store->nodeformat,
> > > +                                          dd->incrementalBitmap,
> > > +                                          &mergebitmapsstore) < 0)
> > > +        return -1;
> >
> > okay, it looks like you are trying to merge the same bitmap into two
> > different merge commands, which will all be part of the same transaction.  I
> > guess it would be helpful to see a trace of the transaction in action to see
> > if we can simplify it (using 2 instead of 4 qemuMonitor* commands).
>
> This is required because the backup blockjob requires the bitmap to be
> present on the source ('device') image of the backup. The same also
> applies for the image exported by NBD. The catch is that we expose the
> scratch file via NBD which is actually the target image of the backup.
> This means that in case of a pull backup we need two instances of the
> bitmap so both the block job and the NBD server can use them. Arguably
> there's a possible simplification here for push-mode backups where the
> target bitmap doesn't make sense.

The backup job requires the bitmap to be on the source, but the qemu NBD 
export code only requires the bitmap to be locatable somewhere on the 
qemu NBD server requires the bitmap to be discoverable from anywhere on 
the chain, and since the temporary target of the block job has the 
source image as its backing file, that should be the case.  That is:

base <- active <- temp
          |
        bitmap0

looking up [active, bitmap0] or [temp, bitmap0] should both succeed; we 
need the former for the blockjob, and the latter for the NBD export.

If the NBD export _can't_ find bitmap0 through the backing chain, that 
may be a symptom of the problem that Max has been trying to fix (his 
upcoming v7 "deal with filters" is hinted at here, but will not be in 4.2):
https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg04520.html

In my original implementation, I did not need a duplicated bitmap in the 
temp file.  But that was pre-blockdev.  Are we really hitting filter 
limitations in qemu where the use of blockdev is preventing [temp, 
bitmap0] from finding the bitmap across the backing chain?  If so, we 
should fix that in qemu - but we're so late for 4.2, that I guess 
libvirt will have to work around it.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org