[Qemu-devel] [PATCH v2 2.1 0/3] Fix two recent event regressions

[Qemu-devel] [PATCH v2 2.1 0/3] Fix two recent event regressions

[Markus Armbruster]

This series plus Paolo's "[PATCH for 2.1] qdev: correctly send
DEVICE_DELETED for recursively-deleted devices" makes qemu-iotests
check -qcow2 again pass for me.

Kevin, Luiz, you decide how to best route these three patches.  Note
that PATCH 3 depends on Luiz's "[PATCH] qmp: add qmp-events.txt back".

v2:
* PATCH 1: Supply missing comments in schema [Kevin]
* PATCH 3: New [Luiz]

Markus Armbruster (3):
  blockjob: Fix recent BLOCK_JOB_READY regression
  blockjob: Fix recent BLOCK_JOB_ERROR regression
  docs/qmp: Fix documentation of BLOCK_JOB_READY to match code

 blockjob.c              |  8 ++++++--
 docs/qmp/qmp-events.txt | 12 ++++++++++--
 qapi/block-core.json    | 17 +++++++++++++++--
 3 files changed, 31 insertions(+), 6 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Markus Armbruster]

Commit bcada37 dropped the (up to now undocumented) members type, len,
offset, speed, breaking tests/qemu-iotests/040 and 041.

Restore and document them.  This fixes 040, and partially fixes 041.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Tested-By: Benoit Canet <benoit@irqsave.net>
---
 blockjob.c           |  6 +++++-
 qapi/block-core.json | 15 ++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 4da86cd..37a8f1f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -256,7 +256,11 @@ void block_job_event_completed(BlockJob *job, const char *msg)
 
 void block_job_event_ready(BlockJob *job)
 {
-    qapi_event_send_block_job_ready(bdrv_get_device_name(job->bs), &error_abort);
+    qapi_event_send_block_job_ready(job->driver->job_type,
+                                    bdrv_get_device_name(job->bs),
+                                    job->len,
+                                    job->offset,
+                                    job->speed, &error_abort);
 }
 
 BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index af6b436..822fe16 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1551,12 +1551,25 @@
 #
 # Emitted when a block job is ready to complete
 #
+# @type: job type
+#
 # @device: device name
 #
+# @len: maximum progress value
+#
+# @offset: current progress value. On success this is equal to len.
+#          On failure this is less than len
+#
+# @speed: rate limit, bytes per second
+#
 # Note: The "ready to complete" status is always reset by a @BLOCK_JOB_ERROR
 # event
 #
 # Since: 1.3
 ##
 { 'event': 'BLOCK_JOB_READY',
-  'data': { 'device': 'str' } }
+  'data': { 'type'  : 'BlockJobType',
+            'device': 'str',
+            'len'   : 'int',
+            'offset': 'int',
+            'speed' : 'int' } }
-- 
1.9.3

[Qemu-devel] [PATCH v2 2.1 2/3] blockjob: Fix recent BLOCK_JOB_ERROR regression

[Markus Armbruster]

Commit 5a2d2cb screwed up the the value of members device and action,
breaking tests/qemu-iotests/041.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Tested-By: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockjob.c           | 2 +-
 qapi/block-core.json | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 37a8f1f..4d8ff45 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -286,7 +286,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
     default:
         abort();
     }
-    qapi_event_send_block_job_error(bdrv_get_device_name(bs),
+    qapi_event_send_block_job_error(bdrv_get_device_name(job->bs),
                                     is_read ? IO_OPERATION_TYPE_READ :
                                     IO_OPERATION_TYPE_WRITE,
                                     action, &error_abort);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 822fe16..fd5b579 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1544,7 +1544,7 @@
 { 'event': 'BLOCK_JOB_ERROR',
   'data': { 'device'   : 'str',
             'operation': 'IoOperationType',
-            'action'   : 'BlockdevOnError' } }
+            'action'   : 'BlockErrorAction' } }
 
 ##
 # @BLOCK_JOB_READY
-- 
1.9.3

[Qemu-devel] [PATCH v2 2.1 3/3] docs/qmp: Fix documentation of BLOCK_JOB_READY to match code

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qmp/qmp-events.txt | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index 22fea58..44be891 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
 
 Data:
 
