Re: [Qemu-devel] [PATCH 3/5] backup: init copy_bitmap from sync_bitmap for incremental

[John Snow <jsnow@redhat.com>]

On 10/12/2017 07:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> 10.10.2017 01:56, John Snow wrote:
>>
>> On 10/02/2017 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> We should not copy non-dirty clusters in write notifiers. So,
>>> initialize copy_bitmap from sync_bitmap.
>>>
>> ...! Duh, good find!!
>>
>> So, previously we'd copy extra sectors if they just so happened to be
>> written to during the backup process. These would be totally redundant
>> compared to the previous backups in the chain, by definition.
>>
>> Now, we allow the write to go through to the source image and let it
>> dirty the bdrv dirty bitmap we have on standby.
>>
>> Good fix.
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/backup.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 44 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 08efec639f..07f4ae846b 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -422,6 +422,43 @@ out:
>>>       return ret;
>>>   }
>>>   +/* init copy_bitmap from sync_bitmap */
>>> +static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
>>> +{
>>> +    BdrvDirtyBitmapIter *dbi;
>>> +    int64_t offset;
>>> +    int64_t end =
>>> DIV_ROUND_UP(bdrv_dirty_bitmap_size(job->sync_bitmap),
>>> +                               job->cluster_size);
>>> +
>>> +    dbi = bdrv_dirty_iter_new(job->sync_bitmap);
>>> +    while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
>>> +        int64_t cluster = offset / job->cluster_size;
>>> +        int64_t next_cluster;
>>> +
>>> +        offset += bdrv_dirty_bitmap_granularity(job->sync_bitmap);
>>> +        if (offset >= bdrv_dirty_bitmap_size(job->sync_bitmap)) {
>>> +            hbitmap_set(job->copy_bitmap, cluster, end - cluster);
>>> +            break;
>>> +        }
>>> +
>>> +        offset = bdrv_dirty_bitmap_next_zero(job->sync_bitmap, offset);
>>> +        if (offset == -1) {
>>> +            hbitmap_set(job->copy_bitmap, cluster, end - cluster);
>>> +            break;
>>> +        }
>>> +
>>> +        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
>>> +        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
>>> +        if (next_cluster >= end) {
>>> +            break;
>>> +        }
>>> +
>> Did you look into initializing this from a lower layer, e.g. a call to
>> initialize-an-hbitmap-from-another-hbitmap?
>>
>> The loop might look a little cleaner that way and we might possibly get
>> more utility out of it than a custom coded loop here in the backup code.
> 
> No, to be honest, I don't want to do it now, there are some difficulties:
> 
> 1. we init from bdrv bitmap, so we'll have to add a bit strange common
> interface init-hbitmap-from-bdrv-bitmap
> 2 (more hard). here copy_bitmap have other size: it's granularity is
> zero, it corresponds to clusters, not to bytes, and I don't want to
> change this.
> so, it's not a common case..
> 

Fair enough, it just struck me as a slightly odd thing to code here in
the backup file, but it's true that nobody else needs this yet (or
perhaps ever.)

No problem. We'll figure it out when we get there. Thank you!

--js