On Tue, Sep 06, 2016 at 06:11:19PM +0300, Denis V. Lunev wrote: > bdrv_is_allocated_above() returns true in the case if qcow2 even for > completely zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both > cases.
Hi Denis, Not just qcow2. BDRV_BLOCK_ALLOCATED for a layer means the content of the block is defined by that layer (even if that is a zero block, that doesn't mean it is unallocated). This applies to all image formats. > Though we have completely zeroed out the image just above or it was > already zeroed. This is only true if we are in 'sync = full' mode. > The patch stops using bdrv_is_allocated_above() wrapper and switches to > bdrv_get_block_status_above() to distinguish zeroed areas and areas with > data to avoid extra IO operations, which could be VERY slow. For 'sync = top', we need to make sure to appropriately allocate the sector, even if it is BDRV_BLOCK_ZERO. > Signed-off-by: Denis V. Lunev <d...@openvz.org> > CC: Jeff Cody <jc...@redhat.com> > CC: Kevin Wolf <kw...@redhat.com> > CC: Max Reitz <mre...@redhat.com> > --- > block/mirror.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index e0b3f41..87edbd8 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -552,7 +552,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob > *s) > BlockDriverState *base = s->base; > BlockDriverState *bs = blk_bs(s->common.blk); > BlockDriverState *target_bs = blk_bs(s->target); > - int ret, n; > + int n; > > end = s->bdev_length / BDRV_SECTOR_SIZE; > > @@ -590,6 +590,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob > *s) > /* Just to make sure we are not exceeding int limit. */ > int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS, > end - sector_num); > + int64_t status; > + BlockDriverState *file; > > mirror_throttle(s); > > @@ -597,13 +599,14 @@ static int coroutine_fn > mirror_dirty_init(MirrorBlockJob *s) > return 0; > } > > - ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n); > - if (ret < 0) { > - return ret; > + status = bdrv_get_block_status_above(bs, base, sector_num, > + nb_sectors, &n, &file); > + if (status < 0) { > + return status; > } > > assert(n > 0); > - if (ret == 1) { > + if (status & BDRV_BLOCK_DATA) { I think this patch would work if this was changed to something like this: mark_dirty = base == NULL ? status & BDRV_BLOCK_DATA : /* 'full' */ status & BDRV_BLOCK_ALLOCATED; if (mark_dirty) { Thanks, Jeff > bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n); > } > sector_num += n; > -- > 2.7.4 >