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