Commit 164a101f authored by Stefan Hajnoczi's avatar Stefan Hajnoczi
Browse files

aio: stop using .io_flush() 



Now that aio_poll() users check their termination condition themselves,
it is no longer necessary to call .io_flush() handlers.

The behavior of aio_poll() changes as follows:

1. .io_flush() is no longer invoked and file descriptors are *always*
monitored.  Previously returning 0 from .io_flush() would skip this file
descriptor.

Due to this change it is essential to check that requests are pending
before calling qemu_aio_wait().  Failure to do so means we block, for
example, waiting for an idle iSCSI socket to become readable when there
are no requests.  Currently all qemu_aio_wait()/aio_poll() callers check
before calling.

2. aio_poll() now returns true if progress was made (BH or fd handlers
executed) and false otherwise.  Previously it would return true whenever
'busy', which means that .io_flush() returned true.  The 'busy' concept
no longer exists so just progress is returned.

Due to this change we need to update tests/test-aio.c which asserts
aio_poll() return values.  Note that QEMU doesn't actually rely on these
return values so only tests/test-aio.c cares.

Note that ctx->notifier, the EventNotifier fd used for aio_notify(), is
now handled as a special case.  This is a little ugly but maintains
aio_poll() semantics, i.e. aio_notify() does not count as 'progress' and
aio_poll() avoids blocking when the user has not set any fd handlers yet.

Patches after this remove .io_flush() handler code until we can finally
drop the io_flush arguments to aio_set_fd_handler() and friends.

Reviewed-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
parent 35ecde26

aio-posix.c

+9 −20
Original line number Diff line number Diff line
@@ -23,7 +23,6 @@ struct AioHandler 
    GPollFD pfd;

    IOHandler *io_read;

    IOHandler *io_write;

    AioFlushHandler *io_flush;

    int deleted;

    int pollfds_idx;

    void *opaque;
@@ -84,7 +83,6 @@ void aio_set_fd_handler(AioContext *ctx, 
        /* Update handler with latest information */

        node->io_read = io_read;

        node->io_write = io_write;

        node->io_flush = io_flush;

        node->opaque = opaque;

        node->pollfds_idx = -1;
@@ -147,8 +145,12 @@ static bool aio_dispatch(AioContext *ctx) 
            (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) &&

            node->io_read) {

            node->io_read(node->opaque);



            /* aio_notify() does not count as progress */

            if (node->opaque != &ctx->notifier) {

                progress = true;

            }

        }

        if (!node->deleted &&

            (revents & (G_IO_OUT | G_IO_ERR)) &&

            node->io_write) {
@@ -173,7 +175,7 @@ bool aio_poll(AioContext *ctx, bool blocking) 
{

    AioHandler *node;

    int ret;

    bool busy, progress;

    bool progress;



    progress = false;
@@ -200,20 +202,8 @@ bool aio_poll(AioContext *ctx, bool blocking) 
    g_array_set_size(ctx->pollfds, 0);



    /* fill pollfds */

    busy = false;

    QLIST_FOREACH(node, &ctx->aio_handlers, node) {

        node->pollfds_idx = -1;



        /* If there aren't pending AIO operations, don't invoke callbacks.

         * 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) {

                continue;

            }

            busy = true;

        }

        if (!node->deleted && node->pfd.events) {

            GPollFD pfd = {

                .fd = node->pfd.fd,
@@ -226,8 +216,8 @@ bool aio_poll(AioContext *ctx, bool blocking) 


    ctx->walking_handlers--;



    /* No AIO operations?  Get us out of here */

    if (!busy) {

    /* early return if we only have the aio_notify() fd */

    if (ctx->pollfds->len == 1) {

        return progress;

    }
@@ -250,6 +240,5 @@ bool aio_poll(AioContext *ctx, bool blocking) 
        }

    }



    assert(progress || busy);

    return true;

    return progress;

}

aio-win32.c

+14 −20
Original line number Diff line number Diff line
@@ -23,7 +23,6 @@ 
struct AioHandler {

    EventNotifier *e;

    EventNotifierHandler *io_notify;

    AioFlushEventNotifierHandler *io_flush;

    GPollFD pfd;

    int deleted;

    QLIST_ENTRY(AioHandler) node;
@@ -73,7 +72,6 @@ void aio_set_event_notifier(AioContext *ctx, 
        }

        /* Update handler with latest information */

        node->io_notify = io_notify;

        node->io_flush = io_flush;

    }



    aio_notify(ctx);
@@ -96,7 +94,7 @@ bool aio_poll(AioContext *ctx, bool blocking) 
{

    AioHandler *node;

    HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];

    bool busy, progress;

    bool progress;

    int count;



    progress = false;
@@ -126,8 +124,12 @@ bool aio_poll(AioContext *ctx, bool blocking) 
        if (node->pfd.revents && node->io_notify) {

            node->pfd.revents = 0;

            node->io_notify(node->e);



            /* aio_notify() does not count as progress */

            if (node->opaque != &ctx->notifier) {

                progress = true;

            }

        }



        tmp = node;

        node = QLIST_NEXT(node, node);
@@ -147,19 +149,8 @@ bool aio_poll(AioContext *ctx, bool blocking) 
    ctx->walking_handlers++;



    /* fill fd sets */

    busy = false;

    count = 0;

    QLIST_FOREACH(node, &ctx->aio_handlers, node) {

        /* If there aren't pending AIO operations, don't invoke callbacks.

         * 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) {

                continue;

            }

            busy = true;

        }

        if (!node->deleted && node->io_notify) {

            events[count++] = event_notifier_get_handle(node->e);

        }
@@ -167,8 +158,8 @@ bool aio_poll(AioContext *ctx, bool blocking) 


    ctx->walking_handlers--;



    /* No AIO operations?  Get us out of here */

    if (!busy) {

    /* early return if we only have the aio_notify() fd */

    if (count == 1) {

        return progress;

    }
@@ -196,8 +187,12 @@ bool aio_poll(AioContext *ctx, bool blocking) 
                event_notifier_get_handle(node->e) == events[ret - WAIT_OBJECT_0] &&

                node->io_notify) {

                node->io_notify(node->e);



                /* aio_notify() does not count as progress */

                if (node->opaque != &ctx->notifier) {

                    progress = true;

                }

            }



            tmp = node;

            node = QLIST_NEXT(node, node);
@@ -214,6 +209,5 @@ bool aio_poll(AioContext *ctx, bool blocking) 
        events[ret - WAIT_OBJECT_0] = events[--count];

    }



    assert(progress || busy);

    return true;

    return progress;

}

