Re: [Qemu-devel] [PATCH 02/12] block/backup: Add mirror sync mode 'bitmap'

[John Snow <jsnow@redhat.com>]

On 6/21/19 7:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.06.2019 4:03, John Snow wrote:
>> We don't need or want a new sync mode for simple differences in
>> semantics.  Create a new mode simply named "BITMAP" that is designed to
>> make use of the new Bitmap Sync Mode field.
>>
>> Because the only bitmap mode is 'conditional', this adds no new
>> functionality to the backup job (yet). The old incremental backup mode
>> is maintained as a syntactic sugar for sync=bitmap, mode=conditional.
>>
>> Add all of the plumbing necessary to support this new instruction.
> 
> I don't follow, why you don't want to just add bitmap-mode optional parameter
> for incremental mode?
> 

Vocabulary reasons, see below.

> For this all looks similar to just two separate things:
> 1. add bitmap-mode parameter
> 2. rename incremental to bitmap

This is exactly correct!

> 
> Why do we need [2.] ?
> If we do only [1.], we'll avoid creating two similar modes, syntax sugar, a bit
> of mess as it seems to me..
> 
> Hmm, about differential backups, as I understood, we call 'differential' an incremental
> backup, but which considers difference not from latest incremental backup but from some
> in the past.. Is it incorrect?
> 

The reason is because I have been treating "INCREMENTAL" as meaning
something very specific -- I gather from you and Max that you don't
consider this term to mean something specific.

So, by other prominent backup vendors, they use these terms in this way:

INCREMENTAL: This backup contains a delta from the last INCREMENTAL
backup made. In effect, this creates a chain of backups that must be
squashed together to recover data, but uses less info on copy.

DIFFERENTIAL: This backup contains a delta from the last FULL backup
made. In effect, each differential backup only requires a base image and
a single differential. This usually wastes more data during the backup
process, but makes restoration processes easier.


I *always* use these terms in these *exact* ways; you can see that the
bitmap behavior we use is exactly what MIRROR_SYNC_MODE_INCREMENTAL
does. Even when we are using bitmap manipulation techniques to get it to
do something else, the block job itself is engineered to think that it
is producing an "Incremental" backup.


In the early days of this feature, Fam actually proposed something like
what I am proposing here:

a BITMAP sync mode with an on_complete parameter for the backup job that
would either roll the bitmap forward or not (like my "conditional",
"never") based on the success of the job.

We removed that because at the time we wanted to target a simpler
feature. As part of that removal, I renamed the mode "INCREMENTAL" under
the premise that if we ever wanted to add a "DIFFERENTIAL" mode like
what Fam's original design allowed for, we could add
MIRROR_SYNC_MODE_DIFFERENTIAL and that would differentiate the two
modes. This rename was done with the specific knowledge and intent that
the mode was named after the exact specific backup paradigm it was
enabling. Otherwise, I would have left it "BITMAP" back then.

I've had patches in my branch to add a DIFFERENTIAL mode ever since
then! However, since we added bitmap merging, you'll notice that we
actually CAN do "Differential" backups by playing around with the
bitmaps ourselves, which has largely stopped me from wanting to
introduce the new mode.

You'll recall that recently Xie Changlong sent patches to add
"incremental" support to mirror, but what they ACTUALLY implemented was
"Differential" mode -- they didn't clear the bitmap afterwards. I
actually responded as such on-list -- that if we implement a
"Differential" mode that their patches would have been appropriate for
that mode.

As a result of that discussion, I went to add a "Differential" mode to
mirror, but in the process realized that it's much easier to make the
bitmap sync behavior its own parameter.

However, because the new parameters no longer mean the backup is
"Incremental" by that definition, I decided to rename the mode "BITMAP"
again to be *less specific* and, perhaps now ironically, avoid confusion.

Even given this confusion ... I actually still think that we should NOT
use "Incremental" to mean something generic, and I will continue to
enforce the idea that "Incremental" should mean a series of
non-overlapping time-sliced deltas.

--js