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

[Stefan Hajnoczi]

On Wed, Jul 02, 2014 at 08:18:47PM +0800, Ming Lei wrote:
> @@ -36,9 +38,19 @@ struct qemu_laiocb {
>      QLIST_ENTRY(qemu_laiocb) node;
>  };
>  
> +struct laio_queue {

This file doesn't follow QEMU coding style for structs but normally this
should be:
typedef struct {
    ...
} LaioQueue;

Up to you if you want to fix it.  See ./HACKING and ./CODING_STYLE.
Please always run patches throught scripts/checkpatch.pl before
submitting them.

> +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;

This leaks requests when ret < len.  I think the loop below should be
used in that case to fail unsubmitted requests with -EIO.

Also, QEMU always uses {} even when there is only one statement in the
if body and 4-space indentation.

> +
> +    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 {}

> +}
> +
> +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;
> +
> +    if (unplug && --s->io_q.plugged > 0)
> +        return 0;

Missing {}

pgpRwh47SpBqf.pgp
Description: PGP signature