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

[John Snow]

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?

> 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?

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

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.

--js