Re: [Qemu-block] [Qemu-devel] [PATCH] blkdebug: Add support for latency rules

[Eric Blake]

On 08/24/2018 12:06 AM, Marc Olson via Qemu-devel wrote:
> Allow rules to be created to inject latency into I/O operations.
>
> Signed-off-by: Marc Olson <address@hidden>
> ---
>   block/blkdebug.c        | 101 ++++++++++++++++++++++++++++++++++++++----------
>   docs/devel/blkdebug.txt |  30 ++++++++++----
>   2 files changed, 103 insertions(+), 28 deletions(-)

The commit message could usefully summarize some of the doc additions 
(so you can learn more about it without having to read the patch itself) 
- but the doc additions are appreciated.

Missing: you should enhance an existing (or add a new) iotests usage of 
blkdebug to specify a latency operation. One possible test would be 
forcing a read to take longer than a write - then issue an aio read, a 
write immediately after, and verify that the write completes first and 
then the read (whether the read sees the old or the new data just 
written would then depend on whether the delay is acted on before or 
after blkdebug forwards the read on to the real block layer).

> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index e216699..762e438 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -65,6 +65,7 @@ typedef struct BlkdebugSuspendedReq {
>   
>   enum {
>       ACTION_INJECT_ERROR,
> +    ACTION_DELAY,
>       ACTION_SET_STATE,
>       ACTION_SUSPEND,
>   };

Does any of this need to be accessible via QMP?  (If QMP can't already 
fine-tune what blkdebug can do, I'm not proposing that you have to make 
it do so - but if QMP can specify error injections, it should also be 
able to specify delays).  Even if it doesn't need to be in QMP, should 
we specify this enum and related types in our qapi/*.json files?


> @@ -123,6 +127,33 @@ static QemuOptsList inject_error_opts = {
>       },
>   };
>   
> +static QemuOptsList delay_opts = {

One reason for suggesting that this be done with QAPI types is that 
QemuOpts is already proving to be painfully hard to extend, where in 
general we are trying to move towards using QAPI types rather than yet 
more QemuOpts munging.

> @@ -182,16 +214,21 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
> **errp)
>           .state  = qemu_opt_get_number(opts, "state", 0),
>       };
>   
> +    rule->once  = qemu_opt_get_bool(opts, "once", 0);
> +    sector = qemu_opt_get_number(opts, "sector", -1);
> +    rule->offset = sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;

I really wish we could spend time moving blkdebug to be byte-based 
instead of sector-based.

> +++ b/docs/devel/blkdebug.txt
> @@ -24,7 +24,7 @@ This way, all error paths can be tested to make sure they are 
> correct.
>   Rules
>   -----
>   The blkdebug block driver takes a list of "rules" that tell the error 
> injection
> -engine when to fail an I/O request.
> +engine when to fail (inject-error) or add latency to (delay) an I/O request.
>   
>   Each I/O request is evaluated against the rules.  If a rule matches the request
>   then its "action" is executed.
> @@ -33,17 +33,25 @@ Rules can be placed in a configuration file; the 
> configuration file
>   follows the same .ini-like format used by QEMU's -readconfig option, and
>   each section of the file represents a rule.
>   
> -The following configuration file defines a single rule:
> +The following configuration file defines multiple rules:
>   
>     $ cat blkdebug.conf
>     [inject-error]
>     event = "read_aio"
>     errno = "28"
>   
> -This rule fails all aio read requests with ENOSPC (28).  Note that the errno
> -value depends on the host.  On Linux, see
> +  [delay]
> +  event = "read_aio"
> +  sector = "2048"
> +  latency = "500000"
> +
> +The error rule fails all aio read requests with ENOSPC (28).  Note that the
> +errno value depends on the host.  On Linux, see
>   /usr/include/asm-generic/errno-base.h for errno values.
>   
> +The delay rule adds 500 ms of latency to a read I/O request containing sector
> +2048.

So in this example, you get a failure no matter what, but that one 
sector takes even longer to fail?

Overall, I like the idea.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org