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

[Kevin Wolf <kwolf@redhat.com>]

Am 15.02.2016 um 15:24 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > >
> > > > 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.
> > >
> > > Only virtual clock is emulated, and host clock is read from the host
> > > real time sources and therefore has to be saved into the log.
> > 
> > Isn't the host clock invisible to the guest anyway?
> 
> It isn't. Host clock is used by guest RTC.
> 
> > > There could be asynchronous events that occur in non-cpu threads.
> > > For now these events are shutdown request and block task execution.
> > > They may "hide" following clock (or another one) events. That is why
> > > we process them until synchronous event (like clock, instructions
> > > execution, or checkpoint) is met.
> > >
> > >
> > > > 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?
> > >
> > > I've solved this problem by slightly modifying the queue.
> > > I haven't yet made BlockDriverState assignment to the request ids.
> > > Therefore aio_poll was temporarily replaced with usleep.
> > > Now execution starts and hangs at some random moment of OS loading.
> > >
> > > Here is the current version of blkreplay functions:
> > >
> > > static int coroutine_fn blkreplay_co_readv(BlockDriverState *bs,
> > >     int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> > > {
> > >     uint32_t reqid = request_id++;
> > >     Request *req;
> > >     req = block_request_insert(reqid, bs, qemu_coroutine_self());
> > >     bdrv_co_readv(bs->file->bs, sector_num, nb_sectors, qiov);
> > >
> > >     if (replay_mode == REPLAY_MODE_RECORD) {
> > >         replay_save_block_event(reqid);
> > >     } else {
> > >         assert(replay_mode == REPLAY_MODE_PLAY);
> > >         qemu_coroutine_yield();
> > >     }
> > >     block_request_remove(req);
> > >
> > >     return 0;
> > > }
> > >
> > > void replay_run_block_event(uint32_t id)
> > > {
> > >     Request *req;
> > >     if (replay_mode == REPLAY_MODE_PLAY) {
> > >         while (!(req = block_request_find(id))) {
> > >             //aio_poll(bdrv_get_aio_context(req->bs), true);
> > >             usleep(1);
> > >         }
> > 
> > How is this loop supposed to make any progress?
> 
> This loop does not supposed to make any progress. It waits until block_request_insert
> call is added to the queue.

Yes. And who is supposed to add something to the queue? You are looping
and doing nothing, so no other code gets to run in this thread. It's
only aio_poll() that would run event handlers that could eventually add
something to the list.

> > And I still don't understand why aio_poll() doesn't work and where it
> > hangs.
> 
> aio_poll hangs if "req = block_request_insert(reqid, bs, qemu_coroutine_self());" line
> is executed after bdrv_co_readv. When bdrv_co_readv yields, replay_run_block_event has no
> information about pending request and cannot jump to its coroutine.
> Maybe I should implement aio_poll execution there to make progress in that case?

Up in the thread, I posted code that was a bit more complex than what
you have, and which considered both cases (completion before
replay/completion after replay). If you implement it like this, it might
just work. And yes, it involved an aio_poll() loop in the replay
function.

Kevin