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

[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.