Re: [Qemu-devel] [PATCH 4/4] linux-aio: simplify removal of completed iocbs from the list

[Kevin Wolf <kwolf@redhat.com>]

Am 10.12.2014 um 15:52 hat Paolo Bonzini geschrieben:
> There is no need to do another O(n) pass on the list; the iocb to
> splice the list at is already available in the array we passed to
> io_submit.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/linux-aio.c    | 12 ++++++------
>  include/qemu/queue.h | 11 +++++++++++
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index b223d9e..6c98f72 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -186,9 +186,10 @@ static void ioq_init(LaioQueue *io_q)
>  
>  static int ioq_submit(struct qemu_laio_state *s)
>  {
> -    int ret, i, len;
> +    int ret, len;
>      struct qemu_laiocb *aiocb;
>      struct iocb *iocbs[MAX_QUEUED_IO];
> +    QSIMPLEQ_HEAD(, qemu_laiocb) completed;
>  
>      do {
>          len = 0;
> @@ -201,16 +202,15 @@ static int ioq_submit(struct qemu_laio_state *s)
>  
>          ret = io_submit(s->ctx, len, iocbs);
>          if (ret == -EAGAIN) {
> -            ret = 0;
> +            break;
>          }

ioq_submit() returns -EAGAIN now instead of 0. Almost all callers ignore
the return value, except laio_io_unplug(), which inherits this change.
Fortunately, raw_aio_unplug() completely ignores the return value.

So seems that this didn't cause a bug, but we have some cleanup to do
and make functions void if their return value isn't used anywhere.

>          if (ret < 0) {
>              abort();
>          }
>  
> -        for (i = 0; i < ret; i++) {
> -            s->io_q.n--;
> -            QSIMPLEQ_REMOVE_HEAD(&s->io_q.pending, next);
> -        }
> +        s->io_q.n -= ret;
> +        aiocb = container_of(iocbs[ret - 1], struct qemu_laiocb, iocb);
> +        QSIMPLEQ_SPLIT_AFTER(&completed, &s->io_q.pending, aiocb, next);
>      } while (ret == len && !QSIMPLEQ_EMPTY(&s->io_q.pending));
>      s->io_q.blocked = (s->io_q.n > 0);
>  
> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> index 0dedd29..2c21d28 100644
> --- a/include/qemu/queue.h
> +++ b/include/qemu/queue.h
> @@ -279,6 +279,17 @@ struct {                                                                \
>          (head)->sqh_last = &(head)->sqh_first;                          \
>  } while (/*CONSTCOND*/0)
>  
> +#define QSIMPLEQ_SPLIT_AFTER(head1, head2, elm, field) do {             \
> +    if (((head1)->sqh_first = (head2)->sqh_first) == NULL) {            \
> +        (head1)->sqh_last = &(head1)->sqh_first;                        \
> +    } else {                                                            \
> +        (head1)->sqh_last = &(elm)->field.sqe_next;                     \
> +        if (((head2)->sqh_first = (elm)->field.sqe_next) == NULL) {     \
> +            (head2)->sqh_last = &(head2)->sqh_first;                    \
> +        }                                                               \
> +    }                                                                   \
> +} while (/*CONSTCOND*/0)

Wouldn't it be easier to write a macro that doesn't split a queue in
two, but simply removes everything up to a given element?

Anyway, if you really want to implement a split operation, I think you
also need to break the actual chain, i.e. (head1)->sqh_last.sqe_next =
NULL.

Kevin