blkdebug: fix one shot rule processing

Break out the more common parts of the BlkdebugRule struct, and make rule_check() more explicit about operating only on error injection types so that additional rule types can be added in the future.

Signed-off-by Marc Olson <[email protected]>

---
 block/blkdebug.c | 59 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

block/blkdebug.c +0 -0

index 327049b..7739849 100644
@@ -73,13 +73,13 @@ typedef struct BlkdebugRule {
     BlkdebugEvent event;
     int action;
     int state;

block/blkdebug.c +22 -21

         struct {
             int new_state;
         } set_state;

@@ -182,16 +182,16 @@ 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 =
+        rule->options.inject_error.error = qemu_opt_get_number(opts, "errno", EIO);
+        rule->options.inject_error.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_SET_STATE:

@@ -474,38 +474,41 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes)
 {
     BDRVBlkdebugState *s = bs->opaque;
     BlkdebugRule *rule = NULL;
+    BlkdebugRule *error_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->action == ACTION_INJECT_ERROR) {
+                error_rule = rule;
+                break;
+            }
         }
     }
 
-    if (!rule) {
-        return 0;
-    }
+    if (error_rule) {
+        immediately = error_rule->options.inject_error.immediately;
+        error = error_rule->options.inject_error.error;
 
-    immediately = rule->options.inject.immediately;
-    error = rule->options.inject.error;
+        if (error_rule->once) {
+            QSIMPLEQ_REMOVE(&s->active_rules, error_rule, BlkdebugRule, active_next);
+            remove_rule(error_rule);
+        }
 
-    if (rule->options.inject.once) {
-        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
-        remove_rule(rule);
-    }

block/blkdebug.c +1 -1

-    return -error;
+    return ret;
 }
 
 static int coroutine_fn
 

Quoted 17 lines · Marc Olson via Qemu-devel

Break out the more common parts of the BlkdebugRule struct, and make rule_check() more explicit about operating only on error injection types so that additional rule types can be added in the future.

Signed-off-by Marc Olson <[email protected]>

---
  block/blkdebug.c | 59 +++++++++++++++++++++++++++++---------------------------
  1 file changed, 31 insertions(+), 28 deletions(-)

block/blkdebug.c +0 -0

index 327049b..7739849 100644
@@ -73,13 +73,13 @@ typedef struct BlkdebugRule {
      BlkdebugEvent event;
      int action;
      int state;

block/blkdebug.c typedef struct BlkdebugRule { · line 82

Quoted 10 lines

block/blkdebug.c +3 -3

+    int once;
+    int64_t offset;
      union {
          struct {
              int error;
              int immediately;
-            int once;
-            int64_t offset;
-        } inject;
+        } inject_error;

...pulling out "once" and "offset" from inject_error (renamed inject) to shared properties. Fine, though this looks like it could use more love. Not your doing.

This adds new dead fields for set_state and suspend which will now work, but hopefully not do anything.

Quoted 77 lines

block/blkdebug.c +26 -21

          struct {
              int new_state;
          } set_state;

@@ -182,16 +182,16 @@ 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 =
+        rule->options.inject_error.error = qemu_opt_get_number(opts, "errno", EIO);
+        rule->options.inject_error.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_SET_STATE:

@@ -474,38 +474,41 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes)
  {
      BDRVBlkdebugState *s = bs->opaque;
      BlkdebugRule *rule = NULL;
+    BlkdebugRule *error_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->action == ACTION_INJECT_ERROR) {
+                error_rule = rule;
+                break;
+            }
          }
      }
 
-    if (!rule) {
-        return 0;
-    }
+    if (error_rule) {
+        immediately = error_rule->options.inject_error.immediately;
+        error = error_rule->options.inject_error.error;
 
-    immediately = rule->options.inject.immediately;
-    error = rule->options.inject.error;
+        if (error_rule->once) {
+            QSIMPLEQ_REMOVE(&s->active_rules, error_rule, BlkdebugRule, active_next);
+            remove_rule(error_rule);
+        }
 
-    if (rule->options.inject.once) {
-        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
-        remove_rule(rule);
-    }
+        if (error && !immediately) {
+            aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
+            qemu_coroutine_yield();
+        }
 

block/blkdebug.c static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes) · line 509

