Re: [Qemu-devel] [PATCH v4 2/3] linux-aio: implement io plug, unplug and flush io queue

[Kevin Wolf <kwolf@redhat.com>]

Am 02.07.2014 um 14:18 hat Ming Lei geschrieben:
> This patch implements .bdrv_io_plug, .bdrv_io_unplug and
> .bdrv_flush_io_queue callbacks for linux-aio Block Drivers,
> so that submitting I/O as a batch can be supported on linux-aio.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>

Just a couple of minor comments, see inline.

>  block/linux-aio.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  block/raw-aio.h   |    2 ++
>  block/raw-posix.c |   45 +++++++++++++++++++++++++++
>  3 files changed, 133 insertions(+), 2 deletions(-)
> 
> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index f0a2c08..1cb4845 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -25,6 +25,8 @@
>   */
>  #define MAX_EVENTS 128
>  
> +#define MAX_QUEUED_IO  128
> +
>  struct qemu_laiocb {
>      BlockDriverAIOCB common;
>      struct qemu_laio_state *ctx;
> @@ -36,9 +38,19 @@ struct qemu_laiocb {
>      QLIST_ENTRY(qemu_laiocb) node;
>  };
>  
> +struct laio_queue {
> +    struct iocb *iocbs[MAX_QUEUED_IO];
> +    int plugged;

Why signed?

> +    unsigned int size;
> +    unsigned int idx;
> +};
> +
>  struct qemu_laio_state {
>      io_context_t ctx;
>      EventNotifier e;
> +
> +    /* io queue for submit at batch */
> +    struct laio_queue io_q;
>  };
>  
>  static inline ssize_t io_event_ret(struct io_event *ev)
> @@ -135,6 +147,72 @@ static const AIOCBInfo laio_aiocb_info = {
>      .cancel             = laio_cancel,
>  };
>  
> +static void ioq_init(struct laio_queue *io_q)
> +{
> +    io_q->size = MAX_QUEUED_IO;
> +    io_q->idx = 0;
> +    io_q->plugged = 0;
> +}
> +
> +static int ioq_submit(struct qemu_laio_state *s)
> +{
> +    int ret, i = 0;
> +    int len = s->io_q.idx;
> +
> +    do {
> +        ret = io_submit(s->ctx, len, s->io_q.iocbs);
> +    } while (i++ < 3 && ret == -EAGAIN);
> +
> +    /* empty io queue */
> +    s->io_q.idx = 0;
> +
> +    if (ret >= 0)
> +      return 0;

Indentation is off and braces are missing.

> +
> +    for (i = 0; i < len; i++) {
> +        struct qemu_laiocb *laiocb =
> +            container_of(s->io_q.iocbs[i], struct qemu_laiocb, iocb);
> +
> +        laiocb->ret = ret;
> +        qemu_laio_process_completion(s, laiocb);
> +    }
> +    return ret;
> +}
> +
> +static void ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb)
> +{
> +    unsigned int idx = s->io_q.idx;
> +
> +    s->io_q.iocbs[idx++] = iocb;
> +    s->io_q.idx = idx;
> +
> +    /* submit immediately if queue is full */
> +    if (idx == s->io_q.size)
> +        ioq_submit(s);

Missing braces.

> +}
> +
> +void laio_io_plug(BlockDriverState *bs, void *aio_ctx)
> +{
> +    struct qemu_laio_state *s = aio_ctx;
> +
> +    s->io_q.plugged++;
> +}
> +
> +int laio_io_unplug(BlockDriverState *bs, void *aio_ctx, bool unplug)
> +{
> +    struct qemu_laio_state *s = aio_ctx;
> +    int ret = 0;
> +

How about an assert(s->io_q.plugged > 0); here?

> +    if (unplug && --s->io_q.plugged > 0)
> +        return 0;

Missing braces.

> +
> +    if (s->io_q.idx > 0) {
> +        ret = ioq_submit(s);
> +    }
> +
> +    return ret;
> +}
> +
>  BlockDriverAIOCB *laio_submit(BlockDriverState *bs, void *aio_ctx, int fd,
>          int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>          BlockDriverCompletionFunc *cb, void *opaque, int type)