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

[Ming Lei]

Hi Kevin,

On Thu, Jul 3, 2014 at 5:40 PM, Kevin Wolf <address@hidden> wrote:
> 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 <address@hidden>
>
> Just a couple of minor comments, see inline.

Thanks for your review.

>
>>  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?

how about just adding a warning because flush io queue uses
the function too?

Also that is why 'plugged' is defined as signed.

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


Thanks,