iotests: Tweak 030 in order to trigger a race condition with parallel jobs
PATCH 1/1 iotests: Tweak 030 in order to trigger a race condition with parallel jobs
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.
--- tests/qemu-iotests/030 | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
tests/qemu-iotests/030 +6 -6
@@ -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
Signature · 1 line
2.11.0
Quoted 6 lines · Alberto Garcia
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/
Quoted 15 lines
away without any chance of being stopped by
block_job_sleep_ns(). This triggers the bug that was fixed by3d5d319e1221082974711af1d09d82f07and 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 <[email protected]>--- 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):
tests/qemu-iotests/030 class TestParallelOps(iotests.QMPTestCase): · line 185
Quoted 10 lines
tests/qemu-iotests/030 +4
-4- 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?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 6 lines · Eric Blake
tests/qemu-iotests/030 class TestParallelOps(iotests.QMPTestCase): · line 185
Quoted 3 lines
qemu_io('-f', iotests.imgfmt,tests/qemu-iotests/030 +1
-1- '-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
0xffpattern 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
John Snow
Reviewed-byQuoted 5 lines · Alberto Garcia
Quoted 4 lines · Eric Blake
Quoted 3 lines
qemu_io('-f', iotests.imgfmt,tests/qemu-iotests/030 +1
-1- '-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb * 1024), + '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb),
Quoted 8 lines
Quoted 2 lines
I guess changing from a variable to a fixed
0xffpattern 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.
Quoted 6 lines
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
Quoted 7 lines · John Snow
Quoted 7 lines
Quoted 6 lines
tests/qemu-iotests/030 class TestParallelOps(iotests.QMPTestCase): · line 185
Quoted 3 lines
qemu_io('-f', iotests.imgfmt,tests/qemu-iotests/030 +1
-1- '-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
0xffpattern doesn't make a difference?
Quoted 6 lines
Quoted 4 lines
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:
Quoted 9 lines
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 by3d5d319e1221082974711af1d09d82f07and 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
Quoted 6 lines · Kevin Wolf
Am 07.12.2017 um 18:08 hat Alberto Garcia geschrieben:
Quoted 5 lines
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.
Quoted 7 lines
Quoted 4 lines
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 by3d5d319e1221082974711af1d09d82f07and 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.
Quoted 1 line
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