| Message ID | 20171115090947.29883-1-kchamart@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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. [...]
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?
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
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(+)