-- "device": device name (json-string)
+- "type":     Job type (json-string; "stream" for image streaming
+                                     "commit" for block commit)
+- "device":   Device name (json-string)
+- "len":      Maximum progress value (json-int)
+- "offset":   Current progress value (json-int)
+              On success this is equal to len.
+              On failure this is less than len.
+- "speed":    Rate limit, bytes per second (json-int)
 
 Example:
 
 { "event": "BLOCK_JOB_READY",
-    "data": { "device": "ide0-hd1" },
+    "data": { "device": "drive0", "type": "mirror", "speed": 0,
+              "len": 2097152, "offset": 2097152 }
     "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 
 Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2.1 3/3] docs/qmp: Fix documentation of BLOCK_JOB_READY to match code

[Luiz Capitulino]

On Fri, 27 Jun 2014 19:24:15 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>

I've cherry picked this one as this file only exists in qmp tree.

> ---
>  docs/qmp/qmp-events.txt | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index 22fea58..44be891 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
>  
>  Data:
>  
> -- "device": device name (json-string)
> +- "type":     Job type (json-string; "stream" for image streaming
> +                                     "commit" for block commit)
> +- "device":   Device name (json-string)
> +- "len":      Maximum progress value (json-int)
> +- "offset":   Current progress value (json-int)
> +              On success this is equal to len.
> +              On failure this is less than len.
> +- "speed":    Rate limit, bytes per second (json-int)
>  
>  Example:
>  
>  { "event": "BLOCK_JOB_READY",
> -    "data": { "device": "ide0-hd1" },
> +    "data": { "device": "drive0", "type": "mirror", "speed": 0,
> +              "len": 2097152, "offset": 2097152 }
>      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
>  Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR

Re: [Qemu-devel] [PATCH v2 2.1 0/3] Fix two recent event regressions

[Kevin Wolf]

Am 27.06.2014 um 19:24 hat Markus Armbruster geschrieben:
> This series plus Paolo's "[PATCH for 2.1] qdev: correctly send
> DEVICE_DELETED for recursively-deleted devices" makes qemu-iotests
> check -qcow2 again pass for me.
> 
> Kevin, Luiz, you decide how to best route these three patches.  Note
> that PATCH 3 depends on Luiz's "[PATCH] qmp: add qmp-events.txt back".
> 
> v2:
> * PATCH 1: Supply missing comments in schema [Kevin]
> * PATCH 3: New [Luiz]

Thanks, applied patches 1 and 2 to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v2 2.1 2/3] blockjob: Fix recent BLOCK_JOB_ERROR regression

[Wenchao Xia]

于 2014/6/28 1:24, Markus Armbruster 写道:
> Commit 5a2d2cb screwed up the the value of members device and action,
> breaking tests/qemu-iotests/041.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Tested-By: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>   blockjob.c           | 2 +-
>   qapi/block-core.json | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 37a8f1f..4d8ff45 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -286,7 +286,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
>       default:
>           abort();
>       }
> -    qapi_event_send_block_job_error(bdrv_get_device_name(bs),
> +    qapi_event_send_block_job_error(bdrv_get_device_name(job->bs),
>                                       is_read ? IO_OPERATION_TYPE_READ :
>                                       IO_OPERATION_TYPE_WRITE,
>                                       action, &error_abort);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 822fe16..fd5b579 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1544,7 +1544,7 @@
>   { 'event': 'BLOCK_JOB_ERROR',
>     'data': { 'device'   : 'str',
>               'operation': 'IoOperationType',
> -            'action'   : 'BlockdevOnError' } }
> +            'action'   : 'BlockErrorAction' } }
>   
  It is my mistake to use BlockdevOnError in code incorrectly.
The define as 'BlockdevOnError' before is on purpose, since the
doc for 'BlockErrorAction' says: stop means a VM is stoped, but
for block-job it is not true, so I chosed a different type, and
'BlockdevOnError' seems the right one(see the doc for it). We can
fix it in C caller or add doc in .json file later.
  I am occupied by other things these days, thanks for fixing
the bugs introduced by me!


>   ##
>   # @BLOCK_JOB_READY
> 

Re: [Qemu-devel] [PATCH v2 2.1 2/3] blockjob: Fix recent BLOCK_JOB_ERROR regression

[Paolo Bonzini]

