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 8:46 AM, Ming Lei <address@hidden> wrote:
> 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.
>
>>
>> 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.

Another interface like bdrv_io_flush() may be needed for
such purpose, and the accumulated IOs is really required to be
flushed before doing flush.

I will add the interface in v4.

Thanks,
--
Ming Lei