Re: [Qemu-block] [Qemu-devel] [PATCH RESEND v4] drive-mirror: add incremental mode

[Vladimir Sementsov-Ogievskiy]

On 13.03.2019 19:52, John Snow wrote:
> 
> 
> On 3/12/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.03.2019 18:58, John Snow wrote:
>>>
>>>
>>> On 3/12/19 9:43 AM, Eric Blake wrote:
>>>> On 3/12/19 3:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>
>>>>>> So one important point about incremental backups is that you can
>>>>>> actually do them incrementally: The bitmap is effectively cleared at the
>>>>>> beginning of the backup process (a successor bitmap is installed that is
>>>>>> cleared and receives all changes; at the end of the backup, it either
>>>>>> replaces the old bitmap (on success) or is merged into it (on failure)).
>>>>>>     Therefore, you can do the next incremental backup by using the same 
>>>>>> bitmap.
>>>>>
>>>>> Hmm, I heard in some other thread that Eric finally decided to always 
>>>>> start
>>>>> backup on copied bitmap in libvirt, so this logic is skipped. Do we need 
>>>>> it
>>>>> for mirror? Sorry if I'm wrong, Eric, am I?
>>>>
>>>> You are correct that my current libvirt patches do incremental backup
>>>> mode with a temporary bitmap (so regardless of whether the temporary
>>>> bitmap is cleared on success or left alone is irrelevant, the temporary
>>>> bitmap goes away afterwards). But just because libvirt isn't using that
>>>> particular feature of qemu's incremental backups does not excuse qemu
>>>> from not thinking about other clients that might be impacted by doing
>>>> incremental backup with a live bitmap, where qemu management of the
>>>> bitmap matters.
>>>>
>>>
>>> I'd prefer adding SYNC=DIFFERENTIAL to intuit that the bitmap isn't
>>> modified after use, if there's no reason to add an actual "incremental"
>>> mode to mirror.
>>>
>>> Then it would be fine for mirror to implement differential and not
>>> incremental, and still use the bitmap.
>>>
>>
>> Agree, this is better.
>>
>> But I have an idea: could we just add parameter @x-bitmap, independent of 
>> @sync ?
>>
> 
> It's a thought. Fam's original sketch for bitmaps included a separate
> parameter for how to clean up the bitmap at the end, but it was removed
> because we didn't have use cases for it at the time.
> 
>> In this case, we can get NONE mode reduced by bitmap, which may be usable 
>> too.
> 
> What do you have in mind for the use case?

I think about backup(sync=none) which lacks bitmap support for the idea 
described below. For mirror - I don't know) To imagine this, I need to 
know original reason for incremental-mirror and original reason for
none-mode-mirror :)

> 
>> And FULL mode with bitmap would be what you called DIFFERENTIAL. Not sure 
>> about
>> could TOP mode with bitmap be meaningful.
>>
>>
>> Also it may be useful at some point to share bitmap between jobs, so we could
>>
>> start backup(sync=none) from active disk to local temp node, and then mirror 
>> from temp
>> to target. Which gives backup which (1) don't slow down guest writes if 
>> target is far and slow
>> and (2) fast as it is mostly mirror, keeping in mind that mirror is faster 
>> than backup as it uses
>> async pattern and block_status.
>>
> 
> In effect, a disk-backed buffer so we don't lose performance on slow
> network links, right?

Yes

> 
>> And to make such a backup incremental we need to share dirty bitmap between 
>> backup and mirror,
>> and this bitmap should be cleared when something is copied out from source 
>> (it may be cleared
>> both by backup(sync=none) and mirror)... But I'm not sure that shared bitmap 
>> should be the same
>> as bitmap which initializes differential/incremental mode. And this is why I 
>> propose new parameter
>> to be experimental.
>>
> 
> This could be very cool; how does this vision fit in with the vision for
> a unified block-copy job? (i.e. unifying mirror, stream, backup and commit.)

My current plan is to finish backup top filter first. Then may be 
implement disk-backed buffer inside backup, to don't start two jobs. And 
then make backup as fast as mirror. Or, may be it would be backup-top 
filter, who will do most of work, so it should be fast too..

I often think about unifying jobs code, but when I try to dig in, it 
becomes too difficult. I think, we should start from separating common 
code to some kind of library, separate file. And first thing to separate
seems to be an async copying loop backed by coroutines and 
block_status.. To share it between all jobs and qemu-img convert. But 
even here, there are differences, for example, mirror is the only job
which needs several iterations of the loop. Backup needs to share
block_status results with backup_top filter, to make it useful for
copy-before-write operations.. Mirror needs block_status of active disk
and backup actually need block_status of point-in-time..

And mirror code is tooo difficult, it's a problem too;)

But there must be some generic part which we may separate to start from.
And the idea is to start from unifying code-path, not interface.

> 
> Worth thinking about, because we also want the ability to "resume"
> mirror jobs, too.
> 
> Regardless, I think the correct thing to do in the short term is to add
> a differential sync, since we already have sync modes that determine how
> to use the bitmap, and this would be part of a newer paradigm that we
> can introduce later.

No objections)