Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9-rc5 v2] block: Drain BH in bdrv_drained_begin

From: Fam Zheng <famz@redhat.com>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: qemu-devel <qemu-devel@nongnu.org>, Kevin Wolf <kwolf@redhat.com>,
	qemu block <qemu-block@nongnu.org>, Max Reitz <mreitz@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9-rc5 v2] block: Drain BH in bdrv_drained_begin
Date: Mon, 17 Apr 2017 16:27:19 +0800	[thread overview]
Message-ID: <20170417082719.GC13582@lemon.lan> (raw)
In-Reply-To: <CAJSP0QVbYRW0VD_1zUqU0T6V48wr_g3co8Dxs_ndpfA70d4TkQ@mail.gmail.com>

On Fri, 04/14 09:51, Stefan Hajnoczi wrote:
> On Fri, Apr 14, 2017 at 9:02 AM, Fam Zheng <famz@redhat.com> wrote:
> > @@ -398,11 +399,15 @@ void bdrv_drain_all(void);
> >           */                                                \
> >          assert(!bs_->wakeup);                              \
> >          bs_->wakeup = true;                                \
> > -        while ((cond)) {                                   \
> > -            aio_context_release(ctx_);                     \
> > -            aio_poll(qemu_get_aio_context(), true);        \
> > -            aio_context_acquire(ctx_);                     \
> > -            waited_ = true;                                \
> > +        while (busy_) {                                    \
> > +            if ((cond)) {                                  \
> > +                waited_ = busy_ = true;                    \
> > +                aio_context_release(ctx_);                 \
> > +                aio_poll(qemu_get_aio_context(), true);    \
> > +                aio_context_acquire(ctx_);                 \
> > +            } else {                                       \
> > +                busy_ = aio_poll(ctx_, false);             \
> > +            }                                              \
> 
> Wait, I'm confused.  The current thread is not in the BDS AioContext.
> We're not allowed to call aio_poll(ctx_, false).
> 
> Did you mean aio_poll(qemu_get_aio_context(), false) in order to
> process defer to main loop BHs?

At this point it's even unclear to me what should be the plan for 2.9.  v1 IMO
was the least intrusive, but didn't cover bdrv_drain_all_begin. v2 has this
controversial "aio_poll(ctx_, false)", however its alternative,
"aio_poll(qemu_get_aio_context(), false)", "introduces" another crash that is
not seen otherwise.

What should we do now?

Fam