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

[Qemu-devel] [PATCH v3 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!

v3: Fix test case failure in 4/4. (Kevin)
    Add Eric's review-by's in 1~3.

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     | 11 ++++++++---
 tests/qemu-iotests/041.out |  4 ++--
 7 files changed, 53 insertions(+), 11 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Eric Blake <eblake@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

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Eric Blake <eblake@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

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Eric Blake <eblake@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 v3 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.

Hide two unapplicable test methods that would fail with 0 image length
because it's also used as cluster size.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/041     | 11 ++++++++---
 tests/qemu-iotests/041.out |  4 ++--
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index ec470b2..ef4f465 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,11 @@ class TestSingleDrive(ImageMirroringTestCase):
                              target=target_img)
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
+class TestSingleDriveZeroLength(TestSingleDrive):
+    image_len = 0
+    test_small_buffer2 = None
+    test_large_cluster = None
+
 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..cfa5c0d 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-...........................
+...................................
 ----------------------------------------------------------------------
-Ran 27 tests
+Ran 35 tests
 
 OK
-- 
2.0.0

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

[Kevin Wolf]

Am 24.06.2014 um 14:26 hat Fam Zheng geschrieben:
> 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, applied to the block branch.

Kevin