When you cancel an in-progress live block operation with QMP
`block-job-cancel`, it emits the event: BLOCK_JOB_CANCELLED. However,
when `block-job-cancel` is issued after `drive-mirror` has indicated (by
emitting the event BLOCK_JOB_READY) that the source and destination
remain synchronized:
[...] # Snip `drive-mirror` invocation & outputs
{
"execute":"block-job-cancel",
"arguments":{
"device":"virtio0"
}
}
{"return": {}}
It (`block-job-cancel`) will counterintuitively emit the event
'BLOCK_JOB_COMPLETED':
{
"timestamp":{
"seconds":1510678024,
"microseconds":526240
},
"event":"BLOCK_JOB_COMPLETED",
"data":{
"device":"virtio0",
"len":41126400,
"offset":41126400,
"speed":0,
"type":"mirror"
}
}
But this is expected behaviour, where the _COMPLETED event indicates
that synchronization has successfully ended (and the destination has a
point-in-time copy, which is at the time of cancel).
So add a small note to this effect. (Thanks: Max Reitz for reminding
me of this on IRC.)
Signed-off-by: Kashyap Chamarthy <kchamart@redhat.com>
---
Changes in v2:
- "Note:" seems to be a special construct in Patchew, my usage caused a
build failure. So do: s/Note:/Note that/
- Add the missing 'Signed-off-by'
---
qapi/block-core.json | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab96e348e6317bb42769ae20f4a4519bac02e93a..4ecfd1fbc5bc231c69da15d489c44e3e8b0706a5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2065,6 +2065,14 @@
# BLOCK_JOB_CANCELLED event. Before that happens the job is still visible when
# enumerated using query-block-jobs.
#
+# Note that the 'block-job-cancel' command will emit the event
+# BLOCK_JOB_COMPLETED if you issue it ('block-job-cancel') after 'drive-mirror'
+# has indicated (by emitting the event BLOCK_JOB_READY) that the source and
+# destination remain synchronized. In this case, the BLOCK_JOB_COMPLETED
+# event indicates that synchronization (from 'drive-mirror') has successfully
+# ended and the destination now has a point-in-time copy, which is at the time
+# of cancel.
+#
# For streaming, the image file retains its backing file unless the streaming
# operation happens to complete just as it is being cancelled. A new streaming
# operation can be started at a later time to finish copying all data from the
--
2.9.5
On 11/15/2017 04:09 AM, Kashyap Chamarthy wrote: > When you cancel an in-progress live block operation with QMP > `block-job-cancel`, it emits the event: BLOCK_JOB_CANCELLED. However, > when `block-job-cancel` is issued after `drive-mirror` has indicated (by > emitting the event BLOCK_JOB_READY) that the source and destination > remain synchronized: > > [...] # Snip `drive-mirror` invocation & outputs > { > "execute":"block-job-cancel", > "arguments":{ > "device":"virtio0" > } > } > > {"return": {}} > > It (`block-job-cancel`) will counterintuitively emit the event > 'BLOCK_JOB_COMPLETED': > > { > "timestamp":{ > "seconds":1510678024, > "microseconds":526240 > }, > "event":"BLOCK_JOB_COMPLETED", > "data":{ > "device":"virtio0", > "len":41126400, > "offset":41126400, > "speed":0, > "type":"mirror" > } > } > > But this is expected behaviour, where the _COMPLETED event indicates > that synchronization has successfully ended (and the destination has a > point-in-time copy, which is at the time of cancel). > > So add a small note to this effect. (Thanks: Max Reitz for reminding > me of this on IRC.) > I suppose this difference probably isn't covered in what was the bitmaps.md doc file (we don't bother covering mirror there, only backup); is it covered sufficiently in live-block-operations.rst ? --js
On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote: > > > On 11/15/2017 04:09 AM, Kashyap Chamarthy wrote: > > When you cancel an in-progress live block operation with QMP > > `block-job-cancel`, it emits the event: BLOCK_JOB_CANCELLED. However, > > when `block-job-cancel` is issued after `drive-mirror` has indicated (by > > emitting the event BLOCK_JOB_READY) that the source and destination > > remain synchronized: [...] > > But this is expected behaviour, where the _COMPLETED event indicates > > that synchronization has successfully ended (and the destination has a > > point-in-time copy, which is at the time of cancel). > > > > So add a small note to this effect. (Thanks: Max Reitz for reminding > > me of this on IRC.) > > > > I suppose this difference probably isn't covered in what was the > bitmaps.md doc file (we don't bother covering mirror there, only > backup); Your supposition is correct: Looking at the now-renamed 'bitmaps.rst'[1], it isn't covered there. > is it covered sufficiently in live-block-operations.rst ? I looked in there[2] too. Short answer: no. Long: In the "Live disk synchronization — drive-mirror and blockdev-mirror" section, I simply seemed to declare: "Issuing the command ``block-job-cancel`` after it emits the event ``BLOCK_JOB_CANCELLED``" As if that's the *only* event it emits, which is clearly not the case. So while at it, wonder if should I also update it ('live-block-operations.rst') too. [1] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/bitmaps.rst [2] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l513 -- /kashyap
On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote: > On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote: >> >> >> On 11/15/2017 04:09 AM, Kashyap Chamarthy wrote: >>> When you cancel an in-progress live block operation with QMP >>> `block-job-cancel`, it emits the event: BLOCK_JOB_CANCELLED. However, >>> when `block-job-cancel` is issued after `drive-mirror` has indicated (by >>> emitting the event BLOCK_JOB_READY) that the source and destination >>> remain synchronized: > > [...] > >>> But this is expected behaviour, where the _COMPLETED event indicates >>> that synchronization has successfully ended (and the destination has a >>> point-in-time copy, which is at the time of cancel). >>> >>> So add a small note to this effect. (Thanks: Max Reitz for reminding >>> me of this on IRC.) >>> >> >> I suppose this difference probably isn't covered in what was the >> bitmaps.md doc file (we don't bother covering mirror there, only >> backup); > > Your supposition is correct: Looking at the now-renamed > 'bitmaps.rst'[1], it isn't covered there. > Makes sense, I don't think we need to correct this, and >> is it covered sufficiently in live-block-operations.rst ? > > I looked in there[2] too. Short answer: no. Long: In the "Live disk > synchronization — drive-mirror and blockdev-mirror" section, I simply > seemed to declare: > > "Issuing the command ``block-job-cancel`` after it emits the event > ``BLOCK_JOB_CANCELLED``" > > As if that's the *only* event it emits, which is clearly not the case. > So while at it, wonder if should I also update it > ('live-block-operations.rst') too. > It's an interesting gotcha that I wasn't really acutely aware of myself, so having it in the doc format for API programmers who aren't necessarily digging through our source sounds like a pleasant courtesy. > [1] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/bitmaps.rst > [2] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l513 > Thanks Kashyap :)
On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote: > > > On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote: > > On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote: [...] > >> is it covered sufficiently in live-block-operations.rst ? > > > > I looked in there[2] too. Short answer: no. Long: In the "Live disk > > synchronization — drive-mirror and blockdev-mirror" section, I simply > > seemed to declare: > > > > "Issuing the command ``block-job-cancel`` after it emits the event > > ``BLOCK_JOB_CANCELLED``" > > > > As if that's the *only* event it emits, which is clearly not the case. > > So while at it, wonder if should I also update it > > ('live-block-operations.rst') too. > > > > It's an interesting gotcha that I wasn't really acutely aware of myself, > so having it in the doc format for API programmers who aren't > necessarily digging through our source sounds like a pleasant courtesy. Indeed, will do. (Just for my own clarity, did you imply: don't update it in block-core.json? FWIW, my first instinct is to check the QAPI documentation for such things, that's why I wrote there first :-)) Thanks for looking. [...] -- /kashyap
On 11/16/2017 04:14 AM, Kashyap Chamarthy wrote: > On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote: >> >> >> On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote: >>> On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote: > > [...] > >>>> is it covered sufficiently in live-block-operations.rst ? >>> >>> I looked in there[2] too. Short answer: no. Long: In the "Live disk >>> synchronization — drive-mirror and blockdev-mirror" section, I simply >>> seemed to declare: >>> >>> "Issuing the command ``block-job-cancel`` after it emits the event >>> ``BLOCK_JOB_CANCELLED``" >>> >>> As if that's the *only* event it emits, which is clearly not the case. >>> So while at it, wonder if should I also update it >>> ('live-block-operations.rst') too. >>> >> >> It's an interesting gotcha that I wasn't really acutely aware of myself, >> so having it in the doc format for API programmers who aren't >> necessarily digging through our source sounds like a pleasant courtesy. > > Indeed, will do. (Just for my own clarity, did you imply: don't update > it in block-core.json? FWIW, my first instinct is to check the QAPI > documentation for such things, that's why I wrote there first :-)) > No, definitely update both. > Thanks for looking. > > [...] >
Am 16.11.2017 um 10:14 hat Kashyap Chamarthy geschrieben: > On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote: > > > > > > On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote: > > > On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote: > > [...] > > > >> is it covered sufficiently in live-block-operations.rst ? > > > > > > I looked in there[2] too. Short answer: no. Long: In the "Live disk > > > synchronization — drive-mirror and blockdev-mirror" section, I simply > > > seemed to declare: > > > > > > "Issuing the command ``block-job-cancel`` after it emits the event > > > ``BLOCK_JOB_CANCELLED``" > > > > > > As if that's the *only* event it emits, which is clearly not the case. > > > So while at it, wonder if should I also update it > > > ('live-block-operations.rst') too. > > > > > > > It's an interesting gotcha that I wasn't really acutely aware of myself, > > so having it in the doc format for API programmers who aren't > > necessarily digging through our source sounds like a pleasant courtesy. > > Indeed, will do. (Just for my own clarity, did you imply: don't update > it in block-core.json? FWIW, my first instinct is to check the QAPI > documentation for such things, that's why I wrote there first :-)) Will that be a separate patch or do you intend to send a v3? Kevin
On Fri, Nov 17, 2017 at 11:49:05AM +0100, Kevin Wolf wrote: > Am 16.11.2017 um 10:14 hat Kashyap Chamarthy geschrieben: > > On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote: [...] > > > It's an interesting gotcha that I wasn't really acutely aware of myself, > > > so having it in the doc format for API programmers who aren't > > > necessarily digging through our source sounds like a pleasant courtesy. > > > > Indeed, will do. (Just for my own clarity, did you imply: don't update > > it in block-core.json? FWIW, my first instinct is to check the QAPI > > documentation for such things, that's why I wrote there first :-)) > > Will that be a separate patch or do you intend to send a v3? I can send a separate one if you prefer, but I was intending to send a v3 with both files fixed as it's just a related trivial change. Is that okay? -- /kashyap
© 2016 - 2024 Red Hat, Inc.