05.12.2019 21:13, Eric Blake wrote: > [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
these problems will hit if some filters are in use, like throttling, copy-on-read, etc. They use file child, which breaks backing chains. But normal backing chains should work well. > > 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. > -- Best regards, Vladimir