From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46174) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bD6O6-0005j7-UZ for qemu-devel@nongnu.org; Wed, 15 Jun 2016 04:41:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bD6O5-0003DI-KT for qemu-devel@nongnu.org; Wed, 15 Jun 2016 04:41:26 -0400 References: <1465917916-22348-1-git-send-email-den@openvz.org> <1465917916-22348-4-git-send-email-den@openvz.org> <5760BF45.9020707@redhat.com> From: "Denis V. Lunev" Message-ID: <576114A7.4000403@openvz.org> Date: Wed, 15 Jun 2016 11:41:11 +0300 MIME-Version: 1.0 In-Reply-To: <5760BF45.9020707@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/9] mirror: optimize dirty bitmap filling in mirror_run a bit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: vsementsov@virtuozzo.com, Stefan Hajnoczi , Fam Zheng , Kevin Wolf , Max Reitz , Jeff Cody 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 >> Reviewed-by: Vladimir Sementsov-Ogievskiy >> CC: Stefan Hajnoczi >> CC: Fam Zheng >> CC: Kevin Wolf >> CC: Max Reitz >> CC: Jeff Cody >> CC: Eric Blake >> --- >> 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.