| Message ID | 992ea9ca130b4fb6dbf82726aa3b1d8040c16944.1568989362.git.pkrempa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Add detection for the 'savevm' fix for blockdev | expand |
Peter Krempa <pkrempa@redhat.com> 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? > This was fixed but clients need to be able to detect whether > this fix is present. Fixed where? Commit hash, if possible. > > Since savevm does not have an QMP alternative add the feature for the Comma after alternative. > 'human-monitor-command' backdoor which is used to call this command in > modern use. Eww. I don't have better ideas short of "design and implement a sane QMP interface to internal snapshots", which is too much work. > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > --- > qapi/misc.json | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/qapi/misc.json b/qapi/misc.json > index 6bd11f50e6..673445dfa9 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -1020,6 +1020,12 @@ > # > # @cpu-index: The CPU to use for commands that require an implicit CPU > # > +# Features: > +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command only Maybe s/the savevm monitor command/HMP command savevm/ > +# snapshots monitor owned nodes if they have no monitor-owned or owner by a monitor again. > +# parents. This allows to use 'savevm' with > +# -blockdev. (since 4.2) > +# > # Returns: the output of the command as a string > # > # Since: 0.14.0 > @@ -1047,7 +1053,8 @@ > ## > { 'command': 'human-monitor-command', > 'data': {'command-line': 'str', '*cpu-index': 'int'}, > - 'returns': 'str' } > + 'returns': 'str', > + 'features' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] } No space before ':'. Short form would do: 'features': [ 'savevm-blockdev-monitor-nodes' ] > > ## > # @change:
On 10/1/19 2:34 PM, Markus Armbruster wrote: > Peter Krempa <pkrempa@redhat.com> 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. > >> This was fixed but clients need to be able to detect whether >> this fix is present. > > Fixed where? Commit hash, if possible. Pull request: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04773.html (assuming it doesn't need a respin before landing, 8ec72832) > >> >> Since savevm does not have an QMP alternative add the feature for the > > Comma after alternative. > >> 'human-monitor-command' backdoor which is used to call this command in >> modern use. > > Eww. I don't have better ideas short of "design and implement a sane > QMP interface to internal snapshots", which is too much work. > >> Signed-off-by: Peter Krempa <pkrempa@redhat.com> >> --- >> qapi/misc.json | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-)
Eric Blake <eblake@redhat.com> writes: > On 10/1/19 2:34 PM, Markus Armbruster wrote: >> Peter Krempa <pkrempa@redhat.com> 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. Your explanation should be worked into the commit message along with ... >>> This was fixed but clients need to be able to detect whether >>> this fix is present. >> >> Fixed where? Commit hash, if possible. > > Pull request: > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04773.html > (assuming it doesn't need a respin before landing, 8ec72832) ... a pointer to this fix. Thanks! [...]
Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben: > Eric Blake <eblake@redhat.com> writes: > > > On 10/1/19 2:34 PM, Markus Armbruster wrote: > >> Peter Krempa <pkrempa@redhat.com> 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. Kevin > Your explanation should be worked into the commit message along with ... > > >>> This was fixed but clients need to be able to detect whether > >>> this fix is present. > >> > >> Fixed where? Commit hash, if possible. > > > > Pull request: > > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04773.html > > (assuming it doesn't need a respin before landing, 8ec72832) > > ... a pointer to this fix. > > Thanks! > > [...]
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben: >> Eric Blake <eblake@redhat.com> writes: >> >> > On 10/1/19 2:34 PM, Markus Armbruster wrote: >> >> Peter Krempa <pkrempa@redhat.com> 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. 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 <pkrempa@redhat.com> Kevin, is this explanation sufficiently correct & complete?
Am 11.10.2019 um 08:08 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben: > >> Eric Blake <eblake@redhat.com> writes: > >> > >> > On 10/1/19 2:34 PM, Markus Armbruster wrote: > >> >> Peter Krempa <pkrempa@redhat.com> 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 <pkrempa@redhat.com> > > Kevin, is this explanation sufficiently correct & complete? Looks good to me otherwise. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 11.10.2019 um 08:08 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben: [...] >> >> 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. I fixed this in v4 by inserting a period. I wasn't aware we had lost a "but". >> 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 <pkrempa@redhat.com> >> >> Kevin, is this explanation sufficiently correct & complete? > > Looks good to me otherwise. Thanks!
diff --git a/qapi/misc.json b/qapi/misc.json index 6bd11f50e6..673445dfa9 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -1020,6 +1020,12 @@ # # @cpu-index: The CPU to use for commands that require an implicit CPU # +# Features: +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command only +# snapshots monitor owned nodes if they have no +# parents. This allows to use 'savevm' with +# -blockdev. (since 4.2) +# # Returns: the output of the command as a string # # Since: 0.14.0 @@ -1047,7 +1053,8 @@ ## { 'command': 'human-monitor-command', 'data': {'command-line': 'str', '*cpu-index': 'int'}, - 'returns': 'str' } + 'returns': 'str', + 'features' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] } ## # @change:
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 nodes monitor owned and thus considered for snapshot. This was fixed but clients need to be able to detect whether this fix is present. 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 <pkrempa@redhat.com> --- qapi/misc.json | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)