From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MvurA-0006aA-Bl for qemu-devel@nongnu.org; Thu, 08 Oct 2009 11:24:40 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mvur5-0006St-LL for qemu-devel@nongnu.org; Thu, 08 Oct 2009 11:24:39 -0400 Received: from [199.232.76.173] (port=57446 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mvur5-0006Sg-Et for qemu-devel@nongnu.org; Thu, 08 Oct 2009 11:24:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63833) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mvur5-0003YP-0s for qemu-devel@nongnu.org; Thu, 08 Oct 2009 11:24:35 -0400 Message-ID: <4ACE03EC.3080803@redhat.com> Date: Thu, 08 Oct 2009 17:23:24 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] qcow2: Bring synchronous read/write back to life References: <1255006928-7600-1-git-send-email-kwolf@redhat.com> <4ACDF797.4010805@codemonkey.ws> <4ACDFB64.1040106@redhat.com> <4ACDFEB9.6090403@codemonkey.ws> In-Reply-To: <4ACDFEB9.6090403@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org Am 08.10.2009 17:01, schrieb Anthony Liguori: > Kevin Wolf wrote: >> Am 08.10.2009 16:30, schrieb Anthony Liguori: >> >>> Kevin Wolf wrote: >>> >>>> When the synchronous read and write functions were dropped, they were replaced >>>> by generic emulation functions. Unfortunately, these emulation functions don't >>>> provide the same semantics as the original functions did. >>>> >>>> The original bdrv_read would mean that we read some data synchronously and that >>>> we won't be interrupted during this read. The latter assumption is no longer >>>> true with the emulation function which needs to use qemu_aio_poll and therefore >>>> allows the callback of any other concurrent AIO request to be run during the >>>> read. >>>> >>> Perhaps you could create a mechanism to freeze the qcow2 image by >>> queuing all completions within qcow2 until the image was unfrozen. This >>> would have the same effect switching to synchronous read/write. >>> >>> You may also have to queue new read/write requests... >>> >>> Introducing sync read/write seems like a major step backwards to me. >>> >> >> Right, I was expecting your reaction. ;-) I do even agree that it's not >> nice to have the synchronous functions back. But removing them caused a >> regression, so the removal should be reverted until it is done right. >> >> I just want to make clear that we're talking about data corruption here. >> This is not just something that we can care about when we are bored some >> time in the future. >> > > Yeah, okay. Can we do a more direct revert though so that it's clearer > in the commit log? Hm, it's not a single commit to revert. The main part is that we need qcow_write back which was removed in ade40677. The removal of the synchronous functions from the BlockDriver must have happened longer ago. And then I needed to put one or two changes on top to make it work with the changes since it was dropped. >> By the way, I don't think queuing things in qcow2 is the right thing to >> do. It probably wouldn't even help if, say, VMDK implemented AIO and I >> used a VMDK image as a backing file for qcow2. The real solution is to >> fix the generic bdrv_read/write emulation. Now we just need to find the >> best way to do this. > > Ideally, you could say qemu_aio_wait(aiocb) and it would only wait for > that particular request. I'm not convinced though that there aren't > dependency issues though since we can generate synthetic aiocbs. You can't only wait for this single request but you also need to allow waiting for all requests that were submitted while processing the request you're waiting for. I think you need to create something like a new context for all requests that are newly started. Kevin