From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:36838) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REgIm-0000OX-Dk for qemu-devel@nongnu.org; Fri, 14 Oct 2011 07:51:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1REgIl-0000AR-8U for qemu-devel@nongnu.org; Fri, 14 Oct 2011 07:51:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60720) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REgIk-0000AK-TH for qemu-devel@nongnu.org; Fri, 14 Oct 2011 07:51:47 -0400 Message-ID: <4E982302.1030100@redhat.com> Date: Fri, 14 Oct 2011 13:54:42 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1318581692-18338-1-git-send-email-pbonzini@redhat.com> <1318581692-18338-3-git-send-email-pbonzini@redhat.com> <4E98183E.5040303@redhat.com> <4E981D6D.1070407@redhat.com> In-Reply-To: <4E981D6D.1070407@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/4] block: unify flush implementations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com Am 14.10.2011 13:30, schrieb Paolo Bonzini: > On 10/14/2011 01:08 PM, Kevin Wolf wrote: >> Am 14.10.2011 10:41, schrieb Paolo Bonzini: >>> Add coroutine support for flush and apply the same emulation that >>> we already do for read/write. bdrv_aio_flush is simplified to always >>> go through a coroutine. >>> >>> Signed-off-by: Paolo Bonzini >> >> To make the implementation more consistent with read/write operations, >> wouldn't it make sense to provide a bdrv_co_flush() globally instead of >> using the synchronous version as the preferred public interface? > > I thought about it, but then it turned out that I would have > > bdrv_flush > -> create coroutine or just fast-path to bdrv_flush_co_entry > -> bdrv_flush_co_entry > -> driver > > and > > bdrv_co_flush > -> bdrv_flush_co_entry > -> driver > > In other words, the code would be exactly the same, save for an "if > (qemu_in_coroutine())". The reason is that, unlike read/write, neither > flush nor discard take a qiov. What I was thinking of looks a bit different: bdrv_flush -> create coroutine or just fast-path to bdrv_flush_co_entry -> bdrv_flush_co_entry -> bdrv_co_flush and bdrv_co_flush -> driver And the reason for this is that bdrv_co_flush would be a function that does only little more than passing the function to the driver (just like most bdrv_* functions do), with no emulation going on at all. > In general, I think that with Stefan's cleanup having specialized > coroutine versions has in general a much smaller benefit. The code > reading benefit of naming routines like bdrv_co_* is already lost, for > example, since bdrv_read can yield when called for coroutine context. Instead of taking a void* and working on a RwCo structure that is really meant for emulation, bdrv_co_flush would take a BlockDriverState and improve readability this way. The more complicated and ugly code would be left separated and only used for emulation. I think that would make it easier to understand the common path without being distracted by emulation code. > Let me show how this might go. Right now you have > > bdrv_read/write > -> bdrv_rw_co > -> create qiov > -> create coroutine or just fast-path to bdrv_rw_co_entry > -> bdrv_rw_co_entry > -> bdrv_co_do_readv/writev > -> driver > > bdrv_co_readv/writev > -> bdrv_co_do_readv/writev > -> driver > > But starting from here, you might just as well reorganize it like this: > > bdrv_read/writev > -> bdrv_rw_co > -> create qiov > -> bdrv_readv/writev > > bdrv_readv/writev > -> create coroutine or just fast-path to bdrv_rw_co_entry > -> bdrv_rw_co_entry > -> bdrv_co_do_readv/writev > -> driver > > and just drop bdrv_co_readv, since it would just be hard-coding the > fast-path of bdrv_readv. I guess it's all a matter of taste. Stefan, what do you think? > Since some amount of synchronous I/O would likely always be there, for > example in qemu-img, I think this unification would make more sense than > providing two separate entrypoints for bdrv_co_flush and bdrv_flush. Actually, I'm not so sure about qemu-img. I think we have thought of scenarios where converting it to a coroutine based version with a main loop would be helpful (can't remember the details, though). Kevin