From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:39910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REiIc-0004oM-4m for qemu-devel@nongnu.org; Fri, 14 Oct 2011 09:59:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1REiIa-0005cu-NT for qemu-devel@nongnu.org; Fri, 14 Oct 2011 09:59:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12190) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1REiIa-0005cg-D3 for qemu-devel@nongnu.org; Fri, 14 Oct 2011 09:59:44 -0400 Message-ID: <4E984104.20905@redhat.com> Date: Fri, 14 Oct 2011 16:02:44 +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> <4E982302.1030100@redhat.com> <4E982E2D.1050009@redhat.com> In-Reply-To: <4E982E2D.1050009@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 14:42, schrieb Paolo Bonzini: > On 10/14/2011 01:54 PM, Kevin Wolf wrote: >> 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? >>> >> 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. > > It would still host the checks on BDRV_O_NO_FLUSH and bs->drv->*_flush. > It would be the same as bdrv_flush_co_entry is now, minus the > marshalling in/out of the RwCo. Right. By the way, I like how you handle all three backends in the same function. I think this is a lot more readable than the solution used by read/write (changing the function pointers on driver registration). >> 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. > > I see. Yeah, that's doable, but I'd still need two coroutines (one for > bdrv_flush, one for bdrv_aio_flush) and the patch would be bigger overall... You already have two of them (bdrv_co_flush for AIO and bdrv_flush_co_entry for synchronous), so I don't think it makes a difference there. >> 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. > > ... and on the other hand the length of the call chain would increse. > It easily gets confusing, it already is for me in the read/write case. Well, depends on what you're looking at. The call chain length would increase for AIO and synchronous bdrv_flush, but it would become shorter for bdrv_co_flush. If we want to declare coroutines as the preferred interface, I think such a change makes sense. > Would bdrv_co_flush be static or not? If not, you also get an > additional entry point of dubious additional value, i.e. more complexity. I think I would make it public. The one that has to go eventually is the synchronous bdrv_flush(). Which is another reason why I wouldn't design everything around it. >> 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). > > qemu-img convert might benefit from multiple in-flight requests if on of > the endpoints is remote or perhaps even sparse, I guess. Quite possible, yes. Kevin