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

[Kevin Wolf]

Am 11.02.2016 um 07:05 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:address@hidden
> > Am 10.02.2016 um 13:51 hat Pavel Dovgalyuk geschrieben:
> > > However, I don't understand yet which layer do you offer as the candidate
> > > for record/replay? What functions should be changed?
> > > I would like to investigate this way, but I don't got it yet.
> > 
> > At the core, I wouldn't change any existing function, but introduce a
> > new block driver. You could copy raw_bsd.c for a start and then tweak
> > it. Leave out functions that you don't want to support, and add the
> > necessary magic to .bdrv_co_readv/writev.
> > 
> > Something like this (can probably be generalised for more than just
> > reads as the part after the bdrv_co_reads() call should be the same for
> > reads, writes and any other request types):
> > 
> > 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 {
> >             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);
> >             /* wait synchronously for completion */
> >             while (!done) {
> >                 aio_poll();
> >             }
> >         }
> >     }
> > }
> > 
> > Where we could consider changing existing code is that it might be
> > desirable to automatically put an instance of this block driver on top
> > of every block device when record/replay is used. If we don't do that,
> > you need to explicitly specify -drive driver=blkreplay,...
> 
> As far, as I understand, all synchronous read/write request are also passed
> through this coroutines layer.

Yes, all read/write requests go through the same function internally, no
matter which external interface was used.

> It means that every disk access in replay phase should match the recording 
> phase.

Right. If I'm not mistaken, this was the fundamental requirement you
have, so I wouldn't have suggested this otherwise.

> Record/replay is intended to be used for debugging and analysis.
> When execution is replayed, guest machine cannot notice analysis overhead.
> Some analysis methods may include disk image reading. E.g., qemu-based
> analysis framework DECAF uses sleuthkit for disk forensics ( 
> https://github.com/sycurelab/DECAF ).
> If similar framework will be used with replay, forensics disk access 
> operations
> won't work if we will record/replay the coroutines.

Sorry, I'm not sure if I can follow.

If such analysis software runs in the guest, it's not a replay any more
and I completely fail to see what you're doing.

If it's a qemu component independent from the guest, then my method
gives you a clean way to bypass the replay driver that wouldn't be
possible with yours.

If your plan was to record/replay only async requests and then use sync
requests to bypass the record/replay, let me clearly state that this is
the wrong approach: There are still guest devices which do synchronous
I/O and need to be considered in the replay log, and you shouldn't
prevent the analysis code from using AIO (in fact, using sync I/O in new
code is very much frowned upon).

I can explain in more detail what the block device structure looks like
and how to access an image with and without record/replay, but first let
me please know whether I guessed right what your problem is. Or if I
missed your point, can you please describe in detail a case that
wouldn't work?

> And if we'll record only high-level asynchronous disk operations, then
> there will be a way to performs disk accesses without matching these
> operations with replay log.
> 
> However, we already tried to record disk operations completion events
> (in previous more complicated version). Recording overhead was the same
> as with new blk_aio_ version of the patch. 

Well, if your code really intentionally ignores synchronous requests,
then the advantage you get from my approach is being correct.

Other than that, I'm personally mainly interested in keeping the core
block layer easily maintainable and the hot I/O paths clean for cases
that don't use all the features qemu has. This means that in the mid
term existing features need to move out of the core block layer rather
than new ones being added. So doing things in a separate block driver
might not be an advantage for your immediate needs, but it's important
for clean modularisation and a requirement for things to get merged
(unless you can prove that it's impossible to do this way, but I don't
think you can - otherwise the whole block layer design would be wrong).

Kevin