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

[Vladimir Sementsov-Ogievskiy]

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