From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39115) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJqGo-0007zb-CM for qemu-devel@nongnu.org; Fri, 06 Feb 2015 16:16:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJqGk-00073V-9R for qemu-devel@nongnu.org; Fri, 06 Feb 2015 16:16:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40214) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJqGk-00073P-2K for qemu-devel@nongnu.org; Fri, 06 Feb 2015 16:16:54 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t16LGqRO001163 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 6 Feb 2015 16:16:53 -0500 Message-ID: <54D52F43.30601@redhat.com> Date: Fri, 06 Feb 2015 16:16:51 -0500 From: John Snow MIME-Version: 1.0 References: <1423246984-26207-1-git-send-email-jsnow@redhat.com> <54D52EDB.2030708@redhat.com> In-Reply-To: <54D52EDB.2030708@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] blkdebug: fix "once" rule List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com 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 >> --- > > 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.