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

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