From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49802) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RIHRy-00054r-Cx for qemu-devel@nongnu.org; Mon, 24 Oct 2011 06:08:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RIHRx-0006hb-Br for qemu-devel@nongnu.org; Mon, 24 Oct 2011 06:08:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32387) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RIHRw-0006hP-Vr for qemu-devel@nongnu.org; Mon, 24 Oct 2011 06:08:09 -0400 Message-ID: <4EA53905.8000804@redhat.com> Date: Mon, 24 Oct 2011 12:08:05 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1319216912-26964-1-git-send-email-kwolf@redhat.com> <4EA1BD95.8030205@redhat.com> <4EA515B9.8070204@redhat.com> <4EA51963.5060904@redhat.com> <4EA51F16.3030700@redhat.com> <4EA5262D.7090901@redhat.com> <4EA527DC.90902@redhat.com> <4EA52F2E.4000302@redhat.com> <4EA53184.8080201@redhat.com> <4EA5329C.3000003@redhat.com> <4EA535AB.3010805@redhat.com> In-Reply-To: <4EA535AB.3010805@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/2] block: Write out internal caches even with cache=unsafe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: avi@redhat.com, Alexander Graf , qemu-devel@nongnu.org On 10/24/2011 11:53 AM, Kevin Wolf wrote: >> > >> > I'm talking about the internal driver API only. The external API is >> > fine as is. > Ok, so external callers don't force us to do it. > > Yes, we could split bdrv_flush internally into two functions for "flush > one level to the OS" and "flush all the way down to the disk", but I'm > not sure if this really buys us anything or just adds complexity. I would say that: 1) the "safe" NBD and iSCSI drivers are not in-tree, but you would have to convert those too. iSCSI uses aio, so it would be non-trivial. 2) long-term you wanted to convert raw-posix to coroutines, but in the meanwhile your patch introduces yet another partial transition; 3) your two patches are more complex compared to something like this: diff --git a/block.c b/block.c index 11c7f91..1e06a8a 100644 --- a/block.c +++ b/block.c @@ -2908,9 +2908,19 @@ static void coroutine_fn bdrv_flush_co_entry(void *opaque) int coroutine_fn bdrv_co_flush(BlockDriverState *bs) { - if (bs->open_flags & BDRV_O_NO_FLUSH) { + if (!bs->drv) { return 0; - } else if (!bs->drv) { + } + + /* First send everything to the OS. */ + if (bs->drv->bdrv_co_flush_metadata) { + int ret = bs->drv->bdrv_co_flush_metadata(bs); + if (ret < 0) { + return ret; + } + } + + /* Now flush the host cache to disk if we need to. */ + if (bs->open_flags & BDRV_O_NO_FLUSH) { return 0; } else if (bs->drv->bdrv_co_flush) { return bs->drv->bdrv_co_flush(bs); diff --git a/block/qcow2.c b/block/qcow2.c index a181932..cd13452 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1105,7 +1105,7 @@ fail: return ret; } -static int qcow2_co_flush(BlockDriverState *bs) +static int qcow2_co_flush_metadata(BlockDriverState *bs) { BDRVQcowState *s = bs->opaque; int ret; @@ -1121,7 +1121,11 @@ static int qcow2_co_flush(BlockDriverState *bs) return ret; } qemu_co_mutex_unlock(&s->lock); + return 0; +} +static int qcow2_co_flush(BlockDriverState *bs) +{ return bdrv_co_flush(bs->file); } @@ -1244,6 +1248,7 @@ static BlockDriver bdrv_qcow2 = { .bdrv_co_readv = qcow2_co_readv, .bdrv_co_writev = qcow2_co_writev, .bdrv_co_flush = qcow2_co_flush, + .bdrv_co_flush_metadata = qcow2_co_flush_metadata, .bdrv_co_discard = qcow2_co_discard, .bdrv_truncate = qcow2_truncate, diff --git a/block_int.h b/block_int.h index dac00f5..ab4ceea 100644 --- a/block_int.h +++ b/block_int.h @@ -84,6 +84,7 @@ struct BlockDriver { int coroutine_fn (*bdrv_co_writev)(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov); int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs); + int coroutine_fn (*bdrv_co_flush_metadata)(BlockDriverState *bs); int coroutine_fn (*bdrv_co_discard)(BlockDriverState *bs, int64_t sector_num, int nb_sectors); Paolo