| Message ID | 1473174679-18413-1-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <den@openvz.org> > CC: Jeff Cody <jcody@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@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 >
On 09/07/2016 09:15 AM, Jeff Cody wrote: > 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 <den@openvz.org> >> CC: Jeff Cody <jcody@redhat.com> >> CC: Kevin Wolf <kwolf@redhat.com> >> CC: Max Reitz <mreitz@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) { reasonable. I have doubts with the code but was unable to formulate. Thank you for clarifications. Will respin. Den
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) { bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n); } sector_num += n;
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. Though we have completely zeroed out the image just above or it was already zeroed. 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. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Jeff Cody <jcody@redhat.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Max Reitz <mreitz@redhat.com> --- block/mirror.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)