On Tue, Sep 26, 2017 at 07:13:43PM +0800, Fam Zheng wrote:
> On Tue, 09/26 17:11, Peter Xu wrote:
> > If glib unset the flag after calling the destructor, it'll be fine.
> > But it's not doing it that way. And with above implementation of
> > glib, I don't see a good way to solve this problem via ordering of
> > glib calls... :(
>
> Does this work?
>
> ---
>
> diff --git a/include/block/aio.h b/include/block/aio.h
> index e9aeeaec94..3237d2be7c 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -147,6 +147,9 @@ struct AioContext {
> int epollfd;
> bool epoll_enabled;
> bool epoll_available;
> +
> + /* Protected by BQL */
Would atomic ops work here?
It can be used possibly in any destructor of AioContext. And, IIUC we
are trying to get rid of BQL when possible (at least, I think the
whole bunch of thing I did in past weeks is a fight with BQL somehow).
> + int refcnt;
> };
>
> /**
> diff --git a/util/async.c b/util/async.c
> index 355af73ee7..c5464611e3 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -293,7 +293,6 @@ aio_ctx_finalize(GSource *source)
> }
> qemu_lockcnt_unlock(&ctx->list_lock);
>
> - aio_set_event_notifier(ctx, &ctx->notifier, false, NULL, NULL);
> event_notifier_cleanup(&ctx->notifier);
> qemu_rec_mutex_destroy(&ctx->lock);
> qemu_lockcnt_destroy(&ctx->list_lock);
> @@ -429,6 +428,8 @@ AioContext *aio_context_new(Error **errp)
> ctx->poll_grow = 0;
> ctx->poll_shrink = 0;
>
> + ctx->refcnt = 1;
> +
> return ctx;
> fail:
> g_source_destroy(&ctx->source);
> @@ -476,11 +477,17 @@ void aio_co_enter(AioContext *ctx, struct Coroutine *co)
>
> void aio_context_ref(AioContext *ctx)
> {
> + assert(ctx->refcnt > 0);
> + ctx->refcnt++;
> g_source_ref(&ctx->source);
> }
>
> void aio_context_unref(AioContext *ctx)
> {
> + assert(ctx->refcnt > 0);
> + if (--ctx->refcnt == 0) {
> + aio_set_event_notifier(ctx, &ctx->notifier, false, NULL, NULL);
> + }
> g_source_unref(&ctx->source);
> }
>
Thanks for the patch, this one with Stefan's suggestion does solve the
problem. I am just not sure whether this is good - then we will have
two refcounts for one AioContext object.
Paolo, Stefan, could you help ack on this? If you think it's ok, I
can repost the iothread series, fixing patch 4 as Stefan suggested,
and add Fam's patch (though the atomic usage to be discussed).
Thanks,
--
Peter Xu