tests/test-aio.c

+5 −5
Original line number Diff line number Diff line
@@ -254,7 +254,7 @@ static void test_wait_event_notifier(void) 
    EventNotifierTestData data = { .n = 0, .active = 1 };

    event_notifier_init(&data.e, false);

    aio_set_event_notifier(ctx, &data.e, event_ready_cb, event_active_cb);

    g_assert(aio_poll(ctx, false));

    g_assert(!aio_poll(ctx, false));

    g_assert_cmpint(data.n, ==, 0);

    g_assert_cmpint(data.active, ==, 1);
@@ -279,7 +279,7 @@ static void test_flush_event_notifier(void) 
    EventNotifierTestData data = { .n = 0, .active = 10, .auto_set = true };

    event_notifier_init(&data.e, false);

    aio_set_event_notifier(ctx, &data.e, event_ready_cb, event_active_cb);

    g_assert(aio_poll(ctx, false));

    g_assert(!aio_poll(ctx, false));

    g_assert_cmpint(data.n, ==, 0);

    g_assert_cmpint(data.active, ==, 10);
@@ -313,7 +313,7 @@ static void test_wait_event_notifier_noflush(void) 
    /* Until there is an active descriptor, aio_poll may or may not call

     * event_ready_cb.  Still, it must not block.  */

    event_notifier_set(&data.e);

    g_assert(!aio_poll(ctx, true));

    g_assert(aio_poll(ctx, true));

    data.n = 0;



    /* An active event notifier forces aio_poll to look at EventNotifiers.  */
@@ -323,13 +323,13 @@ static void test_wait_event_notifier_noflush(void) 
    event_notifier_set(&data.e);

    g_assert(aio_poll(ctx, false));

    g_assert_cmpint(data.n, ==, 1);

    g_assert(aio_poll(ctx, false));

    g_assert(!aio_poll(ctx, false));

    g_assert_cmpint(data.n, ==, 1);



    event_notifier_set(&data.e);

    g_assert(aio_poll(ctx, false));

    g_assert_cmpint(data.n, ==, 2);

    g_assert(aio_poll(ctx, false));

    g_assert(!aio_poll(ctx, false));

    g_assert_cmpint(data.n, ==, 2);



    event_notifier_set(&dummy.e);