Re: [Qemu-devel] [RFC PATCH 3/3] linux-aio: Don't reenter request coroutine recursively

[Kevin Wolf <kwolf@redhat.com>]

Am 26.11.2014 um 15:46 hat Kevin Wolf geschrieben:
> When getting an error while submitting requests, we must be careful to
> wake up only inactive coroutines. Therefore we must special-case the
> currently active coroutine and communicate an error for that request
> using the ordinary return value of ioq_submit().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/linux-aio.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)

Ming, did you have a look at this patch specifically? Does it fix the
issue that you tried to address with a much more complex callback-based
patch?

I'd like to go forward with this as both Peter and I have measured
considerable performance improvements with our optimisation proposals,
and this series is an important part of it.

Kevin


> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index 99b259d..fd8f0e4 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -177,12 +177,19 @@ static int ioq_submit(struct qemu_laio_state *s)
>              container_of(s->io_q.iocbs[i], struct qemu_laiocb, iocb);
>  
>          laiocb->ret = (ret < 0) ? ret : -EIO;
> -        qemu_laio_process_completion(s, laiocb);
> +        if (laiocb->co != qemu_coroutine_self()) {
> +            qemu_coroutine_enter(laiocb->co, NULL);
> +        } else {
> +            /* The return value is used for the currently active coroutine.
> +             * We're always in ioq_enqueue() here, ioq_submit() never runs from
> +             * a request's coroutine.*/
> +            ret = laiocb->ret;
> +        }
>      }
>      return ret;
>  }
>  
> -static void ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb)
> +static int ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb)
>  {
>      unsigned int idx = s->io_q.idx;
>  
> @@ -191,7 +198,9 @@ static void ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb)
>  
>      /* submit immediately if queue is full */
>      if (idx == s->io_q.size) {
> -        ioq_submit(s);
> +        return ioq_submit(s);
> +    } else {
> +        return 0;
>      }
>  }
>  
> @@ -253,11 +262,11 @@ int laio_submit_co(BlockDriverState *bs, void *aio_ctx, int fd,
>  
>      if (!s->io_q.plugged) {
>          ret = io_submit(s->ctx, 1, &iocbs);
> -        if (ret < 0) {
> -            return ret;
> -        }
>      } else {
> -        ioq_enqueue(s, iocbs);
> +        ret = ioq_enqueue(s, iocbs);
> +    }
> +    if (ret < 0) {
> +        return ret;
>      }
>  
>      qemu_coroutine_yield();
> -- 
> 1.8.3.1
>