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

[Kevin Wolf]

Am 16.02.2016 um 12:20 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:address@hidden
> > Am 16.02.2016 um 07:25 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:address@hidden
> > > > Am 15.02.2016 um 15:24 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:address@hidden
> > > > > > > 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.
> > >
> > > blkreplay_co_readv adds request to the queue.
> > 
> > I don't see blkreplay_co_readv() called in this loop without aio_poll().
> 
> Is this mandatory?

If you don't want it to be an endless loop, someone needs to make the
value of the loop condition change...

> I thought that version which you proposed has a race condition at the 
> following lines
> when AIO request is operated by a worker thread. Is this correct?

Unfortunately this ended up linewrapped, trying to restore what you
meant:

> Coroutine                                                         Replay
> bool *done = req_replayed_list_get(reqid) // NULL
>                                                                   co = 
> req_completed_list_get(e.reqid); // NULL

There was no yield, this context switch is impossible to happen. Same
for the switch back.

> req_completed_list_insert(reqid, qemu_coroutine_self());
> qemu_coroutine_yield();

This is the point at which a context switch happens. The only other
point in my code is the qemu_coroutine_enter() in the other function.

>                                                                   bool done = 
> false;
>                                                                   
> req_replayed_list_insert(reqid, &done);
>                                                                   while 
> (!done) { // nobody will change done anymore
>                                                                        
> aio_poll();
>                                                                   }
> 
> 
> > > You are right, but I found kind of fundamental problem here.
> > > There are two possible approaches for replaying coroutine events:
> > >
> > > 1. Waiting with aio_poll in replay_run_block_event.
> > >    In this case replay cannot be made, because of recursive mutex lock:
> > >    aio_poll -> qemu_clock_get_ns -> <lock replay mutex> ->
> > >      replay_run_block_event -> aio_poll -> qemu_clock_get_ns -> <lock 
> > > replay mutex> ->
> > >      <assert failed>
> > >    I.e. we have recursive aio_poll function calls that lead to recursive 
> > > replay calls
> > >    and to recursive mutex lock.
> > 
> > That's what you get for replaying stuff in qemu_clock_get_ns() when it
> > should really be replayed in the hardware emulation.
> 
> I'm not sure that it is possible for RTC. Host clock should run even if VM is 
> stopped.
> If we emulate host clock, its behavior should be somehow synchronized with 
> the host.

I think the RTC behaviour should be replayed, whereas the host clock
should be the right one.

Or maybe it would actually make most sense to forbid clock=host for
record/replay, because in the replay you don't get the host clock
anyway, but just saved values.

> > This is a problem you would also have had with your original patch, as
> > there are some places in the block layer that use aio_poll() internally.
> > Using the blkreplay driver only made you find the bug earlier.
> 
> Not exactly. Host clock replay will use cached clock value if there is no 
> such event in the log.
> The problem I observe here is with asynchronous block events that should be 
> processed
> immediately as they read from log.

Doesn't your earlier patch do the same? I seem to remember that it calls
bdrv_drain_all(), which is essentially the same as calling aio_poll() in
a loop, except that its loop condition is slightly different.

> > But... can you detail how we get from aio_poll() to qemu_clock_get_ns()?
> > I don't see it directly calling that function, so it might just be a
> > callback to some guest device that is running now.
> 
> aio_poll (win32) has the following line:
> 
>         timeout = blocking && !have_select_revents
>             ? qemu_timeout_ns_to_ms(aio_compute_timeout(ctx)) : 0;
> 
> POSIX version also uses that function. aio_compute_timeout() requests time 
> from
> all existing clocks by calling timerlistgroup_deadline_ns().

I see, thanks.

Kevin