Re: [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev

[Kevin Wolf]

Am 11.10.2019 um 08:08 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben:
> >> Eric Blake <address@hidden> writes:
> >> 
> >> > On 10/1/19 2:34 PM, Markus Armbruster wrote:
> >> >> Peter Krempa <address@hidden> writes:
> >> >>
> >> >>> savevm was buggy as it considered all monitor owned block device nodes
> >> >>
> >> >> Recommend "monitor-owned block device nodes" or "block device nodes
> >> >> owned by a monitor"
> >> >>
> >> >>> for snapshot. With introduction of -blockdev the common usage made all
> >> >>> nodes including protocol nodes monitor owned and thus considered for
> >> >>> snapshot.
> >> >>
> >> >> What exactly is / was the problem?
> >> >
> >> >
> >> > Old way: using QMP add_device, you create a drive backend with two BDS
> >> > (format and protocol) assigned to it; the drive backend has your given
> >> > name, and both BDS have a generated name (beginning with '#').  The
> >> > two BDS are not monitor-owned, rather, the drive is.
> >> >
> >> > New way: using QMP blockdev_add, you create the two BDS manually with
> >> > names of your choice, then plug that blockdev into an unnamed
> >> > blockbackend (the drive no longer needs a name, because you can get at
> >> > everything through the BDS name).  You _could_ do this in one step
> >> > (the QAPI allows self-recursion where you can define both the format
> >> > and protocol in one step), but it is easier to do in two steps (define
> >> > the protocol BDS first, then define the format BDS using a "string"
> >> > name of the protocol BDS instead of a { "driver":..., args... } object
> >> > of the protocol layer.  But by making two calls,  now both BDS are
> >> > monitor-owned.
> >> >
> >> > At snapshot-time, the code currently looks for all monitor-owned nodes
> >> > when deciding what to snapshot.  In the old way, this finds the named
> >> > drive, picks up its associated top-most node, and snapshots the format
> >> > layer.  In the new way, the drive is unnamed so it is skipped, while
> >> > there are two named BDS, but we don't want a snapshot of the protocol
> >> > layer.
> >> 
> >> So the problem is certain (common & sane) -blockdev use makes savevm
> >> create additional, unwanted snapshots.
> >
> > Actually, the most common protocol driver is file-posix, which doesn't
> > support snapshots, so usually the result was that savevm just fails
> > because it can't snapshot something that it (incorrectly) thinks it
> > should snapshot.
> 
> v3's commit message:
> 
>     qapi: Allow introspecting fix for savevm's cooperation with blockdev
>     
>     'savevm' was buggy as it considered all monitor-owned block device nodes
>     for snapshot. With introduction of -blockdev the common usage made all
>     nodes including protocol and backing file nodes monitor-owned and thus
>     considered for snapshot.
>     
>     This is a problem since the 'file' protocol nodes can't have internal
>     snapshots and it does not make sense to take snapshot of nodes
>     representing backing files.
>     
>     This was fixed by commit 05f4aced658a02b02 clients need to be able to
>     detect whether this fix is present.

Something is missing in this sentence. I think you lost the "but" from
the original message.

>     Since savevm does not have an QMP alternative, add the feature for the
>     'human-monitor-command' backdoor which is used to call this command in
>     modern use.
>     
>     Signed-off-by: Peter Krempa <address@hidden>
>  
> Kevin, is this explanation sufficiently correct & complete?

Looks good to me otherwise.

Kevin