[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/3] blkdebug: Add latency injection rule typ
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v3 3/3] blkdebug: Add latency injection rule type |
Date: |
Fri, 11 Jan 2019 16:00:37 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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 <address@hidden>
> ---
> 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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v3 3/3] blkdebug: Add latency injection rule type,
Max Reitz <=