[Qemu-devel] [PATCH 0/2] fix [mirror: double performance...]

[Qemu-devel] [PATCH 0/2] fix [mirror: double performance...]

[Vladimir Sementsov-Ogievskiy]

Hi all.

Here are some fixes related to my
[mirror: double performance of the bulk stage if the disc is full]

As Kevin noted, iotest 109 is broken after [mirror: double perf..].
I'm sorry about it, 0002 fixes this test.
0001 is just related improvement and is not necessary.

Vladimir Sementsov-Ogievskiy (2):
  mirror: finish earlier on error
  iotests: fix 109

 block/mirror.c                   | 4 ++++
 tests/qemu-iotests/109           | 2 +-
 tests/qemu-iotests/109.out       | 8 ++++----
 tests/qemu-iotests/common.filter | 6 ++++++
 4 files changed, 15 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] mirror: finish earlier on error

[Vladimir Sementsov-Ogievskiy]

Stop to produce new async copy requests from mirror_iteration if
critical error (error action = BLOCK_ERROR_ACTION_REPORT) detected.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/mirror.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index d6034f5..e0b3f41 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -419,6 +419,10 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
             mirror_wait_for_io(s);
         }
 
+        if (s->ret < 0) {
+            return 0;
+        }
+
         mirror_clip_sectors(s, sector_num, &io_sectors);
         switch (mirror_method) {
         case MIRROR_METHOD_COPY:
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] iotests: fix 109

[Vladimir Sementsov-Ogievskiy]

109 iotest is broken for raw after 0965a41e998ab820b5
[mirror: double performance of the bulk stage if the disc is full]

The problem is with finishing block-job with error: before specified
patch mirror was not very async and it created one big request at disk
start, this request finished with error and qemu produced
BLOCK_JOB_COMPLETED with zero progress.

After 0965a41, mirror starts several smaller requests in parallel, when
BLOCK_JOB_COMPLETED emited we have some successful non-zero progress.

This patch solves the issue by filtering out progress from 109 test
output.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/109           | 2 +-
 tests/qemu-iotests/109.out       | 8 ++++----
 tests/qemu-iotests/common.filter | 6 ++++++
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109
index adf9889..280ed27 100755
--- a/tests/qemu-iotests/109
+++ b/tests/qemu-iotests/109
@@ -100,7 +100,7 @@ for sample_img in empty.bochs iotest-dirtylog-10G-4M.vhdx parallels-v1 \
     _make_test_img 64M
     bzcat "$SAMPLE_IMG_DIR/$sample_img.bz2" > "$TEST_IMG.src"
 
-    run_qemu "$TEST_IMG" "$TEST_IMG.src" "" "BLOCK_JOB_ERROR"
+    run_qemu "$TEST_IMG" "$TEST_IMG.src" "" "BLOCK_JOB_ERROR" | _filter_block_job_offset
     $QEMU_IO -c 'read -P 0 0 64k' "$TEST_IMG" | _filter_qemu_io
 
     run_qemu "$TEST_IMG" "$TEST_IMG.src" "'format': 'raw'," "BLOCK_JOB_READY"
diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out
index 7c797ed..e5d70d7 100644
--- a/tests/qemu-iotests/109.out
+++ b/tests/qemu-iotests/109.out
@@ -135,7 +135,7 @@ Automatically detecting the format is dangerous for raw images, write operations
 Specify the 'raw' format explicitly to remove the restrictions.
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
 {"return": []}
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -155,7 +155,7 @@ Automatically detecting the format is dangerous for raw images, write operations
 Specify the 'raw' format explicitly to remove the restrictions.
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 31457280, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 31457280, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
 {"return": []}
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -175,7 +175,7 @@ Automatically detecting the format is dangerous for raw images, write operations
 Specify the 'raw' format explicitly to remove the restrictions.
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
 {"return": []}
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -195,7 +195,7 @@ Automatically detecting the format is dangerous for raw images, write operations
 Specify the 'raw' format explicitly to remove the restrictions.
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2048, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2048, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
 {"return": []}
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 7853dbb..3ab6e4d 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -77,6 +77,12 @@ _filter_qmp()
         -e '    QMP_VERSION'
 }
 
