From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34482) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VCCdP-0007ZH-Ku for qemu-devel@nongnu.org; Wed, 21 Aug 2013 13:56:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VCCdI-0000Dr-8M for qemu-devel@nongnu.org; Wed, 21 Aug 2013 13:55:55 -0400 Received: from v220110690675601.yourvserver.net ([37.221.199.173]:48095) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VCCdH-0000De-Kh for qemu-devel@nongnu.org; Wed, 21 Aug 2013 13:55:47 -0400 Message-ID: <5214FF1E.1000201@weilnetz.de> Date: Wed, 21 Aug 2013 19:55:42 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1376921877-9576-1-git-send-email-stefanha@redhat.com> <1376921877-9576-7-git-send-email-stefanha@redhat.com> In-Reply-To: <1376921877-9576-7-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 06/18] aio: stop using .io_flush() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Paolo Bonzini , Anthony Liguori , qemu-devel@nongnu.org Hi Stefan, this patch was committed yesterday. It breaks MinGW builds: aio-win32.c: In function =E2=80=98aio_poll=E2=80=99: aio-win32.c:128:21: error: =E2=80=98AioHandler=E2=80=99 has no member nam= ed =E2=80=98opaque=E2=80=99 aio-win32.c:191:25: error: =E2=80=98AioHandler=E2=80=99 has no member nam= ed =E2=80=98opaque=E2=80=99 Regards, Stefan Am 19.08.2013 16:17, schrieb Stefan Hajnoczi: > 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 fil= e > 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 chec= k > 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 wheneve= r > '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 thes= e > 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' an= d > aio_poll() avoids blocking when the user has not set any fd handlers ye= t. > > 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: Paolo Bonzini > Signed-off-by: Stefan Hajnoczi > --- > aio-posix.c | 29 +++++++++-------------------- > aio-win32.c | 34 ++++++++++++++-------------------- > tests/test-aio.c | 10 +++++----- > 3 files changed, 28 insertions(+), 45 deletions(-) > [...] > diff --git a/aio-win32.c b/aio-win32.c > index 38723bf..4309c16 100644 > --- a/aio-win32.c > +++ b/aio-win32.c > @@ -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 =3D io_notify; > - node->io_flush =3D io_flush; > } > =20 > 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; > =20 > progress =3D false; > @@ -126,7 +124,11 @@ bool aio_poll(AioContext *ctx, bool blocking) > if (node->pfd.revents && node->io_notify) { > node->pfd.revents =3D 0; > node->io_notify(node->e); > - progress =3D true; > + > + /* aio_notify() does not count as progress */ > + if (node->opaque !=3D &ctx->notifier) { There is currently no member opaque for MinGW. > + progress =3D true; > + } > } > =20 > tmp =3D node; > @@ -147,19 +149,8 @@ bool aio_poll(AioContext *ctx, bool blocking) > ctx->walking_handlers++; > =20 > /* fill fd sets */ > - busy =3D false; > count =3D 0; > QLIST_FOREACH(node, &ctx->aio_handlers, node) { > - /* If there aren't pending AIO operations, don't invoke callba= cks. > - * Otherwise, if there are no AIO requests, qemu_aio_wait() wo= uld > - * wait indefinitely. > - */ > - if (!node->deleted && node->io_flush) { > - if (node->io_flush(node->e) =3D=3D 0) { > - continue; > - } > - busy =3D true; > - } > if (!node->deleted && node->io_notify) { > events[count++] =3D event_notifier_get_handle(node->e); > } > @@ -167,8 +158,8 @@ bool aio_poll(AioContext *ctx, bool blocking) > =20 > ctx->walking_handlers--; > =20 > - /* No AIO operations? Get us out of here */ > - if (!busy) { > + /* early return if we only have the aio_notify() fd */ > + if (count =3D=3D 1) { > return progress; > } > =20 > @@ -196,7 +187,11 @@ bool aio_poll(AioContext *ctx, bool blocking) > event_notifier_get_handle(node->e) =3D=3D events[ret -= WAIT_OBJECT_0] && > node->io_notify) { > node->io_notify(node->e); > - progress =3D true; > + > + /* aio_notify() does not count as progress */ > + if (node->opaque !=3D &ctx->notifier) { > + progress =3D true; > + } > } > =20 > tmp =3D node; > @@ -214,6 +209,5 @@ bool aio_poll(AioContext *ctx, bool blocking) > events[ret - WAIT_OBJECT_0] =3D events[--count]; > } > =20 > - assert(progress || busy); > - return true; > + return progress; > } > diff --git a/tests/test-aio.c b/tests/test-aio.c > index 20bf5e6..1251952 100644 > --- a/tests/test-aio.c > +++ b/tests/test-aio.c > @@ -254,7 +254,7 @@ static void test_wait_event_notifier(void) > EventNotifierTestData data =3D { .n =3D 0, .active =3D 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, =3D=3D, 0); > g_assert_cmpint(data.active, =3D=3D, 1); > =20 > @@ -279,7 +279,7 @@ static void test_flush_event_notifier(void) > EventNotifierTestData data =3D { .n =3D 0, .active =3D 10, .auto_s= et =3D 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, =3D=3D, 0); > g_assert_cmpint(data.active, =3D=3D, 10); > =20 > @@ -313,7 +313,7 @@ static void test_wait_event_notifier_noflush(void) > /* Until there is an active descriptor, aio_poll may or may not ca= ll > * 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 =3D 0; > =20 > /* An active event notifier forces aio_poll to look at EventNotifi= ers. */ > @@ -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, =3D=3D, 1); > - g_assert(aio_poll(ctx, false)); > + g_assert(!aio_poll(ctx, false)); > g_assert_cmpint(data.n, =3D=3D, 1); > =20 > event_notifier_set(&data.e); > g_assert(aio_poll(ctx, false)); > g_assert_cmpint(data.n, =3D=3D, 2); > - g_assert(aio_poll(ctx, false)); > + g_assert(!aio_poll(ctx, false)); > g_assert_cmpint(data.n, =3D=3D, 2); > =20 > event_notifier_set(&dummy.e);