Il 01/07/2014 16:42, Wenchao Xia ha scritto:
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 822fe16..fd5b579 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1544,7 +1544,7 @@
>>   { 'event': 'BLOCK_JOB_ERROR',
>>     'data': { 'device'   : 'str',
>>               'operation': 'IoOperationType',
>> -            'action'   : 'BlockdevOnError' } }
>> +            'action'   : 'BlockErrorAction' } }
>>   
>   It is my mistake to use BlockdevOnError in code incorrectly.
> The define as 'BlockdevOnError' before is on purpose, since the
> doc for 'BlockErrorAction' says: stop means a VM is stoped, but
> for block-job it is not true, so I chosed a different type, and
> 'BlockdevOnError' seems the right one(see the doc for it). We can
> fix it in C caller or add doc in .json file later.

I think consistency between BLOCK_IO_ERROR and BLOCK_JOB_ERROR is
better.  Let's fix the BlockErrorAction documentation instead.

Paolo

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]

On 06/27/2014 11:24 AM, Markus Armbruster wrote:
> Commit bcada37 dropped the (up to now undocumented) members type, len,
> offset, speed, breaking tests/qemu-iotests/040 and 041.
> 
> Restore and document them.  This fixes 040, and partially fixes 041.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Tested-By: Benoit Canet <benoit@irqsave.net>
> ---
>  blockjob.c           |  6 +++++-
>  qapi/block-core.json | 15 ++++++++++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)

Nothing wrong with this commit, but a design issue that I've recently
run into:

what happens if management misses the BLOCK_JOB_COMPLETED event?  How is
it supposed to learn whether the job succeeded or failed?
'query-blockjobs' no longer reports the job (because it is completed),
so all information about the job is lost.  Normally, we've tried hard to
make sure that all information learned from an event can also be polled
(the ideal is use of events to minimize cpu overhead, but to rely on the
poll in situations where events may have been lost such as on a libvirtd
restart).

Should we enhance job failure to be sticky, in that it not only causes
an event, but also remains around so that it can be reported in the next
'query-blockjobs'?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Paolo Bonzini]

Il 01/07/2014 19:08, Eric Blake ha scritto:
> On 06/27/2014 11:24 AM, Markus Armbruster wrote:
>> Commit bcada37 dropped the (up to now undocumented) members type, len,
>> offset, speed, breaking tests/qemu-iotests/040 and 041.
>>
>> Restore and document them.  This fixes 040, and partially fixes 041.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Tested-By: Benoit Canet <benoit@irqsave.net>
>> ---
>>  blockjob.c           |  6 +++++-
>>  qapi/block-core.json | 15 ++++++++++++++-
>>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> Nothing wrong with this commit, but a design issue that I've recently
> run into:
>
> what happens if management misses the BLOCK_JOB_COMPLETED event?  How is
> it supposed to learn whether the job succeeded or failed?
> 'query-blockjobs' no longer reports the job (because it is completed),
> so all information about the job is lost.  Normally, we've tried hard to
> make sure that all information learned from an event can also be polled
> (the ideal is use of events to minimize cpu overhead, but to rely on the
> poll in situations where events may have been lost such as on a libvirtd
> restart).
>
> Should we enhance job failure to be sticky, in that it not only causes
> an event, but also remains around so that it can be reported in the next
> 'query-blockjobs'?

I think this fixes itself automatically if you use 
rerror=stop/werror=stop on block jobs.  At least that was part of the 
design, whether the implementation gets it right I cannot say without 
looking at the code more carefully.

Paolo

Re: [Qemu-devel] [PATCH v2 2.1 3/3] docs/qmp: Fix documentation of BLOCK_JOB_READY to match code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

On 06/27/2014 11:24 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qmp/qmp-events.txt | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index 22fea58..44be891 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
>  
>  Data:
>  
> -- "device": device name (json-string)
> +- "type":     Job type (json-string; "stream" for image streaming
> +                                     "commit" for block commit)
> +- "device":   Device name (json-string)
> +- "len":      Maximum progress value (json-int)
> +- "offset":   Current progress value (json-int)
> +              On success this is equal to len.
> +              On failure this is less than len.
> +- "speed":    Rate limit, bytes per second (json-int)
>  

