Re: [PATCH v2 1/3] linux-aio: use LinuxAioState from the running thread

[Emanuele Giuseppe Esposito]

Am 28/10/2022 um 13:51 schrieb Paolo Bonzini:
> On 10/28/22 09:16, Emanuele Giuseppe Esposito wrote:
>> From: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Remove usage of aio_context_acquire by always submitting asynchronous
>> AIO to the current thread's LinuxAioState.
>>
>> In order to prevent mistakes from the caller side, avoid passing
>> LinuxAioState
>> in laio_io_{plug/unplug} and laio_co_submit.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   block/file-posix.c      | 10 +++-------
>>   block/linux-aio.c       | 34 ++++++++++++++++++----------------
>>   include/block/aio.h     |  4 ----
>>   include/block/raw-aio.h | 10 ++++------
>>   4 files changed, 25 insertions(+), 33 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 23acffb9a4..23fe98eb3e 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -2099,10 +2099,8 @@ static int coroutine_fn
>> raw_co_prw(BlockDriverState *bs, uint64_t offset,
>>   #endif
>>   #ifdef CONFIG_LINUX_AIO
>>       } else if (s->use_linux_aio) {
>> -        LinuxAioState *aio =
>> aio_get_linux_aio(bdrv_get_aio_context(bs));
>>           assert(qiov->size == bytes);
>> -        return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
>> -                              s->aio_max_batch);
>> +        return laio_co_submit(s->fd, offset, qiov, type,
>> s->aio_max_batch);
>>   #endif
>>       }
>>   @@ -2142,8 +2140,7 @@ static void raw_aio_plug(BlockDriverState *bs)
>>       BDRVRawState __attribute__((unused)) *s = bs->opaque;
>>   #ifdef CONFIG_LINUX_AIO
>>       if (s->use_linux_aio) {
>> -        LinuxAioState *aio =
>> aio_get_linux_aio(bdrv_get_aio_context(bs));
>> -        laio_io_plug(bs, aio);
>> +        laio_io_plug();
>>       }
>>   #endif
>>   #ifdef CONFIG_LINUX_IO_URING
>> @@ -2159,8 +2156,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
>>       BDRVRawState __attribute__((unused)) *s = bs->opaque;
>>   #ifdef CONFIG_LINUX_AIO
>>       if (s->use_linux_aio) {
>> -        LinuxAioState *aio =
>> aio_get_linux_aio(bdrv_get_aio_context(bs));
>> -        laio_io_unplug(bs, aio, s->aio_max_batch);
>> +        laio_io_unplug(s->aio_max_batch);
>>       }
>>   #endif
>>   #ifdef CONFIG_LINUX_IO_URING
>> diff --git a/block/linux-aio.c b/block/linux-aio.c
>> index d2cfb7f523..c806d3bb91 100644
>> --- a/block/linux-aio.c
>> +++ b/block/linux-aio.c
>> @@ -16,6 +16,9 @@
>>   #include "qemu/coroutine.h"
>>   #include "qapi/error.h"
>>   +/* Only used for assertions.  */
>> +#include "qemu/coroutine_int.h"
>> +
>>   #include <libaio.h>
>>     /*
>> @@ -56,10 +59,8 @@ struct LinuxAioState {
>>       io_context_t ctx;
>>       EventNotifier e;
>>   -    /* io queue for submit at batch.  Protected by AioContext lock. */
>> +    /* All data is only used in one I/O thread.  */
>>       LaioQueue io_q;
>> -
>> -    /* I/O completion processing.  Only runs in I/O thread.  */
>>       QEMUBH *completion_bh;
>>       int event_idx;
>>       int event_max;
>> @@ -102,9 +103,8 @@ static void qemu_laio_process_completion(struct
>> qemu_laiocb *laiocb)
>>        * later.  Coroutines cannot be entered recursively so avoid doing
>>        * that!
>>        */
>> -    if (!qemu_coroutine_entered(laiocb->co)) {
>> -        aio_co_wake(laiocb->co);
>> -    }
>> +    assert(laiocb->co->ctx == laiocb->ctx->aio_context);
>> +    qemu_coroutine_enter_if_inactive(laiocb->co);
> 
> This is wrong, it misses the aio_context_acquire/aio_context_release
> pair in aio_co_enter() (which is called by aio_co_wake()).
> 
> Likewise in patch 2.

Makes sense, I'll drop this hunk in v3.

Emanuele