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

[Ming Lei]

Hi Stefan,

Sorry for missing your comments.

On Thu, Jul 3, 2014 at 8:24 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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.

OK.

> Please always run patches throught scripts/checkpatch.pl before
> submitting them.

Looks no failure and warning.

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

I thought about the problem before, but looks it may not return 'ret'
which is less than
len, follows the man io_submit():

       The  function  returns immediately after having enqueued all
the requests.  On suc‐
       cess, io_submit returns the number of  iocbs  submitted
successfully.   Otherwise,
       -error  is return, where error is one of the Exxx values
defined in the Errors sec‐
       tion.

The above description looks a bit confusing, and I will check linux
fs/aio.c to see
if there is the case.

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

All these coding style problems have been fixed.

Thanks,