| Message ID | 20170815130502.8736-1-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/15/2017 08:05 AM, Stefan Hajnoczi wrote: > The 093 throttling test submits twice as many requests as the throttle > limit in order to ensure that we reach the limit. The remaining > requests are left in-flight at the end of each test iteration. > > Commit 452589b6b47e8dc6353df257fc803dfc1383bed8 ("vl.c/exit: pause cpus > before closing block devices") exposed a hang in 093. This happens > because requests are still in flight when QEMU terminates but > QEMU_CLOCK_VIRTUAL time is frozen. bdrv_drain_all() hangs forever since > throttled requests cannot complete. > > Step the clock at the end of each test iteration so in-flight requests > actually finish. This solves the hang and is cleaner than leaving tests > in-flight. > > Note that this could also be "fixed" by disabling throttling when drives > are closed in QEMU. That approach has two issues: > > 1. We must drain requests before disabling throttling, so the hang > cannot be easily avoided! > > 2. Any time QEMU disables throttling internally there is a chance that > malicious users can abuse the code path to bypass throttling limits. > > Therefore it makes more sense to fix the test case than to modify QEMU. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > tests/qemu-iotests/093 | 4 ++++ > 1 file changed, 4 insertions(+) > Reviewed-by: Eric Blake <eblake@redhat.com> I can take this through the NBD tree (since that's one environment that trips up on the test), if Peter doesn't apply it directly.
On Tue 15 Aug 2017 03:05:02 PM CEST, Stefan Hajnoczi wrote: > The 093 throttling test submits twice as many requests as the throttle > limit in order to ensure that we reach the limit. The remaining > requests are left in-flight at the end of each test iteration. > > Commit 452589b6b47e8dc6353df257fc803dfc1383bed8 ("vl.c/exit: pause cpus > before closing block devices") exposed a hang in 093. This happens > because requests are still in flight when QEMU terminates but > QEMU_CLOCK_VIRTUAL time is frozen. bdrv_drain_all() hangs forever since > throttled requests cannot complete. > > Step the clock at the end of each test iteration so in-flight requests > actually finish. This solves the hang and is cleaner than leaving tests > in-flight. > > Note that this could also be "fixed" by disabling throttling when drives > are closed in QEMU. That approach has two issues: > > 1. We must drain requests before disabling throttling, so the hang > cannot be easily avoided! > > 2. Any time QEMU disables throttling internally there is a chance that > malicious users can abuse the code path to bypass throttling limits. > > Therefore it makes more sense to fix the test case than to modify QEMU. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093 index 2ed393a548..ef3997206b 100755 --- a/tests/qemu-iotests/093 +++ b/tests/qemu-iotests/093 @@ -133,6 +133,10 @@ class ThrottleTestCase(iotests.QMPTestCase): self.assertTrue(check_limit(params['iops_rd'], rd_iops)) self.assertTrue(check_limit(params['iops_wr'], wr_iops)) + # Allow remaining requests to finish. We submitted twice as many to + # ensure the throttle limit is reached. + self.vm.qtest("clock_step %d" % ns) + # Connect N drives to a VM and test I/O in all of them def test_all(self): params = {"bps": 4096,
The 093 throttling test submits twice as many requests as the throttle limit in order to ensure that we reach the limit. The remaining requests are left in-flight at the end of each test iteration. Commit 452589b6b47e8dc6353df257fc803dfc1383bed8 ("vl.c/exit: pause cpus before closing block devices") exposed a hang in 093. This happens because requests are still in flight when QEMU terminates but QEMU_CLOCK_VIRTUAL time is frozen. bdrv_drain_all() hangs forever since throttled requests cannot complete. Step the clock at the end of each test iteration so in-flight requests actually finish. This solves the hang and is cleaner than leaving tests in-flight. Note that this could also be "fixed" by disabling throttling when drives are closed in QEMU. That approach has two issues: 1. We must drain requests before disabling throttling, so the hang cannot be easily avoided! 2. Any time QEMU disables throttling internally there is a chance that malicious users can abuse the code path to bypass throttling limits. Therefore it makes more sense to fix the test case than to modify QEMU. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- tests/qemu-iotests/093 | 4 ++++ 1 file changed, 4 insertions(+)