Re: [Qemu-devel] [PATCH v6 1/3] linux-aio: fix submit aio as a batch

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

On Tue, Nov 25, 2014 at 10:45:05PM +0800, Ming Lei wrote:
> On Tue, Nov 25, 2014 at 9:08 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Tue, Nov 25, 2014 at 03:23:11PM +0800, Ming Lei wrote:
> >> @@ -296,12 +370,14 @@ void laio_detach_aio_context(void *s_, AioContext *old_context)
> >>
> >>      aio_set_event_notifier(old_context, &s->e, NULL);
> >>      qemu_bh_delete(s->completion_bh);
> >> +    qemu_bh_delete(s->io_q.abort_bh);
> >>  }
> >>
> >>  void laio_attach_aio_context(void *s_, AioContext *new_context)
> >>  {
> >>      struct qemu_laio_state *s = s_;
> >>
> >> +    s->io_q.abort_bh = aio_bh_new(new_context, ioq_abort_bh, s);
> >>      s->completion_bh = aio_bh_new(new_context, qemu_laio_completion_bh, s);
> >>      aio_set_event_notifier(new_context, &s->e, qemu_laio_completion_cb);
> >>  }
> >
> > These functions are incomplete when ->aborting == true.  I can't think
> 
> Could you explain in a bit when ->aborting is true during attach callback?
> 
> > of a reason why we are guaranteed never to hit that state, and fixing it
> > is easy.  Just add the following to the end of
> > laio_attach_aio_context():
> >
> > if (s->aborting) {
> >     qemu_bh_schedule(s->io_q.abort_bh);
> > }
> 
> You mean the abort BH may not have chance to run before its deletion
> in the detach callback?

Exactly.  Any time you schedule a BH you need to be aware of things that
may happen before the BH is invoked.

> If so, bdrv_drain_all() from bdrv_set_aio_context() should have
> handled the pending BH, right?

I'm not sure if it's good to make subtle assumptions like that.  If the
code changes they will break.

Since it is very easy to protect against this case (the code I posted
before), it seems worthwhile to be on the safe side.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]