Re: [Qemu-devel] [PATCH v3 09/25] iscsi: implement .bdrv_detach/attach_aio_context()

[Peter Lieven]

Am 08.05.2014 16:34, schrieb Stefan Hajnoczi:
> Drop the assumption that we're using the main AioContext for Linux
> AIO.  Convert qemu_aio_set_fd_handler() to aio_set_fd_handler() and
> timer_new_ms() to aio_timer_new().
>
> The .bdrv_detach/attach_aio_context() interfaces also need to be
> implemented to move the fd and timer from the old to the new AioContext.
>
> Cc: Peter Lieven <address@hidden>
> Cc: Ronnie Sahlberg <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> v3:
>  * Stash away IscsiLun pointer for future use instead of AioContext pointer 
> [Peter Lieven]
>  * Use IScsiLun->aio_context where possible instead of bdrv_get_aio_context()
> ---
>  block/iscsi.c | 80 
> +++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index a30202b..535032f 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -47,6 +47,7 @@
>  
>  typedef struct IscsiLun {
>      struct iscsi_context *iscsi;
> +    AioContext *aio_context;
>      int lun;
>      enum scsi_inquiry_peripheral_device_type type;
>      int block_size;
> @@ -69,6 +70,7 @@ typedef struct IscsiTask {
>      struct scsi_task *task;
>      Coroutine *co;
>      QEMUBH *bh;
> +    IscsiLun *iscsilun;
>  } IscsiTask;
>  
>  typedef struct IscsiAIOCB {
> @@ -120,7 +122,7 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
>      if (acb->bh) {
>          return;
>      }
> -    acb->bh = qemu_bh_new(iscsi_bh_cb, acb);
> +    acb->bh = aio_bh_new(acb->iscsilun->aio_context, iscsi_bh_cb, acb);
>      qemu_bh_schedule(acb->bh);
>  }
>  
> @@ -156,7 +158,8 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int 
> status,
>  
>  out:
>      if (iTask->co) {
> -        iTask->bh = qemu_bh_new(iscsi_co_generic_bh_cb, iTask);
> +        iTask->bh = aio_bh_new(iTask->iscsilun->aio_context,
> +                               iscsi_co_generic_bh_cb, iTask);
>          qemu_bh_schedule(iTask->bh);
>      }
>  }
> @@ -164,8 +167,9 @@ out:
>  static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct IscsiTask 
> *iTask)
>  {
>      *iTask = (struct IscsiTask) {
> -        .co         = qemu_coroutine_self(),
> -        .retries    = ISCSI_CMD_RETRIES,
> +        .co             = qemu_coroutine_self(),
> +        .retries        = ISCSI_CMD_RETRIES,
> +        .iscsilun       = iscsilun,
>      };
>  }
>  
> @@ -196,7 +200,7 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
>                                       iscsi_abort_task_cb, acb);
>  
>      while (acb->status == -EINPROGRESS) {
> -        qemu_aio_wait();
> +        aio_poll(iscsilun->aio_context, true);
>      }
>  }
>  
> @@ -219,10 +223,11 @@ iscsi_set_events(IscsiLun *iscsilun)
>      ev = POLLIN;
>      ev |= iscsi_which_events(iscsi);
>      if (ev != iscsilun->events) {
> -        qemu_aio_set_fd_handler(iscsi_get_fd(iscsi),
> -                      iscsi_process_read,
> -                      (ev & POLLOUT) ? iscsi_process_write : NULL,
> -                      iscsilun);
> +        aio_set_fd_handler(iscsilun->aio_context,
> +                           iscsi_get_fd(iscsi),
> +                           iscsi_process_read,
> +                           (ev & POLLOUT) ? iscsi_process_write : NULL,
> +                           iscsilun);
>  
>      }
>  
> @@ -620,7 +625,7 @@ static int iscsi_ioctl(BlockDriverState *bs, unsigned 
> long int req, void *buf)
>          iscsi_aio_ioctl(bs, req, buf, ioctl_cb, &status);
>  
>          while (status == -EINPROGRESS) {
> -            qemu_aio_wait();
> +            aio_poll(iscsilun->aio_context, true);
>          }
>  
>          return 0;
> @@ -1110,6 +1115,40 @@ fail_with_err:
>      return NULL;
>  }
>  
> +static void iscsi_detach_aio_context(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +
> +    aio_set_fd_handler(iscsilun->aio_context,
> +                       iscsi_get_fd(iscsilun->iscsi),
> +                       NULL, NULL, NULL);
> +    iscsilun->events = 0;
> +
> +    if (iscsilun->nop_timer) {
> +        timer_del(iscsilun->nop_timer);
> +        timer_free(iscsilun->nop_timer);
> +        iscsilun->nop_timer = NULL;
> +    }
> +}
> +
> +static void iscsi_attach_aio_context(BlockDriverState *bs,
> +                                     AioContext *new_context)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +
> +    iscsilun->aio_context = new_context;
> +    iscsi_set_events(iscsilun);
> +
> +#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> +    /* Set up a timer for sending out iSCSI NOPs */
> +    iscsilun->nop_timer = aio_timer_new(iscsilun->aio_context,
> +                                        QEMU_CLOCK_REALTIME, SCALE_MS,
> +                                        iscsi_nop_timed_event, iscsilun);
> +    timer_mod(iscsilun->nop_timer,
> +              qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> +#endif
> +}
> +
>  /*
>   * We support iscsi url's on the form
>   * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
> @@ -1216,6 +1255,7 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      }
>  
>      iscsilun->iscsi = iscsi;
> +    iscsilun->aio_context = bdrv_get_aio_context(bs);
>      iscsilun->lun   = iscsi_url->lun;
>      iscsilun->has_write_same = true;
>  
> @@ -1289,11 +1329,7 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      scsi_free_scsi_task(task);
>      task = NULL;
>  
> -#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> -    /* Set up a timer for sending out iSCSI NOPs */
> -    iscsilun->nop_timer = timer_new_ms(QEMU_CLOCK_REALTIME, 
> iscsi_nop_timed_event, iscsilun);
> -    timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 
> NOP_INTERVAL);
> -#endif
> +    iscsi_attach_aio_context(bs, iscsilun->aio_context);
>  
>  out:
>      qemu_opts_del(opts);
> @@ -1321,11 +1357,7 @@ static void iscsi_close(BlockDriverState *bs)
>      IscsiLun *iscsilun = bs->opaque;
>      struct iscsi_context *iscsi = iscsilun->iscsi;
>  
> -    if (iscsilun->nop_timer) {
> -        timer_del(iscsilun->nop_timer);
> -        timer_free(iscsilun->nop_timer);
> -    }
> -    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL);
> +    iscsi_detach_aio_context(bs);
>      iscsi_destroy_context(iscsi);
>      g_free(iscsilun->zeroblock);
>      memset(iscsilun, 0, sizeof(IscsiLun));
> @@ -1421,10 +1453,7 @@ static int iscsi_create(const char *filename, 
> QEMUOptionParameter *options,
>      if (ret != 0) {
>          goto out;
>      }
> -    if (iscsilun->nop_timer) {
> -        timer_del(iscsilun->nop_timer);
> -        timer_free(iscsilun->nop_timer);
> -    }
> +    iscsi_detach_aio_context(bs);
>      if (iscsilun->type != TYPE_DISK) {
>          ret = -ENODEV;
>          goto out;
> @@ -1501,6 +1530,9 @@ static BlockDriver bdrv_iscsi = {
>      .bdrv_ioctl       = iscsi_ioctl,
>      .bdrv_aio_ioctl   = iscsi_aio_ioctl,
>  #endif
> +
> +    .bdrv_detach_aio_context = iscsi_detach_aio_context,
> +    .bdrv_attach_aio_context = iscsi_attach_aio_context,
>  };
>  
>  static QemuOptsList qemu_iscsi_opts = {

Reviewed-by: Peter Lieven <address@hidden>