From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60706) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2Fkh-0006zb-AX for qemu-devel@nongnu.org; Wed, 02 Jul 2014 04:18:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2FkZ-0007rT-Ow for qemu-devel@nongnu.org; Wed, 02 Jul 2014 04:18:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46529) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2FkZ-0007rN-Gy for qemu-devel@nongnu.org; Wed, 02 Jul 2014 04:18:43 -0400 Message-ID: <53B3C059.3060601@redhat.com> Date: Wed, 02 Jul 2014 10:18:33 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1404201119-6646-1-git-send-email-ming.lei@canonical.com> <1404201119-6646-2-git-send-email-ming.lei@canonical.com> <20140701111843.GE4587@noname.str.redhat.com> <20140701152130.GK4587@noname.str.redhat.com> <53B2E82D.8020000@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/3] block: block: introduce bdrv_io_plug() and bdrv_io_unplug() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ming Lei Cc: Kevin Wolf , Peter Maydell , Fam Zheng , "Michael S. Tsirkin" , qemu-devel , Stefan Hajnoczi Il 02/07/2014 02:46, Ming Lei ha scritto: > On Wed, Jul 2, 2014 at 12:56 AM, Paolo Bonzini wrote: >> Il 01/07/2014 17:21, Kevin Wolf ha scritto: >>>>>> Does this bs->file forwarding work for more than the raw driver? For >>>>>> example, if drv is an image format driver that needs to read some >>>>>> metadata from the image before it can submit the payload, does this >>>>>> still do what you were intending? >>>> >>>> >>>> Sorry for not understanding the problem, and you are right, these >>>> patches can't support other formats, and for solving the dependency, >>>> changes to image format driver should be needed. >>> >>> >>> Then let's drop the bs->file recursion here and add an explicit >>> .bdrv_io_plug/unplug callback to the raw driver. >> >> >> Actually I thought about this in my review, and there's no reason for this >> not to work for image formats. >> >> While bs->file is plugged, image formats will start executing their >> bdrv_co_readv/bdrv_co_writev callbacks, and issue reads or writes as >> necessary. The reads and writes will accumulate in bs->file until it is >> unplugged, which is exactly the effect we want. > > For some image formats, meta data need to be read first before > the payload can be read since how/what to read payload might > depend on content of meta data. Yes, but everything is done asynchronously so it should just work. You have plug(bs) plug(bs->file) read start coroutine for image format read issue metadata read on bs->file bdrv_read ultimately calls bdrv_aio_readv on bs->file I/O not submitted because bs->file is plugged coroutine yields unplug(bs) unplug(bs->file) bs->file now submits metadata read ... metadata read completes coroutine re-entered image format read restarts, unaware of plug/unplug If things do _not_ work as above I would like to understand where my reasoning is wrong. On top of this, there _could_ be reasons for formats to implement plug/unplug themselves. They could coalesce metadata reads or copy-on-write operations, for example. This however is independent from the default behavior, which IMO is "plugging should propagate along bs->file" (and possibly bs->backing_hd too, but not now because that opens more cans of worms). >> The change in bdrv_drain_all is ugly though. I don't have a better idea, >> but I would like to understand better why it is needed. Ming Lei, did you >> see a deadlock without it? > > Not yet, just for safe reason to make sure all queued data has chance > to be flushed. If you think it isn't necessary, I can remove it. If you could make it an assertion, that would also be great. (BTW, it's probably best to add a nesting count to plug/unplug). Paolo