Re: [Qemu-devel] [for-4.2 PATCH 3/6] replay: update docs for record/replay with block devices

[Kevin Wolf]

Am 18.09.2019 um 11:52 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:address@hidden]
> > Am 18.09.2019 um 11:37 hat Pavel Dovgalyuk geschrieben:
> > > > From: Kevin Wolf [mailto:address@hidden]
> > > > Am 18.09.2019 um 11:22 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:address@hidden]
> > > > > > Am 17.09.2019 um 13:58 hat Pavel Dovgalyuk geschrieben:
> > > > > > > From: Pavel Dovgalyuk <address@hidden>
> > > > > > >
> > > > > > > This patch updates the description of the command lines for using
> > > > > > > record/replay with attached block devices.
> > > > > > >
> > > > > > > Signed-off-by: Pavel Dovgalyuk <address@hidden>
> > > > > > > ---
> > > > > > >  docs/replay.txt |   12 +++++++++---
> > > > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/docs/replay.txt b/docs/replay.txt
> > > > > > > index ee6aee9861..ce97c3f72f 100644
> > > > > > > --- a/docs/replay.txt
> > > > > > > +++ b/docs/replay.txt
> > > > > > > @@ -27,7 +27,7 @@ Usage of the record/replay:
> > > > > > >   * First, record the execution with the following command line:
> > > > > > >      qemu-system-i386 \
> > > > > > >       -icount shift=7,rr=record,rrfile=replay.bin \
> > > > > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > > > > >       -drive 
> > > > > > > driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > > > > >       -device ide-hd,drive=img-blkreplay \
> > > > > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > > > > @@ -35,7 +35,7 @@ Usage of the record/replay:
> > > > > > >   * After recording, you can replay it by using another command 
> > > > > > > line:
> > > > > > >      qemu-system-i386 \
> > > > > > >       -icount shift=7,rr=replay,rrfile=replay.bin \
> > > > > > > -     -drive file=disk.qcow2,if=none,id=img-direct \
> > > > > > > +     -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
> > > > > > >       -drive 
> > > > > > > driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
> > > > > > >       -device ide-hd,drive=img-blkreplay \
> > > > > > >       -netdev user,id=net1 -device rtl8139,netdev=net1 \
> > > > > > > @@ -223,7 +223,7 @@ Block devices record/replay module intercepts 
> > > > > > > calls of
> > > > > > >  bdrv coroutine functions at the top of block drivers stack.
> > > > > > >  To record and replay block operations the drive must be 
> > > > > > > configured
> > > > > > >  as following:
> > > > > > > - -drive file=disk.qcow2,if=none,id=img-direct
> > > > > > > + -drive file=disk.qcow2,if=none,snapshot,id=img-direct
> > > > > > >   -drive 
> > > > > > > driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > > > >   -device ide-hd,drive=img-blkreplay
> > > > > > >
> > > > > > > @@ -252,6 +252,12 @@ This snapshot is created at start of 
> > > > > > > recording and restored at
> > > > start
> > > > > > >  of replaying. It also can be loaded while replaying to roll back
> > > > > > >  the execution.
> > > > > > >
> > > > > > > +'snapshot' flag of the disk image must be removed to save the 
> > > > > > > snapshots
> > > > > > > +in the overlay (or original image) instead of using the 
> > > > > > > temporary overlay.
> > > > > > > + -drive file=disk.ovl,if=none,id=img-direct
> > > > > > > + -drive 
> > > > > > > driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> > > > > > > + -device ide-hd,drive=img-blkreplay
> > > > > >
> > > > > > Wait, didn't patch 2 just make -snapshot unconditionally 
> > > > > > incompatible
> > > > > > with replay? So isn't saving the snapshot in the original image the 
> > > > > > only
> > > > > > supported mode now?
> > > > >
> > > > > There are two ways to run record/replay:
> > > > > 1. Disk with snapshot option and any image to make it unchanged
> > > > > 2. Disk with overlay without snapshot option to save VM snapshots on 
> > > > > it
> > > >
> > > > Yes, I think I understand the two options that you intend to make
> > > > available, but when -snapshot sets a replay blocker, how can 1. still
> > > > work? Is there some code that ignores replay blockers?
> > >
> > > I checked the text and don't see anything about global "-snapshot" option.
> > > All references are about drive snapshot flag.
> > > Can you point me where did I missed that?
> > 
> > Oh, sorry, you're right and I misread.
> > 
> > However, global -snapshot is just a convenient shortcut for specifying
> > snapshot=on for all -drive arguments. So if -snapshot is incompatible
> > with replay, shouldn't manually marking all drives as snapshot=on be
> > incompatible as well?
> > 
> > Maybe you're really interested in some specific drive not having
> > snapshot=on? But then it might be better to check that specific drive
> > instad of forbidding just the shortcut for setting it.
> 
> -snapshot adds the flag for top-level drive, making driver operations
> dependent on temporary file structure.
> 
> Moving this overlay beneath blkreplay driver makes drive operations
> deterministic for the top-level device.

So the real requirement is that blkreplay is the top-level node of any
guest device, right? And only because of this, you can't use -snapshot
(or snapshot=on on the blkreplay driver).

If we instead check e.g. in blk_insert_bs() or blk_attach_dev() that in
record/replay mode, the root node of the BlockBackend is blkreplay,
wouldn't we catch many more incorrect setups?

Kevin