This patch tweaks TestParallelOps in iotest 030 so it allocates data
in smaller regions (256KB/512KB instead of 512KB/1MB) and the
block-stream job in test_stream_commit() only needs to copy data that
is at the very end of the image.

This way when the block-stream job is waken up it will finish right
away without any chance of being stopped by block_job_sleep_ns(). This
triggers the bug that was fixed by 3d5d319e1221082974711af1d09d82f07
and is therefore a more useful test case for parallel block jobs.

After this patch the aforementiond bug can also be reproduced with the
test_stream_parallel() test case.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/030 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 457984b8e9..230eb761dd 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -156,7 +156,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 class TestParallelOps(iotests.QMPTestCase):
     num_ops = 4 # Number of parallel block-stream operations
     num_imgs = num_ops * 2 + 1
-    image_len = num_ops * 1024 * 1024
+    image_len = num_ops * 512 * 1024
     imgs = []
 
     def setUp(self):
@@ -177,12 +177,12 @@ class TestParallelOps(iotests.QMPTestCase):
 
         # Put data into the images we are copying data from
         for i in range(self.num_imgs / 2):
-            img_index = i * 2 + 1
-            # Alternate between 512k and 1M.
+            img_index = self.num_imgs - i * 2 - 2
+            # Alternate between 256KB and 512KB.
             # This way jobs will not finish in the same order they were created
-            num_kb = 512 + 512 * (i % 2)
+            num_kb = 256 + 256 * (i % 2)
             qemu_io('-f', iotests.imgfmt,
-                    '-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb * 1024),
+                    '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb),
                     self.imgs[img_index])
 
         # Attach the drive to the VM
@@ -323,7 +323,7 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         # Stream from node0 into node2
-        result = self.vm.qmp('block-stream', device='node2', job_id='node2')
+        result = self.vm.qmp('block-stream', device='node2', base_node='node0', job_id='node2')
         self.assert_qmp(result, 'return', {})
 
         # Commit from the active layer into node3
-- 
2.11.0

On 12/07/2017 11:08 AM, Alberto Garcia wrote:
> This patch tweaks TestParallelOps in iotest 030 so it allocates data
> in smaller regions (256KB/512KB instead of 512KB/1MB) and the
> block-stream job in test_stream_commit() only needs to copy data that
> is at the very end of the image.
> 
> This way when the block-stream job is waken up it will finish right

s/waken up/awakened/

> away without any chance of being stopped by block_job_sleep_ns(). This
> triggers the bug that was fixed by 3d5d319e1221082974711af1d09d82f07
> and is therefore a more useful test case for parallel block jobs.
> 
> After this patch the aforementiond bug can also be reproduced with the
> test_stream_parallel() test case.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  tests/qemu-iotests/030 | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 

