On 12.11.18 08:06, Marc Olson wrote: > Add a new rule type for blkdebug that instead of returning an error, can > inject latency to an IO. > > Signed-off-by: Marc Olson <marco...@amazon.com> > --- > block/blkdebug.c | 79 > +++++++++++++++++++++++++++++++++++++++++++--- > docs/devel/blkdebug.txt | 35 ++++++++++++++------ > qapi/block-core.json | 31 ++++++++++++++++++ > tests/qemu-iotests/071 | 63 ++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/071.out | 31 ++++++++++++++++++ > 5 files changed, 226 insertions(+), 13 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 7739849..6b1f2d6 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c
[...] > @@ -194,6 +227,11 @@ static int add_rule(void *opaque, QemuOpts *opts, Error > **errp) > qemu_opt_get_bool(opts, "immediately", 0); > break; > > + case ACTION_INJECT_DELAY: > + rule->options.delay.latency = > + qemu_opt_get_number(opts, "latency", 100) * SCALE_US; Why the default of 100? I think it would be best if this option were mandatory. > + break; > + > case ACTION_SET_STATE: > rule->options.set_state.new_state = > qemu_opt_get_number(opts, "new_state", 0); [...] > @@ -484,20 +538,36 @@ static int rule_check(BlockDriverState *bs, uint64_t > offset, uint64_t bytes) > (bytes && rule->offset >= offset && > rule->offset < offset + bytes)) > { > - if (rule->action == ACTION_INJECT_ERROR) { > + if (!error_rule && rule->action == ACTION_INJECT_ERROR) { > error_rule = rule; > + } else if (!delay_rule && rule->action == ACTION_INJECT_DELAY) { > + delay_rule = rule; > + } > + How about handling "once" here? (by adding: else { continue; } if (rule->once) { remove_active_rule(s, rule); } and making the QSIMPLEQ_FOREACH a QSIMPLEQ_FOREACH_SAFE. (Or maybe in that case we want to inline remove_active_rule().)) It isn't like the state here is too bad, but if we can't handle "once" in a common code path, I'm torn whether it has a place in the BlkdebugRule root. (Doing that makes parsing a bit easier, but OTOH we just shouldn't parse it for set-state at all, so I'd keep it in the "unionized structs" if it isn't handled in a common code path.) > + if (error_rule && delay_rule) { > break; > } > } > } > > + if (delay_rule) { > + latency = delay_rule->options.delay.latency; > + > + if (delay_rule->once) { > + remove_active_rule(s, delay_rule); > + } > + > + if (latency != 0) { > + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, latency); > + } > + } > + > if (error_rule) { > immediately = error_rule->options.inject_error.immediately; > error = error_rule->options.inject_error.error; > > if (error_rule->once) { > - QSIMPLEQ_REMOVE(&s->active_rules, error_rule, BlkdebugRule, > active_next); > - remove_rule(error_rule); > + remove_active_rule(s, error_rule); > } > > if (error && !immediately) { [...] > diff --git a/qapi/block-core.json b/qapi/block-core.json > index d4fe710..72f7861 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3057,6 +3057,34 @@ > '*immediately': 'bool' } } > > ## > +# @BlkdebugDelayOptions: > +# > +# Describes a single latency injection for blkdebug. > +# > +# @event: trigger event > +# > +# @state: the state identifier blkdebug needs to be in to > +# actually trigger the event; defaults to "any" > +# > +# @latency: The delay to add to an I/O, in microseconds. > +# > +# @sector: specifies the sector index which has to be affected > +# in order to actually trigger the event; defaults to "any > +# sector" > +# > +# @once: disables further events after this one has been > +# triggered; defaults to false > +# > +# Since: 3.1 Well, 4.0 now, sorry... > +## > +{ 'struct': 'BlkdebugDelayOptions', > + 'data': { 'event': 'BlkdebugEvent', > + '*state': 'int', > + '*latency': 'int', I'd make this option mandatory. > + '*sector': 'int', > + '*once': 'bool' } } > + > +## > # @BlkdebugSetStateOptions: > # > # Describes a single state-change event for blkdebug. > @@ -3115,6 +3143,8 @@ > # > # @inject-error: array of error injection descriptions > # > +# @inject-delay: array of delay injection descriptions This needs a "(Since 4.0)". > +# > # @set-state: array of state-change descriptions > # > # Since: 2.9 [...] > diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071 > index 48b4955..976f747 100755 > --- a/tests/qemu-iotests/071 > +++ b/tests/qemu-iotests/071 > @@ -100,6 +100,69 @@ $QEMU_IO -c "open -o > driver=$IMGFMT,file.driver=blkdebug,file.inject-error.event > -c 'read -P 42 0x38000 512' > > echo > +echo "=== Testing blkdebug latency through filename ===" > +echo > + > +$QEMU_IO -c "open -o > file.driver=blkdebug,file.inject-delay.event=write_aio,file.inject-delay.latency=10000 > $TEST_IMG" \ > + -c 'aio_write -P 42 0x28000 512' \ > + -c 'aio_read -P 42 0x38000 512' \ > + | _filter_qemu_io > + > +echo > +echo "=== Testing blkdebug latency through file blockref ===" > +echo > + > +$QEMU_IO -c "open -o > driver=$IMGFMT,file.driver=blkdebug,file.inject-delay.event=write_aio,file.inject-delay.latency=10000,file.image.filename=$TEST_IMG" > \ > + -c 'aio_write -P 42 0x28000 512' \ > + -c 'aio_read -P 42 0x38000 512' \ > + | _filter_qemu_io > + > +# Using QMP is synchronous by default, so even though we would > +# expect reordering due to using the aio_* commands, they are > +# not. The purpose of this test is to verify that the driver > +# can be setup via QMP, and IO can complete. See the qemu-io > +# test above to prove delay functionality But it doesn't prove that because the output is filtered. To prove it, you'd probably need to use null-co as the protocol (so there is as little noise as possible) and then parse the qemu-io output to show that the time is always above 10 ms. I leave it to you whether you'd like to go through that pain. [...] > diff --git a/tests/qemu-iotests/071.out b/tests/qemu-iotests/071.out > index 1d5e28d..1952990 100644 > --- a/tests/qemu-iotests/071.out > +++ b/tests/qemu-iotests/071.out > @@ -36,6 +36,37 @@ read failed: Input/output error > > read failed: Input/output error > > +=== Testing blkdebug latency through filename === > + > +read 512/512 bytes at offset 229376 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +wrote 512/512 bytes at offset 163840 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > + > +=== Testing blkdebug latency through file blockref === > + > +read 512/512 bytes at offset 229376 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +wrote 512/512 bytes at offset 163840 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) (As you can see, the output is filtered.) Max
signature.asc
Description: OpenPGP digital signature