| Message ID | 1364507550-25093-2-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Il 28/03/2013 22:52, Anthony Liguori ha scritto: > Today, all callers of qemu_aio_set_fd_handler() pass a valid io_flush > function. Except one: aio_set_event_notifier(ctx, &ctx->notifier, (EventNotifierHandler *) event_notifier_test_and_clear, NULL); This is the EventNotifier that is used by qemu_notify_event. It's quite surprising that this patch works and passes the tests. /me reads cover letter... ah, it is untested. :) But if you can eliminate the sole usage of aio_wait()'s return value (in bdrv_drain_all()), everything would be much simpler. There is a relatively convenient assert(QLIST_EMPTY(&bs->tracked_requests)); that you can use as the exit condition instead. Perhaps it's not trivial to do it efficiently, but it's not a fast path. Paolo > However, the function allows the handler to be omitted > and the behavior is a bit strange. > > It will still add the file descriptor to the GSource but it will > not consider the source to be "busy". This could lead to aio_flush() > returning prematurely. > > Since we never rely on this behavior today, it doesn't matter but > the next patch will start relying on an absent io_flush function > to assume the handler is always busy.
Am 29.03.2013 um 00:37 hat Paolo Bonzini geschrieben: > Il 28/03/2013 22:52, Anthony Liguori ha scritto: > > Today, all callers of qemu_aio_set_fd_handler() pass a valid io_flush > > function. > > Except one: > > aio_set_event_notifier(ctx, &ctx->notifier, > (EventNotifierHandler *) > event_notifier_test_and_clear, NULL); > > This is the EventNotifier that is used by qemu_notify_event. > > It's quite surprising that this patch works and passes the tests. /me > reads cover letter... ah, it is untested. :) > > But if you can eliminate the sole usage of aio_wait()'s return value (in > bdrv_drain_all()), everything would be much simpler. There is a > relatively convenient > > assert(QLIST_EMPTY(&bs->tracked_requests)); > > that you can use as the exit condition instead. Perhaps it's not > trivial to do it efficiently, but it's not a fast path. We just need to move to .bdrv_drain() for all block driver that register an AioHandler. I'm pretty sure that each one has its own data structures to manage in-flight requests (basically what is the aio_flush handler today would become the .bdrv_drain callback). Then bdrv_drain_all() can directly use the bdrv_drain() return value and doesn't need to have it passed through aio_wait() any more. Kevin
On Fri, Mar 29, 2013 at 12:37:18AM +0100, Paolo Bonzini wrote: > Il 28/03/2013 22:52, Anthony Liguori ha scritto: > > Today, all callers of qemu_aio_set_fd_handler() pass a valid io_flush > > function. > > Except one: > > aio_set_event_notifier(ctx, &ctx->notifier, > (EventNotifierHandler *) > event_notifier_test_and_clear, NULL); > > This is the EventNotifier that is used by qemu_notify_event. > > It's quite surprising that this patch works and passes the tests. /me > reads cover letter... ah, it is untested. :) This one instance is easy enough to fix by a return 0 io_flush handler. Stefan
diff --git a/aio-posix.c b/aio-posix.c index b68eccd..a2349f6 100644 --- a/aio-posix.c +++ b/aio-posix.c @@ -208,8 +208,8 @@ bool aio_poll(AioContext *ctx, bool blocking) * Otherwise, if there are no AIO requests, qemu_aio_wait() would * wait indefinitely. */ - if (!node->deleted && node->io_flush) { - if (node->io_flush(node->opaque) == 0) { + if (!node->deleted) { + if (node->io_flush && node->io_flush(node->opaque) == 0) { continue; } busy = true; diff --git a/aio-win32.c b/aio-win32.c index 38723bf..b02fd40 100644 --- a/aio-win32.c +++ b/aio-win32.c @@ -154,8 +154,8 @@ bool aio_poll(AioContext *ctx, bool blocking) * Otherwise, if there are no AIO requests, qemu_aio_wait() would * wait indefinitely. */ - if (!node->deleted && node->io_flush) { - if (node->io_flush(node->e) == 0) { + if (!node->deleted) { + if (node->io_flush && node->io_flush(node->e) == 0) { continue; } busy = true;
Today, all callers of qemu_aio_set_fd_handler() pass a valid io_flush function. However, the function allows the handler to be omitted and the behavior is a bit strange. It will still add the file descriptor to the GSource but it will not consider the source to be "busy". This could lead to aio_flush() returning prematurely. Since we never rely on this behavior today, it doesn't matter but the next patch will start relying on an absent io_flush function to assume the handler is always busy. Cc: Paolo Bonzini <bonzini@redhat.com> Cc: Mike Roth <mdroth@linux.vnet.ibm.com> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- aio-posix.c | 4 ++-- aio-win32.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)