[Qemu-devel] [PATCH v2] blkdebug: fix "once" rule

[Qemu-devel] [PATCH v2] blkdebug: fix "once" rule

[John Snow]

Background:
  The blkdebug scripts are currently engineered so that when a debug
event occurs, a prefilter browses a master list of parsed rules for a
certain event and adds them to an "active list" of rules to be used for
the forthcoming action, provided the events and state numbers match.

  Then, once the request is received, the last active rule is used to
inject an error if certain parameters match.

  This active list is cleared every time the prefilter injects a new
rule for the first time during a debug event.

  The "once" rule currently causes the error injection, if it is
triggered, to only clear the active list. This is insufficient for
preventing future injections of the same rule.

Remedy:
  This patch /deletes/ the rule from the list that the prefilter
browses, so it is gone for good. In V2, we remove only the rule of
interest from the active list instead of allowing the "once" rule to
clear the entire list of active rules.

Impact:
  This affects iotests 026. Several ENOSPC tests that used "once" can
be seen to have output that shows multiple failure messages. After
this patch, the error messages tend to be smaller and less severe, but
the injection can still be seen to be working. I have patched the
expected output to expect the smaller error messages.

V2:
  - Remove only the offending "once" rule from the active list instead
    of clearing the entire active list.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/blkdebug.c           |  9 +++++----
 tests/qemu-iotests/026.out | 24 ++++--------------------
 2 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 9ce35cd..185695b 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -473,14 +473,15 @@ static BlockAIOCB *inject_error(BlockDriverState *bs,
     struct BlkdebugAIOCB *acb;
     QEMUBH *bh;
 
-    if (rule->options.inject.once) {
-        QSIMPLEQ_INIT(&s->active_rules);
-    }
-
     if (rule->options.inject.immediately) {
         return NULL;
     }
 
+    if (rule->options.inject.once) {
+        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
+        remove_rule(rule);
+    }
+
     acb = qemu_aio_get(&blkdebug_aiocb_info, bs, cb, opaque);
     acb->ret = -error;
 
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index 524f7ee..5e964fb 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -140,19 +140,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 5; imm: off; once: on; write
-Failed to flush the L2 table cache: Input/output error
 write failed: Input/output error
-
-127 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 5; imm: off; once: on; write -b
-Failed to flush the L2 table cache: Input/output error
 write failed: Input/output error
-
-127 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 5; imm: off; once: off; write
@@ -174,19 +168,13 @@ This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 28; imm: off; once: on; write
-Failed to flush the L2 table cache: No space left on device
 write failed: No space left on device
-
-127 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 28; imm: off; once: on; write -b
-Failed to flush the L2 table cache: No space left on device
 write failed: No space left on device
-
-127 leaked clusters were found on the image.
-This means waste of disk space, but no harm to data.
+No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 28; imm: off; once: off; write
@@ -356,13 +344,11 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_update_part; errno: 5; imm: off; once: on; write
-Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_update_part; errno: 5; imm: off; once: on; write -b
-Failed to flush the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -382,13 +368,11 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_update_part; errno: 28; imm: off; once: on; write
-Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_update_part; errno: 28; imm: off; once: on; write -b
-Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2] blkdebug: fix "once" rule

[Max Reitz]

On 2015-02-06 at 13:23, John Snow wrote:
> Background:
>    The blkdebug scripts are currently engineered so that when a debug
> event occurs, a prefilter browses a master list of parsed rules for a
> certain event and adds them to an "active list" of rules to be used for
> the forthcoming action, provided the events and state numbers match.
>
>    Then, once the request is received, the last active rule is used to
> inject an error if certain parameters match.
>
>    This active list is cleared every time the prefilter injects a new
> rule for the first time during a debug event.
>
>    The "once" rule currently causes the error injection, if it is
> triggered, to only clear the active list. This is insufficient for
> preventing future injections of the same rule.
>
> Remedy:
>    This patch /deletes/ the rule from the list that the prefilter
> browses, so it is gone for good. In V2, we remove only the rule of
> interest from the active list instead of allowing the "once" rule to
> clear the entire list of active rules.
>
> Impact:
>    This affects iotests 026. Several ENOSPC tests that used "once" can
> be seen to have output that shows multiple failure messages. After
> this patch, the error messages tend to be smaller and less severe, but
> the injection can still be seen to be working. I have patched the
> expected output to expect the smaller error messages.
>
> V2:
>    - Remove only the offending "once" rule from the active list instead
>      of clearing the entire active list.

Could you move this version information under the three dashes so it 
won't be included in the commit message?

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

Here, that is.

