Re: [Qemu-devel] [PATCH v2 1/3] block: block: introduce bdrv_io_plug() and bdrv_io_unplug()

[Ming Lei]

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