comments in-line On 02/04/13 13:12, Stefan Hajnoczi wrote: > AioHandler already has a GPollFD so we can directly use its > events/revents. > > Add the int pollfds_idx field to AioContext so we can map g_poll(3) > results back to AioHandlers. > > Reuse aio_dispatch() to invoke handlers after g_poll(3). > > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > --- > aio-posix.c | 67 > +++++++++++++++++++---------------------------------- > async.c | 2 ++ > include/block/aio.h | 3 +++ > 3 files changed, 29 insertions(+), 43 deletions(-)
Question 0: I'm thoroughly confused by aio_poll(), even the pre-patch version. It seems to walk two lists in the AioContext: first the bottom halves, then aio_handlers. The interesting thing is that the "query" is different between the two lists (aio_bh_poll() vs. manual iteration in aio_poll()), but the "action" is the same (after the patch anyway): aio_dispatch(). (q0 cont) I don't understand how aio_bh_poll() communicates with the subsequent aio_dispatch(). Do the BH callbacks set some "ctx->aio_handlers{->next}->pfd.revents"? More to-the-earth questions (unfortunately, quite interleaved -- it's likely best to iterate over the rest of this email four times, once for each question separately): (q1) you're adding AioContext.pollfds: > diff --git a/include/block/aio.h b/include/block/aio.h > index 8eda924..5b54d38 100644 > --- a/include/block/aio.h > +++ b/include/block/aio.h > @@ -63,6 +63,9 @@ typedef struct AioContext { > > /* Used for aio_notify. */ > EventNotifier notifier; > + > + /* GPollFDs for aio_poll() */ > + GArray *pollfds; > } AioContext; > > /* Returns 1 if there are still outstanding AIO requests; 0 otherwise */ (q1) pollfds destroy/create: > diff --git a/async.c b/async.c > index 72d268a..f2d47ba 100644 > --- a/async.c > +++ b/async.c > @@ -174,6 +174,7 @@ aio_ctx_finalize(GSource *source) > > aio_set_event_notifier(ctx, &ctx->notifier, NULL, NULL); > event_notifier_cleanup(&ctx->notifier); > + g_array_free(ctx->pollfds, TRUE); > } > > static GSourceFuncs aio_source_funcs = { > @@ -198,6 +199,7 @@ AioContext *aio_context_new(void) > { > AioContext *ctx; > ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext)); > + ctx->pollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); > event_notifier_init(&ctx->notifier, false); > aio_set_event_notifier(ctx, &ctx->notifier, > (EventNotifierHandler *) (q1) Then > @@ -177,10 +179,7 @@ static bool aio_dispatch(AioContext *ctx) > > bool aio_poll(AioContext *ctx, bool blocking) > { > - static struct timeval tv0; > AioHandler *node; > - fd_set rdfds, wrfds; > - int max_fd = -1; > int ret; > bool busy, progress; > > @@ -206,12 +205,13 @@ bool aio_poll(AioContext *ctx, bool blocking) > > ctx->walking_handlers++; > > - FD_ZERO(&rdfds); > - FD_ZERO(&wrfds); > + g_array_set_size(ctx->pollfds, 0); (q1) the pollfds array is truncated here, > > - /* fill fd sets */ > + /* fill pollfds */ > busy = false; > QLIST_FOREACH(node, &ctx->aio_handlers, node) { > + node->pollfds_idx = -1; > + (q2) the new pollfds_idx is set to -1 here, > /* If there aren't pending AIO operations, don't invoke callbacks. > * Otherwise, if there are no AIO requests, qemu_aio_wait() would > * wait indefinitely. > @@ -222,13 +222,13 @@ bool aio_poll(AioContext *ctx, bool blocking) > } > busy = true; > } > - if (!node->deleted && node->io_read) { > - FD_SET(node->pfd.fd, &rdfds); > - max_fd = MAX(max_fd, node->pfd.fd + 1); > - } > - if (!node->deleted && node->io_write) { > - FD_SET(node->pfd.fd, &wrfds); > - max_fd = MAX(max_fd, node->pfd.fd + 1); > + if (!node->deleted && node->pfd.events) { > + GPollFD pfd = { > + .fd = node->pfd.fd, > + .events = node->pfd.events, > + }; > + node->pollfds_idx = ctx->pollfds->len; > + g_array_append_val(ctx->pollfds, pfd); (q1) pollfds is extended here, (q3) the controlling expressions' conversion seems fine; "node->pfd.events" should be nonzero iff io_read and/or io_write are present, according to aio_set_fd_handler(). (q3 cont) FWIW I wonder if you could have dealt away with the "pfd" local variable here, and just added (deep-copied) "node->pfd" into "ctx->pollfds". The difference is that the array entry's "revents" field would come from "node->pfd" as well (instead of being constant-zero). (q3 cont) Question 3: can node->pfd.revents be nonzero here? (And even so, would it matter for g_poll()?) I think not; aio_dispatch() seems to clear it. > } > } > > @@ -240,41 +240,22 @@ bool aio_poll(AioContext *ctx, bool blocking) > } > > /* wait until next event */ > - ret = select(max_fd, &rdfds, &wrfds, NULL, blocking ? NULL : &tv0); > + ret = g_poll((GPollFD *)ctx->pollfds->data, > + ctx->pollfds->len, > + blocking ? -1 : 0); (q1) pollfds being polled, > > /* if we have any readable fds, dispatch event */ > if (ret > 0) { > - /* we have to walk very carefully in case > - * qemu_aio_set_fd_handler is called while we're walking */ > - node = QLIST_FIRST(&ctx->aio_handlers); > - while (node) { > - AioHandler *tmp; > - > - ctx->walking_handlers++; > - > - if (!node->deleted && > - FD_ISSET(node->pfd.fd, &rdfds) && > - node->io_read) { > - node->io_read(node->opaque); > - progress = true; > - } > - if (!node->deleted && > - FD_ISSET(node->pfd.fd, &wrfds) && > - node->io_write) { > - node->io_write(node->opaque); > - progress = true; > - } > - > - tmp = node; > - node = QLIST_NEXT(node, node); > - > - ctx->walking_handlers--; > - > - if (!ctx->walking_handlers && tmp->deleted) { > - QLIST_REMOVE(tmp, node); > - g_free(tmp); > + QLIST_FOREACH(node, &ctx->aio_handlers, node) { > + if (node->pollfds_idx != -1) { (q2) pollfds_idx is queried here. I think new aio_handlers cannot be installed between the previous (q2) spot and here, via: @@ -85,6 +86,7 @@ void aio_set_fd_handler(AioContext *ctx, node->io_write = io_write; node->io_flush = io_flush; node->opaque = opaque; + node->pollfds_idx = -1; node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP : 0); node->pfd.events |= (io_write ? G_IO_OUT : 0); (q2 cont) So Question 2: setting pollfds_idx to -1 in set_fd_handler() is more like general hygiene, right? Back to aio_poll(), > + GPollFD *pfd = &g_array_index(ctx->pollfds, GPollFD, > + node->pollfds_idx); (q1) and pollfds is processed here. Question 1: I'm unable to see how the contents of "ctx->pollfds" could survive aio_poll(). It looks like a variable local to aio_poll() would suffice. (q1 cont) One point where the in-patch solution is more convenient is the "No AIO operations? Get us out of here" return statement: you don't have to free the array. But I think, if this array is indeed temporary for a single invocation of aio_poll(), then it would be cleaner not to leak it into AioContext. > + node->pfd.revents |= pfd->revents; (q4) Question 4: any specific reason to use |= here (not counting general versatility)? Tying in with (q3), "node->pfd.revents" seems to be zero here. > } > } > + if (aio_dispatch(ctx)) { > + progress = true; > + } > } > > assert(progress || busy); Anyway my confusion should not get into the way of this patch. In general questions should rather help the submitter spot any possible problems. Reviewed-by: Laszlo Ersek <ler...@redhat.com>