Quoted 5 lines

block/blkdebug.c +1 -3

-    if (error && !immediately) {
-        aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
-        qemu_coroutine_yield();
+        ret = -error;
      }

Bit messy as a diff, but it seems to check out. As a bonus we now actually check the tag of the rules we're iterating through, so that seems like an improvement.

Reviewed-by John Snow <[email protected]>

Quoted 7 lines

block/blkdebug.c +1 -1

-    return -error;
+    return ret;
  }
 
  static int coroutine_fn
 

Quoted 34 lines · John Snow

Quoted 18 lines · Marc Olson via Qemu-devel

Break out the more common parts of the BlkdebugRule struct, and make rule_check() more explicit about operating only on error injection types so that additional rule types can be added in the future.

Signed-off-by Marc Olson <[email protected]>

---
  block/blkdebug.c | 59
+++++++++++++++++++++++++++++---------------------------

  1 file changed, 31 insertions(+), 28 deletions(-)

block/blkdebug.c +0 -0

index 327049b..7739849 100644
@@ -73,13 +73,13 @@ typedef struct BlkdebugRule {
       BlkdebugEvent event;
       int action;
       int state;

block/blkdebug.c typedef struct BlkdebugRule { · line 82

Quoted 10 lines

block/blkdebug.c +3 -3

+    int once;
+    int64_t offset;
       union {
           struct {
               int error;
               int immediately;
-            int once;
-            int64_t offset;
-        } inject;
+        } inject_error;

...pulling out "once" and "offset" from inject_error (renamed inject) to shared properties. Fine, though this looks like it could use more love. Not your doing.

Quoted 2 lines

This adds new dead fields for set_state and suspend which will now work, but hopefully not do anything.

I think set_state was already there?

Quoted 79 lines

Quoted 79 lines

block/blkdebug.c +3 -0

           struct {
               int new_state;
           } set_state;

@@ -182,16 +182,16 @@ 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 =
+        rule->options.inject_error.error = qemu_opt_get_number(opts, "errno", EIO);
+        rule->options.inject_error.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_SET_STATE:

block/blkdebug.c +20 -12

@@ -474,38 +474,41 @@ static int rule_check(BlockDriverState *bs,
 uint64_t offset, uint64_t bytes)
   {
       BDRVBlkdebugState *s = bs->opaque;
       BlkdebugRule *rule = NULL;
+    BlkdebugRule *error_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->action == ACTION_INJECT_ERROR) {
+                error_rule = rule;
+                break;
+            }
           }
       }
   -    if (!rule) {
-        return 0;
-    }
+    if (error_rule) {
+        immediately = error_rule->options.inject_error.immediately;
+        error = error_rule->options.inject_error.error;
   -    immediately = rule->options.inject.immediately;
-    error = rule->options.inject.error;
+        if (error_rule->once) {
+            QSIMPLEQ_REMOVE(&s->active_rules, error_rule,
 BlkdebugRule, active_next);
+            remove_rule(error_rule);
+        }
   -    if (rule->options.inject.once) {
-        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule,
 active_next);
-        remove_rule(rule);
-    }
+        if (error && !immediately) {
+            aio_co_schedule(qemu_get_current_aio_context(),
 qemu_coroutine_self());
+            qemu_coroutine_yield();
+        }
   -    if (error && !immediately) {

Quoted 8 lines

block/blkdebug.c static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes) · line 509

Quoted 5 lines

block/blkdebug.c +1 -2

-        aio_co_schedule(qemu_get_current_aio_context(),
 qemu_coroutine_self());
-        qemu_coroutine_yield();
+        ret = -error;
       }

