Re: [Qemu-devel] [PATCH v3 3/3] blkdebug: Add latency injection rule type

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7487 bytes --]

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 <marcolso@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]