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

[Denis V. Lunev]

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 <address@hidden>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy<address@hidden>
> > CC: Stefan Hajnoczi <address@hidden>
> > CC: Fam Zheng <address@hidden>
> > CC: Kevin Wolf <address@hidden>
> > CC: Max Reitz <address@hidden>
> > CC: Jeff Cody <address@hidden>
> > CC: Eric Blake <address@hidden>
> > ---
> >   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.