Bit messy as a diff, but it seems to check out. As a bonus we now actually check the tag of the rules we're iterating through, so that seems like an improvement.

Unfortunately git made a bit of a mess out of the diff.

Quoted 8 lines

Reviewed-by John Snow <[email protected]>

Quoted 5 lines

block/blkdebug.c +1 -0

   -    return -error;
+    return ret;

  }
    static int coroutine_fn

Break out the more common parts of the BlkdebugRule struct, and make rule_check() more explicit about operating only on error injection types so that additional rule types can be added in the future.

Signed-off-by Marc Olson <[email protected]>

---
 block/blkdebug.c | 59 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

Add a new rule type for blkdebug that instead of returning an error, can inject latency to an IO.

Signed-off-by Marc Olson <[email protected]>

---
 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(-)

block/blkdebug.c +22 -0

index 7739849..6b1f2d6 100644
@@ -65,6 +65,7 @@ typedef struct BlkdebugSuspendedReq {
 
 enum {
     ACTION_INJECT_ERROR,
+    ACTION_INJECT_DELAY,
     ACTION_SET_STATE,
     ACTION_SUSPEND,
 };

@@ -81,6 +82,9 @@ typedef struct BlkdebugRule {
             int immediately;
         } inject_error;
         struct {
+            int64_t latency;
+        } delay;
+        struct {
             int new_state;
         } set_state;
         struct {

@@ -123,6 +127,34 @@ static QemuOptsList inject_error_opts = {
     },
 };
 
+static QemuOptsList inject_delay_opts = {
+    .name = "inject-delay",
+    .head = QTAILQ_HEAD_INITIALIZER(inject_delay_opts.head),
+    .desc = {
+        {
+            .name = "event",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "state",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
+            .name = "latency",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
+            .name = "sector",

block/blkdebug.c +43 -4

 static QemuOptsList set_state_opts = {
     .name = "set-state",
     .head = QTAILQ_HEAD_INITIALIZER(set_state_opts.head),

@@ -145,6 +177,7 @@ static QemuOptsList set_state_opts = {
 
 static QemuOptsList *config_groups[] = {
     &inject_error_opts,
+    &inject_delay_opts,
     &set_state_opts,
     NULL
 };

@@ -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;
+        break;
+
     case ACTION_SET_STATE:
         rule->options.set_state.new_state =
             qemu_opt_get_number(opts, "new_state", 0);

@@ -226,6 +264,12 @@ static void remove_rule(BlkdebugRule *rule)
     g_free(rule);
 }
 
+static void remove_active_rule(BDRVBlkdebugState *s, BlkdebugRule *rule)
+{
+    QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
+    remove_rule(rule);
+}
+
 static int read_config(BDRVBlkdebugState *s, const char *filename,
                        QDict *options, Error **errp)
 {

@@ -264,6 +308,14 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
         goto fail;
     }
 
+    d.action = ACTION_INJECT_DELAY;
+    qemu_opts_foreach(&inject_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 +327,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
     ret = 0;
 fail:
     qemu_opts_reset(&inject_error_opts);
+    qemu_opts_reset(&inject_delay_opts);
     qemu_opts_reset(&set_state_opts);
     if (f) {
         fclose(f);

@@ -474,7 +527,8 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes)
 {
     BDRVBlkdebugState *s = bs->opaque;
     BlkdebugRule *rule = NULL;
-    BlkdebugRule *error_rule = NULL;
+    BlkdebugRule *error_rule = NULL, *delay_rule = NULL;
+    int64_t latency;
     int error;
     bool immediately;
     int ret = 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;
+            }
+
+            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) {

@@ -697,6 +767,7 @@ static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
     /* Take the action */
     switch (rule->action) {
     case ACTION_INJECT_ERROR:
+    case ACTION_INJECT_DELAY:
         if (!injected) {
             QSIMPLEQ_INIT(&s->active_rules);
             injected = true;

docs/devel/blkdebug.txt +26 -9

index 43d8e8f..1719835 100644
@@ -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 either fail or add latency to 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,24 +33,35 @@ 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
+  [inject-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.
+
+An error rule and a delay rule can overlap, and both will execute. Only one
+rule of a given type will be executed for each I/O.
+
 Invoke QEMU as follows:
 
   $ qemu-system-x86_64
         -drive if=none,cache=none,file=blkdebug:blkdebug.conf:test.img,id=drive0 \
         -device virtio-blk-pci,drive=drive0,id=virtio-blk-pci0
 
-Rules support the following attributes:
+All rules support the following attributes:
 
   event - which type of operation to match (e.g. read_aio, write_aio,
           flush_to_os, flush_to_disk).  See the "Events" section for

@@ -60,21 +71,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 additional 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 additional 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

qapi/block-core.json +9 -0

index d4fe710..72f7861 100644
@@ -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"
+#

qapi/block-core.json +3 -0

 # @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
+#
 # @set-state:       array of state-change descriptions
 #
 # Since: 2.9

@@ -3126,6 +3156,7 @@
             '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
             '*opt-discard': 'int32', '*max-discard': 'int32',
             '*inject-error': ['BlkdebugInjectErrorOptions'],
+            '*inject-delay': ['BlkdebugDelayOptions'],
             '*set-state': ['BlkdebugSetStateOptions'] } }
 
 ##

tests/qemu-iotests/071 +63 -0

index 48b4955..976f747 100755
@@ -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
+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",
+            "inject-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
 

tests/qemu-iotests/071.out +24 -0

index 1d5e28d..1952990 100644
@@ -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": ""}

Add a new rule type for blkdebug that instead of returning an error, can inject latency to an IO.

Signed-off-by Marc Olson <[email protected]>

---
 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(-)

block/blkdebug.c +0 -0

index 7739849..6b1f2d6 100644

block/blkdebug.c +2 -0

+        break;
+
     case ACTION_SET_STATE:
         rule->options.set_state.new_state =
             qemu_opt_get_number(opts, "new_state", 0);

block/blkdebug.c +14 -2

+            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) {

qapi/block-core.json +9 -0

index d4fe710..72f7861 100644
@@ -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"
+#

qapi/block-core.json +1 -0

+#
 # @set-state:       array of state-change descriptions
 #
 # Since: 2.9

tests/qemu-iotests/071 +12 -0

index 48b4955..976f747 100755
@@ -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
+

tests/qemu-iotests/071.out +3 -0

index 1d5e28d..1952990 100644
@@ -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

Quoted 16 lines · Marc Olson

Add a new rule type for blkdebug that instead of returning an error, can inject latency to an IO.

Signed-off-by Marc Olson <[email protected]>

---
  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(-)

block/blkdebug.c +0 -0

index 7739849..6b1f2d6 100644

[...]

Quoted 4 lines

block/blkdebug.c +0 -0

@@ -194,6 +227,11 @@ static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
              qemu_opt_get_bool(opts, "immediately", 0);
          break;
 

block/blkdebug.c static int add_rule(void *opaque, QemuOpts *opts, Error **errp) · line 232

Quoted 3 lines

block/blkdebug.c +3 -0

+    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.

Quoted 6 lines

block/blkdebug.c +2 -0

+        break;
+
      case ACTION_SET_STATE:
          rule->options.set_state.new_state =
              qemu_opt_get_number(opts, "new_state", 0);

[...]

block/blkdebug.c static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes) · line 546

Quoted 11 lines

block/blkdebug.c +5 -1

@@ -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.)

Quoted 30 lines

block/blkdebug.c +14 -2

+            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) {

[...]

Quoted 20 lines

qapi/block-core.json +12 -0

index d4fe710..72f7861 100644
@@ -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

qapi/block-core.json line 3078

Quoted 7 lines

qapi/block-core.json +7 -0

+#               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...

qapi/block-core.json line 3083

Quoted 6 lines

qapi/block-core.json +5 -0

+##
+{ 'struct': 'BlkdebugDelayOptions',
+  'data': { 'event': 'BlkdebugEvent',
+            '*state': 'int',
+            '*latency': 'int',

I'd make this option mandatory.

qapi/block-core.json line 3146

Quoted 12 lines

qapi/block-core.json +5 -0

+            '*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)".

Quoted 4 lines

qapi/block-core.json +1 -0

+#
  # @set-state:       array of state-change descriptions
  #
  # Since: 2.9

[...]

Quoted 20 lines

tests/qemu-iotests/071 +12 -0

index 48b4955..976f747 100755
@@ -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
+

tests/qemu-iotests/071 $QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.inject-error.event · line 124

Quoted 10 lines

tests/qemu-iotests/071 +10 -0

+$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.

[...]

Quoted 11 lines

tests/qemu-iotests/071.out +3 -0

index 1d5e28d..1952990 100644
@@ -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

tests/qemu-iotests/071.out read failed: Input/output error · line 51

Quoted 10 lines

tests/qemu-iotests/071.out +10 -0

+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

Quoted 1 line · Max Reitz

On 12.11.18 08:06, Marc Olson wrote:

Quoted 20 lines

Quoted 20 lines

qapi/block-core.json +13 -0

index d4fe710..72f7861 100644
@@ -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

Quoted 8 lines

qapi/block-core.json line 3078

Quoted 7 lines

"any

qapi/block-core.json +6 -0

+#               sector"
+#
+# @once:        disables further events after this one has been
+#               triggered; defaults to false
+#
+# Since: 3.1

Well, 4.0 now, sorry...

Baking version numbers into code is downright silly. Every single version of this patch has made a comment along the lines of, "oops, it didn't get reviewed in time for the next version bump, so you have to resubmit." With a fast moving project, this is nonsense. If you're looking at the code, you should have access to the git history as well to determine the version.

Quoted 39 lines

Quoted 28 lines

tests/qemu-iotests/071 +0 -0

index 48b4955..976f747 100755
@@ -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

tests/qemu-iotests/071 +4 -0

+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

tests/qemu-iotests/071 +6 -0

+
+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

tests/qemu-iotests/071 $QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.inject-error.event · line 124

Quoted 6 lines

tests/qemu-iotests/071 +6 -0

+
+# 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.

Quoted 1 line

I leave it to you whether you'd like to go through that pain.

There's not a great way to prove it without doing a lot of parsing changes in testing. I'll consider an update to this patch, but I think this series has carried on long enough.

Quoted 2 lines · Marc Olson

Quoted 1 line · Max Reitz

On 12.11.18 08:06, Marc Olson wrote:

[...]

Quoted 28 lines

Quoted 20 lines

Quoted 20 lines

qapi/block-core.json +13 -0

index d4fe710..72f7861 100644
@@ -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

Quoted 8 lines

qapi/block-core.json line 3078

Quoted 7 lines

"any

qapi/block-core.json +6 -0

+#               sector"
+#
+# @once:        disables further events after this one has been
+#               triggered; defaults to false
+#
+# Since: 3.1

Well, 4.0 now, sorry...

Quoted 6 lines

Baking version numbers into code is downright silly. Every single version of this patch has made a comment along the lines of, "oops, it didn't get reviewed in time for the next version bump, so you have to resubmit." With a fast moving project, this is nonsense. If you're looking at the code, you should have access to the git history as well to determine the version.

True, but these comments are used to generate documentation (e.g. docs/interopt/qemu-qmp-ref.7 in the build directory). So they are used by people who don't have access to the git history.

It might be possible to generate that information from git-blame when generating the documentation, but how would trivial fixes be handled that are no functional changes? For instance, it seems difficult to me to distinguish between a spelling change for some parameter description and a change in behavior.

(Then again, we shouldn't have such behavioral changes, hm.)

To me personally, the easiest thing would seem to be some convention to write ***INSERT VERSION HERE*** into the code and then the maintainer can just find an replace all instances of that when applying the patches. But that sounds a bit silly...

(I don't have an issue with adjusting the version numbers myself as they are, but it's just as hard for me to find and replace all of them as it is for you. And since you're probably going to send a v4 anyway...)

In the end, it's up to the QAPI maintainers.

[...]

Quoted 40 lines

Quoted 40 lines

Quoted 28 lines

tests/qemu-iotests/071 +0 -0

index 48b4955..976f747 100755
@@ -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

tests/qemu-iotests/071 +4 -0

+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

tests/qemu-iotests/071 +6 -0

+
+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

tests/qemu-iotests/071 $QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.inject-error.event · line 124

Quoted 6 lines

tests/qemu-iotests/071 +6 -0

+
+# 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.

Quoted 3 lines

There's not a great way to prove it without doing a lot of parsing changes in testing. I'll consider an update to this patch, but I think this series has carried on long enough.

I agree in this instance, but I'd like to note still that "this series has carried on long enough" is not an argument to merge bad code (or something incomplete without promise of a follow-up). This is just a test for blkdebug, though, so it's OK (with the comment fixed, because it doesn't prove anything).

(I'm sorry for not having looked at this series for so long, but qemu is not my own, so it isn't like I could pay for my wrong by accepting something incomplete -- if it were more important than this single test case.)

Also, we do support Python for iotests, and parsing should be simpler there. Since blkdebug is just a debugging driver, I'm fine with not knowing whether its features work, though.

Maybe I'll write the test, that would kind of pay for my wrong...

Quoted 56 lines · Max Reitz

Quoted 2 lines · Marc Olson

Quoted 1 line · Max Reitz

On 12.11.18 08:06, Marc Olson wrote:

[...]

Quoted 28 lines

Quoted 20 lines

Quoted 20 lines

qapi/block-core.json +13 -0

index d4fe710..72f7861 100644
@@ -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

Quoted 8 lines

qapi/block-core.json line 3078

Quoted 7 lines

"any

qapi/block-core.json +6 -0

+#               sector"
+#
+# @once:        disables further events after this one has been
+#               triggered; defaults to false
+#
+# Since: 3.1

Well, 4.0 now, sorry...

Quoted 6 lines

Baking version numbers into code is downright silly. Every single version of this patch has made a comment along the lines of, "oops, it didn't get reviewed in time for the next version bump, so you have to resubmit." With a fast moving project, this is nonsense. If you're looking at the code, you should have access to the git history as well to determine the version.

True, but these comments are used to generate documentation (e.g. docs/interopt/qemu-qmp-ref.7 in the build directory).  So they are used by people who don't have access to the git history.

It might be possible to generate that information from git-blame when generating the documentation, but how would trivial fixes be handled that are no functional changes?  For instance, it seems difficult to me to distinguish between a spelling change for some parameter description and a change in behavior.

(Then again, we shouldn't have such behavioral changes, hm.)

To me personally, the easiest thing would seem to be some convention to write ***INSERT VERSION HERE*** into the code and then the maintainer can just find an replace all instances of that when applying the patches.  But that sounds a bit silly...

Quoted 5 lines

(I don't have an issue with adjusting the version numbers myself as they are, but it's just as hard for me to find and replace all of them as it is for you.  And since you're probably going to send a v4 anyway...)

In the end, it's up to the QAPI maintainers.

IFF I submit by the end of the week, what version number shall I choose?

Quoted 45 lines

[...]

Quoted 43 lines

Quoted 43 lines

Quoted 30 lines

tests/qemu-iotests/071 +0 -0

index 48b4955..976f747 100755
@@ -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

tests/qemu-iotests/071 +4 -0

+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

tests/qemu-iotests/071 +6 -0

+
+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

tests/qemu-iotests/071 $QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.inject-error.event · line 124

Quoted 6 lines

tests/qemu-iotests/071 +6 -0

+
+# 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.

Quoted 8 lines

Quoted 3 lines

There's not a great way to prove it without doing a lot of parsing changes in testing. I'll consider an update to this patch, but I think this series has carried on long enough.

I agree in this instance, but I'd like to note still that "this series has carried on long enough" is not an argument to merge bad code (or something incomplete without promise of a follow-up).  This is just a test for blkdebug, though, so it's OK (with the comment fixed, because it doesn't prove anything).

I wasn't implying that it's ok to merge bad code, but that at some point we have to just paint the shed a color and move on. At the risk of excess drama, at what point does a small addition become a complete tear-down and rebuild?

Quoted 5 lines

(I'm sorry for not having looked at this series for so long, but qemu is not my own, so it isn't like I could pay for my wrong by accepting something incomplete -- if it were more important than this single test case.)

Quoted 5 lines

Also, we do support Python for iotests, and parsing should be simpler there.  Since blkdebug is just a debugging driver, I'm fine with not knowing whether its features work, though.

Maybe I'll write the test, that would kind of pay for my wrong...

I think the real fix is to make QMP support async IO, but if you'd like to do more parsing, that's fine too.

Quoted 39 lines · Max Reitz

Quoted 2 lines · Marc Olson

Quoted 1 line · Max Reitz

On 12.11.18 08:06, Marc Olson wrote:

[...]

Quoted 34 lines

Quoted 20 lines

Quoted 20 lines

qapi/block-core.json +13 -0

index d4fe710..72f7861 100644
@@ -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

Quoted 8 lines

qapi/block-core.json line 3078

Quoted 7 lines

"any

qapi/block-core.json +6 -0

+#               sector"
+#
+# @once:        disables further events after this one has been
+#               triggered; defaults to false
+#
+# Since: 3.1

Well, 4.0 now, sorry...

Baking version numbers into code is downright silly. Every single version of this patch has made a comment along the lines of, "oops, it didn't get reviewed in time for the next version bump, so you have to resubmit." With a fast moving project, this is nonsense. If you're looking at the code, you should have access to the git history as well to determine the version.

Quoted 3 lines

True, but these comments are used to generate documentation (e.g. docs/interopt/qemu-qmp-ref.7 in the build directory). So they are used by people who don't have access to the git history.

Quoted 7 lines

It might be possible to generate that information from git-blame when generating the documentation, but how would trivial fixes be handled that are no functional changes? For instance, it seems difficult to me to distinguish between a spelling change for some parameter description and a change in behavior.

(Then again, we shouldn't have such behavioral changes, hm.)

Quoted 4 lines

To me personally, the easiest thing would seem to be some convention to write ***INSERT VERSION HERE*** into the code and then the maintainer can just find an replace all instances of that when applying the patches. But that sounds a bit silly...

Quoted 5 lines

(I don't have an issue with adjusting the version numbers myself as they are, but it's just as hard for me to find and replace all of them as it is for you. And since you're probably going to send a v4 anyway...)

In the end, it's up to the QAPI maintainers.

IFF I submit by the end of the week, what version number shall I choose?

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 once set to true, and one with a non-zero error.

Signed-off-by Marc Olson <[email protected]>

---
 block/blkdebug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

block/blkdebug.c +2 -2

index 0759452..327049b 100644
@@ -488,7 +488,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes)
         }
     }
 
-    if (!rule || !rule->options.inject.error) {
+    if (!rule) {
         return 0;
     }
 

@@ -500,7 +500,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes)
         remove_rule(rule);
     }
 
-    if (!immediately) {
+    if (error && !immediately) {
         aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
         qemu_coroutine_yield();
     }

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 once set to true, and one with a non-zero error.

Signed-off-by Marc Olson <[email protected]>

---
 block/blkdebug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

block/blkdebug.c +0 -0

index 0759452..327049b 100644

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 once set to true, and one with a non-zero error.

Signed-off-by Marc Olson <[email protected]>

---
 block/blkdebug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)