From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37651) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2G53-0007cb-OY for qemu-devel@nongnu.org; Wed, 02 Jul 2014 04:40:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2G4w-000677-8Y for qemu-devel@nongnu.org; Wed, 02 Jul 2014 04:39:53 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:45178) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2G4w-000671-1t for qemu-devel@nongnu.org; Wed, 02 Jul 2014 04:39:46 -0400 Received: from mail-ve0-f182.google.com ([209.85.128.182]) by youngberry.canonical.com with esmtpsa (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1X2G4v-0003aj-CQ for qemu-devel@nongnu.org; Wed, 02 Jul 2014 08:39:45 +0000 Received: by mail-ve0-f182.google.com with SMTP id oy12so10769295veb.41 for ; Wed, 02 Jul 2014 01:39:44 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <53B3C059.3060601@redhat.com> 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> <53B3C059.3060601@redhat.com> Date: Wed, 2 Jul 2014 16:39:44 +0800 Message-ID: From: Ming Lei Content-Type: text/plain; charset=UTF-8 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: Paolo Bonzini Cc: Kevin Wolf , Peter Maydell , Fam Zheng , "Michael S. Tsirkin" , qemu-devel , Stefan Hajnoczi On Wed, Jul 2, 2014 at 4:18 PM, Paolo Bonzini wrote: > 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 Then start to read payload in original path, but no plug/unplug any more. Also another request may follows, and another plug&unplug comes too, which makes thing more complicated, so I suggest to enable plug&unplug only for raw driver now. And in future we can use explicit bdrv_io_flush() to handle the dependency in non-raw format drivers. > > 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). I'd rather to add it in future if we really need that. Thanks, -- Ming Lei