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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3584 bytes --]

On Mon, Oct 31, 2022 at 08:59:34AM -0400, Emanuele Giuseppe Esposito wrote:
> @@ -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;

/* No locking required, only accessed from AioContext home thread */

This is more general because it includes the main loop, which is not an
IOThread.

(Please write "IOThread" for consistency. Most of the documentation and
comments uses "IOThread".)

> -
> -    /* I/O completion processing.  Only runs in I/O thread.  */
>      QEMUBH *completion_bh;
>      int event_idx;
>      int event_max;
> @@ -102,6 +103,7 @@ static void qemu_laio_process_completion(struct qemu_laiocb *laiocb)
>       * later.  Coroutines cannot be entered recursively so avoid doing
>       * that!
>       */
> +    assert(laiocb->co->ctx == laiocb->ctx->aio_context);
>      if (!qemu_coroutine_entered(laiocb->co)) {
>          aio_co_wake(laiocb->co);
>      }
> @@ -232,13 +234,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 +354,19 @@ 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++;

I see the following code path:

blk_io_plug -> bdrv_io_plug -> raw_aio_plug -> laio_io_plug

blk_io_plug() can be called from any thread but laio_io_plug()
implicitly operates on the current thread's AioContext's LinuxAioState.

This changes the semantics of blk_io_plug() from a global BDS operation
to a thread-local one. The new blk_io_plug() semantics need to be
documented, because it's not obvious that multiple threads can
blk_io_plug/unplug() independently and don't affect each other. It means
the caller must be careful to pair plug/unplug in the same thread.

>  }
>  
> -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 +416,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)

This function needs documentation. It submits I/O requests in the
thread's current AioContext. Before it was explicit via the function
arguments but now it's no longer obvious.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]