Re: [Qemu-devel] [PATCH v5 7/9] block: don't make snapshots for filters

[Kevin Wolf <kwolf@redhat.com>]

Am 05.12.2016 um 12:49 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Am 05.12.2016 um 08:43 hat Pavel Dovgalyuk geschrieben:
> > > Paolo,
> > >
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Am 21.11.2016 um 13:22 hat Pavel Dovgalyuk geschrieben:
> > > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > > Am 16.11.2016 um 10:49 hat Pavel Dovgalyuk geschrieben:
> > > > > >
> > > > > > How does that bypass blkreplay? blk->root is supposed to be the blkreply
> > > > > > node, do you see something different? If it were the qcow2 node, then I
> > > > > > would expect that no requests at all go through the blkreplay layer.
> > > > >
> > > > > It seems, that the problem is in -snapshot option.
> > > > > We have one of the following block layers depending on command line:
> > > > >  tmp_overlay1 -> blkreplay -> tmp_overlay2 -> disk_image
> > > > >  tmp_overlay1 -> blkreplay -> disk_image
> > > > >
> > > > > But the correct scheme is intended to be the following:
> > > > >  blkreplay -> tmp_overlay1 -> disk_image
> > > > >
> > > > > How can we fix it?
> > > > > Maybe we should add blkreplay automatically to all block devices and not
> > > > > to specify it in the command line?
> > > >
> > > > I think you found a pretty fundamental design problem with "global"
> > > > drive options that add a filter node such as -snapshot and replay mode
> > > > (replay mode isn't one of them today, but your suggestion to make it
> > > > automatic would turn it into one).
> > > >
> > > > At the core of the problem I think we have two questions:
> > > >
> > > > 1. Which block nodes should be affected and get a filter node added, and
> > > >    which nodes shouldn't get one? In your case, disl_image is defined
> > > >    with a -drive option, but shouldn't get the snapshot.
> > > >
> > > > 2. In which order should filter nodes be added?
> > > >
> > > > Both of these questions feel hard. As long as we haven't thought through
> > > > the concept as such (rather than discussing one-off hacks) and we're not
> > > > completely certain what the right answer to the questions is, we
> > > > shouldn't add more automatic filter nodes, because chances are that we
> > > > get it wrong and would regret it.
> > > >
> > > > The obvious answer for a workaround would be: Make everything manual,
> > > > i.e. don't use -snapshot, but create a qcow2 overlay manually.
> > >
> > > What about to switching to manual overlay creation by default?
> > > We can make rrsnapshot option mandatory.
> > > Therefore user will have to create snapshot in image or overlay and
> > > the disk image will not be corrupted.
> > >
> > > It is not very convenient, but we could disable rrsnapshot again when
> > > the solution for -snapshot will be found.
> > 
> > Hm, what is this rrsnapshot option? git grep can't find it.
> 
> It was a patch that was not included yet.
> This option creates/loads vm snapshot in record/replay mode
> leaving original disk image unchanged.

You mean internal qcow2 snapshots? Ok.

> Record/replay without this option uses '-snapshot' to preserve
> the state of the disk images.
> 
> > Anyway, it seems that doing things manually is the safe way as long as
> > we don't know the final solution, so I think I agree.
> > 
> > For a slightly more convenient way, one of the problems to solve seems
> > to be that snapshot=on always affects the top level node and you can't
> > create a temporary snapshot in the middle of the chain. Perhaps we
> > should introduce a 'temporary-overlay' driver or something like that, so
> > that you could specify things like this:
> > 
> >     -drive if=none,driver=file,filename=test.img,id=orig
> >     -drive if=none,driver=temporary-overlay,file=orig,id=snap
> >     -drive if=none,driver=blkreplay,image=snap
> 
> This seems reasonable for manual way.

Maybe another, easier to implement option could be something like this:

    -drive if=none,driver=file,filename=test.img,snapshot=on,overlay.node-name=snap
    -drive if=none,driver=blkreplay,image=snap

It would require that we implement support for overlay.* options like we
already support backing.* options. Allowing to specify options for the
overlay node is probably nice to have anyway.

However, there could be a few tricky parts there. For example, what
happens if someone uses overlay.backing=something-else? Perhaps
completely disallowing backing and backing.* for overlays would already
solve this.

> > Which makes me wonder... Is blkreplay usable without the temporary
> > snapshot or is this pretty much a requirement? 
> 
> It's not a requirement. But to make replay deterministic we have to
> start with the same image every time. As I know, this may be achieved by:
> 1. Restoring original disk image manually
> 2. Using vm snapshot to start execution from
> 3. Using -snapshot option
> 4. Not using disks at all
> 
> > Because if it has to be
> > there, the next step could be that blkreplay creates temporary-overlay
> > internally in its .bdrv_open().
> 
> Here is your answer about such an approach :)
> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04687.html

Right, and unfortunately these are still good points.

Especially the part where you allowed to give the overlay filename
really needs to work the way it does now with the 'image' option. We
might not need to be that strict with temporary overlays, restricting to
qcow2 with default options could be acceptable there - but whatever I
think of to support both cases results in something that isn't really
easier than the manual way that we figured out above.

Kevin