From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ve2pY-00052O-Kb for qemu-devel@nongnu.org; Wed, 06 Nov 2013 08:07:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ve2pS-0005ky-GX for qemu-devel@nongnu.org; Wed, 06 Nov 2013 08:07:32 -0500 Received: from mail6.webfaction.com ([74.55.86.74]:50043 helo=smtp.webfaction.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ve2pS-0005kM-9M for qemu-devel@nongnu.org; Wed, 06 Nov 2013 08:07:26 -0500 Message-ID: <527A3F0C.9050708@ctshepherd.com> Date: Wed, 06 Nov 2013 13:07:24 +0000 From: Charlie Shepherd MIME-Version: 1.0 References: <1383742617-14742-1-git-send-email-charlie@ctshepherd.com> <1383742617-14742-4-git-send-email-charlie@ctshepherd.com> <527A3E27.3050705@redhat.com> In-Reply-To: <527A3E27.3050705@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/3] COW: Skip setting already set bits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, stefanha@gmail.com, gabriel@kerneis.info, qemu-devel@nongnu.org On 06/11/2013 13:03, Paolo Bonzini wrote: > Il 06/11/2013 13:56, Charlie Shepherd ha scritto: >> Rather than unnecessarily setting bits that are already set, re-use cow_find_streak to find how >> many bits are already set for this sector, and only set unset bits. Do this before the flush to >> avoid it if no bits need to be set at all. >> >> Signed-off-by: Charlie Shepherd >> --- >> block/cow.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/block/cow.c b/block/cow.c >> index 41097d8..93207eb 100644 >> --- a/block/cow.c >> +++ b/block/cow.c >> @@ -203,7 +203,7 @@ static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num, >> bool first = true; >> >> while (nb_sectors) { > You still need to make this a "for" and put "offset += BDRV_SECTOR_SIZE" > in the third clause. Ah true, I thought I could get away without doing that because it looked a bit ugly but I'll update it to use a for. >> - int ret; >> + int ret, set; >> uint8_t bitmap[BDRV_SECTOR_SIZE]; >> >> bitnum &= BITS_PER_BITMAP_SECTOR - 1; >> @@ -214,6 +214,15 @@ static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num, >> return ret; >> } >> >> + /* Skip over any already set bits */ >> + set = cow_find_streak(bitmap, 1, bitnum, sector_bits); >> + bitnum += set; >> + sector_bits -= set; >> + nb_sectors -= set; >> + if (set == sector_bits) { >> + continue; >> + } > This now has to be "if (set == 0)". With the change to the "for" above, > that's correct. > >> if (first) { >> ret = bdrv_flush(bs->file); >> if (ret < 0) { >> @@ -228,7 +237,6 @@ static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num, >> if (ret < 0) { >> return ret; >> } >> - >> bitnum += sector_bits; >> nb_sectors -= sector_bits; >> offset += BDRV_SECTOR_SIZE; >> > I also noticed that patch 1 introduces a small regression, in that it > will always flush data even if the metadata doesn't change. This patch > fixes it, because the "bdrv_flush" is preceded by the check to skip over > any already set bits. > > So I suggest that, together with the above fixes, you squash patches 1 > and 3 together. Ah that's true, I'll do that. Charlie