Re: [Qemu-devel] [PATCH 3/3] replay: introduce block devices record/replay

[Kevin Wolf]

Am 15.02.2016 um 12:19 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:address@hidden
> > Am 15.02.2016 um 10:14 hat Pavel Dovgalyuk geschrieben:
> > > > From: Pavel Dovgalyuk [mailto:address@hidden
> > > > > From: Kevin Wolf [mailto:address@hidden
> > > > > > >
> > > > > > > int blkreplay_co_readv()
> > > > > > > { 
> > > > > > >     BlockReplayState *s = bs->opaque;
> > > > > > >     int reqid = s->reqid++;
> > > > > > >
> > > > > > >     bdrv_co_readv(bs->file, ...);
> > > > > > >
> > > > > > >     if (mode == record) {
> > > > > > >         log(reqid, time);
> > > > > > >     } else {
> > > > > > >         assert(mode == replay);
> > > > > > >         bool *done = req_replayed_list_get(reqid)
> > > > > > >         if (done) {
> > > > > > >             *done = true;
> > > > > > >         } else {
> > > > > > point A
> > > > > > >             req_completed_list_insert(reqid, 
> > > > > > > qemu_coroutine_self());
> > > > > > >             qemu_coroutine_yield();
> > > > > > >         }
> > > > > > >     }
> > > > > > > }
> > > > > > >
> > > > > > > /* called by replay.c */
> > > > > > > int blkreplay_run_event()
> > > > > > > {
> > > > > > >     if (mode == replay) {
> > > > > > >         co = req_completed_list_get(e.reqid);
> > > > > > >         if (co) {
> > > > > > >             qemu_coroutine_enter(co);
> > > > > > >         } else {
> > > > > > >             bool done = false;
> > > > > > >             req_replayed_list_insert(reqid, &done);
> > > > > > point B
> > > > > > >             /* wait synchronously for completion */
> > > > > > >             while (!done) {
> > > > > > >                 aio_poll();
> > > > > > >             }
> > > > > > >         }
> > > > > > >     }
> > > > > > > }
> > > > >
> > > Now I've encountered a situation where blkreplay_run_event is called from 
> > > read coroutine:
> > > bdrv_prwv_co -> aio_poll -> qemu_clock_get_ns -> replay_read_clock -> 
> > > blkreplay_run_event
> > >            \--> bdrv_co_readv -> blkreplay_co_readv -> 
> > > bdrv_co_readv(lower layer)
> > >
> > > bdrv_co_readv inside blkreplay_co_readv can't proceed in this situation.
> > > This is probably because aio_poll has taken the aio context?
> > > How can I resolve this?
> > 
> > First of all, I'm not sure if running replay events from
> > qemu_clock_get_ns() is such a great idea. This is not a function that
> > callers expect to change any state. If you absolutely have to do it
> > there instead of in the clock device emulations, maybe restricting it to
> > replaying clock events could make it a bit more harmless.
> 
> qemu_clock_get_ns() wants to read some clock data from the log.
> While reading, it finds block driver event and tries to proceed it.
> These block events may occur at any moment, because blkreplay_co_readv()
> writes them immediately as executes.

I understand what's happening. I just think it's asking for bugs when
you do this in a low-level function like qemu_clock_get_ns() that nobody
expects to have any side effects. But we'll see how many bugs this will
cause.

> Alternative approach is adding these events to the queue and executing them
> when checkpoint will be met. I'm not sure that this is easy to implement with
> coroutines.

Shouldn't be hard, you're queueing requests already. But it's unlikely
to be the right solution.

Why does qemu_clock_get_ns() need to replay events to begin with?
Shouldn't it just return the current state without replaying anything
new?

> > Anyway, what does "can't proceed" mean? The coroutine yields because
> > it's waiting for I/O, but it is never reentered? Or is it hanging while
> > trying to acquire a lock?
> 
> bdrv_co_io_em() (raw file read/write) yields and waits inifinitely to return, 
> because
> aio_poll hands in some clock read.

Okay, so the coroutine is sleeping while it waits for I/O to complete.
What's missing is the I/O completion callback.

When the coroutine yields in bdrv_co_io_em(), the caller coroutines
continues execution at blkreplay_run_event(). My naive expectation would
be that it returns to replay_read_clock(), which in turn returns to
qemu_clock_get_ns(), which knows the right time now and returns to
aio_poll(), which would process I/O completions and call the callback.
The callback would reenter the other coroutine in bdrv_co_io_em() and
the request would be completed.

As the callback isn't happening, something in this unwinding process
don't work as expected and we're actually hanging somewhere else. Do you
know where that is?

Kevin