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:56 PM, Paolo Bonzini <address@hidden> wrote:
> Il 02/07/2014 10:39, Ming Lei ha scritto:
>
>> 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.
>
>
> That's just a performance issue (and actually one that wasn't in 2.0 because
> qcow2 on dataplane wasn't supported there).  In many cases the cache hit of
> the qcow2 metadata cache can be very high, and avoiding plug/unplug would
> prevent an easy performance bonus.

I just gave a test on qcow2 image with dataplane on, and looks it
can work, so I will take original way to implement the api.

But making full use of plug/unplug for non-raw formats need
changes in format drivers in future.

>
> I don't especially like plug/unplug as an API (I think it's better to extend
> aio_multiwrite to include other kind of requests), but:
>
> - either we have qualms on the correctness of it, and then we should live
> with the regressions
>
> - or if the patches are not messy and reverting them is easy, we should go
> for it.  This is what we did for dataplane in the first place, and we can
> keep doing it in the 2.1 dataplane code.

OK, let's keep the API's as it now.

Actually I have thoughts in mind for plug&unplug changes too,
and now it is better to make it a bit simple,

>
>> > 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.
>
>
> I think this is a matter of correctness as Kevin's review pointed out.

OK, specifically it is needed for non-raw format driver.

Thanks,
--
Ming Lei