Hi Stefan,
Sorry for missing your comments.
On Thu, Jul 3, 2014 at 8:24 PM, Stefan Hajnoczi <stefa...@gmail.com> 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,