>          # Put data into the images we are copying data from
>          for i in range(self.num_imgs / 2):
> -            img_index = i * 2 + 1
> -            # Alternate between 512k and 1M.
> +            img_index = self.num_imgs - i * 2 - 2
> +            # Alternate between 256KB and 512KB.
>              # This way jobs will not finish in the same order they were created
> -            num_kb = 512 + 512 * (i % 2)
> +            num_kb = 256 + 256 * (i % 2)
>              qemu_io('-f', iotests.imgfmt,
> -                    '-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb * 1024),
> +                    '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb),

I guess changing from a variable to a fixed 0xff pattern doesn't make a
difference?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Thu 07 Dec 2017 08:16:41 PM CET, Eric Blake wrote:
>>              qemu_io('-f', iotests.imgfmt,
>> -                    '-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb * 1024),
>> +                    '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb),
>
> I guess changing from a variable to a fixed 0xff pattern doesn't make
> a difference?

I noticed that with the previous code we would write zeroes to the first
image (i == 0), and with that I can't reproduce the bug. I assume that
block-stream doesn't copy the data in that case. Changing it to anything
!= 0 solves the problem.

And answering your question, it doesn't really matter if we write the
same value in all places, we only check the output of 'qemu-io -c map'.
Plus the areas don't even overlap.

Berto

On 12/07/2017 02:34 PM, Alberto Garcia wrote:
> On Thu 07 Dec 2017 08:16:41 PM CET, Eric Blake wrote:
>>>              qemu_io('-f', iotests.imgfmt,
>>> -                    '-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb * 1024),
>>> +                    '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb),
>>
>> I guess changing from a variable to a fixed 0xff pattern doesn't make
>> a difference?
> 
> I noticed that with the previous code we would write zeroes to the first
> image (i == 0), and with that I can't reproduce the bug. I assume that
> block-stream doesn't copy the data in that case. Changing it to anything
> != 0 solves the problem.
> 

I think I ran into a similar problem with an AHCI test once.

Reviewed-by: John Snow <jsnow@redhat.com>

> And answering your question, it doesn't really matter if we write the
> same value in all places, we only check the output of 'qemu-io -c map'.
> Plus the areas don't even overlap.
> 
> Berto
> 

On Fri 08 Dec 2017 08:13:48 PM CET, John Snow <jsnow@redhat.com> wrote:

>>>>              qemu_io('-f', iotests.imgfmt,
>>>> -                    '-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb * 1024),
>>>> +                    '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb),
>>>
>>> I guess changing from a variable to a fixed 0xff pattern doesn't
>>> make a difference?
>> 
>> I noticed that with the previous code we would write zeroes to the
>> first image (i == 0), and with that I can't reproduce the bug. I
>> assume that block-stream doesn't copy the data in that case. Changing
>> it to anything != 0 solves the problem.
>
> I think I ran into a similar problem with an AHCI test once.

It's this bit from bdrv_co_do_copy_on_readv() (which is the way
block-stream is implemented):

    if (drv->bdrv_co_pwrite_zeroes &&
        buffer_is_zero(bounce_buffer, pnum)) {
        /* FIXME: Should we (perhaps conditionally) be setting
         * BDRV_REQ_MAY_UNMAP, if it will allow for a sparser copy
         * that still correctly reads as zero? */
        ret = bdrv_co_do_pwrite_zeroes(bs, cluster_offset, pnum, 0);
    } else {
        /* This does not change the data on the disk, it is not
         * necessary to flush even in cache=writethrough mode.
         */
        ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
                                  &local_qiov, 0);
    }

Berto

Am 07.12.2017 um 18:08 hat Alberto Garcia geschrieben:
> This patch tweaks TestParallelOps in iotest 030 so it allocates data
> in smaller regions (256KB/512KB instead of 512KB/1MB) and the
> block-stream job in test_stream_commit() only needs to copy data that
> is at the very end of the image.
> 
> This way when the block-stream job is waken up it will finish right
> away without any chance of being stopped by block_job_sleep_ns(). This
> triggers the bug that was fixed by 3d5d319e1221082974711af1d09d82f07
> and is therefore a more useful test case for parallel block jobs.

But if all jobs complete right away, doesn't this kill the parallelism
that the test case intended to test?

Maybe we need both cases?

Kevin

On Wed 13 Dec 2017 12:31:20 PM CET, Kevin Wolf wrote:
> Am 07.12.2017 um 18:08 hat Alberto Garcia geschrieben:
>> This patch tweaks TestParallelOps in iotest 030 so it allocates data
>> in smaller regions (256KB/512KB instead of 512KB/1MB) and the
>> block-stream job in test_stream_commit() only needs to copy data that
>> is at the very end of the image.
>> 
>> This way when the block-stream job is waken up it will finish right
>> away without any chance of being stopped by block_job_sleep_ns(). This
>> triggers the bug that was fixed by 3d5d319e1221082974711af1d09d82f07
>> and is therefore a more useful test case for parallel block jobs.
>
> But if all jobs complete right away, doesn't this kill the parallelism
> that the test case intended to test?

Good question. There's two cases where there's parallel block jobs:

- test_stream_parallel(): in this case we run four block-stream jobs so
  although one finishes early the others will run the stream_run() loop
  in parallel. So no need to worry here.

- test_stream_commit(): here there's only two jobs. block-stream starts
  first, then block-commit starts and calls bdrv_reopen() and it's
  during that reopen that the first job finishes. So while the main
  purpose of this test case was to ensure that op blockers were working
  fine and that you can launch both jobs at the same time, it's true
  that with this patch you won't have both main iterations (stream_run()
  and mirror_run()) running at the same time.

> Maybe we need both cases?

I guess it's a good idea, it should be as simple as creating a subclass
of TestParallelOps with different sizes, all test cases will be
inherited from the parent.

I'll send a new patch.

Berto