Re: [Qemu-devel] [PATCH v3 2/3] linux-aio: handling -EAGAIN for !s-io_q.plugged case
On 18/11/2014 15:06, Paolo Bonzini wrote:
+/* don't submit until next completion for -EAGAIN of non plug case */
+if (unlikely(!s-io_q.plugged)) {
+return 0;
+}
+
Is this an optimization or a fix for something?
Ah, if !io_q.plugged we must be in the -EAGAIN case, so no need to
submit immediately if queue depth is above 2/3. Can you rewrite the
comment like this:
/* This is reached in two cases: queue not plugged but io_submit
* returned -EAGAIN, or queue plugged. In the latter case, start
* submitting some I/O if the queue is getting too full. In the
* former case, instead, wait until an I/O operation is completed.
*/
if (likely(idx = s-io_q.size * 2 / 3) || unlikely(!s-io_q.plugged) {
return 0;
}
Paolo
Re: [Qemu-devel] [PATCH v3 2/3] linux-aio: handling -EAGAIN for !s-io_q.plugged case
On Mon, Nov 24, 2014 at 5:52 PM, Paolo Bonzini pbonz...@redhat.com wrote:
On 18/11/2014 15:06, Paolo Bonzini wrote:
+/* don't submit until next completion for -EAGAIN of non plug case */
+if (unlikely(!s-io_q.plugged)) {
+return 0;
+}
+
Is this an optimization or a fix for something?
Ah, if !io_q.plugged we must be in the -EAGAIN case, so no need to
submit immediately if queue depth is above 2/3. Can you rewrite the
The above comment is only for io_q.plugged case, and for
!io_q.plugged with -EAGAIN, the requests is submitted in
retry path.
comment like this:
/* This is reached in two cases: queue not plugged but io_submit
* returned -EAGAIN, or queue plugged. In the latter case, start
* submitting some I/O if the queue is getting too full. In the
* former case, instead, wait until an I/O operation is completed.
*/
if (likely(idx = s-io_q.size * 2 / 3) || unlikely(!s-io_q.plugged) {
return 0;
}
Yes, the above change need the corresponding comment, but this
patch needn't since the 1st case won't reach here, :-)
Thanks,
Ming Lei
Re: [Qemu-devel] [PATCH v3 2/3] linux-aio: handling -EAGAIN for !s-io_q.plugged case
On 24/11/2014 11:11, Ming Lei wrote: Yes, the above change need the corresponding comment, but this patch needn't since the 1st case won't reach here, :-) I'm not sure I follow, but you can send v4 with a clearer comment I guess. :) Paolo
Re: [Qemu-devel] [PATCH v3 2/3] linux-aio: handling -EAGAIN for !s-io_q.plugged case
On Tue, Nov 18, 2014 at 10:06 PM, Paolo Bonzini pbonz...@redhat.com wrote:
On 06/11/2014 16:10, Ming Lei wrote:
+/* don't submit until next completion for -EAGAIN of non plug case */
+if (unlikely(!s-io_q.plugged)) {
+return 0;
+}
+
Is this an optimization or a fix for something?
It is for avoiding unnecessary submission which will cause
another -EAGAIN.
+/*
+ * Switch to queue mode until -EAGAIN is handled, we suppose
+ * there is always uncompleted I/O, so try to enqueue it first,
+ * and will be submitted again in following aio completion cb.
+ */
+if (ret == -EAGAIN) {
+goto enqueue;
+} else if (ret 0) {
goto out_free_aiocb;
}
Better:
if (!s-io_q.plugged !s-io_q.idx) {
ret = io_submit(s-ctx, 1, iocbs);
if (ret = 0) {
return laiocb-common;
}
if (ret != -EAGAIN) {
goto out_free_aiocb;
}
}
Right.
Thanks,
Ming Lei
Re: [Qemu-devel] [PATCH v3 2/3] linux-aio: handling -EAGAIN for !s-io_q.plugged case
On 06/11/2014 16:10, Ming Lei wrote:
+/* don't submit until next completion for -EAGAIN of non plug case */
+if (unlikely(!s-io_q.plugged)) {
+return 0;
+}
+
Is this an optimization or a fix for something?
+/*
+ * Switch to queue mode until -EAGAIN is handled, we suppose
+ * there is always uncompleted I/O, so try to enqueue it first,
+ * and will be submitted again in following aio completion cb.
+ */
+if (ret == -EAGAIN) {
+goto enqueue;
+} else if (ret 0) {
goto out_free_aiocb;
}
Better:
if (!s-io_q.plugged !s-io_q.idx) {
ret = io_submit(s-ctx, 1, iocbs);
if (ret = 0) {
return laiocb-common;
}
if (ret != -EAGAIN) {
goto out_free_aiocb;
}
}
/* code for queue mode. */
Paolo
[Qemu-devel] [PATCH v3 2/3] linux-aio: handling -EAGAIN for !s-io_q.plugged case
Previously -EAGAIN is simply ignored for !s-io_q.plugged case,
and sometimes it is easy to cause -EIO to VM, such as NVME device.
This patch handles -EAGAIN by io queue for !s-io_q.plugged case,
and it will be retried in following aio completion cb.
Suggested-by: Paolo Bonzini pbonz...@redhat.com
Signed-off-by: Ming Lei ming@canonical.com
---
block/linux-aio.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/block/linux-aio.c b/block/linux-aio.c
index f66e8ad..f5ca41d 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -263,6 +263,11 @@ static int ioq_enqueue(struct qemu_laio_state *s, struct
iocb *iocb)
s-io_q.iocbs[idx++] = iocb;
s-io_q.idx = idx;
+/* don't submit until next completion for -EAGAIN of non plug case */
+if (unlikely(!s-io_q.plugged)) {
+return 0;
+}
+
/* submit immediately if queue depth is above 2/3 */
if (idx s-io_q.size * 2 / 3) {
return ioq_submit(s, true);
@@ -330,10 +335,25 @@ BlockAIOCB *laio_submit(BlockDriverState *bs, void
*aio_ctx, int fd,
io_set_eventfd(laiocb-iocb, event_notifier_get_fd(s-e));
if (!s-io_q.plugged) {
-if (io_submit(s-ctx, 1, iocbs) 0) {
+int ret;
+
+if (!s-io_q.idx) {
+ret = io_submit(s-ctx, 1, iocbs);
+} else {
+ret = -EAGAIN;
+}
+/*
+ * Switch to queue mode until -EAGAIN is handled, we suppose
+ * there is always uncompleted I/O, so try to enqueue it first,
+ * and will be submitted again in following aio completion cb.
+ */
+if (ret == -EAGAIN) {
+goto enqueue;
+} else if (ret 0) {
goto out_free_aiocb;
}
} else {
+ enqueue:
if (ioq_enqueue(s, iocbs) 0) {
goto out_free_aiocb;
}
--
1.7.9.5
