From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60526) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XPYWi-00071u-2q for qemu-devel@nongnu.org; Thu, 04 Sep 2014 11:00:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XPYWc-0005sp-2W for qemu-devel@nongnu.org; Thu, 04 Sep 2014 11:00:43 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:46734 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XPYWb-0005sc-Hr for qemu-devel@nongnu.org; Thu, 04 Sep 2014 11:00:37 -0400 Date: Thu, 4 Sep 2014 16:59:49 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140904145949.GC28417@irqsave.net> References: <1408009304-19241-1-git-send-email-ming.lei@canonical.com> <1408009304-19241-2-git-send-email-ming.lei@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1408009304-19241-2-git-send-email-ming.lei@canonical.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/4] linux-aio: fix submit aio as a batch List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ming Lei Cc: Kevin Wolf , Peter Maydell , qemu-devel@nongnu.org, Stefan Hajnoczi , Paolo Bonzini The Thursday 14 Aug 2014 =E0 17:41:41 (+0800), Ming Lei wrote : > In the enqueue path, we can't complete request, otherwise > "Co-routine re-entered recursively" may be caused, so this > patch fixes the issue with below ideas: s/with below ideas/with the following ideas/g >=20 > - for -EAGAIN or partial completion, retry the submision by s/submision/submission/ > schedule an BH in following completion cb s/schedule an/sheduling a/ > - for part of completion, also update the io queue > - for other failure, return the failure if in enqueue path, > otherwise, abort all queued I/O >=20 > Signed-off-by: Ming Lei > --- > block/linux-aio.c | 99 +++++++++++++++++++++++++++++++++++++++++----= -------- > 1 file changed, 77 insertions(+), 22 deletions(-) >=20 > diff --git a/block/linux-aio.c b/block/linux-aio.c > index 7ac7e8c..4cdf507 100644 > --- a/block/linux-aio.c > +++ b/block/linux-aio.c > @@ -38,11 +38,19 @@ struct qemu_laiocb { > QLIST_ENTRY(qemu_laiocb) node; > }; > =20 > +/* > + * TODO: support to batch I/O from multiple bs in one same > + * AIO context, one important use case is multi-lun scsi, > + * so in future the IO queue should be per AIO context. > + */ > typedef struct { In QEMU we typically write the name twice in these kind of declarations: typedef struct LaioQueue { ... stuff ... } LaioQueue; > struct iocb *iocbs[MAX_QUEUED_IO]; > int plugged; Are plugged values either 0 and 1 ? If so it should be "bool plugged;" > unsigned int size; > unsigned int idx; See: benoit@Laure:~/code/qemu$ git grep "unsigned int"|wc 2283 14038 154201 benoit@Laure:~/code/qemu$ git grep "uint32"|wc 12535 63129 810822 Maybe you could use the most popular type. > + > + /* handle -EAGAIN and partial completion */ > + QEMUBH *retry; > } LaioQueue; > =20 > struct qemu_laio_state { > @@ -86,6 +94,12 @@ static void qemu_laio_process_completion(struct qemu= _laio_state *s, > qemu_aio_release(laiocb); > } > =20 > +static void qemu_laio_start_retry(struct qemu_laio_state *s) > +{ > + if (s->io_q.idx) > + qemu_bh_schedule(s->io_q.retry); In QEMU this test is writen like this: if (s->io_q.idx) { qemu_bh_schedule(s->io_q.retry); } I suggest you ran ./scripts/checkpatch.pl on your series before submittin= g it. > +} > + > static void qemu_laio_completion_cb(EventNotifier *e) > { > struct qemu_laio_state *s =3D container_of(e, struct qemu_laio_sta= te, e); > @@ -108,6 +122,7 @@ static void qemu_laio_completion_cb(EventNotifier *= e) > qemu_laio_process_completion(s, laiocb); > } > } > + qemu_laio_start_retry(s); > } > =20 > static void laio_cancel(BlockDriverAIOCB *blockacb) > @@ -127,6 +142,7 @@ static void laio_cancel(BlockDriverAIOCB *blockacb) > ret =3D io_cancel(laiocb->ctx->ctx, &laiocb->iocb, &event); > if (ret =3D=3D 0) { > laiocb->ret =3D -ECANCELED; > + qemu_laio_start_retry(laiocb->ctx); > return; > } > =20 > @@ -154,45 +170,80 @@ static void ioq_init(LaioQueue *io_q) > io_q->plugged =3D 0; > } > =20 > -static int ioq_submit(struct qemu_laio_state *s) > +static void abort_queue(struct qemu_laio_state *s) > +{ > + int i; > + for (i =3D 0; i < s->io_q.idx; i++) { > + struct qemu_laiocb *laiocb =3D container_of(s->io_q.iocbs[i], > + struct qemu_laiocb, > + iocb); > + laiocb->ret =3D -EIO; > + qemu_laio_process_completion(s, laiocb); > + } > +} > + > +static int ioq_submit(struct qemu_laio_state *s, bool enqueue) > { > int ret, i =3D 0; > int len =3D s->io_q.idx; > + int j =3D 0; > =20 > - do { > - ret =3D io_submit(s->ctx, len, s->io_q.iocbs); > - } while (i++ < 3 && ret =3D=3D -EAGAIN); > + if (!len) { > + return 0; > + } > + > + ret =3D io_submit(s->ctx, len, s->io_q.iocbs); > + if (ret =3D=3D -EAGAIN) { /* retry in following completion cb */ > + return 0; > + } else if (ret < 0) { > + if (enqueue) { > + return ret; > + } > =20 > - /* empty io queue */ > - s->io_q.idx =3D 0; > + /* in non-queue path, all IOs have to be completed */ > + abort_queue(s); > + ret =3D len; > + } else if (ret =3D=3D 0) { > + goto out; > + } > =20 > - if (ret < 0) { > - i =3D 0; > - } else { > - i =3D ret; > + for (i =3D ret; i < len; i++) { > + s->io_q.iocbs[j++] =3D s->io_q.iocbs[i]; > } > =20 > - for (; i < len; i++) { > - struct qemu_laiocb *laiocb =3D > - container_of(s->io_q.iocbs[i], struct qemu_laiocb, iocb); > + out: > + /* > + * update io queue, for partial completion, retry will be > + * started automatically in following completion cb. > + */ > + s->io_q.idx -=3D ret; > =20 > - laiocb->ret =3D (ret < 0) ? ret : -EIO; > - qemu_laio_process_completion(s, laiocb); > - } > return ret; > } > =20 > -static void ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb) > +static void ioq_submit_retry(void *opaque) > +{ > + struct qemu_laio_state *s =3D opaque; > + ioq_submit(s, false); > +} > + > +static int ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb) > { > unsigned int idx =3D s->io_q.idx; > =20 > + if (unlikely(idx =3D=3D s->io_q.size)) { > + return -1; > + } > + > s->io_q.iocbs[idx++] =3D iocb; > s->io_q.idx =3D idx; > =20 > - /* submit immediately if queue is full */ > - if (idx =3D=3D s->io_q.size) { > - ioq_submit(s); > + /* submit immediately if queue depth is above 2/3 */ > + if (idx > s->io_q.size * 2 / 3) { > + return ioq_submit(s, true); > } > + > + return 0; > } > =20 > void laio_io_plug(BlockDriverState *bs, void *aio_ctx) > @@ -214,7 +265,7 @@ int laio_io_unplug(BlockDriverState *bs, void *aio_= ctx, bool unplug) > } > =20 > if (s->io_q.idx > 0) { > - ret =3D ioq_submit(s); > + ret =3D ioq_submit(s, false); > } > =20 > return ret; > @@ -258,7 +309,9 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs,= void *aio_ctx, int fd, > goto out_free_aiocb; > } > } else { > - ioq_enqueue(s, iocbs); > + if (ioq_enqueue(s, iocbs) < 0) { > + goto out_free_aiocb; > + } > } > return &laiocb->common; > =20 > @@ -272,6 +325,7 @@ void laio_detach_aio_context(void *s_, AioContext *= old_context) > struct qemu_laio_state *s =3D s_; > =20 > aio_set_event_notifier(old_context, &s->e, NULL); > + qemu_bh_delete(s->io_q.retry); > } > =20 > void laio_attach_aio_context(void *s_, AioContext *new_context) > @@ -279,6 +333,7 @@ void laio_attach_aio_context(void *s_, AioContext *= new_context) > struct qemu_laio_state *s =3D s_; > =20 > aio_set_event_notifier(new_context, &s->e, qemu_laio_completion_cb= ); > + s->io_q.retry =3D aio_bh_new(new_context, ioq_submit_retry, s); > } > =20 > void *laio_init(void) > --=20 > 1.7.9.5 >=20 >=20