Re: [Qemu-devel] [PATCH 3/9] mirror: optimize dirty bitmap filling in mirror_run a bit

["Denis V. Lunev" <den@openvz.org>]

On 06/15/2016 05:36 AM, Eric Blake wrote:
> On 06/14/2016 09:25 AM, Denis V. Lunev wrote:
>> There is no need to scan allocation tables if we have mark_all_dirty flag
>> set. Just mark it all dirty.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Fam Zheng <famz@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> ---
>>   block/mirror.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 797659d..c7b3639 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -507,12 +507,16 @@ static int mirror_dirty_init(MirrorBlockJob *s)
>>       BlockDriverState *base = s->base;
>>       BlockDriverState *bs = blk_bs(s->common.blk);
>>       BlockDriverState *target_bs = blk_bs(s->target);
>> -    bool mark_all_dirty = base == NULL && !bdrv_has_zero_init(target_bs);
>>       uint64_t last_pause_ns;
>>       int ret, n;
>>   
>>       end = s->bdev_length / BDRV_SECTOR_SIZE;
>>   
>> +    if (base == NULL && !bdrv_has_zero_init(target_bs)) {
>> +        bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
> Won't work as written.  'end' is 64 bits, but bdrv_set_dirty_bitmap() is
> limited to a 32-bit sector count.  Might be first worth updating
> bdrv_set_dirty_bitmap() and friends to be byte-based rather than
> sector-based (but still tracking a sector per bit, obviously), as well
> as expand it to operate on 64-bit sizes rather than 32-bit.
very nice catch! thank you

> I'm also worried slightly that the existing code repeated things in a
> loop, and therefore had pause points every iteration and could thus
> remain responsive to an early cancel.  But doing the entire operation in
> one chunk (assuming you fix bitmap code to handle a 64-bit size) may end
> up running for so long without interruption that you lose the benefits
> of an early interruption that you have by virtue of a 32-bit limit.
>
I do not think that this should be worried actually. We just perform memset
inside for not that big area (1 Tb disk will have 2 Mb dirty area 
bitmap) under
default parameters.