Hi Kevin,
On Thu, Jul 3, 2014 at 5:40 PM, Kevin Wolf <kw...@redhat.com> 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 <ming....@canonical.com>
>
> 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,