Design question - if BLOCK_JOB_READY reports failure (that is, offset <
len), are we still guaranteed to get a BLOCK_JOB_COMPLETED that also
reports failure, or does 'query-blockjobs' completely forget about the
job? If the job is completely lost, what recourse does management have
to learn about the failure (that is, if libvirtd restarts, how will it
learn whether a previously running job was aborted due to an error, if
it missed the event)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2.1 2/3] blockjob: Fix recent BLOCK_JOB_ERROR regression

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 01/07/2014 16:42, Wenchao Xia ha scritto:
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 822fe16..fd5b579 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1544,7 +1544,7 @@
>>>   { 'event': 'BLOCK_JOB_ERROR',
>>>     'data': { 'device'   : 'str',
>>>               'operation': 'IoOperationType',
>>> -            'action'   : 'BlockdevOnError' } }
>>> +            'action'   : 'BlockErrorAction' } }
>>>   
>>   It is my mistake to use BlockdevOnError in code incorrectly.
>> The define as 'BlockdevOnError' before is on purpose, since the
>> doc for 'BlockErrorAction' says: stop means a VM is stoped, but
>> for block-job it is not true, so I chosed a different type, and
>> 'BlockdevOnError' seems the right one(see the doc for it). We can
>> fix it in C caller or add doc in .json file later.
>
> I think consistency between BLOCK_IO_ERROR and BLOCK_JOB_ERROR is
> better.

Moreover, BlockdevOnError's enospc is meaningless here.

>          Let's fix the BlockErrorAction documentation instead.

I didn't quite understand what's wrong with it; I expect a patch will
enlighten me :)

Re: [Qemu-devel] [PATCH v2 2.1 3/3] docs/qmp: Fix documentation of BLOCK_JOB_READY to match code

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/27/2014 11:24 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/qmp/qmp-events.txt | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
>> index 22fea58..44be891 100644
>> --- a/docs/qmp/qmp-events.txt
>> +++ b/docs/qmp/qmp-events.txt
>> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
>>  
>>  Data:
>>  
>> -- "device": device name (json-string)
>> +- "type":     Job type (json-string; "stream" for image streaming
>> +                                     "commit" for block commit)
>> +- "device":   Device name (json-string)
>> +- "len":      Maximum progress value (json-int)
>> +- "offset":   Current progress value (json-int)
>> +              On success this is equal to len.
>> +              On failure this is less than len.
>> +- "speed":    Rate limit, bytes per second (json-int)
>>  
>
> Design question - if BLOCK_JOB_READY reports failure (that is, offset <
> len), are we still guaranteed to get a BLOCK_JOB_COMPLETED that also
> reports failure, or does 'query-blockjobs' completely forget about the
> job?

Good one.  It's underspecified, as far as I can tell.  First step to fix
that is to find out what the code does.

>      If the job is completely lost, what recourse does management have
> to learn about the failure (that is, if libvirtd restarts, how will it
> learn whether a previously running job was aborted due to an error, if
> it missed the event)?

Let's worry about that when we know what the code does.

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 01/07/2014 19:08, Eric Blake ha scritto:
>> On 06/27/2014 11:24 AM, Markus Armbruster wrote:
>>> Commit bcada37 dropped the (up to now undocumented) members type, len,
>>> offset, speed, breaking tests/qemu-iotests/040 and 041.
>>>
>>> Restore and document them.  This fixes 040, and partially fixes 041.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Tested-By: Benoit Canet <benoit@irqsave.net>
>>> ---
>>>  blockjob.c           |  6 +++++-
>>>  qapi/block-core.json | 15 ++++++++++++++-
>>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> Nothing wrong with this commit, but a design issue that I've recently
>> run into:
>>
>> what happens if management misses the BLOCK_JOB_COMPLETED event?  How is
>> it supposed to learn whether the job succeeded or failed?
>> 'query-blockjobs' no longer reports the job (because it is completed),
>> so all information about the job is lost.  Normally, we've tried hard to
>> make sure that all information learned from an event can also be polled

Yes.  Every time we neglect that, we find out it's a design bug later.

We should review all events for pollability, and add "how to poll"
information to their documentation.  Then enforce presence of "how to
poll" information in review.

>> (the ideal is use of events to minimize cpu overhead, but to rely on the
>> poll in situations where events may have been lost such as on a libvirtd
>> restart).
>>
>> Should we enhance job failure to be sticky, in that it not only causes
>> an event, but also remains around so that it can be reported in the next
>> 'query-blockjobs'?
>
> I think this fixes itself automatically if you use
> rerror=stop/werror=stop on block jobs.  At least that was part of the
> design, whether the implementation gets it right I cannot say without
> looking at the code more carefully.

What if an underlying device doesn't support [rw]error=stop?  Not all
do...

Re: [Qemu-devel] [PATCH v2 2.1 3/3] docs/qmp: Fix documentation of BLOCK_JOB_READY to match code

