| Message ID | 1465917916-22348-4-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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.
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.
On 06/15/2016 02:41 AM, Denis V. Lunev wrote: > 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. >>> >>> 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 > [meta-comment - your reply style is a bit hard to read. It's best to include a blank line both before and after any text you write when replying in context, as the extra spacing calls visual attention to your reply rather than being part of a wall of text] >> 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. > Okay, so the real slowdown in the loop was the rest of the code (checking backing status in bdrv_is_allocated_above() and not in actually writing to the bitmap (bdrv_set_dirty_bitmap()).
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); + return 0; + } + last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME); /* First part, loop on the sectors and initialize the dirty bitmap. */ @@ -537,7 +541,7 @@ static int mirror_dirty_init(MirrorBlockJob *s) } assert(n > 0); - if (ret == 1 || mark_all_dirty) { + if (ret == 1) { bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n); } sector_num += n;