From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41420) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ve5o2-0006Sl-RI for qemu-devel@nongnu.org; Wed, 06 Nov 2013 11:18:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ve5nx-00074k-QO for qemu-devel@nongnu.org; Wed, 06 Nov 2013 11:18:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13817) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ve5nx-00074e-IO for qemu-devel@nongnu.org; Wed, 06 Nov 2013 11:18:05 -0500 Message-ID: <527A6BAC.3040008@redhat.com> Date: Wed, 06 Nov 2013 17:17:48 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1383753568-15844-1-git-send-email-charlie@ctshepherd.com> <1383753568-15844-2-git-send-email-charlie@ctshepherd.com> In-Reply-To: <1383753568-15844-2-git-send-email-charlie@ctshepherd.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/2] COW: Speed up writes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Charlie Shepherd Cc: kwolf@redhat.com, stefanha@gmail.com, gabriel@kerneis.info, qemu-devel@nongnu.org Il 06/11/2013 16:59, Charlie Shepherd ha scritto: > Process a whole sector's worth of COW bits by reading a sector, setting the bits after skipping > any already set bits, then writing it out again. Make sure we only flush once before writing > metadata, and only if we need to write metadata. > > Signed-off-by: Charlie Shepherd > --- > block/cow.c | 87 ++++++++++++++++++++++++++++++++++--------------------------- > 1 file changed, 49 insertions(+), 38 deletions(-) > > diff --git a/block/cow.c b/block/cow.c > index 909c3e7..bf447fd 100644 > --- a/block/cow.c > +++ b/block/cow.c > @@ -103,40 +103,18 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, > return ret; > } > > -/* > - * XXX(hch): right now these functions are extremely inefficient. > - * We should just read the whole bitmap we'll need in one go instead. > - */ > -static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum, bool *first) > +static inline void cow_set_bits(uint8_t *bitmap, int start, int64_t nb_sectors) > { > - uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8; > - uint8_t bitmap; > - int ret; > - > - ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap)); > - if (ret < 0) { > - return ret; > - } > - > - if (bitmap & (1 << (bitnum % 8))) { > - return 0; > - } > - > - if (*first) { > - ret = bdrv_flush(bs->file); > - if (ret < 0) { > - return ret; > + int64_t bitnum = start, last = start + nb_sectors; > + while (bitnum < last) { > + if ((bitnum & 7) == 0 && bitnum + 8 <= last) { > + bitmap[bitnum / 8] = 0xFF; > + bitnum += 8; > + continue; > } > - *first = false; > + bitmap[bitnum/8] |= (1 << (bitnum % 8)); > + bitnum++; > } > - > - bitmap |= (1 << (bitnum % 8)); > - > - ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap)); > - if (ret < 0) { > - return ret; > - } > - return 0; > } > > #define BITS_PER_BITMAP_SECTOR (512 * 8) > @@ -204,18 +182,51 @@ static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs, > static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num, > int nb_sectors) > { > - int error = 0; > - int i; > + int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8; > + uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE; > bool first = true; > > - for (i = 0; i < nb_sectors; i++) { > - error = cow_set_bit(bs, sector_num + i, &first); > - if (error) { > - break; > + for ( ; nb_sectors; > + bitnum += sector_bits, > + nb_sectors -= sector_bits, > + offset += BDRV_SECTOR_SIZE) { > + int ret, set; > + uint8_t bitmap[BDRV_SECTOR_SIZE]; > + > + bitnum &= BITS_PER_BITMAP_SECTOR - 1; > + int sector_bits = MIN(nb_sectors, BITS_PER_BITMAP_SECTOR - bitnum); > + > + ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap)); > + if (ret < 0) { > + 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 (!sector_bits) { > + continue; > + } > + > + if (first) { > + ret = bdrv_flush(bs->file); > + if (ret < 0) { > + return ret; > + } > + first = false; > + } > + > + cow_set_bits(bitmap, bitnum, sector_bits); > + > + ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap)); > + if (ret < 0) { > + return ret; > } > } > > - return error; > + return 0; > } > > static int coroutine_fn cow_read(BlockDriverState *bs, int64_t sector_num, > Reviewed-by: Paolo Bonzini