[Paolo Bonzini]

Il 01/07/2014 19:12, Eric Blake ha scritto:
> On 06/27/2014 11:24 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/qmp/qmp-events.txt | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
>> index 22fea58..44be891 100644
>> --- a/docs/qmp/qmp-events.txt
>> +++ b/docs/qmp/qmp-events.txt
>> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
>>
>>  Data:
>>
>> -- "device": device name (json-string)
>> +- "type":     Job type (json-string; "stream" for image streaming
>> +                                     "commit" for block commit)
>> +- "device":   Device name (json-string)
>> +- "len":      Maximum progress value (json-int)
>> +- "offset":   Current progress value (json-int)
>> +              On success this is equal to len.
>> +              On failure this is less than len.
>> +- "speed":    Rate limit, bytes per second (json-int)
>>
>
> Design question - if BLOCK_JOB_READY reports failure (that is, offset <
> len), are we still guaranteed to get a BLOCK_JOB_COMPLETED that also
> reports failure, or does 'query-blockjobs' completely forget about the
> job? If the job is completely lost, what recourse does management have
> to learn about the failure (that is, if libvirtd restarts, how will it
> learn whether a previously running job was aborted due to an error, if
> it missed the event)?

If you use rerror=stop/werror=stop you do have the behavior you request. 
  The job will not abort, it will be paused and libvirt will be able to 
see the error.  In fact, that was the rationale for adding 
rerror=stop/werror=stop to streaming (where you can just restart a 
failed job and only have to do minimal extra work, unlike mirror and 
commit).

There may be a couple of holes in the logic (for example a failure in 
bdrv_change_backing_file), but they can be plugged if necessary.

Paolo

Re: [Qemu-devel] [PATCH v2 2.1 2/3] blockjob: Fix recent BLOCK_JOB_ERROR regression

[Paolo Bonzini]

Il 02/07/2014 08:46, Markus Armbruster ha scritto:
>> > I think consistency between BLOCK_IO_ERROR and BLOCK_JOB_ERROR is
>> > better.
> Moreover, BlockdevOnError's enospc is meaningless here.
>
>> >          Let's fix the BlockErrorAction documentation instead.
> I didn't quite understand what's wrong with it; I expect a patch will
> enlighten me :)

For BLOCK_JOB_ERROR, "stop" means the job is paused rather than the VM. 
  I have more documentation work queued for hard freeze, so you or 
Wenchao should feel free to submit this bit too. ;)

Paolo

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Paolo Bonzini]

Il 02/07/2014 08:55, Markus Armbruster ha scritto:
>> > I think this fixes itself automatically if you use
>> > rerror=stop/werror=stop on block jobs.  At least that was part of the
>> > design, whether the implementation gets it right I cannot say without
>> > looking at the code more carefully.
> What if an underlying device doesn't support [rw]error=stop?  Not all
> do...

Then the "fix" is to add support to the underlying device.  IDE, SCSI 
and virtio-blk (plus virtio-scsi via SCSI of course) are covered; the 
main one that's left out is SD.

Paolo

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Kevin Wolf]

Am 02.07.2014 um 10:23 hat Paolo Bonzini geschrieben:
> Il 02/07/2014 08:55, Markus Armbruster ha scritto:
> >>> I think this fixes itself automatically if you use
> >>> rerror=stop/werror=stop on block jobs.  At least that was part of the
> >>> design, whether the implementation gets it right I cannot say without
> >>> looking at the code more carefully.
> >What if an underlying device doesn't support [rw]error=stop?  Not all
> >do...
> 
> Then the "fix" is to add support to the underlying device.  IDE,
> SCSI and virtio-blk (plus virtio-scsi via SCSI of course) are
> covered; the main one that's left out is SD.

Isn't block job rerror/werror completely independent from the guest
device using the same BDS? They are different users, so if we somehow
coupled them, that would be a design problem (which I think we were
careful enough to avoid).

Kevin

Re: [Qemu-devel] [PATCH v2 2.1 3/3] docs/qmp: Fix documentation of BLOCK_JOB_READY to match code

[Kevin Wolf]