>   block/blkdebug.c           |  9 +++++----
>   tests/qemu-iotests/026.out | 24 ++++--------------------
>   2 files changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 9ce35cd..185695b 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -473,14 +473,15 @@ static BlockAIOCB *inject_error(BlockDriverState *bs,
>       struct BlkdebugAIOCB *acb;
>       QEMUBH *bh;
>   
> -    if (rule->options.inject.once) {
> -        QSIMPLEQ_INIT(&s->active_rules);
> -    }
> -
>       if (rule->options.inject.immediately) {
>           return NULL;
>       }
>   
> +    if (rule->options.inject.once) {
> +        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
> +        remove_rule(rule);
> +    }
> +
>       acb = qemu_aio_get(&blkdebug_aiocb_info, bs, cb, opaque);
>       acb->ret = -error;
>   

The new code itself looks good, but why did you move it after the 
immediately block? Because now, if "immediately" is set, "once" will be 
ignored:

$ ./qemu-img create -f qcow2 test.qcow2 64M; ./qemu-io -c 'aio_write 0 
64k' 
"json:{'driver':'qcow2','file':{'driver':'blkdebug','image':{'driver':'file','filename':'test.qcow2'},'inject-error':[{'event':'write_aio','once':true,'immediately':true}]}}" 

Formatting 'test.qcow2', fmt=qcow2 size=67108864 encryption=off 
cluster_size=65536 lazy_refcounts=off
aio_write failed: Input/output error
Failed to flush the L2 table cache: Input/output error
Failed to flush the refcount block cache: Input/output error

(I know, I know, I should have noticed in v1...)

Max

Re: [Qemu-devel] [PATCH v2] blkdebug: fix "once" rule

[John Snow]

On 02/06/2015 04:15 PM, Max Reitz wrote:
> On 2015-02-06 at 13:23, John Snow wrote:
>> Background:
>>    The blkdebug scripts are currently engineered so that when a debug
>> event occurs, a prefilter browses a master list of parsed rules for a
>> certain event and adds them to an "active list" of rules to be used for
>> the forthcoming action, provided the events and state numbers match.
>>
>>    Then, once the request is received, the last active rule is used to
>> inject an error if certain parameters match.
>>
>>    This active list is cleared every time the prefilter injects a new
>> rule for the first time during a debug event.
>>
>>    The "once" rule currently causes the error injection, if it is
>> triggered, to only clear the active list. This is insufficient for
>> preventing future injections of the same rule.
>>
>> Remedy:
>>    This patch /deletes/ the rule from the list that the prefilter
>> browses, so it is gone for good. In V2, we remove only the rule of
>> interest from the active list instead of allowing the "once" rule to
>> clear the entire list of active rules.
>>
>> Impact:
>>    This affects iotests 026. Several ENOSPC tests that used "once" can
>> be seen to have output that shows multiple failure messages. After
>> this patch, the error messages tend to be smaller and less severe, but
>> the injection can still be seen to be working. I have patched the
>> expected output to expect the smaller error messages.
>>
>> V2:
>>    - Remove only the offending "once" rule from the active list instead
>>      of clearing the entire active list.
>
> Could you move this version information under the three dashes so it
> won't be included in the commit message?
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>
> Here, that is.
>
>>   block/blkdebug.c           |  9 +++++----
>>   tests/qemu-iotests/026.out | 24 ++++--------------------
>>   2 files changed, 9 insertions(+), 24 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 9ce35cd..185695b 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -473,14 +473,15 @@ static BlockAIOCB *inject_error(BlockDriverState
>> *bs,
>>       struct BlkdebugAIOCB *acb;
>>       QEMUBH *bh;
>> -    if (rule->options.inject.once) {
>> -        QSIMPLEQ_INIT(&s->active_rules);
>> -    }
>> -
>>       if (rule->options.inject.immediately) {
>>           return NULL;
>>       }
>> +    if (rule->options.inject.once) {
>> +        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule,
>> active_next);
>> +        remove_rule(rule);
>> +    }
>> +
>>       acb = qemu_aio_get(&blkdebug_aiocb_info, bs, cb, opaque);
>>       acb->ret = -error;
>
> The new code itself looks good, but why did you move it after the
> immediately block? Because now, if "immediately" is set, "once" will be
> ignored:
>
> $ ./qemu-img create -f qcow2 test.qcow2 64M; ./qemu-io -c 'aio_write 0
> 64k'
> "json:{'driver':'qcow2','file':{'driver':'blkdebug','image':{'driver':'file','filename':'test.qcow2'},'inject-error':[{'event':'write_aio','once':true,'immediately':true}]}}"
>
> Formatting 'test.qcow2', fmt=qcow2 size=67108864 encryption=off
> cluster_size=65536 lazy_refcounts=off
> aio_write failed: Input/output error
> Failed to flush the L2 table cache: Input/output error
> Failed to flush the refcount block cache: Input/output error
>
> (I know, I know, I should have noticed in v1...)
>
> Max
>

augh, durr.

Because I didn't want the deletion of the rule to trigger a segfault, 
but I wasn't thinking about the interaction of both rules here. Duhhh.