* [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
* 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 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
* [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
* 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
* [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 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 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