Am 02.07.2014 um 08:49 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 06/27/2014 11:24 AM, Markus Armbruster wrote:
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  docs/qmp/qmp-events.txt | 12 ++++++++++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> >> index 22fea58..44be891 100644
> >> --- a/docs/qmp/qmp-events.txt
> >> +++ b/docs/qmp/qmp-events.txt
> >> @@ -157,12 +157,20 @@ Emitted when a block job is ready to complete.
> >>  
> >>  Data:
> >>  
> >> -- "device": device name (json-string)
> >> +- "type":     Job type (json-string; "stream" for image streaming
> >> +                                     "commit" for block commit)
> >> +- "device":   Device name (json-string)
> >> +- "len":      Maximum progress value (json-int)
> >> +- "offset":   Current progress value (json-int)
> >> +              On success this is equal to len.
> >> +              On failure this is less than len.
> >> +- "speed":    Rate limit, bytes per second (json-int)
> >>  
> >
> > Design question - if BLOCK_JOB_READY reports failure (that is, offset <
> > len), are we still guaranteed to get a BLOCK_JOB_COMPLETED that also
> > reports failure, or does 'query-blockjobs' completely forget about the
> > job?
> 
> Good one.  It's underspecified, as far as I can tell.  First step to fix
> that is to find out what the code does.

As far as I can see, BLOCK_JOB_READY implies success up to the point
where the event was emitted. Should we document that len = offset in all
cases? Perhaps add an assertion to block_job_event_ready(), too?

Kevin

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Paolo Bonzini]

Il 02/07/2014 10:44, Kevin Wolf ha scritto:
>> > Then the "fix" is to add support to the underlying device.  IDE,
>> > SCSI and virtio-blk (plus virtio-scsi via SCSI of course) are
>> > covered; the main one that's left out is SD.
> Isn't block job rerror/werror completely independent from the guest
> device using the same BDS? They are different users, so if we somehow
> coupled them, that would be a design problem (which I think we were
> careful enough to avoid).

It is independent, but for some reason rerror/werror cannot be used on 
block jobs unless iostatus is enabled on the source BDS too.  Off-hand I 
don't remember why.

Paolo

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 02/07/2014 08:55, Markus Armbruster ha scritto:
>>> > I think this fixes itself automatically if you use
>>> > rerror=stop/werror=stop on block jobs.  At least that was part of the
>>> > design, whether the implementation gets it right I cannot say without
>>> > looking at the code more carefully.
>> What if an underlying device doesn't support [rw]error=stop?  Not all
>> do...
>
> Then the "fix" is to add support to the underlying device.  IDE, SCSI
> and virtio-blk (plus virtio-scsi via SCSI of course) are covered;

Where "covered" means "device model calls bdrv_error_action() somewhere"
rather than "device model calls bdrv_error_action() exactly when it
should".

Case in point: SCSI calls it when UNMAP fails, but IDE doesn't call it
when TRIM fails.  IDE and virtio-blk call it for I/O beyond the end of
the medium, but SCSI doesn't.

This is of course fixable.  I'm working on it.

>                                                                   the
> main one that's left out is SD.

Qdevified devices with a qdev_prop_drive: isa-fdc, sysbus-fdc,
SUNW,fdtwo, nand, onenand, cfi.pflash01, cfi.pflash02, spapr-nvram,
scsi-generic, nvme.  SD isn't in this list, because it still hasn't been
qdevified.  There may be more.

I'm afraid we're better at adding more devices than we are at updating
devices to current interfaces.

Thus, I'm inclined to accept existence of devices that don't (reliably)
support [rw]error=stop as yet another sad fact of life.

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Paolo Bonzini]

Il 02/07/2014 16:58, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 02/07/2014 08:55, Markus Armbruster ha scritto:
>>>>> I think this fixes itself automatically if you use
>>>>> rerror=stop/werror=stop on block jobs.  At least that was part of the
>>>>> design, whether the implementation gets it right I cannot say without
>>>>> looking at the code more carefully.
>>> What if an underlying device doesn't support [rw]error=stop?  Not all
>>> do...
>>
>> Then the "fix" is to add support to the underlying device.  IDE, SCSI
>> and virtio-blk (plus virtio-scsi via SCSI of course) are covered;
>
> Where "covered" means "device model calls bdrv_error_action() somewhere"
> rather than "device model calls bdrv_error_action() exactly when it
> should".
>
> Case in point: SCSI calls it when UNMAP fails, but IDE doesn't call it
> when TRIM fails.  IDE and virtio-blk call it for I/O beyond the end of
> the medium, but SCSI doesn't.
>
> This is of course fixable.  I'm working on it.
>
>>                                                                   the
>> main one that's left out is SD.
>
> Qdevified devices with a qdev_prop_drive: isa-fdc, sysbus-fdc,
> SUNW,fdtwo, nand, onenand, cfi.pflash01, cfi.pflash02, spapr-nvram,
> scsi-generic, nvme.  SD isn't in this list, because it still hasn't been
> qdevified.  There may be more.

