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
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