Re: [Qemu-devel] [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/06/2017 19:08, Kevin Wolf wrote:
> Am 01.06.2017 um 18:40 hat Paolo Bonzini geschrieben:
>> On 01/06/2017 18:28, Kevin Wolf wrote:
>>>> - qed_acquire/qed_release can be removed as you inline stuff, but this
>>>> should not cause bugs so you can either do it as a final patch or let
>>>> me remove it later.
>>> To be honest, I don't completely understand what they are protecting in
>>> the first place. The places where they are look quite strange to me. So
>>> I tried to simply leave them alone.
>>>
>>> What is the reason that we can remove them when we inline stuff?
>>> Shouldn't the inlining be semantically equivalent?
>>
>> You're right, they can be removed when going from callback to direct
>> call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
>> does it for the callbacks).
> 
> So if we take qed_read_table_cb() for example:
> 
>     qed_acquire(s);
>     for (i = 0; i < noffsets; i++) {
>         table->offsets[i] = le64_to_cpu(table->offsets[i]);
>     }
>     qed_release(s);
> 
> First of all, I don't see what it protects. If we wanted to avoid that
> someone else sees table->offsets with wrong endianness, we would be
> taking the lock much too late. And if nobody else knows about the table
> yet, what is there to be locked?

That is the product of a mechanical conversion where all callbacks grew
a qed_acquire/qed_release pair (commit b9e413dd37, "block: explicitly
acquire aiocontext in aio callbacks that need it", 2017-02-21).

In this case:

        qed_acquire(s);
        bdrv_aio_flush(write_table_cb->s->bs, qed_write_table_cb,
                       write_table_cb);
        qed_release(s);

the AioContext protects write_table_cb->s->bs.

> But anyway, given your explanation that acquiring the AioContext lock is
> getting replaced by coroutine magic, the qed_acquire/release() pair
> actually can't be removed in patch 2 when the callback is converted to a
> direct call, but only when the whole call path between .bdrv_aio_readv/
> writev and this specific callback is converted, so that we never drop
> out of coroutine context before reaching this code. Correct?

Yes.

> This happens only very late in the series, so it probably also means
> that patch 5 is indeed wrong because it removes the lock too early?

bdrv_qed_co_get_block_status is entirely in coroutine context, so I
think that one is fine.

Paolo