From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42287) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REfym-0002KS-Dz for qemu-devel@nongnu.org; Fri, 14 Oct 2011 07:31:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1REfyk-0003nS-OJ for qemu-devel@nongnu.org; Fri, 14 Oct 2011 07:31:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52745) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REfyk-0003n7-H3 for qemu-devel@nongnu.org; Fri, 14 Oct 2011 07:31:06 -0400 Message-ID: <4E981D6D.1070407@redhat.com> Date: Fri, 14 Oct 2011 13:30:53 +0200 From: Paolo Bonzini 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> In-Reply-To: <4E98183E.5040303@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed 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: Kevin Wolf Cc: qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com 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. 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. 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. 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. Paolo