Re: [Qemu-devel] A glib warning encountered with internal iothread

[Peter Xu]

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