From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44289) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bI08p-0000VH-N7 for qemu-devel@nongnu.org; Tue, 28 Jun 2016 17:02:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bI08k-0007cv-OX for qemu-devel@nongnu.org; Tue, 28 Jun 2016 17:01:54 -0400 Sender: Paolo Bonzini References: <1466780802-30424-1-git-send-email-den@openvz.org> <1466780802-30424-2-git-send-email-den@openvz.org> From: Paolo Bonzini Message-ID: Date: Tue, 28 Jun 2016 23:01:40 +0200 MIME-Version: 1.0 In-Reply-To: <1466780802-30424-2-git-send-email-den@openvz.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] block: ignore flush requests when storage is clean List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , Evgeny Yakovlev , Max Reitz , Stefan Hajnoczi , John Snow On 24/06/2016 17:06, Denis V. Lunev wrote: > From: Evgeny Yakovlev > > Some guests (win2008 server for example) do a lot of unnecessary > flushing when underlying media has not changed. This adds additional > overhead on host when calling fsync/fdatasync. > > This change introduces a dirty flag in BlockDriverState which is set > in bdrv_set_dirty and is checked in bdrv_co_flush. This allows us to > avoid unnesessary flushing when storage is clean. > > The problem with excessive flushing was found by a performance test > which does parallel directory tree creation (from 2 processes). > Results improved from 0.424 loops/sec to 0.432 loops/sec. > Each loop creates 10^3 directories with 10 files in each. > > Signed-off-by: Evgeny Yakovlev > Signed-off-by: Denis V. Lunev > CC: Kevin Wolf > CC: Max Reitz > CC: Stefan Hajnoczi > CC: Fam Zheng > CC: John Snow > --- > block.c | 1 + > block/dirty-bitmap.c | 3 +++ > block/io.c | 19 +++++++++++++++++++ > include/block/block_int.h | 2 ++ > 4 files changed, 25 insertions(+) > > diff --git a/block.c b/block.c > index f4648e9..e36f148 100644 > --- a/block.c > +++ b/block.c > @@ -2582,6 +2582,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset) > ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); > bdrv_dirty_bitmap_truncate(bs); > bdrv_parent_cb_resize(bs); > + bs->dirty = true; /* file node sync is needed after truncate */ > } > return ret; > } > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 4902ca5..54e0413 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -370,6 +370,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > } > hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > } > + > + /* Set global block driver dirty flag even if bitmap is disabled */ > + bs->dirty = true; > } > > /** > diff --git a/block/io.c b/block/io.c > index 7cf3645..8078af2 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2239,6 +2239,25 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs) > goto flush_parent; > } > > + /* Check if storage is actually dirty before flushing to disk */ > + if (!bs->dirty) { > + /* Flush requests are appended to tracked request list in order so that > + * most recent request is at the head of the list. Following code uses > + * this ordering to wait for the most recent flush request to complete > + * to ensure that requests return in order */ > + BdrvTrackedRequest *prev_req; > + QLIST_FOREACH(prev_req, &bs->tracked_requests, list) { > + if (prev_req == &req || prev_req->type != BDRV_TRACKED_FLUSH) { > + continue; > + } > + > + qemu_co_queue_wait(&prev_req->wait_queue); > + break; > + } > + goto flush_parent; Can you just have a CoQueue specific to flushes, where a completing flush does a restart_all on the CoQueue? Flushes are never serialising, so there's no reason for them to be in tracked_requests (I posted patches a while ago that instead use a simple atomic counter, but they will only be in 2.8). Paolo > + } > + bs->dirty = false; > + > BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK); > if (bs->drv->bdrv_co_flush_to_disk) { > ret = bs->drv->bdrv_co_flush_to_disk(bs); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 2057156..616058b 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -418,6 +418,8 @@ struct BlockDriverState { > int sg; /* if true, the device is a /dev/sg* */ > int copy_on_read; /* if true, copy read backing sectors into image > note this is a reference count */ > + > + bool dirty; > bool probed; > > BlockDriver *drv; /* NULL means no media */ >