Re: [Qemu-devel] [PATCH v2 02/10] block: set snapshot option for block devices in blkreplay module

[Pavel Dovgalyuk]

> From: Paolo Bonzini [mailto:address@hidden On Behalf Of Paolo Bonzini
> On 16/09/2016 09:55, Pavel Dovgalyuk wrote:
> >> Since you have to create
> >> overlay.qcow2 outside QEMU anyway, overlay.qcow2 might as well be the
> >> "image".  That is, you could choose between:
> >>
> >>    -drive driver=blkreplay,if=none,image=overlay.qcow2,id=img-blkreplay \
> >>    -rr snapshot=replay_init,...
> >>
> >>    -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
> >>
> >> The temporary snapshot would be created if there's no "-rr snapshot" option
> >> on the command line.
> >>
> >> Does this make sense?
> >
> > There are two different parts:
> >  - creating an overlay
> >  - creating the snapshot
> >
> > Overlay is needed to preserve the state of the original backing file.
> > In the current version temporary overlay is always created at start of qemu.
> 
> Yes, this would still be the default for rr mode.
> 
> > I don't think that it is convenient forcing user to create overlay manually.
> 
> Note that all I'm only saying that _only for the case where the user
> creates the overlay manually anyway_ there is no need to specify both
> image and overlay.  (I also don't like particularly the hard-coded
> snapshot name replay_init, which can be overridden by -loadvm but not
> when saving).

Ok, this seems reasonable to fix.

> 
> So there are various possibilites:
> 
> First proposal:
> 
> - automatically created overlay is -icount rr=record|replay (then
> snapshot name doesn't matter, it can be replay_init)
> 
> - manually created overlay is -icount
> rr=record|replay,rrsnapshot=snapname (then snapshot name matters because
> you can have different snapshots in the same file)

We can't create overlay with icount suboptions, because there could be several
block devices. Each one needs its own overlay.

> If rr is enabled but rrsnapshot is absent, configure_icount can just set
> "snapshot = 1" to force creation of the temporary overlay.  This
> requires no change to the blkreplay driver
> 
> 
> Second proposal:
> 
> - automatically created overlay is -icount rr=record|replay -snapshot
> 
> - manually created overlay is -icount rr=record|replay and an rrsnapshot
> suboption can be added anyway if considered useful.

See above.

> This requires no change to the blkreplay driver either.  It's a little
> more verbose in the common case, but perhaps less surprising if you're
> already used to -snapshot.

Our internal version automatically creates overlays with generated names.
But now I think, that it is inconvenient and manual name specification is 
better.

> > Common debugging scenario includes multiple recording passes until the bug 
> > manifests
> > itself. Every new execution recorded should be accompanied by creating an 
> > overlay
> > to assure that the execution is started from the same disk state.
> >
> > Specifying initial snapshot name makes sense if we want to suppress -loadvm 
> > option.
> 
> My rationale was to have similar command lines between record and replay
> modes (just changing rr=record to rr=replay).

This seems reasonable, I'll try to fix this part.

Pavel Dovgalyuk