On Wed, Jul 2, 2014 at 4:18 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > Il 02/07/2014 02:46, Ming Lei ha scritto: > >> On Wed, Jul 2, 2014 at 12:56 AM, Paolo Bonzini <pbonz...@redhat.com> >> 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