I think there is a page with unfinished transition.  Can you add this one?

Paolo

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

On 07/02/2014 09:03 AM, Paolo Bonzini wrote:

>>>> What if an underlying device doesn't support [rw]error=stop?  Not all
>>>> do...
>>>
>>> Then the "fix" is to add support to the underlying device.  IDE, SCSI
>>> and virtio-blk (plus virtio-scsi via SCSI of course) are covered;
>>
>> Where "covered" means "device model calls bdrv_error_action() somewhere"
>> rather than "device model calls bdrv_error_action() exactly when it
>> should".
>>
>> Case in point: SCSI calls it when UNMAP fails, but IDE doesn't call it
>> when TRIM fails.  IDE and virtio-blk call it for I/O beyond the end of
>> the medium, but SCSI doesn't.
>>
>> This is of course fixable.  I'm working on it.
>>
>>>                                                                   the
>>> main one that's left out is SD.
>>
>> Qdevified devices with a qdev_prop_drive: isa-fdc, sysbus-fdc,
>> SUNW,fdtwo, nand, onenand, cfi.pflash01, cfi.pflash02, spapr-nvram,
>> scsi-generic, nvme.  SD isn't in this list, because it still hasn't been
>> qdevified.  There may be more.
> 
> I think there is a page with unfinished transition.  Can you add this one?

Listed: http://wiki.qemu.org/CodeTransitions#Reliable_block_job_polling

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 02/07/2014 16:58, Markus Armbruster ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 02/07/2014 08:55, Markus Armbruster ha scritto:
>>>>>> I think this fixes itself automatically if you use
>>>>>> rerror=stop/werror=stop on block jobs.  At least that was part of the
>>>>>> design, whether the implementation gets it right I cannot say without
>>>>>> looking at the code more carefully.
>>>> What if an underlying device doesn't support [rw]error=stop?  Not all
>>>> do...
>>>
>>> Then the "fix" is to add support to the underlying device.  IDE, SCSI
>>> and virtio-blk (plus virtio-scsi via SCSI of course) are covered;
>>
>> Where "covered" means "device model calls bdrv_error_action() somewhere"
>> rather than "device model calls bdrv_error_action() exactly when it
>> should".
>>
>> Case in point: SCSI calls it when UNMAP fails, but IDE doesn't call it
>> when TRIM fails.  IDE and virtio-blk call it for I/O beyond the end of
>> the medium, but SCSI doesn't.
>>
>> This is of course fixable.  I'm working on it.
>>
>>>                                                                   the
>>> main one that's left out is SD.
>>
>> Qdevified devices with a qdev_prop_drive: isa-fdc, sysbus-fdc,
>> SUNW,fdtwo, nand, onenand, cfi.pflash01, cfi.pflash02, spapr-nvram,
>> scsi-generic, nvme.  SD isn't in this list, because it still hasn't been
>> qdevified.  There may be more.
>
> I think there is a page with unfinished transition.  Can you add this one?

Do you mean the transition to qdev?

If yes, isn't that covered by "Making all devices QOM objects"?
Sometimes we create new transitions faster than we finish them...

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Paolo Bonzini]

Il 03/07/2014 09:19, Markus Armbruster ha scritto:
>> > I think there is a page with unfinished transition.  Can you add this one?
> Do you mean the transition to qdev?
>
> If yes, isn't that covered by "Making all devices QOM objects"?
> Sometimes we create new transitions faster than we finish them...

No, I meant the transition to supporting rerror/werror.  Eric started 
the patch and I edited it.

Paolo

Re: [Qemu-devel] [PATCH v2 2.1 1/3] blockjob: Fix recent BLOCK_JOB_READY regression

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 03/07/2014 09:19, Markus Armbruster ha scritto:
>>> > I think there is a page with unfinished transition.  Can you add this one?
>> Do you mean the transition to qdev?
>>
>> If yes, isn't that covered by "Making all devices QOM objects"?
>> Sometimes we create new transitions faster than we finish them...
>
> No, I meant the transition to supporting rerror/werror.  Eric started
> the patch and I edited it.

Okay.  I just added another section on I/O accounting, which is almost
as incomplete.