[Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image

[Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image

[Fam Zheng]

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

[Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image

[Stefan Hajnoczi]

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.

Re: [Qemu-devel] [PATCH 1/3] mirror: Go through ready -> complete process for 0 len image

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 2/3] qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH 2/3] qemu-iotests: Test BLOCK_JOB_READY event for 0Kb image active commit

[Stefan Hajnoczi]

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

[Qemu-devel] [PATCH 3/3] qemu-iotests: Test 0-length image for mirror

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test 0-length image for mirror

[Stefan Hajnoczi]

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.

Re: [Qemu-devel] [PATCH 0/3] mirror: Fix behavior for zero byte image

[Eric Blake]

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