* [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image
@ 2014-06-05 3:42 Fam Zheng
2014-06-05 3:42 ` [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image Fam Zheng
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Fam Zheng @ 2014-06-05 3:42 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, jcody, Max Reitz, Stefan Hajnoczi, Paolo Bonzini
The current behavior of mirroring zero byte image is slightly different from
non-zero image: the BLOCK_JOB_READY event is skipped and job is completed
immediately. This is not a big problem for human user but only makes management
software harder to write. Since we are focusing on an good API instead of UI,
let's make the behavior more consistent and predictable.
The first patch fixes the behavior. The following two patches add test case for
the involved code path.
Thanks for Eric Blake to report this!
Fam
Fam Zheng (3):
mirror: Go through ready -> complete process for 0 len image
qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit
qemu-iotests: Test 0-length image for mirror
block/mirror.c | 11 ++++++++++-
tests/qemu-iotests/040 | 11 ++++++++++-
tests/qemu-iotests/040.out | 4 ++--
tests/qemu-iotests/041 | 5 +++++
tests/qemu-iotests/041.out | 4 ++--
5 files changed, 29 insertions(+), 6 deletions(-)
--
2.0.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image
2014-06-05 3:42 [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image Fam Zheng
@ 2014-06-05 3:42 ` Fam Zheng
2014-06-05 11:17 ` Stefan Hajnoczi
2014-06-05 3:42 ` [Qemu-devel] [PATCH 2/3] qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit Fam Zheng
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Fam Zheng @ 2014-06-05 3:42 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, jcody, Max Reitz, Stefan Hajnoczi, Paolo Bonzini
When mirroring or active committing a zero length image, BLOCK_JOB_READY
is not reported now, instead the job completes because we short circuit
the mirror job loop.
This is inconsistent with non-zero length images, and only confuses
management software.
Let's do the same thing when seeing a 0-length image: report ready
immediately; wait for block-job-cancel or block-job-complete; clear the
cancel flag as existing non-zero image synced case (cancelled after
ready); then jump to the exit.
Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/block/mirror.c b/block/mirror.c
index 94c8661..2bef5f3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -324,9 +324,18 @@ static void coroutine_fn mirror_run(void *opaque)
}
s->common.len = bdrv_getlength(bs);
- if (s->common.len <= 0) {
+ if (s->common.len < 0) {
ret = s->common.len;
goto immediate_exit;
+ } else if (s->common.len == 0) {
+ /* Report BLOCK_JOB_READY and wait for complete. */
+ block_job_ready(&s->common);
+ s->synced = true;
+ while (!block_job_is_cancelled(&s->common) && !s->should_complete) {
+ block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, SLICE_TIME);
+ }
+ s->common.cancelled = false;
+ goto immediate_exit;
}
length = DIV_ROUND_UP(s->common.len, s->granularity);
--
2.0.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/3] qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit
2014-06-05 3:42 [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image Fam Zheng
2014-06-05 3:42 ` [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image Fam Zheng
@ 2014-06-05 3:42 ` Fam Zheng
2014-06-05 11:22 ` Stefan Hajnoczi
2014-06-05 3:42 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test 0-length image for mirror Fam Zheng
2014-06-05 3:57 ` [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image Eric Blake
3 siblings, 1 reply; 9+ messages in thread
From: Fam Zheng @ 2014-06-05 3:42 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, jcody, Max Reitz, Stefan Hajnoczi, Paolo Bonzini
There should be a BLOCK_JOB_READY event with active commit, regardless
of image length. Let's test the 0 length image case, and make sure it
goes through the ready->complete process.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
tests/qemu-iotests/040 | 11 ++++++++++-
tests/qemu-iotests/040.out | 4 ++--
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 734b6a6..02e2cd5 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -35,12 +35,13 @@ test_img = os.path.join(iotests.test_dir, 'test.img')
class ImageCommitTestCase(iotests.QMPTestCase):
'''Abstract base class for image commit test cases'''
- def run_commit_test(self, top, base):
+ def run_commit_test(self, top, base, need_ready=False):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
self.assert_qmp(result, 'return', {})
completed = False
+ ready = False
while not completed:
for event in self.vm.get_qmp_events(wait=True):
if event['event'] == 'BLOCK_JOB_COMPLETED':
@@ -48,8 +49,11 @@ class ImageCommitTestCase(iotests.QMPTestCase):
self.assert_qmp(event, 'data/device', 'drive0')
self.assert_qmp(event, 'data/offset', self.image_len)
self.assert_qmp(event, 'data/len', self.image_len)
+ if need_ready:
+ assert ready, "Expceting BLOCK_JOB_COMPLETED event"
completed = True
elif event['event'] == 'BLOCK_JOB_READY':
+ ready = True
self.assert_qmp(event, 'data/type', 'commit')
self.assert_qmp(event, 'data/device', 'drive0')
self.assert_qmp(event, 'data/len', self.image_len)
@@ -238,6 +242,11 @@ class TestSetSpeed(ImageCommitTestCase):
self.cancel_and_wait(resume=True)
+class TestActiveZeroLengthImage(TestSingleDrive):
+
+ def setUp(self):
+ TestSingleDrive.image_len = 0
+ return TestSingleDrive.setUp(self)
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index b6f2576..42314e9 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-................
+........................
----------------------------------------------------------------------
-Ran 16 tests
+Ran 24 tests
OK
--
2.0.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 3/3] qemu-iotests: Test 0-length image for mirror
2014-06-05 3:42 [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image Fam Zheng
2014-06-05 3:42 ` [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image Fam Zheng
2014-06-05 3:42 ` [Qemu-devel] [PATCH 2/3] qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit Fam Zheng
@ 2014-06-05 3:42 ` Fam Zheng
2014-06-05 11:26 ` Stefan Hajnoczi
2014-06-05 3:57 ` [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image Eric Blake
3 siblings, 1 reply; 9+ messages in thread
From: Fam Zheng @ 2014-06-05 3:42 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, jcody, Max Reitz, Stefan Hajnoczi, Paolo Bonzini
All behavior and invariant should hold for images with 0 length, so
add a class to repeat all the tests in TestSingleDrive.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
tests/qemu-iotests/041 | 5 +++++
tests/qemu-iotests/041.out | 4 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index ec470b2..ae876ad 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -206,6 +206,11 @@ class TestSingleDrive(ImageMirroringTestCase):
target=target_img)
self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+class TestSingleDriveZeroLength(TestSingleDrive):
+ def setUp(self):
+ TestSingleDrive.image_len = 0
+ return TestSingleDrive.setUp(self)
+
class TestMirrorNoBacking(ImageMirroringTestCase):
image_len = 2 * 1024 * 1024 # MB
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 6d9bee1..cafb816 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-...........................
+.....................................
----------------------------------------------------------------------
-Ran 27 tests
+Ran 37 tests
OK
--
2.0.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image
2014-06-05 3:42 [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image Fam Zheng
` (2 preceding siblings ...)
2014-06-05 3:42 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test 0-length image for mirror Fam Zheng
@ 2014-06-05 3:57 ` Eric Blake
3 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2014-06-05 3:57 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, jcody, Stefan Hajnoczi, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]
On 06/04/2014 09:42 PM, Fam Zheng wrote:
> The current behavior of mirroring zero byte image is slightly different from
> non-zero image: the BLOCK_JOB_READY event is skipped and job is completed
> immediately. This is not a big problem for human user but only makes management
> software harder to write. Since we are focusing on an good API instead of UI,
> let's make the behavior more consistent and predictable.
>
> The first patch fixes the behavior. The following two patches add test case for
> the involved code path.
>
> Thanks for Eric Blake to report this!
>
> Fam
>
Thanks for a speedy fix.
Series: Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Fam Zheng (3):
> mirror: Go through ready -> complete process for 0 len image
> qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit
> qemu-iotests: Test 0-length image for mirror
>
> block/mirror.c | 11 ++++++++++-
> tests/qemu-iotests/040 | 11 ++++++++++-
> tests/qemu-iotests/040.out | 4 ++--
> tests/qemu-iotests/041 | 5 +++++
> tests/qemu-iotests/041.out | 4 ++--
> 5 files changed, 29 insertions(+), 6 deletions(-)
>
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image
2014-06-05 3:42 ` [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image Fam Zheng
@ 2014-06-05 11:17 ` Stefan Hajnoczi
2014-06-05 11:26 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Hajnoczi @ 2014-06-05 11:17 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, jcody, qemu-devel, Max Reitz, Stefan Hajnoczi, Paolo Bonzini
On Thu, Jun 05, 2014 at 11:42:34AM +0800, Fam Zheng wrote:
> diff --git a/block/mirror.c b/block/mirror.c
> index 94c8661..2bef5f3 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -324,9 +324,18 @@ static void coroutine_fn mirror_run(void *opaque)
> }
>
> s->common.len = bdrv_getlength(bs);
> - if (s->common.len <= 0) {
> + if (s->common.len < 0) {
> ret = s->common.len;
> goto immediate_exit;
> + } else if (s->common.len == 0) {
> + /* Report BLOCK_JOB_READY and wait for complete. */
> + block_job_ready(&s->common);
> + s->synced = true;
> + while (!block_job_is_cancelled(&s->common) && !s->should_complete) {
> + block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, SLICE_TIME);
Please take a look at block_job_resume() and how it's used when
cancelling or completing block jobs. There is no need to sleep, instead
we can yield until we get resumed.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit
2014-06-05 3:42 ` [Qemu-devel] [PATCH 2/3] qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit Fam Zheng
@ 2014-06-05 11:22 ` Stefan Hajnoczi
0 siblings, 0 replies; 9+ messages in thread
From: Stefan Hajnoczi @ 2014-06-05 11:22 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, jcody, qemu-devel, Max Reitz, Stefan Hajnoczi, Paolo Bonzini
On Thu, Jun 05, 2014 at 11:42:35AM +0800, Fam Zheng wrote:
> @@ -48,8 +49,11 @@ class ImageCommitTestCase(iotests.QMPTestCase):
> self.assert_qmp(event, 'data/device', 'drive0')
> self.assert_qmp(event, 'data/offset', self.image_len)
> self.assert_qmp(event, 'data/len', self.image_len)
> + if need_ready:
> + assert ready, "Expceting BLOCK_JOB_COMPLETED event"
s/Expceting/Expecting/
Please use TestCase.assert*() methods instead of the assert keyword:
self.assertTrue(ready, msg='Expecting BLOCK_JOB_COMPLETED event')
The assert keyword raises the built-in AssertionError, which may not be
supported by the unittest module:
https://docs.python.org/2/library/unittest.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test 0-length image for mirror
2014-06-05 3:42 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test 0-length image for mirror Fam Zheng
@ 2014-06-05 11:26 ` Stefan Hajnoczi
0 siblings, 0 replies; 9+ messages in thread
From: Stefan Hajnoczi @ 2014-06-05 11:26 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, jcody, qemu-devel, Max Reitz, Stefan Hajnoczi, Paolo Bonzini
On Thu, Jun 05, 2014 at 11:42:36AM +0800, Fam Zheng wrote:
> +class TestSingleDriveZeroLength(TestSingleDrive):
> + def setUp(self):
> + TestSingleDrive.image_len = 0
> + return TestSingleDrive.setUp(self)
This is buggy since it assigns to TestSingleDrive.image_len. It
modifies the class variable for everyone else!
I think we need something like this instead:
>>> class Foo(object):
... a = 1
... def test(self):
... return self.a
...
>>> Foo().a
1
>>> Foo().test()
1
>>> class Bar(Foo):
... a = 2
...
>>> Bar().a
2
>>> Bar().test()
2
Please also fix the previous patch. It uses the same pattern.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image
2014-06-05 11:17 ` Stefan Hajnoczi
@ 2014-06-05 11:26 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2014-06-05 11:26 UTC (permalink / raw)
To: Stefan Hajnoczi, Fam Zheng
Cc: Kevin Wolf, jcody, qemu-devel, Stefan Hajnoczi, Max Reitz
Il 05/06/2014 13:17, Stefan Hajnoczi ha scritto:
> On Thu, Jun 05, 2014 at 11:42:34AM +0800, Fam Zheng wrote:
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 94c8661..2bef5f3 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -324,9 +324,18 @@ static void coroutine_fn mirror_run(void *opaque)
>> }
>>
>> s->common.len = bdrv_getlength(bs);
>> - if (s->common.len <= 0) {
>> + if (s->common.len < 0) {
>> ret = s->common.len;
>> goto immediate_exit;
>> + } else if (s->common.len == 0) {
>> + /* Report BLOCK_JOB_READY and wait for complete. */
>> + block_job_ready(&s->common);
>> + s->synced = true;
>> + while (!block_job_is_cancelled(&s->common) && !s->should_complete) {
>> + block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, SLICE_TIME);
>
> Please take a look at block_job_resume() and how it's used when
> cancelling or completing block jobs. There is no need to sleep, instead
> we can yield until we get resumed.
>
You still need to set job->busy = false. So I guess we need a new
function block_job_yield.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-06-05 11:27 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-05 3:42 [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image Fam Zheng
2014-06-05 3:42 ` [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image Fam Zheng
2014-06-05 11:17 ` Stefan Hajnoczi
2014-06-05 11:26 ` Paolo Bonzini
2014-06-05 3:42 ` [Qemu-devel] [PATCH 2/3] qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit Fam Zheng
2014-06-05 11:22 ` Stefan Hajnoczi
2014-06-05 3:42 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: Test 0-length image for mirror Fam Zheng
2014-06-05 11:26 ` Stefan Hajnoczi
2014-06-05 3:57 ` [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.