Re: [Qemu-block] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[Max Reitz]

On 20.06.19 20:44, John Snow wrote:
> 
> 
> On 6/20/19 1:00 PM, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> This adds an "always" policy for bitmap synchronization. Regardless of if
>>> the job succeeds or fails, the bitmap is *always* synchronized. This means
>>> that for backups that fail part-way through, the bitmap retains a record of
>>> which sectors need to be copied out to accomplish a new backup using the
>>> old, partial result.
>>>
>>> In effect, this allows us to "resume" a failed backup; however the new 
>>> backup
>>> will be from the new point in time, so it isn't a "resume" as much as it is
>>> an "incremental retry." This can be useful in the case of extremely large
>>> backups that fail considerably through the operation and we'd like to not 
>>> waste
>>> the work that was already performed.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  qapi/block-core.json |  5 ++++-
>>>  block/backup.c       | 10 ++++++----
>>>  2 files changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 0332dcaabc..58d267f1f5 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1143,6 +1143,9 @@
>>>  # An enumeration of possible behaviors for the synchronization of a bitmap
>>>  # when used for data copy operations.
>>>  #
>>> +# @always: The bitmap is always synchronized with remaining blocks to copy,
>>> +#          whether or not the operation has completed successfully or not.
>>> +#
>>>  # @conditional: The bitmap is only synchronized when the operation is 
>>> successul.
>>>  #               This is useful for Incremental semantics.
>>>  #
>>> @@ -1153,7 +1156,7 @@
>>>  # Since: 4.1
>>>  ##
>>>  { 'enum': 'BitmapSyncMode',
>>> -  'data': ['conditional', 'never'] }
>>> +  'data': ['always', 'conditional', 'never'] }
>>>  
>>>  ##
>>>  # @MirrorCopyMode:
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 627f724b68..beb2078696 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob 
>>> *job, int ret)
>>>      BlockDriverState *bs = blk_bs(job->common.blk);
>>>  
>>>      if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
>>> -        /* Failure, or we don't want to synchronize the bitmap.
>>> -         * Merge the successor back into the parent, delete nothing. */
>>> +        /* Failure, or we don't want to synchronize the bitmap. */
>>> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
>>> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
>>
>> Hmm...  OK, bitmaps in backup always confuse me, so bear with me, please.
>>
> 
> I realize this is an extremely dense section that actually covers a
> *lot* of pathways.
> 
>> (Hi, I’m a time traveler from the end of this section and I can tell you
>> that everything is fine.  I was just confused.  I’ll still keep this
>> here, because it was so much work.)
>>
>> The copy_bitmap is copied from the sync_bitmap at the beginning, so the
>> sync_bitmap can continue to be dirtied, but that won’t affect the job.
>> In normal incremental mode, this means that the sync point is always at
>> the beginning of the job.  (Well, naturally, because that’s how backup
>> is supposed to go.)
>>
> 
> sync_bitmap: This is used as an initial manifest for which sectors to
> copy out. It is the user-provided bitmap. We actually *never* edit this
> bitmap in the body of the job.
> 
> copy_bitmap: This is the manifest for which blocks remain to be copied
> out. We clear bits in this as we go, because we use it as our loop
> condition.
> 
> So what you say is actually only half-true: the sync_bitmap actually
> remains static during the duration of the job, and it has an anonymous
> child that accrues new writes. This is a holdover from before we had a
> copy_bitmap, and we used to use a sync_bitmap directly as our loop
> condition.
> 
> (This could be simplified upstream at present; but after this patch it
> cannot be for reasons explained below. We do wish to maintain three
> distinct sets of bits:
> 1. The bits at the start of the operation,
> 2. The bits accrued during the operation, and
> 3. The bits that remain to be, or were not, copied during the operation.)
> 
> So there's actually three bitmaps:
> 
> - sync_bitmap: actually just static and read-only
> - sync_bitmap's anonymous child: accrues new writes.

Ah, right...  Thanks for writing that up.

> - copy_bitmap: loop conditional.
> 
>> But then replacing the sync_bitmap with the copy_bitmap here means that
>> all of these dirtyings that happened during the job are lost.  Hmm, but
>> that doesn’t matter, does it?  Because whenever something was dirtied in
>> sync_bitmap, the corresponding area must have been copied to the backup
>> due to the job.
>>
> 
> The new dirty bits were accrued very secretly in the anonymous child.
> The new dirty bits are merged in via the reclaim() function.
> 
> So, what happens is:
> 
> - Sync_bitmap gets the bit pattern of copy_bitmap (one way or another)
> - Sync_bitmap reclaims (merges with) its anonymous child.
> 
>> Ah, yes, it would actually be wrong to keep the new dirty bits, because
>> in this mode, sync_bitmap should (on failure) reflect what is left to
>> copy to make the backup complete.  Copying these newly dirtied sectors
>> would be wrong.  (Yes, I know you wrote that in the documentation of
>> @always.  I just tried to get a different perspective.)
>>
>> Yes, yes, and copy_bitmap is always set whenever a CBW to the target
>> fails before the source can be updated.  Good, good.
>>
> 
> You might have slightly the wrong idea; it's important to keep track of
> what was dirtied during the operation because that data is important for
> the next bitmap backup.
> 
> The merging of "sectors left to copy" (in the case of a failed backup)
> and "sectors dirtied since we started the operation" forms the actual
> minimal set needed to re-write to this target to achieve a new
> functioning point in time. This is what you get with the "always" mode
> in a failure case.
> 
> In a success case, it just so happens that "sectors left to copy" is the
> empty set.
> 
> It's like an incremental on top of the incremental.
> 
> Consider this:
> 
> We have a 4TB drive and we have dirtied 3TB of it since our full backup.
> We copy out 2TB as part of a new incremental backup before suffering
> some kind of failure.
> 
> Today, you'd need to start a new incremental backup that copies that
> entire 3TB *plus* whatever was dirtied since the job failed.
> 
> With this mode, you'd only need to copy the remaining 1TB + whatever was
> dirtied since.
> 
> So, what this logic is really doing is:
> 
> If we failed, OR if we want the "never" sync policy:
> 
> Merge the anonymous child (bits written during op) back into sync_bitmap
> (bits we were instructed to copy), leaving us as if we have never
> started this operation.
> 
> If, however, we failed and we have the "always" sync policy, we destroy
> the sync_bitmap (bits we were instructed to copy) and replace it with
> the copy_bitmap (bits remaining to copy). Then, we merge that with the
> anonymous child (bits written during op).

Oh, so that’s the way it works.  I thought “always” meant that you can
repeat the backup.  But it just means you keep your partial backup and
pretend it’s a full incremental one.

Now that I think about it again...  Yeah, you can’t repeat a backup at a
later point, of course.  If data is gone in the meantime, it’s gone.

So, uh, I was wrong that it’s all good, because it would have been
wrong?  But thankfully I was just wrong myself, and so it is all good
after all?  My confusion with bitmaps as lifted, now I’m just confused
with myself.

I revoke my R-b and give a new one:

Reviewed-by: Max Reitz <address@hidden>

Or something like that.

Again, thanks a lot for clarifying.

Max

> Or, in success cases (when sync policy is not never), we simply delete
> the sync_bitmap (bits we were instructed to copy) and replace it with
> its anonymous child (bits written during op).

signature.asc
Description: OpenPGP digital signature