+# replace block job offset
+_filter_block_job_offset()
+{
+    sed -e 's/, "offset": [0-9]\+,/, "offset": OFFSET,/'
+}
+
 # replace driver-specific options in the "Formatting..." line
 _filter_img_create()
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] iotests: fix 109

[Sascha Silbe]

Dear Vladimir,

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 109 iotest is broken for raw after 0965a41e998ab820b5
> [mirror: double performance of the bulk stage if the disc is full]
>
> The problem is with finishing block-job with error: before specified
> patch mirror was not very async and it created one big request at disk
> start, this request finished with error and qemu produced
> BLOCK_JOB_COMPLETED with zero progress.
>
> After 0965a41, mirror starts several smaller requests in parallel, when
> BLOCK_JOB_COMPLETED emited we have some successful non-zero progress.
[...]

> --- a/tests/qemu-iotests/109.out
> +++ b/tests/qemu-iotests/109.out
> @@ -135,7 +135,7 @@ Automatically detecting the format is dangerous for raw images, write operations
>  Specify the 'raw' format explicitly to remove the restrictions.
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}

What are the exact semantics of the "offset" field for
BLOCK_JOB_COMPLETED?

docs/qmp-events.txt is rather vague. As an API consumer I'd have assumed
that everything up to offset has been completed successfully. If that
interpretation is correct, offset must be 0 for this test because the
very first sector wasn't mirrored successfully.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH 2/2] iotests: fix 109

[Max Reitz]

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

On 03.08.2016 17:22, Sascha Silbe wrote:
> Dear Vladimir,
> 
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 109 iotest is broken for raw after 0965a41e998ab820b5
>> [mirror: double performance of the bulk stage if the disc is full]
>>
>> The problem is with finishing block-job with error: before specified
>> patch mirror was not very async and it created one big request at disk
>> start, this request finished with error and qemu produced
>> BLOCK_JOB_COMPLETED with zero progress.
>>
>> After 0965a41, mirror starts several smaller requests in parallel, when
>> BLOCK_JOB_COMPLETED emited we have some successful non-zero progress.
> [...]
> 
>> --- a/tests/qemu-iotests/109.out
>> +++ b/tests/qemu-iotests/109.out
>> @@ -135,7 +135,7 @@ Automatically detecting the format is dangerous for raw images, write operations
>>  Specify the 'raw' format explicitly to remove the restrictions.
>>  {"return": {}}
>>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
>> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
>> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
> 
> What are the exact semantics of the "offset" field for
> BLOCK_JOB_COMPLETED?
> 
> docs/qmp-events.txt is rather vague. As an API consumer I'd have assumed
> that everything up to offset has been completed successfully. If that
> interpretation is correct, offset must be 0 for this test because the
> very first sector wasn't mirrored successfully.

As far as I'm aware, it doesn't have any real semantics besides the fact
that $offset / $len is the progress of the block job; so it's the offset
"in the job", but not the offset in the source disk.

Max


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

Re: [Qemu-devel] [PATCH 2/2] iotests: fix 109

[Sascha Silbe]

Dear Max,

Max Reitz <mreitz@redhat.com> writes:

> On 03.08.2016 17:22, Sascha Silbe wrote:
[...]
>> What are the exact semantics of the "offset" field for
>> BLOCK_JOB_COMPLETED?
>> 
>> docs/qmp-events.txt is rather vague. As an API consumer I'd have assumed
>> that everything up to offset has been completed successfully. If that
>> interpretation is correct, offset must be 0 for this test because the
>> very first sector wasn't mirrored successfully.
>
> As far as I'm aware, it doesn't have any real semantics besides the fact
> that $offset / $len is the progress of the block job; so it's the offset
> "in the job", but not the offset in the source disk.

Ok. Would be nice to mention that in docs/qmp-events.txt, to avoid API
consumers relying on more exact semantics (resuming at the block after
offset to step over error locations for example). The name 'offset' is
pretty suggestive...

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH 0/2] fix [mirror: double performance...]

[Kevin Wolf]

Am 03.08.2016 um 14:56 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all.
> 
> Here are some fixes related to my
> [mirror: double performance of the bulk stage if the disc is full]
> 
> As Kevin noted, iotest 109 is broken after [mirror: double perf..].
> I'm sorry about it, 0002 fixes this test.
> 0001 is just related improvement and is not necessary.

Thanks, applied to the block branch.

Kevin