* [Qemu-devel] [PATCH] blkdebug: Add support for latency rules
@ 2018-08-24 5:06 Marc Olson
2018-08-24 16:11 ` Eric Blake
2018-09-05 0:24 ` [Qemu-devel] [PATCH v2] " Marc Olson
0 siblings, 2 replies; 7+ messages in thread
From: Marc Olson @ 2018-08-24 5:06 UTC (permalink / raw)
Cc: Marc Olson, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
Allow rules to be created to inject latency into I/O operations.
Signed-off-by: Marc Olson <marcolso@amazon.com>
---
block/blkdebug.c | 101 ++++++++++++++++++++++++++++++++++++++----------
docs/devel/blkdebug.txt | 30 ++++++++++----
2 files changed, 103 insertions(+), 28 deletions(-)
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,
};
@@ -73,14 +74,17 @@ typedef struct BlkdebugRule {
BlkdebugEvent event;
int action;
int state;
+ int once;
+ int64_t offset;
union {
struct {
int error;
int immediately;
- int once;
- int64_t offset;
} inject;
struct {
+ int64_t latency;
+ } delay;
+ struct {
int new_state;
} set_state;
struct {
@@ -123,6 +127,33 @@ static QemuOptsList inject_error_opts = {
},
};
+static QemuOptsList delay_opts = {
+ .name = "delay",
+ .head = QTAILQ_HEAD_INITIALIZER(delay_opts.head),
+ .desc = {
+ {
+ .name = "event",
+ },
+ {
+ .name = "state",
+ .type = QEMU_OPT_NUMBER,
+ },
+ {
+ .name = "latency",
+ .type = QEMU_OPT_NUMBER,
+ },
+ {
+ .name = "sector",
+ .type = QEMU_OPT_NUMBER,
+ },
+ {
+ .name = "once",
+ .type = QEMU_OPT_BOOL,
+ },
+ { /* end of list */ }
+ },
+};
+
static QemuOptsList set_state_opts = {
.name = "set-state",
.head = QTAILQ_HEAD_INITIALIZER(set_state_opts.head),
@@ -145,6 +176,7 @@ static QemuOptsList set_state_opts = {
static QemuOptsList *config_groups[] = {
&inject_error_opts,
+ &delay_opts,
&set_state_opts,
NULL
};
@@ -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;
+
/* Parse action-specific options */
switch (d->action) {
case ACTION_INJECT_ERROR:
rule->options.inject.error = qemu_opt_get_number(opts, "errno", EIO);
- rule->options.inject.once = qemu_opt_get_bool(opts, "once", 0);
rule->options.inject.immediately =
qemu_opt_get_bool(opts, "immediately", 0);
- sector = qemu_opt_get_number(opts, "sector", -1);
- rule->options.inject.offset =
- sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
+ break;
+
+ case ACTION_DELAY:
+ rule->options.delay.latency =
+ qemu_opt_get_number(opts, "latency", 100) * SCALE_US;
break;
case ACTION_SET_STATE:
@@ -264,6 +301,14 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
goto fail;
}
+ d.action = ACTION_DELAY;
+ qemu_opts_foreach(&delay_opts, add_rule, &d, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto fail;
+ }
+
d.action = ACTION_SET_STATE;
qemu_opts_foreach(&set_state_opts, add_rule, &d, &local_err);
if (local_err) {
@@ -275,6 +320,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
ret = 0;
fail:
qemu_opts_reset(&inject_error_opts);
+ qemu_opts_reset(&delay_opts);
qemu_opts_reset(&set_state_opts);
if (f) {
fclose(f);
@@ -475,36 +521,50 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes)
BlkdebugRule *rule = NULL;
int error;
bool immediately;
+ int ret = 0;
QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
- uint64_t inject_offset = rule->options.inject.offset;
-
- if (inject_offset == -1 ||
- (bytes && inject_offset >= offset &&
- inject_offset < offset + bytes))
+ if (rule->offset == -1 ||
+ (bytes && rule->offset >= offset &&
+ rule->offset < offset + bytes))
{
break;
}
}
- if (!rule || !rule->options.inject.error) {
+ if (!rule ||
+ (rule->action == ACTION_INJECT_ERROR && !rule->options.inject.error) ||
+ (rule->action == ACTION_DELAY && !rule->options.delay.latency)) {
return 0;
}
- immediately = rule->options.inject.immediately;
- error = rule->options.inject.error;
-
- if (rule->options.inject.once) {
+ if (rule->once) {
QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
remove_rule(rule);
}
- if (!immediately) {
- aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
- qemu_coroutine_yield();
+ switch (rule->action) {
+ case ACTION_INJECT_ERROR:
+ immediately = rule->options.inject.immediately;
+ error = rule->options.inject.error;
+
+ if (!immediately) {
+ aio_co_schedule(qemu_get_current_aio_context(),
+ qemu_coroutine_self());
+ qemu_coroutine_yield();
+ }
+
+ ret = -error;
+ break;
+
+ case ACTION_DELAY:
+ if (rule->options.delay.latency) {
+ qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, rule->options.delay.latency);
+ }
+ break;
}
- return -error;
+ return ret;
}
static int coroutine_fn
@@ -691,6 +751,7 @@ static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
/* Take the action */
switch (rule->action) {
case ACTION_INJECT_ERROR:
+ case ACTION_DELAY:
if (!injected) {
QSIMPLEQ_INIT(&s->active_rules);
injected = true;
diff --git a/docs/devel/blkdebug.txt b/docs/devel/blkdebug.txt
index 43d8e8f..1befcf8 100644
--- a/docs/devel/blkdebug.txt
+++ 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.
+
Invoke QEMU as follows:
$ qemu-system-x86_64
@@ -60,21 +68,27 @@ Rules support the following attributes:
rule to match. See the "State transitions" section for information
on states.
- errno - the numeric errno value to return when a request matches this rule.
- The errno values depend on the host since the numeric values are not
- standarized in the POSIX specification.
-
sector - (optional) a sector number that the request must overlap in order to
match this rule
once - (optional, default "off") only execute this action on the first
matching request
+Error injection rules support the following attributes:
+
+ errno - the numeric errno value to return when a request matches this rule.
+ The errno values depend on the host since the numeric values are not
+ standarized in the POSIX specification.
+
immediately - (optional, default "off") return a NULL BlockAIOCB
pointer and fail without an errno instead. This
exercises the code path where BlockAIOCB fails and the
caller's BlockCompletionFunc is not invoked.
+Delay rules support the following attribute:
+
+ latency - the delay to add to an I/O request, in microseconds.
+
Events
------
Block drivers provide information about the type of I/O request they are about
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] blkdebug: Add support for latency rules
2018-08-24 5:06 [Qemu-devel] [PATCH] blkdebug: Add support for latency rules Marc Olson
@ 2018-08-24 16:11 ` Eric Blake
2018-08-28 1:34 ` Marc Olson
2018-09-05 0:24 ` [Qemu-devel] [PATCH v2] " Marc Olson
1 sibling, 1 reply; 7+ messages in thread
From: Eric Blake @ 2018-08-24 16:11 UTC (permalink / raw)
To: Marc Olson; +Cc: Kevin Wolf, qemu-devel, qemu-block, Max Reitz
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 <marcolso@amazon.com>
> ---
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] blkdebug: Add support for latency rules
2018-08-24 16:11 ` Eric Blake
@ 2018-08-28 1:34 ` Marc Olson
0 siblings, 0 replies; 7+ messages in thread
From: Marc Olson @ 2018-08-28 1:34 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, qemu-devel, qemu-block, Max Reitz
On 08/24/2018 09:11 AM, Eric Blake wrote:
> 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 <marcolso@amazon.com>
>> ---
>> 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.
I can be more verbose in the commit.
>
> 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).
>
I'll add at least a simple test to the repo.
>>
>> 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?
It does appear that blkdebug is in the qapi/*.json files. I've added the
appropriate types, but I'm less familiar with the QMP incantations, and
seem to have misconfigured my tests as they result in an uninitialized
mutex when trying to do aio*. Using the existing tests as a guide,
there's no global BlockBackend created, and so hmp_qemu_io() creates a
local backend that is cleaned up before the aio completes. Removing the
blk_unref() clears it up, but of course leaks memory. I don't see an API
that creates the appropriate QMP BlockBackend for file backed devices.
>
>> @@ -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.
I'm not certain I see the benefit of moving to byte based. Storage
devices are sector based. The biggest pain point with blkdebug is not
that it's sector based, but that offsets are based on the backing file,
and not as viewed by the guest. In order to properly use blkdebug with
qcow2, for example, you have to do some gymnastics.
>
>> +++ 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?
No, in the case of injection that overlaps, it takes the first rule.
This is similar to the existing behavior--if two errors match, you don't
know which you're getting. I like the suggestion, and I've reworked it a
bit to delay first and then take any errors, but still take the first of
each. I could see a follow on that looks for the most specific rule in
either case (or the greatest delay in the latency case).
>
> Overall, I like the idea.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2] blkdebug: Add support for latency rules
2018-08-24 5:06 [Qemu-devel] [PATCH] blkdebug: Add support for latency rules Marc Olson
2018-08-24 16:11 ` Eric Blake
@ 2018-09-05 0:24 ` Marc Olson
2018-09-13 16:48 ` Marc Olson
1 sibling, 1 reply; 7+ messages in thread
From: Marc Olson @ 2018-09-05 0:24 UTC (permalink / raw)
To: qemu-devel
Cc: Marc Olson, Kevin Wolf, Max Reitz, Markus Armbruster, Eric Blake,
qemu-block
Sometimes storage devices can be slow to respond, due to media errors, firmware
issues, SSD garbage collection, etc. This patch adds a new rule type to
blkdebug that allows injection of latency to I/O operations. Similar to error
injection rules, latency rules can be specified with or without an offset, and
can also apply upon state transitions.
Signed-off-by: Marc Olson <marcolso@amazon.com>
---
v2:
- Change so that delay rules are executed before error rules
- Add QMP support
- Add tests
---
block/blkdebug.c | 119 +++++++++++++++++++++++++++++++++++----------
docs/devel/blkdebug.txt | 30 +++++++++---
qapi/block-core.json | 39 +++++++++++++--
tests/qemu-iotests/071 | 63 ++++++++++++++++++++++++
tests/qemu-iotests/071.out | 31 ++++++++++++
5 files changed, 244 insertions(+), 38 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 0759452..1785fe3 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,
};
@@ -73,14 +74,17 @@ typedef struct BlkdebugRule {
BlkdebugEvent event;
int action;
int state;
+ int once;
+ int64_t offset;
union {
struct {
int error;
int immediately;
- int once;
- int64_t offset;
} inject;
struct {
+ int64_t latency;
+ } delay;
+ struct {
int new_state;
} set_state;
struct {
@@ -123,6 +127,33 @@ static QemuOptsList inject_error_opts = {
},
};
+static QemuOptsList delay_opts = {
+ .name = "delay",
+ .head = QTAILQ_HEAD_INITIALIZER(delay_opts.head),
+ .desc = {
+ {
+ .name = "event",
+ },
+ {
+ .name = "state",
+ .type = QEMU_OPT_NUMBER,
+ },
+ {
+ .name = "latency",
+ .type = QEMU_OPT_NUMBER,
+ },
+ {
+ .name = "sector",
+ .type = QEMU_OPT_NUMBER,
+ },
+ {
+ .name = "once",
+ .type = QEMU_OPT_BOOL,
+ },
+ { /* end of list */ }
+ },
+};
+
static QemuOptsList set_state_opts = {
.name = "set-state",
.head = QTAILQ_HEAD_INITIALIZER(set_state_opts.head),
@@ -145,6 +176,7 @@ static QemuOptsList set_state_opts = {
static QemuOptsList *config_groups[] = {
&inject_error_opts,
+ &delay_opts,
&set_state_opts,
NULL
};
@@ -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;
+
/* Parse action-specific options */
switch (d->action) {
case ACTION_INJECT_ERROR:
rule->options.inject.error = qemu_opt_get_number(opts, "errno", EIO);
- rule->options.inject.once = qemu_opt_get_bool(opts, "once", 0);
rule->options.inject.immediately =
qemu_opt_get_bool(opts, "immediately", 0);
- sector = qemu_opt_get_number(opts, "sector", -1);
- rule->options.inject.offset =
- sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
+ break;
+
+ case ACTION_DELAY:
+ rule->options.delay.latency =
+ qemu_opt_get_number(opts, "latency", 100) * SCALE_US;
break;
case ACTION_SET_STATE:
@@ -264,6 +301,14 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
goto fail;
}
+ d.action = ACTION_DELAY;
+ qemu_opts_foreach(&delay_opts, add_rule, &d, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto fail;
+ }
+
d.action = ACTION_SET_STATE;
qemu_opts_foreach(&set_state_opts, add_rule, &d, &local_err);
if (local_err) {
@@ -275,6 +320,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
ret = 0;
fail:
qemu_opts_reset(&inject_error_opts);
+ qemu_opts_reset(&delay_opts);
qemu_opts_reset(&set_state_opts);
if (f) {
fclose(f);
@@ -473,39 +519,61 @@ out:
static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes)
{
BDRVBlkdebugState *s = bs->opaque;
- BlkdebugRule *rule = NULL;
+ BlkdebugRule *rule = NULL, *delay_rule = NULL, *error_rule = NULL;
+ int64_t latency;
int error;
bool immediately;
+ int ret = 0;
QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
- uint64_t inject_offset = rule->options.inject.offset;
-
- if (inject_offset == -1 ||
- (bytes && inject_offset >= offset &&
- inject_offset < offset + bytes))
+ if (rule->offset == -1 ||
+ (bytes && rule->offset >= offset &&
+ rule->offset < offset + bytes))
{
- break;
+ if (!error_rule && rule->action == ACTION_INJECT_ERROR) {
+ error_rule = rule;
+ } else if (!delay_rule && rule->action == ACTION_DELAY) {
+ delay_rule = rule;
+ }
+
+ if (error_rule && delay_rule) {
+ break;
+ }
}
}
- if (!rule || !rule->options.inject.error) {
- return 0;
- }
+ if (delay_rule) {
+ latency = delay_rule->options.delay.latency;
- immediately = rule->options.inject.immediately;
- error = rule->options.inject.error;
+ if (delay_rule->once) {
+ QSIMPLEQ_REMOVE(&s->active_rules, delay_rule, BlkdebugRule, active_next);
+ remove_rule(delay_rule);
+ }
- if (rule->options.inject.once) {
- QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
- remove_rule(rule);
+ if (latency != 0) {
+ qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, latency);
+ }
}
- if (!immediately) {
- aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
- qemu_coroutine_yield();
+ if (error_rule) {
+ error = error_rule->options.inject.error;
+ immediately = error_rule->options.inject.immediately;
+
+ if (error_rule->once) {
+ QSIMPLEQ_REMOVE(&s->active_rules, error_rule, BlkdebugRule, active_next);
+ remove_rule(error_rule);
+ }
+
+ if (error && !immediately) {
+ aio_co_schedule(qemu_get_current_aio_context(),
+ qemu_coroutine_self());
+ qemu_coroutine_yield();
+ }
+
+ ret = -error;
}
- return -error;
+ return ret;
}
static int coroutine_fn
@@ -694,6 +762,7 @@ static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
/* Take the action */
switch (rule->action) {
case ACTION_INJECT_ERROR:
+ case ACTION_DELAY:
if (!injected) {
QSIMPLEQ_INIT(&s->active_rules);
injected = true;
diff --git a/docs/devel/blkdebug.txt b/docs/devel/blkdebug.txt
index 43d8e8f..1befcf8 100644
--- a/docs/devel/blkdebug.txt
+++ 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.
+
Invoke QEMU as follows:
$ qemu-system-x86_64
@@ -60,21 +68,27 @@ Rules support the following attributes:
rule to match. See the "State transitions" section for information
on states.
- errno - the numeric errno value to return when a request matches this rule.
- The errno values depend on the host since the numeric values are not
- standarized in the POSIX specification.
-
sector - (optional) a sector number that the request must overlap in order to
match this rule
once - (optional, default "off") only execute this action on the first
matching request
+Error injection rules support the following attributes:
+
+ errno - the numeric errno value to return when a request matches this rule.
+ The errno values depend on the host since the numeric values are not
+ standarized in the POSIX specification.
+
immediately - (optional, default "off") return a NULL BlockAIOCB
pointer and fail without an errno instead. This
exercises the code path where BlockAIOCB fails and the
caller's BlockCompletionFunc is not invoked.
+Delay rules support the following attribute:
+
+ latency - the delay to add to an I/O request, in microseconds.
+
Events
------
Block drivers provide information about the type of I/O request they are about
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4c7a37a..819e3f9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2941,11 +2941,11 @@
'refblock_alloc_write_blocks', 'refblock_alloc_write_table',
'refblock_alloc_switch_table', 'cluster_alloc',
'cluster_alloc_bytes', 'cluster_free', 'flush_to_os',
- 'flush_to_disk', 'pwritev_rmw_head', 'pwritev_rmw_after_head',
- 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
- 'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
- 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
- 'cor_write'] }
+ 'flush_to_disk', 'preadv', 'pwritev_rmw_head',
+ 'pwritev_rmw_after_head', 'pwritev_rmw_tail',
+ 'pwritev_rmw_after_tail', 'pwritev', 'pwritev_zero', 'pwritev_done',
+ 'empty_image_prepare', 'l1_shrink_write_table',
+ 'l1_shrink_free_l2_clusters', 'cor_write'] }
##
# @BlkdebugInjectErrorOptions:
@@ -2980,6 +2980,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.0
+##
+{ 'struct': 'BlkdebugDelayOptions',
+ 'data': { 'event': 'BlkdebugEvent',
+ '*state': 'int',
+ '*latency': 'int',
+ '*sector': 'int',
+ '*once': 'bool' } }
+
+##
# @BlkdebugSetStateOptions:
#
# Describes a single state-change event for blkdebug.
@@ -3049,6 +3077,7 @@
'*opt-write-zero': 'int32', '*max-write-zero': 'int32',
'*opt-discard': 'int32', '*max-discard': 'int32',
'*inject-error': ['BlkdebugInjectErrorOptions'],
+ '*delay': ['BlkdebugDelayOptions'],
'*set-state': ['BlkdebugSetStateOptions'] } }
##
diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
index 48b4955..3d0610c 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.delay.event=write_aio,file.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.delay.event=write_aio,file.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
+echo
+echo "=== Testing blkdebug on existing block device ==="
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+ "arguments": {
+ "node-name": "drive0",
+ "driver": "file",
+ "filename": "$TEST_IMG"
+ }
+}
+{ "execute": "blockdev-add",
+ "arguments": {
+ "driver": "$IMGFMT",
+ "node-name": "drive0-debug",
+ "file": {
+ "driver": "blkdebug",
+ "image": "drive0",
+ "delay": [{
+ "event": "write_aio",
+ "latency": 10000
+ }]
+ }
+ }
+}
+{ "execute": "human-monitor-command",
+ "arguments": {
+ "command-line": 'qemu-io drive0-debug "aio_write 0 512"'
+ }
+}
+{ "execute": "human-monitor-command",
+ "arguments": {
+ "command-line": 'qemu-io drive0-debug "aio_read 0 512"'
+ }
+}
+{ "execute": "quit" }
+EOF
+
+echo
echo "=== Testing blkdebug on existing block device ==="
echo
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)
+
+=== Testing blkdebug on existing block device ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+
=== Testing blkdebug on existing block device ===
Testing:
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] blkdebug: Add support for latency rules
2018-09-05 0:24 ` [Qemu-devel] [PATCH v2] " Marc Olson
@ 2018-09-13 16:48 ` Marc Olson
2018-09-14 20:46 ` [Qemu-devel] [Qemu-block] " John Snow
0 siblings, 1 reply; 7+ messages in thread
From: Marc Olson @ 2018-09-13 16:48 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Max Reitz, Markus Armbruster, Eric Blake, qemu-block
Are there further thoughts on this patch?
On 09/04/2018 05:24 PM, Marc Olson wrote:
> Sometimes storage devices can be slow to respond, due to media errors, firmware
> issues, SSD garbage collection, etc. This patch adds a new rule type to
> blkdebug that allows injection of latency to I/O operations. Similar to error
> injection rules, latency rules can be specified with or without an offset, and
> can also apply upon state transitions.
>
> Signed-off-by: Marc Olson <marcolso@amazon.com>
>
> ---
> v2:
> - Change so that delay rules are executed before error rules
> - Add QMP support
> - Add tests
> ---
> block/blkdebug.c | 119 +++++++++++++++++++++++++++++++++++----------
> docs/devel/blkdebug.txt | 30 +++++++++---
> qapi/block-core.json | 39 +++++++++++++--
> tests/qemu-iotests/071 | 63 ++++++++++++++++++++++++
> tests/qemu-iotests/071.out | 31 ++++++++++++
> 5 files changed, 244 insertions(+), 38 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 0759452..1785fe3 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,
> };
> @@ -73,14 +74,17 @@ typedef struct BlkdebugRule {
> BlkdebugEvent event;
> int action;
> int state;
> + int once;
> + int64_t offset;
> union {
> struct {
> int error;
> int immediately;
> - int once;
> - int64_t offset;
> } inject;
> struct {
> + int64_t latency;
> + } delay;
> + struct {
> int new_state;
> } set_state;
> struct {
> @@ -123,6 +127,33 @@ static QemuOptsList inject_error_opts = {
> },
> };
>
> +static QemuOptsList delay_opts = {
> + .name = "delay",
> + .head = QTAILQ_HEAD_INITIALIZER(delay_opts.head),
> + .desc = {
> + {
> + .name = "event",
> + },
> + {
> + .name = "state",
> + .type = QEMU_OPT_NUMBER,
> + },
> + {
> + .name = "latency",
> + .type = QEMU_OPT_NUMBER,
> + },
> + {
> + .name = "sector",
> + .type = QEMU_OPT_NUMBER,
> + },
> + {
> + .name = "once",
> + .type = QEMU_OPT_BOOL,
> + },
> + { /* end of list */ }
> + },
> +};
> +
> static QemuOptsList set_state_opts = {
> .name = "set-state",
> .head = QTAILQ_HEAD_INITIALIZER(set_state_opts.head),
> @@ -145,6 +176,7 @@ static QemuOptsList set_state_opts = {
>
> static QemuOptsList *config_groups[] = {
> &inject_error_opts,
> + &delay_opts,
> &set_state_opts,
> NULL
> };
> @@ -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;
> +
> /* Parse action-specific options */
> switch (d->action) {
> case ACTION_INJECT_ERROR:
> rule->options.inject.error = qemu_opt_get_number(opts, "errno", EIO);
> - rule->options.inject.once = qemu_opt_get_bool(opts, "once", 0);
> rule->options.inject.immediately =
> qemu_opt_get_bool(opts, "immediately", 0);
> - sector = qemu_opt_get_number(opts, "sector", -1);
> - rule->options.inject.offset =
> - sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
> + break;
> +
> + case ACTION_DELAY:
> + rule->options.delay.latency =
> + qemu_opt_get_number(opts, "latency", 100) * SCALE_US;
> break;
>
> case ACTION_SET_STATE:
> @@ -264,6 +301,14 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
> goto fail;
> }
>
> + d.action = ACTION_DELAY;
> + qemu_opts_foreach(&delay_opts, add_rule, &d, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> d.action = ACTION_SET_STATE;
> qemu_opts_foreach(&set_state_opts, add_rule, &d, &local_err);
> if (local_err) {
> @@ -275,6 +320,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
> ret = 0;
> fail:
> qemu_opts_reset(&inject_error_opts);
> + qemu_opts_reset(&delay_opts);
> qemu_opts_reset(&set_state_opts);
> if (f) {
> fclose(f);
> @@ -473,39 +519,61 @@ out:
> static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes)
> {
> BDRVBlkdebugState *s = bs->opaque;
> - BlkdebugRule *rule = NULL;
> + BlkdebugRule *rule = NULL, *delay_rule = NULL, *error_rule = NULL;
> + int64_t latency;
> int error;
> bool immediately;
> + int ret = 0;
>
> QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
> - uint64_t inject_offset = rule->options.inject.offset;
> -
> - if (inject_offset == -1 ||
> - (bytes && inject_offset >= offset &&
> - inject_offset < offset + bytes))
> + if (rule->offset == -1 ||
> + (bytes && rule->offset >= offset &&
> + rule->offset < offset + bytes))
> {
> - break;
> + if (!error_rule && rule->action == ACTION_INJECT_ERROR) {
> + error_rule = rule;
> + } else if (!delay_rule && rule->action == ACTION_DELAY) {
> + delay_rule = rule;
> + }
> +
> + if (error_rule && delay_rule) {
> + break;
> + }
> }
> }
>
> - if (!rule || !rule->options.inject.error) {
> - return 0;
> - }
> + if (delay_rule) {
> + latency = delay_rule->options.delay.latency;
>
> - immediately = rule->options.inject.immediately;
> - error = rule->options.inject.error;
> + if (delay_rule->once) {
> + QSIMPLEQ_REMOVE(&s->active_rules, delay_rule, BlkdebugRule, active_next);
> + remove_rule(delay_rule);
> + }
>
> - if (rule->options.inject.once) {
> - QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
> - remove_rule(rule);
> + if (latency != 0) {
> + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, latency);
> + }
> }
>
> - if (!immediately) {
> - aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
> - qemu_coroutine_yield();
> + if (error_rule) {
> + error = error_rule->options.inject.error;
> + immediately = error_rule->options.inject.immediately;
> +
> + if (error_rule->once) {
> + QSIMPLEQ_REMOVE(&s->active_rules, error_rule, BlkdebugRule, active_next);
> + remove_rule(error_rule);
> + }
> +
> + if (error && !immediately) {
> + aio_co_schedule(qemu_get_current_aio_context(),
> + qemu_coroutine_self());
> + qemu_coroutine_yield();
> + }
> +
> + ret = -error;
> }
>
> - return -error;
> + return ret;
> }
>
> static int coroutine_fn
> @@ -694,6 +762,7 @@ static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
> /* Take the action */
> switch (rule->action) {
> case ACTION_INJECT_ERROR:
> + case ACTION_DELAY:
> if (!injected) {
> QSIMPLEQ_INIT(&s->active_rules);
> injected = true;
> diff --git a/docs/devel/blkdebug.txt b/docs/devel/blkdebug.txt
> index 43d8e8f..1befcf8 100644
> --- a/docs/devel/blkdebug.txt
> +++ 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.
> +
> Invoke QEMU as follows:
>
> $ qemu-system-x86_64
> @@ -60,21 +68,27 @@ Rules support the following attributes:
> rule to match. See the "State transitions" section for information
> on states.
>
> - errno - the numeric errno value to return when a request matches this rule.
> - The errno values depend on the host since the numeric values are not
> - standarized in the POSIX specification.
> -
> sector - (optional) a sector number that the request must overlap in order to
> match this rule
>
> once - (optional, default "off") only execute this action on the first
> matching request
>
> +Error injection rules support the following attributes:
> +
> + errno - the numeric errno value to return when a request matches this rule.
> + The errno values depend on the host since the numeric values are not
> + standarized in the POSIX specification.
> +
> immediately - (optional, default "off") return a NULL BlockAIOCB
> pointer and fail without an errno instead. This
> exercises the code path where BlockAIOCB fails and the
> caller's BlockCompletionFunc is not invoked.
>
> +Delay rules support the following attribute:
> +
> + latency - the delay to add to an I/O request, in microseconds.
> +
> Events
> ------
> Block drivers provide information about the type of I/O request they are about
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4c7a37a..819e3f9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2941,11 +2941,11 @@
> 'refblock_alloc_write_blocks', 'refblock_alloc_write_table',
> 'refblock_alloc_switch_table', 'cluster_alloc',
> 'cluster_alloc_bytes', 'cluster_free', 'flush_to_os',
> - 'flush_to_disk', 'pwritev_rmw_head', 'pwritev_rmw_after_head',
> - 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
> - 'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
> - 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
> - 'cor_write'] }
> + 'flush_to_disk', 'preadv', 'pwritev_rmw_head',
> + 'pwritev_rmw_after_head', 'pwritev_rmw_tail',
> + 'pwritev_rmw_after_tail', 'pwritev', 'pwritev_zero', 'pwritev_done',
> + 'empty_image_prepare', 'l1_shrink_write_table',
> + 'l1_shrink_free_l2_clusters', 'cor_write'] }
>
> ##
> # @BlkdebugInjectErrorOptions:
> @@ -2980,6 +2980,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.0
> +##
> +{ 'struct': 'BlkdebugDelayOptions',
> + 'data': { 'event': 'BlkdebugEvent',
> + '*state': 'int',
> + '*latency': 'int',
> + '*sector': 'int',
> + '*once': 'bool' } }
> +
> +##
> # @BlkdebugSetStateOptions:
> #
> # Describes a single state-change event for blkdebug.
> @@ -3049,6 +3077,7 @@
> '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
> '*opt-discard': 'int32', '*max-discard': 'int32',
> '*inject-error': ['BlkdebugInjectErrorOptions'],
> + '*delay': ['BlkdebugDelayOptions'],
> '*set-state': ['BlkdebugSetStateOptions'] } }
>
> ##
> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
> index 48b4955..3d0610c 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.delay.event=write_aio,file.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.delay.event=write_aio,file.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
> +echo
> +echo "=== Testing blkdebug on existing block device ==="
> +echo
> +
> +run_qemu <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "blockdev-add",
> + "arguments": {
> + "node-name": "drive0",
> + "driver": "file",
> + "filename": "$TEST_IMG"
> + }
> +}
> +{ "execute": "blockdev-add",
> + "arguments": {
> + "driver": "$IMGFMT",
> + "node-name": "drive0-debug",
> + "file": {
> + "driver": "blkdebug",
> + "image": "drive0",
> + "delay": [{
> + "event": "write_aio",
> + "latency": 10000
> + }]
> + }
> + }
> +}
> +{ "execute": "human-monitor-command",
> + "arguments": {
> + "command-line": 'qemu-io drive0-debug "aio_write 0 512"'
> + }
> +}
> +{ "execute": "human-monitor-command",
> + "arguments": {
> + "command-line": 'qemu-io drive0-debug "aio_read 0 512"'
> + }
> +}
> +{ "execute": "quit" }
> +EOF
> +
> +echo
> echo "=== Testing blkdebug on existing block device ==="
> echo
>
> 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)
> +
> +=== Testing blkdebug on existing block device ===
> +
> +Testing:
> +QMP_VERSION
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +wrote 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{"return": ""}
> +read 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{"return": ""}
> +{"return": {}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
> +
> +
> === Testing blkdebug on existing block device ===
>
> Testing:
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2] blkdebug: Add support for latency rules
2018-09-13 16:48 ` Marc Olson
@ 2018-09-14 20:46 ` John Snow
2018-09-14 21:03 ` Marc Olson
0 siblings, 1 reply; 7+ messages in thread
From: John Snow @ 2018-09-14 20:46 UTC (permalink / raw)
To: Marc Olson, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, qemu-block, Max Reitz
On 09/13/2018 12:48 PM, Marc Olson wrote:
> Are there further thoughts on this patch?
>
The CI tools may have missed it since it appears to have been sent in
reply to the V1 instead of as a new thread. When you send a revision
out, can you send it as its own thread?
(We're also in a bit of a logjam at the moment with no pull requests
having been processed recently, so we're a bit gummed up.)
Some comments below.
>
> On 09/04/2018 05:24 PM, Marc Olson wrote:
>> Sometimes storage devices can be slow to respond, due to media errors,
>> firmware
>> issues, SSD garbage collection, etc. This patch adds a new rule type to
>> blkdebug that allows injection of latency to I/O operations. Similar
>> to error
>> injection rules, latency rules can be specified with or without an
>> offset, and
>> can also apply upon state transitions.
>>
>> Signed-off-by: Marc Olson <marcolso@amazon.com>
>>
>> ---
>> v2:
>> - Change so that delay rules are executed before error rules
>> - Add QMP support
>> - Add tests
>> ---
>> block/blkdebug.c | 119
>> +++++++++++++++++++++++++++++++++++----------
>> docs/devel/blkdebug.txt | 30 +++++++++---
>> qapi/block-core.json | 39 +++++++++++++--
>> tests/qemu-iotests/071 | 63 ++++++++++++++++++++++++
>> tests/qemu-iotests/071.out | 31 ++++++++++++
>> 5 files changed, 244 insertions(+), 38 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 0759452..1785fe3 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,
>> };
>> @@ -73,14 +74,17 @@ typedef struct BlkdebugRule {
>> BlkdebugEvent event;
>> int action;
>> int state;
>> + int once;
>> + int64_t offset;
Hm, I guess we're just promoting these to universal values that might
exist for any of the rule types, but not allowing the user to set them
for rules where it doesn't make sense.
>> union {
>> struct {
>> int error;
>> int immediately;
>> - int once;
>> - int64_t offset;
>> } inject;
>> struct {
>> + int64_t latency;
>> + } delay;
>> + struct {
>> int new_state;
>> } set_state;
>> struct {
>> @@ -123,6 +127,33 @@ static QemuOptsList inject_error_opts = {
>> },
>> };
>> +static QemuOptsList delay_opts = {
>> + .name = "delay",
>> + .head = QTAILQ_HEAD_INITIALIZER(delay_opts.head),
>> + .desc = {
>> + {
>> + .name = "event",
>> + },
>> + {
>> + .name = "state",
>> + .type = QEMU_OPT_NUMBER,
>> + },
>> + {
>> + .name = "latency",
>> + .type = QEMU_OPT_NUMBER,
>> + },
>> + {
>> + .name = "sector",
>> + .type = QEMU_OPT_NUMBER,
>> + },
>> + {
>> + .name = "once",
>> + .type = QEMU_OPT_BOOL,
>> + },
>> + { /* end of list */ }
>> + },
>> +};
>> +
>> static QemuOptsList set_state_opts = {
>> .name = "set-state",
>> .head = QTAILQ_HEAD_INITIALIZER(set_state_opts.head),
>> @@ -145,6 +176,7 @@ static QemuOptsList set_state_opts = {
>> static QemuOptsList *config_groups[] = {
>> &inject_error_opts,
>> + &delay_opts,
>> &set_state_opts,
>> NULL
>> };
>> @@ -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;
>> +
>> /* Parse action-specific options */
>> switch (d->action) {
>> case ACTION_INJECT_ERROR:
>> rule->options.inject.error = qemu_opt_get_number(opts,
>> "errno", EIO);
>> - rule->options.inject.once = qemu_opt_get_bool(opts, "once", 0);
>> rule->options.inject.immediately =
>> qemu_opt_get_bool(opts, "immediately", 0);
>> - sector = qemu_opt_get_number(opts, "sector", -1);
>> - rule->options.inject.offset =
>> - sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
>> + break;
>> +
>> + case ACTION_DELAY:
>> + rule->options.delay.latency =
>> + qemu_opt_get_number(opts, "latency", 100) * SCALE_US;
>> break;
>> case ACTION_SET_STATE:
>> @@ -264,6 +301,14 @@ static int read_config(BDRVBlkdebugState *s,
>> const char *filename,
>> goto fail;
>> }
>> + d.action = ACTION_DELAY;
>> + qemu_opts_foreach(&delay_opts, add_rule, &d, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> +
>> d.action = ACTION_SET_STATE;
>> qemu_opts_foreach(&set_state_opts, add_rule, &d, &local_err);
>> if (local_err) {
>> @@ -275,6 +320,7 @@ static int read_config(BDRVBlkdebugState *s, const
>> char *filename,
>> ret = 0;
>> fail:
>> qemu_opts_reset(&inject_error_opts);
>> + qemu_opts_reset(&delay_opts);
>> qemu_opts_reset(&set_state_opts);
>> if (f) {
>> fclose(f);
>> @@ -473,39 +519,61 @@ out:
>> static int rule_check(BlockDriverState *bs, uint64_t offset,
>> uint64_t bytes)
>> {
>> BDRVBlkdebugState *s = bs->opaque;
>> - BlkdebugRule *rule = NULL;
>> + BlkdebugRule *rule = NULL, *delay_rule = NULL, *error_rule = NULL;
>> + int64_t latency;
>> int error;
>> bool immediately;
>> + int ret = 0;
>> QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
>> - uint64_t inject_offset = rule->options.inject.offset;
>> -
>> - if (inject_offset == -1 ||
>> - (bytes && inject_offset >= offset &&
>> - inject_offset < offset + bytes))
>> + if (rule->offset == -1 ||
>> + (bytes && rule->offset >= offset &&
>> + rule->offset < offset + bytes))
>> {
>> - break;
>> + if (!error_rule && rule->action == ACTION_INJECT_ERROR) {
>> + error_rule = rule;
>> + } else if (!delay_rule && rule->action == ACTION_DELAY) {
>> + delay_rule = rule;
>> + }
>> +
>> + if (error_rule && delay_rule) {
>> + break;
>> + }
>> }
>> }
>> - if (!rule || !rule->options.inject.error) {
>> - return 0;
>> - }
>> + if (delay_rule) {
>> + latency = delay_rule->options.delay.latency;
>> - immediately = rule->options.inject.immediately;
>> - error = rule->options.inject.error;
>> + if (delay_rule->once) {
>> + QSIMPLEQ_REMOVE(&s->active_rules, delay_rule,
>> BlkdebugRule, active_next);
>> + remove_rule(delay_rule);
>> + }
>> - if (rule->options.inject.once) {
>> - QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule,
>> active_next);
>> - remove_rule(rule);
>> + if (latency != 0) {
>> + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, latency);
>> + }
>> }
>> - if (!immediately) {
>> - aio_co_schedule(qemu_get_current_aio_context(),
>> qemu_coroutine_self());
>> - qemu_coroutine_yield();
>> + if (error_rule) {
>> + error = error_rule->options.inject.error;
>> + immediately = error_rule->options.inject.immediately;
>> +
>> + if (error_rule->once) {
>> + QSIMPLEQ_REMOVE(&s->active_rules, error_rule,
>> BlkdebugRule, active_next);
>> + remove_rule(error_rule);
>> + }
>> +
>> + if (error && !immediately) {
>> + aio_co_schedule(qemu_get_current_aio_context(),
>> + qemu_coroutine_self());
>> + qemu_coroutine_yield();
>> + }
>> +
>> + ret = -error;
>> }
>> - return -error;
>> + return ret;
>> }
That's a big hunk of stuff all changing at once, it might have been nice
to do this in two passes:
1. refactor the loop to look for error rules, then
2. add login in for the delay rules.
It looks like we change the handling of error rules just a little bit;
before if (!rule->options.inject.error) we return 0 immediately, but now
we actually check for `once` and remove the the rule first.
I think that's probably fine; isn't it? It's not as if the error will
show up if it wasn't there already, so that's probably an improvement.
The delay rule looks fine.
>> static int coroutine_fn
>> @@ -694,6 +762,7 @@ static bool process_rule(BlockDriverState *bs,
>> struct BlkdebugRule *rule,
>> /* Take the action */
>> switch (rule->action) {
>> case ACTION_INJECT_ERROR:
>> + case ACTION_DELAY:
>> if (!injected) {
>> QSIMPLEQ_INIT(&s->active_rules);
>> injected = true;
>> diff --git a/docs/devel/blkdebug.txt b/docs/devel/blkdebug.txt
>> index 43d8e8f..1befcf8 100644
>> --- a/docs/devel/blkdebug.txt
>> +++ 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.
>> +
>> Invoke QEMU as follows:
>> $ qemu-system-x86_64
>> @@ -60,21 +68,27 @@ Rules support the following attributes:
>> rule to match. See the "State transitions" section for
>> information
>> on states.
>> - errno - the numeric errno value to return when a request matches
>> this rule.
>> - The errno values depend on the host since the numeric
>> values are not
>> - standarized in the POSIX specification.
>> -
>> sector - (optional) a sector number that the request must overlap
>> in order to
>> match this rule
>> once - (optional, default "off") only execute this action on the
>> first
>> matching request
>> +Error injection rules support the following attributes:
The section rewrite here actually makes it a little less clear which
rules support which attributes, it might be time for a heavier rewrite.
This is OK for now, though, I think...
>> +
>> + errno - the numeric errno value to return when a request matches
>> this rule.
>> + The errno values depend on the host since the numeric
>> values are not
>> + standarized in the POSIX specification.
>> +
>> immediately - (optional, default "off") return a NULL BlockAIOCB
>> pointer and fail without an errno instead. This
>> exercises the code path where BlockAIOCB fails and the
>> caller's BlockCompletionFunc is not invoked.
>> +Delay rules support the following attribute:
>> +
>> + latency - the delay to add to an I/O request, in microseconds.
>> +
>> Events
>> ------
>> Block drivers provide information about the type of I/O request they
>> are about
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 4c7a37a..819e3f9 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2941,11 +2941,11 @@
>> 'refblock_alloc_write_blocks',
>> 'refblock_alloc_write_table',
>> 'refblock_alloc_switch_table', 'cluster_alloc',
>> 'cluster_alloc_bytes', 'cluster_free', 'flush_to_os',
>> - 'flush_to_disk', 'pwritev_rmw_head',
>> 'pwritev_rmw_after_head',
>> - 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
>> - 'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
>> - 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
>> - 'cor_write'] }
>> + 'flush_to_disk', 'preadv', 'pwritev_rmw_head',
>> + 'pwritev_rmw_after_head', 'pwritev_rmw_tail',
>> + 'pwritev_rmw_after_tail', 'pwritev', 'pwritev_zero',
>> 'pwritev_done',
>> + 'empty_image_prepare', 'l1_shrink_write_table',
>> + 'l1_shrink_free_l2_clusters', 'cor_write'] }
Is this an unrelated change that snuck in? If it's intentional, you
ought to justify it in a standalone commit with a commit message
containing the rationale.
>> ##
>> # @BlkdebugInjectErrorOptions:
>> @@ -2980,6 +2980,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.0
3.1, sorry :)
>> +##
>> +{ 'struct': 'BlkdebugDelayOptions',
>> + 'data': { 'event': 'BlkdebugEvent',
>> + '*state': 'int',
>> + '*latency': 'int',
>> + '*sector': 'int',
>> + '*once': 'bool' } }
>> +
>> +##
>> # @BlkdebugSetStateOptions:
>> #
>> # Describes a single state-change event for blkdebug.
>> @@ -3049,6 +3077,7 @@
>> '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
>> '*opt-discard': 'int32', '*max-discard': 'int32',
>> '*inject-error': ['BlkdebugInjectErrorOptions'],
>> + '*delay': ['BlkdebugDelayOptions'],
>> '*set-state': ['BlkdebugSetStateOptions'] } }
>> ##
>> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
>> index 48b4955..3d0610c 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.delay.event=write_aio,file.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.delay.event=write_aio,file.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
>> +echo
>> +echo "=== Testing blkdebug on existing block device ==="
>> +echo
>> +
>> +run_qemu <<EOF
>> +{ "execute": "qmp_capabilities" }
>> +{ "execute": "blockdev-add",
>> + "arguments": {
>> + "node-name": "drive0",
>> + "driver": "file",
>> + "filename": "$TEST_IMG"
>> + }
>> +}
>> +{ "execute": "blockdev-add",
>> + "arguments": {
>> + "driver": "$IMGFMT",
>> + "node-name": "drive0-debug",
>> + "file": {
>> + "driver": "blkdebug",
>> + "image": "drive0",
>> + "delay": [{
>> + "event": "write_aio",
>> + "latency": 10000
>> + }]
>> + }
>> + }
>> +}
>> +{ "execute": "human-monitor-command",
>> + "arguments": {
>> + "command-line": 'qemu-io drive0-debug "aio_write 0 512"'
>> + }
>> +}
>> +{ "execute": "human-monitor-command",
>> + "arguments": {
>> + "command-line": 'qemu-io drive0-debug "aio_read 0 512"'
>> + }
>> +}
>> +{ "execute": "quit" }
>> +EOF
>> +
>> +echo
>> echo "=== Testing blkdebug on existing block device ==="
>> echo
>> 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)
>> +
>> +=== Testing blkdebug on existing block device ===
>> +
>> +Testing:
>> +QMP_VERSION
>> +{"return": {}}
>> +{"return": {}}
>> +{"return": {}}
>> +wrote 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +{"return": ""}
>> +read 512/512 bytes at offset 0
>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +{"return": ""}
>> +{"return": {}}
>> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
>> "event": "SHUTDOWN", "data": {"guest": false}}
>> +
>> +
>> === Testing blkdebug on existing block device ===
>> Testing:
>
>
Thanks, this seems like a good addition to have.
--js
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2] blkdebug: Add support for latency rules
2018-09-14 20:46 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2018-09-14 21:03 ` Marc Olson
0 siblings, 0 replies; 7+ messages in thread
From: Marc Olson @ 2018-09-14 21:03 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, qemu-block, Max Reitz
On 09/14/2018 01:46 PM, John Snow wrote:
> On 09/13/2018 12:48 PM, Marc Olson wrote:
>> Are there further thoughts on this patch?
>>
> The CI tools may have missed it since it appears to have been sent in
> reply to the V1 instead of as a new thread. When you send a revision
> out, can you send it as its own thread?
Ah, sorry. Happy to do that for v3.
> (We're also in a bit of a logjam at the moment with no pull requests
> having been processed recently, so we're a bit gummed up.)
>
> Some comments below.
>
>> On 09/04/2018 05:24 PM, Marc Olson wrote:
>>> Sometimes storage devices can be slow to respond, due to media errors,
>>> firmware
>>> issues, SSD garbage collection, etc. This patch adds a new rule type to
>>> blkdebug that allows injection of latency to I/O operations. Similar
>>> to error
>>> injection rules, latency rules can be specified with or without an
>>> offset, and
>>> can also apply upon state transitions.
>>>
>>> Signed-off-by: Marc Olson <marcolso@amazon.com>
>>>
>>> ---
>>> v2:
>>> - Change so that delay rules are executed before error rules
>>> - Add QMP support
>>> - Add tests
>>> ---
>>> block/blkdebug.c | 119
>>> +++++++++++++++++++++++++++++++++++----------
>>> docs/devel/blkdebug.txt | 30 +++++++++---
>>> qapi/block-core.json | 39 +++++++++++++--
>>> tests/qemu-iotests/071 | 63 ++++++++++++++++++++++++
>>> tests/qemu-iotests/071.out | 31 ++++++++++++
>>> 5 files changed, 244 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>>> index 0759452..1785fe3 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,
>>> };
>>> @@ -73,14 +74,17 @@ typedef struct BlkdebugRule {
>>> BlkdebugEvent event;
>>> int action;
>>> int state;
>>> + int once;
>>> + int64_t offset;
> Hm, I guess we're just promoting these to universal values that might
> exist for any of the rule types, but not allowing the user to set them
> for rules where it doesn't make sense.
It seemed a bit odd to me to duplicate them in the union, but happy to
move them back if you're concerned about it.
>
>>> union {
>>> struct {
>>> int error;
>>> int immediately;
>>> - int once;
>>> - int64_t offset;
>>> } inject;
>>> struct {
>>> + int64_t latency;
>>> + } delay;
>>> + struct {
>>> int new_state;
>>> } set_state;
>>> struct {
>>> @@ -123,6 +127,33 @@ static QemuOptsList inject_error_opts = {
>>> },
>>> };
>>> +static QemuOptsList delay_opts = {
>>> + .name = "delay",
>>> + .head = QTAILQ_HEAD_INITIALIZER(delay_opts.head),
>>> + .desc = {
>>> + {
>>> + .name = "event",
>>> + },
>>> + {
>>> + .name = "state",
>>> + .type = QEMU_OPT_NUMBER,
>>> + },
>>> + {
>>> + .name = "latency",
>>> + .type = QEMU_OPT_NUMBER,
>>> + },
>>> + {
>>> + .name = "sector",
>>> + .type = QEMU_OPT_NUMBER,
>>> + },
>>> + {
>>> + .name = "once",
>>> + .type = QEMU_OPT_BOOL,
>>> + },
>>> + { /* end of list */ }
>>> + },
>>> +};
>>> +
>>> static QemuOptsList set_state_opts = {
>>> .name = "set-state",
>>> .head = QTAILQ_HEAD_INITIALIZER(set_state_opts.head),
>>> @@ -145,6 +176,7 @@ static QemuOptsList set_state_opts = {
>>> static QemuOptsList *config_groups[] = {
>>> &inject_error_opts,
>>> + &delay_opts,
>>> &set_state_opts,
>>> NULL
>>> };
>>> @@ -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;
>>> +
>>> /* Parse action-specific options */
>>> switch (d->action) {
>>> case ACTION_INJECT_ERROR:
>>> rule->options.inject.error = qemu_opt_get_number(opts,
>>> "errno", EIO);
>>> - rule->options.inject.once = qemu_opt_get_bool(opts, "once", 0);
>>> rule->options.inject.immediately =
>>> qemu_opt_get_bool(opts, "immediately", 0);
>>> - sector = qemu_opt_get_number(opts, "sector", -1);
>>> - rule->options.inject.offset =
>>> - sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
>>> + break;
>>> +
>>> + case ACTION_DELAY:
>>> + rule->options.delay.latency =
>>> + qemu_opt_get_number(opts, "latency", 100) * SCALE_US;
>>> break;
>>> case ACTION_SET_STATE:
>>> @@ -264,6 +301,14 @@ static int read_config(BDRVBlkdebugState *s,
>>> const char *filename,
>>> goto fail;
>>> }
>>> + d.action = ACTION_DELAY;
>>> + qemu_opts_foreach(&delay_opts, add_rule, &d, &local_err);
>>> + if (local_err) {
>>> + error_propagate(errp, local_err);
>>> + ret = -EINVAL;
>>> + goto fail;
>>> + }
>>> +
>>> d.action = ACTION_SET_STATE;
>>> qemu_opts_foreach(&set_state_opts, add_rule, &d, &local_err);
>>> if (local_err) {
>>> @@ -275,6 +320,7 @@ static int read_config(BDRVBlkdebugState *s, const
>>> char *filename,
>>> ret = 0;
>>> fail:
>>> qemu_opts_reset(&inject_error_opts);
>>> + qemu_opts_reset(&delay_opts);
>>> qemu_opts_reset(&set_state_opts);
>>> if (f) {
>>> fclose(f);
>>> @@ -473,39 +519,61 @@ out:
>>> static int rule_check(BlockDriverState *bs, uint64_t offset,
>>> uint64_t bytes)
>>> {
>>> BDRVBlkdebugState *s = bs->opaque;
>>> - BlkdebugRule *rule = NULL;
>>> + BlkdebugRule *rule = NULL, *delay_rule = NULL, *error_rule = NULL;
>>> + int64_t latency;
>>> int error;
>>> bool immediately;
>>> + int ret = 0;
>>> QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
>>> - uint64_t inject_offset = rule->options.inject.offset;
>>> -
>>> - if (inject_offset == -1 ||
>>> - (bytes && inject_offset >= offset &&
>>> - inject_offset < offset + bytes))
>>> + if (rule->offset == -1 ||
>>> + (bytes && rule->offset >= offset &&
>>> + rule->offset < offset + bytes))
>>> {
>>> - break;
>>> + if (!error_rule && rule->action == ACTION_INJECT_ERROR) {
>>> + error_rule = rule;
>>> + } else if (!delay_rule && rule->action == ACTION_DELAY) {
>>> + delay_rule = rule;
>>> + }
>>> +
>>> + if (error_rule && delay_rule) {
>>> + break;
>>> + }
>>> }
>>> }
>>> - if (!rule || !rule->options.inject.error) {
>>> - return 0;
>>> - }
>>> + if (delay_rule) {
>>> + latency = delay_rule->options.delay.latency;
>>> - immediately = rule->options.inject.immediately;
>>> - error = rule->options.inject.error;
>>> + if (delay_rule->once) {
>>> + QSIMPLEQ_REMOVE(&s->active_rules, delay_rule,
>>> BlkdebugRule, active_next);
>>> + remove_rule(delay_rule);
>>> + }
>>> - if (rule->options.inject.once) {
>>> - QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule,
>>> active_next);
>>> - remove_rule(rule);
>>> + if (latency != 0) {
>>> + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, latency);
>>> + }
>>> }
>>> - if (!immediately) {
>>> - aio_co_schedule(qemu_get_current_aio_context(),
>>> qemu_coroutine_self());
>>> - qemu_coroutine_yield();
>>> + if (error_rule) {
>>> + error = error_rule->options.inject.error;
>>> + immediately = error_rule->options.inject.immediately;
>>> +
>>> + if (error_rule->once) {
>>> + QSIMPLEQ_REMOVE(&s->active_rules, error_rule,
>>> BlkdebugRule, active_next);
>>> + remove_rule(error_rule);
>>> + }
>>> +
>>> + if (error && !immediately) {
>>> + aio_co_schedule(qemu_get_current_aio_context(),
>>> + qemu_coroutine_self());
>>> + qemu_coroutine_yield();
>>> + }
>>> +
>>> + ret = -error;
>>> }
>>> - return -error;
>>> + return ret;
>>> }
> That's a big hunk of stuff all changing at once, it might have been nice
> to do this in two passes:
>
> 1. refactor the loop to look for error rules, then
> 2. add login in for the delay rules.
>
> It looks like we change the handling of error rules just a little bit;
> before if (!rule->options.inject.error) we return 0 immediately, but now
> we actually check for `once` and remove the the rule first.
>
> I think that's probably fine; isn't it? It's not as if the error will
> show up if it wasn't there already, so that's probably an improvement.
Indeed. I should have made this more explicit in the commit, and
separating this out would have been better.
If 'once' is specified, the rule should execute just once, regardless if
it is supposed to return an error or not. Take the example where you
want the first IO to an LBA to succeed, but subsequent IOs to fail. You
could either use state transitions, or create two rules, one with
error=0, and one with error !0. (The reality is that this doesn't work
because rules aren't ordered, per se, but we'll leave that for a
different patch.)
>
> The delay rule looks fine.
>
>>> static int coroutine_fn
>>> @@ -694,6 +762,7 @@ static bool process_rule(BlockDriverState *bs,
>>> struct BlkdebugRule *rule,
>>> /* Take the action */
>>> switch (rule->action) {
>>> case ACTION_INJECT_ERROR:
>>> + case ACTION_DELAY:
>>> if (!injected) {
>>> QSIMPLEQ_INIT(&s->active_rules);
>>> injected = true;
>>> diff --git a/docs/devel/blkdebug.txt b/docs/devel/blkdebug.txt
>>> index 43d8e8f..1befcf8 100644
>>> --- a/docs/devel/blkdebug.txt
>>> +++ 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.
>>> +
>>> Invoke QEMU as follows:
>>> $ qemu-system-x86_64
>>> @@ -60,21 +68,27 @@ Rules support the following attributes:
>>> rule to match. See the "State transitions" section for
>>> information
>>> on states.
>>> - errno - the numeric errno value to return when a request matches
>>> this rule.
>>> - The errno values depend on the host since the numeric
>>> values are not
>>> - standarized in the POSIX specification.
>>> -
>>> sector - (optional) a sector number that the request must overlap
>>> in order to
>>> match this rule
>>> once - (optional, default "off") only execute this action on the
>>> first
>>> matching request
>>> +Error injection rules support the following attributes:
> The section rewrite here actually makes it a little less clear which
> rules support which attributes, it might be time for a heavier rewrite.
>
> This is OK for now, though, I think...
>
>>> +
>>> + errno - the numeric errno value to return when a request matches
>>> this rule.
>>> + The errno values depend on the host since the numeric
>>> values are not
>>> + standarized in the POSIX specification.
>>> +
>>> immediately - (optional, default "off") return a NULL BlockAIOCB
>>> pointer and fail without an errno instead. This
>>> exercises the code path where BlockAIOCB fails and the
>>> caller's BlockCompletionFunc is not invoked.
>>> +Delay rules support the following attribute:
>>> +
>>> + latency - the delay to add to an I/O request, in microseconds.
>>> +
>>> Events
>>> ------
>>> Block drivers provide information about the type of I/O request they
>>> are about
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 4c7a37a..819e3f9 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -2941,11 +2941,11 @@
>>> 'refblock_alloc_write_blocks',
>>> 'refblock_alloc_write_table',
>>> 'refblock_alloc_switch_table', 'cluster_alloc',
>>> 'cluster_alloc_bytes', 'cluster_free', 'flush_to_os',
>>> - 'flush_to_disk', 'pwritev_rmw_head',
>>> 'pwritev_rmw_after_head',
>>> - 'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
>>> - 'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
>>> - 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
>>> - 'cor_write'] }
>>> + 'flush_to_disk', 'preadv', 'pwritev_rmw_head',
>>> + 'pwritev_rmw_after_head', 'pwritev_rmw_tail',
>>> + 'pwritev_rmw_after_tail', 'pwritev', 'pwritev_zero',
>>> 'pwritev_done',
>>> + 'empty_image_prepare', 'l1_shrink_write_table',
>>> + 'l1_shrink_free_l2_clusters', 'cor_write'] }
> Is this an unrelated change that snuck in? If it's intentional, you
> ought to justify it in a standalone commit with a commit message
> containing the rationale.
Thanks for the catch.
>
>>> ##
>>> # @BlkdebugInjectErrorOptions:
>>> @@ -2980,6 +2980,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.0
> 3.1, sorry :)
Ack.
>
>>> +##
>>> +{ 'struct': 'BlkdebugDelayOptions',
>>> + 'data': { 'event': 'BlkdebugEvent',
>>> + '*state': 'int',
>>> + '*latency': 'int',
>>> + '*sector': 'int',
>>> + '*once': 'bool' } }
>>> +
>>> +##
>>> # @BlkdebugSetStateOptions:
>>> #
>>> # Describes a single state-change event for blkdebug.
>>> @@ -3049,6 +3077,7 @@
>>> '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
>>> '*opt-discard': 'int32', '*max-discard': 'int32',
>>> '*inject-error': ['BlkdebugInjectErrorOptions'],
>>> + '*delay': ['BlkdebugDelayOptions'],
>>> '*set-state': ['BlkdebugSetStateOptions'] } }
>>> ##
>>> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
>>> index 48b4955..3d0610c 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.delay.event=write_aio,file.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.delay.event=write_aio,file.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
>>> +echo
>>> +echo "=== Testing blkdebug on existing block device ==="
>>> +echo
>>> +
>>> +run_qemu <<EOF
>>> +{ "execute": "qmp_capabilities" }
>>> +{ "execute": "blockdev-add",
>>> + "arguments": {
>>> + "node-name": "drive0",
>>> + "driver": "file",
>>> + "filename": "$TEST_IMG"
>>> + }
>>> +}
>>> +{ "execute": "blockdev-add",
>>> + "arguments": {
>>> + "driver": "$IMGFMT",
>>> + "node-name": "drive0-debug",
>>> + "file": {
>>> + "driver": "blkdebug",
>>> + "image": "drive0",
>>> + "delay": [{
>>> + "event": "write_aio",
>>> + "latency": 10000
>>> + }]
>>> + }
>>> + }
>>> +}
>>> +{ "execute": "human-monitor-command",
>>> + "arguments": {
>>> + "command-line": 'qemu-io drive0-debug "aio_write 0 512"'
>>> + }
>>> +}
>>> +{ "execute": "human-monitor-command",
>>> + "arguments": {
>>> + "command-line": 'qemu-io drive0-debug "aio_read 0 512"'
>>> + }
>>> +}
>>> +{ "execute": "quit" }
>>> +EOF
>>> +
>>> +echo
>>> echo "=== Testing blkdebug on existing block device ==="
>>> echo
>>> 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)
>>> +
>>> +=== Testing blkdebug on existing block device ===
>>> +
>>> +Testing:
>>> +QMP_VERSION
>>> +{"return": {}}
>>> +{"return": {}}
>>> +{"return": {}}
>>> +wrote 512/512 bytes at offset 0
>>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>> +{"return": ""}
>>> +read 512/512 bytes at offset 0
>>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>> +{"return": ""}
>>> +{"return": {}}
>>> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
>>> "event": "SHUTDOWN", "data": {"guest": false}}
>>> +
>>> +
>>> === Testing blkdebug on existing block device ===
>>> Testing:
>>
> Thanks, this seems like a good addition to have.
>
> --js
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-09-14 21:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-24 5:06 [Qemu-devel] [PATCH] blkdebug: Add support for latency rules Marc Olson
2018-08-24 16:11 ` Eric Blake
2018-08-28 1:34 ` Marc Olson
2018-09-05 0:24 ` [Qemu-devel] [PATCH v2] " Marc Olson
2018-09-13 16:48 ` Marc Olson
2018-09-14 20:46 ` [Qemu-devel] [Qemu-block] " John Snow
2018-09-14 21:03 ` Marc Olson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.