Re: [Qemu-devel] [RFC PATCH 3/3] linux-aio: Don't reenter request coroutine recursively

[Ming Lei]

On Thu, Dec 4, 2014 at 10:37 PM, Kevin Wolf <address@hidden> wrote:
> Am 26.11.2014 um 15:46 hat Kevin Wolf geschrieben:
>> When getting an error while submitting requests, we must be careful to
>> wake up only inactive coroutines. Therefore we must special-case the
>> currently active coroutine and communicate an error for that request
>> using the ordinary return value of ioq_submit().
>>
>> Signed-off-by: Kevin Wolf <address@hidden>
>> ---
>>  block/linux-aio.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> Ming, did you have a look at this patch specifically? Does it fix the
> issue that you tried to address with a much more complex callback-based
> patch?

I think your patch can't fix my issue.

As I explained, we have to handle -EAGAIN and partial submission,
which can be triggered quite easily in case of multi-queue, and other
case like NVME.

Thanks,

>
> I'd like to go forward with this as both Peter and I have measured
> considerable performance improvements with our optimisation proposals,
> and this series is an important part of it.
>
> Kevin
>
>
>> diff --git a/block/linux-aio.c b/block/linux-aio.c
>> index 99b259d..fd8f0e4 100644
>> --- a/block/linux-aio.c
>> +++ b/block/linux-aio.c
>> @@ -177,12 +177,19 @@ static int ioq_submit(struct qemu_laio_state *s)
>>              container_of(s->io_q.iocbs[i], struct qemu_laiocb, iocb);
>>
>>          laiocb->ret = (ret < 0) ? ret : -EIO;
>> -        qemu_laio_process_completion(s, laiocb);
>> +        if (laiocb->co != qemu_coroutine_self()) {
>> +            qemu_coroutine_enter(laiocb->co, NULL);
>> +        } else {
>> +            /* The return value is used for the currently active coroutine.
>> +             * We're always in ioq_enqueue() here, ioq_submit() never runs 
>> from
>> +             * a request's coroutine.*/
>> +            ret = laiocb->ret;
>> +        }
>>      }
>>      return ret;
>>  }
>>
>> -static void ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb)
>> +static int ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb)
>>  {
>>      unsigned int idx = s->io_q.idx;
>>
>> @@ -191,7 +198,9 @@ static void ioq_enqueue(struct qemu_laio_state *s, 
>> struct iocb *iocb)
>>
>>      /* submit immediately if queue is full */
>>      if (idx == s->io_q.size) {
>> -        ioq_submit(s);
>> +        return ioq_submit(s);
>> +    } else {
>> +        return 0;
>>      }
>>  }
>>
>> @@ -253,11 +262,11 @@ int laio_submit_co(BlockDriverState *bs, void 
>> *aio_ctx, int fd,
>>
>>      if (!s->io_q.plugged) {
>>          ret = io_submit(s->ctx, 1, &iocbs);
>> -        if (ret < 0) {
>> -            return ret;
>> -        }
>>      } else {
>> -        ioq_enqueue(s, iocbs);
>> +        ret = ioq_enqueue(s, iocbs);
>> +    }
>> +    if (ret < 0) {
>> +        return ret;
>>      }
>>
>>      qemu_coroutine_yield();
>> --
>> 1.8.3.1
>>
>