Re: [Qemu-devel] [PATCH v2 17/25] block/raw-win32: implement .bdrv_detach/attach_aio_context()

[Paolo Bonzini <pbonzini@redhat.com>]

Il 07/05/2014 12:27, Stefan Hajnoczi ha scritto:
> Drop the assumption that we're using the main AioContext for raw-win32.
> Convert the aio-win32 code to support detach/attach and replace
> qemu_aio_wait() with aio_poll().
>
> The .bdrv_detach/attach_aio_context() interfaces move the aio-win32
> event notifier from the old to the new AioContext.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/raw-aio.h   |  4 ++++
>  block/raw-win32.c | 25 +++++++++++++++++++++++++
>  block/win32-aio.c | 17 +++++++++++++----
>  3 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/block/raw-aio.h b/block/raw-aio.h
> index 6269f3d..8cf084e 100644
> --- a/block/raw-aio.h
> +++ b/block/raw-aio.h
> @@ -51,6 +51,10 @@ BlockDriverAIOCB *win32_aio_submit(BlockDriverState *bs,
>          QEMUWin32AIOState *aio, HANDLE hfile,
>          int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>          BlockDriverCompletionFunc *cb, void *opaque, int type);
> +void win32_aio_detach_aio_context(QEMUWin32AIOState *aio,
> +                                  AioContext *old_context);
> +void win32_aio_attach_aio_context(QEMUWin32AIOState *aio,
> +                                  AioContext *new_context);
>  #endif
>
>  #endif /* QEMU_RAW_AIO_H */
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index cf1e9ce..324e818 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -200,6 +200,25 @@ static int set_sparse(int fd)
>  				 NULL, 0, NULL, 0, &returned, NULL);
>  }
>
> +static void raw_detach_aio_context(BlockDriverState *bs)
> +{
> +    BDRVRawState *s = bs->opaque;
> +
> +    if (s->aio) {
> +        win32_aio_detach_aio_context(s->aio, bdrv_get_aio_context(bs));
> +    }
> +}
> +
> +static void raw_attach_aio_context(BlockDriverState *bs,
> +                                   AioContext *new_context)
> +{
> +    BDRVRawState *s = bs->opaque;
> +
> +    if (s->aio) {
> +        win32_aio_attach_aio_context(s->aio, new_context);
> +    }
> +}
> +
>  static void raw_probe_alignment(BlockDriverState *bs)
>  {
>      BDRVRawState *s = bs->opaque;
> @@ -339,6 +358,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>              error_setg_errno(errp, -ret, "Could not enable AIO");
>              goto fail;
>          }
> +
> +        win32_aio_attach_aio_context(s->aio, bdrv_get_aio_context(bs));
>      }
>
>      raw_probe_alignment(bs);
> @@ -388,6 +409,7 @@ static void raw_close(BlockDriverState *bs)
>      BDRVRawState *s = bs->opaque;
>
>      if (s->aio) {
> +        win32_aio_detach_aio_context(s->aio, bdrv_get_aio_context(bs));
>          win32_aio_cleanup(s->aio);
>          s->aio = NULL;
>      }
> @@ -687,6 +709,9 @@ static BlockDriver bdrv_host_device = {
>      .bdrv_aio_writev    = raw_aio_writev,
>      .bdrv_aio_flush     = raw_aio_flush,
>
> +    .bdrv_detach_aio_context = raw_detach_aio_context,
> +    .bdrv_attach_aio_context = raw_attach_aio_context,
> +
>      .bdrv_getlength      = raw_getlength,
>      .has_variable_length = true,
>
> diff --git a/block/win32-aio.c b/block/win32-aio.c
> index b43b166..7bc5d38 100644
> --- a/block/win32-aio.c
> +++ b/block/win32-aio.c
> @@ -114,7 +114,7 @@ static void win32_aio_cancel(BlockDriverAIOCB *blockacb)
>       * wait for completion.
>       */
>      while (!HasOverlappedIoCompleted(&waiocb->ov)) {
> -        qemu_aio_wait();
> +        aio_poll(bdrv_get_aio_context(blockacb->bs), true);
>      }
>  }
>
> @@ -180,6 +180,18 @@ int win32_aio_attach(QEMUWin32AIOState *aio, HANDLE hfile)
>      }
>  }
>
> +void win32_aio_detach_aio_context(QEMUWin32AIOState *aio,
> +                                  AioContext *old_context)
> +{
> +    aio_set_event_notifier(old_context, &aio->e, NULL);
> +}
> +
> +void win32_aio_attach_aio_context(QEMUWin32AIOState *aio,
> +                                  AioContext *new_context)
> +{
> +    aio_set_event_notifier(new_context, &aio->e, win32_aio_completion_cb);
> +}
> +
>  QEMUWin32AIOState *win32_aio_init(void)
>  {
>      QEMUWin32AIOState *s;
> @@ -194,8 +206,6 @@ QEMUWin32AIOState *win32_aio_init(void)
>          goto out_close_efd;
>      }
>
> -    qemu_aio_set_event_notifier(&s->e, win32_aio_completion_cb);
> -
>      return s;
>
>  out_close_efd:
> @@ -207,7 +217,6 @@ out_free_state:
>
>  void win32_aio_cleanup(QEMUWin32AIOState *aio)
>  {
> -    qemu_aio_set_event_notifier(&aio->e, NULL);

Perhaps it would be safer not to leave a dangling pointer in the 
AioContext, and detach manually here (or at least add an assertion). 
Can be done as a follow-up though.

>      CloseHandle(aio->hIOCP);
>      event_notifier_cleanup(&aio->e);
>      g_free(aio);
>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>