diff mbox series

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

Message ID 20221028071635.3037348-2-eesposit@redhat.com (mailing list archive)
State New, archived
Headers show
Series AioContext removal: LinuxAioState and ThreadPool | expand

Commit Message

Emanuele Giuseppe Esposito Oct. 28, 2022, 7:16 a.m. UTC 
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(-)

Comments

Paolo Bonzini Oct. 28, 2022, 11:51 a.m. UTC | #1 
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.

Paolo
Emanuele Giuseppe Esposito Oct. 28, 2022, 12:16 p.m. UTC | #2 
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
diff mbox series

Patch

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);
 }
 
 /**
@@ -232,13 +232,11 @@  static void qemu_laio_process_completions(LinuxAioState *s)
 
 static void qemu_laio_process_completions_and_submit(LinuxAioState *s)
 {
-    aio_context_acquire(s->aio_context);
     qemu_laio_process_completions(s);
 
     if (!s->io_q.plugged && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
         ioq_submit(s);
     }
-    aio_context_release(s->aio_context);
 }
 
 static void qemu_laio_completion_bh(void *opaque)
@@ -354,14 +352,18 @@  static uint64_t laio_max_batch(LinuxAioState *s, uint64_t dev_max_batch)
     return max_batch;
 }
 
-void laio_io_plug(BlockDriverState *bs, LinuxAioState *s)
-{
+void laio_io_plug(void){
+    AioContext *ctx = qemu_get_current_aio_context();
+    LinuxAioState *s = aio_get_linux_aio(ctx);
+
     s->io_q.plugged++;
 }
 
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
-                    uint64_t dev_max_batch)
+void laio_io_unplug(uint64_t dev_max_batch)
 {
+    AioContext *ctx = qemu_get_current_aio_context();
+    LinuxAioState *s = aio_get_linux_aio(ctx);
+
     assert(s->io_q.plugged);
     s->io_q.plugged--;
 
@@ -411,15 +413,15 @@  static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset,
     return 0;
 }
 
-int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
-                                uint64_t offset, QEMUIOVector *qiov, int type,
-                                uint64_t dev_max_batch)
+int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov,
+                                int type, uint64_t dev_max_batch)
 {
     int ret;
+    AioContext *ctx = qemu_get_current_aio_context();
     struct qemu_laiocb laiocb = {
         .co         = qemu_coroutine_self(),
         .nbytes     = qiov->size,
-        .ctx        = s,
+        .ctx        = aio_get_linux_aio(ctx),
         .ret        = -EINPROGRESS,
         .is_read    = (type == QEMU_AIO_READ),
         .qiov       = qiov,
diff --git a/include/block/aio.h b/include/block/aio.h
index d128558f1d..8bb5eea4a9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -200,10 +200,6 @@  struct AioContext {
     struct ThreadPool *thread_pool;
 
 #ifdef CONFIG_LINUX_AIO
-    /*
-     * State for native Linux AIO.  Uses aio_context_acquire/release for
-     * locking.
-     */
     struct LinuxAioState *linux_aio;
 #endif
 #ifdef CONFIG_LINUX_IO_URING
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index 21fc10c4c9..f0f14f14f8 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -50,14 +50,12 @@ 
 typedef struct LinuxAioState LinuxAioState;
 LinuxAioState *laio_init(Error **errp);
 void laio_cleanup(LinuxAioState *s);
-int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
-                                uint64_t offset, QEMUIOVector *qiov, int type,
-                                uint64_t dev_max_batch);
+int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov,
+                                int type, uint64_t dev_max_batch);
 void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
 void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
-void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
-                    uint64_t dev_max_batch);
+void laio_io_plug(void);
+void laio_io_unplug(uint64_t dev_max_batch);
 #endif
 /* io_uring.c - Linux io_uring implementation */
 #ifdef CONFIG_LINUX_IO_URING