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

[Qemu-devel] [PATCH v2 0/4] 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!


v2: Address Stefan's comments.

    - Added patch 01: block_job_yield.
    - Use block_job_yield in 02.
    - Fix test case updates.

Thanks to Stefan, Paolo and Eric for reviewing!

Fam


Fam Zheng (4):
  blockjob: Add block_job_yield()
  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 ++++++++++-
 blockjob.c                 | 14 ++++++++++++++
 include/block/blockjob.h   |  8 ++++++++
 tests/qemu-iotests/040     | 12 +++++++++---
 tests/qemu-iotests/040.out |  4 ++--
 tests/qemu-iotests/041     |  9 ++++++---
 tests/qemu-iotests/041.out |  4 ++--
 7 files changed, 51 insertions(+), 11 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH v2 1/4] blockjob: Add block_job_yield()

[Fam Zheng]

This will unset busy flag and put coroutine to sleep, can be used to
wait for QMP complete/cancel.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockjob.c               | 14 ++++++++++++++
 include/block/blockjob.h |  8 ++++++++
 2 files changed, 22 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 7d84ca1..a85146a 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -210,6 +210,20 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
     job->busy = true;
 }
 
+void block_job_yield(BlockJob *job)
+{
+    assert(job->busy);
+
+    /* Check cancellation *before* setting busy = false, too!  */
+    if (block_job_is_cancelled(job)) {
+        return;
+    }
+
+    job->busy = false;
+    qemu_coroutine_yield();
+    job->busy = true;
+}
+
 BlockJobInfo *block_job_query(BlockJob *job)
 {
     BlockJobInfo *info = g_new0(BlockJobInfo, 1);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index c0a7875..ac313ea 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -147,6 +147,14 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
 void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
 
 /**
+ * block_job_yield:
+ * @job: The job that calls the function.
+ *
+ * Yield the block job coroutine.
+ */
+void block_job_yield(BlockJob *job);
+
+/**
  * block_job_completed:
  * @job: The job being completed.
  * @ret: The status code.
-- 
2.0.0

Re: [Qemu-devel] [PATCH v2 1/4] blockjob: Add block_job_yield()

[Eric Blake]

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

On 06/08/2014 08:56 PM, Fam Zheng wrote:
> This will unset busy flag and put coroutine to sleep, can be used to
> wait for QMP complete/cancel.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockjob.c               | 14 ++++++++++++++
>  include/block/blockjob.h |  8 ++++++++
>  2 files changed, 22 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH v2 2/4] 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..705260a 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_yield(&s->common);
+        }
+        s->common.cancelled = false;
+        goto immediate_exit;
     }
 
     length = DIV_ROUND_UP(s->common.len, s->granularity);
-- 
2.0.0

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

[Eric Blake]

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

On 06/08/2014 08:56 PM, Fam Zheng wrote:
> 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH v2 3/4] 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     | 12 +++++++++---
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 734b6a6..d166810 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:
+                        self.assertTrue(ready, "Expecting 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)
@@ -63,7 +67,7 @@ class TestSingleDrive(ImageCommitTestCase):
     test_len = 1 * 1024 * 256
 
     def setUp(self):
-        iotests.create_image(backing_img, TestSingleDrive.image_len)
+        iotests.create_image(backing_img, self.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
         qemu_io('-c', 'write -P 0xab 0 524288', backing_img)
@@ -105,7 +109,7 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assert_qmp(result, 'error/desc', 'Base \'badfile\' not found')
 
     def test_top_is_active(self):
-        self.run_commit_test(test_img, backing_img)
+        self.run_commit_test(test_img, backing_img, need_ready=True)
         self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
         self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
 
@@ -238,6 +242,8 @@ class TestSetSpeed(ImageCommitTestCase):
 
         self.cancel_and_wait(resume=True)
 
+class TestActiveZeroLengthImage(TestSingleDrive):
+    image_len = 0
 
 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

[Qemu-devel] [PATCH v2 4/4] 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     | 9 ++++++---
 tests/qemu-iotests/041.out | 4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index ec470b2..b1db27c 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -64,7 +64,7 @@ class TestSingleDrive(ImageMirroringTestCase):
     image_len = 1 * 1024 * 1024 # MB
 
     def setUp(self):
-        iotests.create_image(backing_img, TestSingleDrive.image_len)
+        iotests.create_image(backing_img, self.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()
@@ -163,7 +163,7 @@ class TestSingleDrive(ImageMirroringTestCase):
         self.assert_no_active_block_jobs()
 
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,size=%d'
-                        % (TestSingleDrive.image_len, TestSingleDrive.image_len), target_img)
+                        % (self.image_len, self.image_len), target_img)
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              buf_size=65536, mode='existing', target=target_img)
         self.assert_qmp(result, 'return', {})
@@ -179,7 +179,7 @@ class TestSingleDrive(ImageMirroringTestCase):
         self.assert_no_active_block_jobs()
 
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,backing_file=%s'
-                        % (TestSingleDrive.image_len, backing_img), target_img)
+                        % (self.image_len, backing_img), target_img)
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              mode='existing', target=target_img)
         self.assert_qmp(result, 'return', {})
@@ -206,6 +206,9 @@ class TestSingleDrive(ImageMirroringTestCase):
                              target=target_img)
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
+class TestSingleDriveZeroLength(TestSingleDrive):
+    image_len = 0
+
 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 v2 4/4] qemu-iotests: Test 0-length image for mirror

[Eric Blake]

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

On 06/08/2014 08:56 PM, Fam Zheng wrote:
> 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     | 9 ++++++---
>  tests/qemu-iotests/041.out | 4 ++--
>  2 files changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 0/4] mirror: Fix behavior for zero byte image

[Fam Zheng]

On Mon, 06/09 10:56, 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!

Ping. Shall we merge this before it rots?

Thanks,
Fam

> 
> 
> v2: Address Stefan's comments.
> 
>     - Added patch 01: block_job_yield.
>     - Use block_job_yield in 02.
>     - Fix test case updates.
> 
> Thanks to Stefan, Paolo and Eric for reviewing!
> 
> Fam
> 
> 
> Fam Zheng (4):
>   blockjob: Add block_job_yield()
>   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 ++++++++++-
>  blockjob.c                 | 14 ++++++++++++++
>  include/block/blockjob.h   |  8 ++++++++
>  tests/qemu-iotests/040     | 12 +++++++++---
>  tests/qemu-iotests/040.out |  4 ++--
>  tests/qemu-iotests/041     |  9 ++++++---
>  tests/qemu-iotests/041.out |  4 ++--
>  7 files changed, 51 insertions(+), 11 deletions(-)
> 
> -- 
> 2.0.0
> 

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

[Kevin Wolf]

Am 24.06.2014 um 06:25 hat Fam Zheng geschrieben:
> On Mon, 06/09 10:56, 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!
> 
> Ping. Shall we merge this before it rots?

I had it already applied, but now test case 041 fails for me, so I'm
going to remove it from my tree again:

041 7s ...        [13:28:48] [13:29:27] [failed, exit status 1] - output mismatch (see 041.out.bad)
--- 041.out     2014-06-24 13:23:13.597949977 +0200
+++ 041.out.bad 2014-06-24 13:29:27.024871226 +0200
@@ -1,5 +1,31 @@
-.....................................
+qemu-img: /home/kwolf/source/qemu/tests/qemu-iotests/scratch/target.img: Cluster size must be a power of two between 512 and 2048k
+qemu-img: /home/kwolf/source/qemu/tests/qemu-iotests/scratch/target.img: Cluster size must be a power of two between 512 and 2048k
+..........................F...F......
+======================================================================
+FAIL: test_large_cluster (__main__.TestSingleDriveZeroLength)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 185, in test_large_cluster
+    self.assert_qmp(result, 'return', {})
+  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', u'desc': u"Could not open '/home/kwolf/source/qemu/tests/qemu-iotests/scratch/target.img': No such file or directory"}}"
+
+======================================================================
+FAIL: test_small_buffer2 (__main__.TestSingleDriveZeroLength)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 169, in test_small_buffer2
+    self.assert_qmp(result, 'return', {})
+  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath                                                                                
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))                                                                                             
+AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', u'desc': u"Could not open '/home/kwolf/source/qemu/tests/qemu-iotests/scratch/target.img': No such file or directory"}}"                                                                                                                     
+                                                                                                                                                                     
 ----------------------------------------------------------------------                                                                                               
 Ran 37 tests                                                                                                                                                         
 
-OK
+FAILED (failures=2)
Failures: 041

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

[Fam Zheng]

> Am 24.06.2014 um 06:25 hat Fam Zheng geschrieben:
> > On Mon, 06/09 10:56, 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!
> > 
> > Ping. Shall we merge this before it rots?
> 
> I had it already applied, but now test case 041 fails for me, so I'm
> going to remove it from my tree again:
> 

I'm pretty sure it passed when I sent it, so it's probably due do some recent code changes. Rebased and sent v3. Please try again!

Thanks for the effort!

Fam

> 041 7s ...        [13:28:48] [13:29:27] [failed, exit status 1] - output
> mismatch (see 041.out.bad)
> --- 041.out     2014-06-24 13:23:13.597949977 +0200
> +++ 041.out.bad 2014-06-24 13:29:27.024871226 +0200
> @@ -1,5 +1,31 @@
> -.....................................
> +qemu-img: /home/kwolf/source/qemu/tests/qemu-iotests/scratch/target.img:
> Cluster size must be a power of two between 512 and 2048k
> +qemu-img: /home/kwolf/source/qemu/tests/qemu-iotests/scratch/target.img:
> Cluster size must be a power of two between 512 and 2048k
> +..........................F...F......
> +======================================================================
> +FAIL: test_large_cluster (__main__.TestSingleDriveZeroLength)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "./041", line 185, in test_large_cluster
> +    self.assert_qmp(result, 'return', {})
> +  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 232, in
> assert_qmp
> +    result = self.dictpath(d, path)
> +  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 211, in
> dictpath
> +    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
> +AssertionError: failed path traversal for "return" in "{u'error': {u'class':
> u'GenericError', u'desc': u"Could not open
> '/home/kwolf/source/qemu/tests/qemu-iotests/scratch/target.img': No such
> file or directory"}}"
> +
> +======================================================================
> +FAIL: test_small_buffer2 (__main__.TestSingleDriveZeroLength)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "./041", line 169, in test_small_buffer2
> +    self.assert_qmp(result, 'return', {})
> +  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 232, in
> assert_qmp
> +    result = self.dictpath(d, path)
> +  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 211, in
> dictpath
> +    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
> +AssertionError: failed path traversal for "return" in "{u'error': {u'class':
> u'GenericError', u'desc': u"Could not open
> '/home/kwolf/source/qemu/tests/qemu-iotests/scratch/target.img': No such
> file or directory"}}"
> +
>  ----------------------------------------------------------------------
>  Ran 37 tests
>  
> -OK
> +FAILED (failures=2)
> Failures: 041
> 
>