Separate QUORUM_REPORT_BAD events according to their node name
PATCH 4/4 iotests: Add test for QMP event rates
In reply to Alberto GarciaThis test verifies that the rate-limited QMP events are emitted at a maximum rate of 1 per second as defined in monitor_qapi_event_conf in monitor.c
It also checks that QUORUM_REPORT_BAD events generated from different nodes are kept in separate queues so they don't mask each other.
--- tests/qemu-iotests/146 | 129 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/146.out | 5 ++ tests/qemu-iotests/group | 1 + 3 files changed, 135 insertions(+) create mode 100644 tests/qemu-iotests/146 create mode 100644 tests/qemu-iotests/146.out
tests/qemu-iotests/146 new file +129 -0
@@ -0,0 +1,129 @@ +#!/usr/bin/env python +# +# Test the rate limit of QMP events +# +# Copyright (C) 2016 Igalia, S.L. +# Author: Alberto Garcia <[email protected]> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +import os +import iotests + +imgs = (os.path.join(iotests.test_dir, 'quorum0.img'), + os.path.join(iotests.test_dir, 'quorum1.img'), + os.path.join(iotests.test_dir, 'quorum2.img')) + +img_conf = (os.path.join(iotests.test_dir, 'quorum0.conf'), + os.path.join(iotests.test_dir, 'quorum1.conf'), + os.path.join(iotests.test_dir, 'quorum2.conf')) + +event_rate = 1000000000 +sector_size = 512 +offset = 10 + +class TestQuorumEvents(iotests.QMPTestCase): + + def create_blkdebug_file(self, blkdebug_file, bad_sector): + file = open(blkdebug_file, 'w') + file.write(''' +[inject-error] +event = "read_aio" +errno = "5" +sector = "%d" +''' % bad_sector) + file.close() + + def setUp(self): + driveopts = ['driver=quorum', 'vote-threshold=2'] + for i in range(len(imgs)): + iotests.qemu_img('create', '-f', iotests.imgfmt, imgs[i], '1M') + self.create_blkdebug_file(img_conf[i], i + offset) + driveopts.append('children.%d.driver=%s' % (i, iotests.imgfmt)) + driveopts.append('children.%d.file.driver=blkdebug' % i) + driveopts.append('children.%d.file.config=%s' % (i, img_conf[i])) + driveopts.append('children.%d.file.image.filename=%s' % (i, imgs[i])) + driveopts.append('children.%d.node-name=img%d' % (i, i)) + self.vm = iotests.VM() + self.vm.add_drive(None, opts = ','.join(driveopts)) + self.vm.launch() + + def tearDown(self): + self.vm.shutdown() + for i in range(len(imgs)): + os.remove(imgs[i]) + os.remove(img_conf[i]) + + def do_check_event(self, node, sector = 0): + if node == None: + self.assertEqual(self.vm.get_qmp_event(), None) + return + + for event in self.vm.get_qmp_events(wait=True): + if event['event'] == 'QUORUM_REPORT_BAD': + self.assert_qmp(event, 'data/node-name', node) + self.assert_qmp(event, 'data/sector-num', sector) + + def testQuorum(self): + if not 'quorum' in iotests.qemu_img_pipe('--help'): + return + + # Generate an error and get an event + self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % + (offset * sector_size, sector_size)) + self.vm.qtest("clock_step 10") + self.do_check_event('img0', offset) + + # I/O errors in the same child: only one event is emitted + delay = 10 + for i in range(3): + self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % + (offset * sector_size, sector_size)) + self.vm.qtest("clock_step %d" % delay) + self.do_check_event(None) + + # Wait enough so the event is finally emitted + self.vm.qtest("clock_step %d" % (2 * event_rate)) + self.do_check_event('img0', offset) + + # I/O errors in the same child: all events are emitted + delay = 2 * event_rate + for i in range(3): + self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % + (offset * sector_size, sector_size)) + self.vm.qtest("clock_step %d" % delay) + self.do_check_event('img0', offset) + + # I/O errors in different children: all events are emitted + delay = 10 + for i in range(len(imgs)): + self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % + ((offset + i) * sector_size, sector_size)) + self.vm.qtest("clock_step %d" % delay) + self.do_check_event('img%d' % i, offset + i) + + # I/O errors in different children: all events are emitted + delay = 2 * event_rate + for i in range(len(imgs)): + self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % + ((offset + i) * sector_size, sector_size)) + self.vm.qtest("clock_step %d" % delay) + self.do_check_event('img%d' % i, offset + i) + + # No more pending events + self.do_check_event(None) + +if __name__ == '__main__': + iotests.main(supported_fmts=["raw"])
tests/qemu-iotests/146.out new file +5 -0
@@ -0,0 +1,5 @@ +. +---------------------------------------------------------------------- +Ran 1 tests + +OK
tests/qemu-iotests/group +1 -0
@@ -148,3 +148,4 @@ 143 auto quick 144 rw auto quick 145 auto quick +146 rw auto quick
Signature · 1 line
2.7.0
PATCH 2/4 monitor: Separate QUORUM_REPORT_BAD events according to the node name
In reply to Alberto GarciaThe QUORUM_REPORT_BAD event is emitted whenever there's an I/O error in a child of a Quorum device. This event is emitted at a maximum rate of 1 per second. This means that an error in one of the children will mask errors in the other children if they happen within the same 1 second interval.
This patch modifies qapi_event_throttle_equal() so QUORUM_REPORT_BAD events are kept separately if they come from different children.
--- monitor.c | 9 +++++++++ 1 file changed, 9 insertions(+)
monitor.c +9 -0
@@ -572,6 +572,10 @@ static unsigned int qapi_event_throttle_hash(const void *key) hash +=g_str_hash (qdict_get_str (evstate->data,"id" )); } +if (evstate->event ==QAPI_EVENT_QUORUM_REPORT_BAD ) { + hash +=g_str_hash (qdict_get_str (evstate->data,"node-name" )); + } +return hash; }
@@ -589,6 +593,11 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b) qdict_get_str(evb->data, "id")); } +if (eva->event ==QAPI_EVENT_QUORUM_REPORT_BAD ) { +return !strcmp (qdict_get_str (eva->data,"node-name" ), +qdict_get_str (evb->data,"node-name" )); + } +return TRUE; }
Signature · 1 line
2.7.0
PATCH 3/4 monitor: Use QEMU_CLOCK_VIRTUAL for the event queue in qtest mode
In reply to Alberto GarciaThis allows us to perform tests on the monitor queues to verify that the rate limits are enforced.
--- monitor.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
monitor.c +10 -3
@@ -76,6 +76,7 @@#include "qapi-event.h" #include "qmp-introspect.h" #include "sysemu/block-backend.h" +#include "sysemu/qtest.h" /* for hmp_info_irq/pic */ #if defined(TARGET_SPARC )
@@ -232,6 +233,8 @@ static const mon_cmd_t qmp_cmds[];Monitor *cur_mon; +static QEMUClockType clock_type =QEMU_CLOCK_REALTIME ; +static void monitor_command_cb (void *opaque,const char *cmdline,void *readline_opaque);
@@ -513,7 +516,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)* monitor_qapi_event_handler() in evconf->rate ns. Any * events arriving before then will be delayed until then. */ -int64_t now =qemu_clock_get_ns (QEMU_CLOCK_REALTIME ); +int64_t now =qemu_clock_get_ns (clock_type);monitor_qapi_event_emit (event, qdict);
@@ -522,7 +525,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp) evstate->data = data;QINCREF (evstate->data); evstate->qdict =NULL ; - evstate->timer =timer_new_ns (QEMU_CLOCK_REALTIME , + evstate->timer =timer_new_ns (clock_type, monitor_qapi_event_handler, evstate);g_hash_table_add (monitor_qapi_event_state, evstate);
@@ -547,7 +550,7 @@ static void monitor_qapi_event_handler(void *opaque)qemu_mutex_lock (&monitor_lock);if (evstate->qdict) { -int64_t now =qemu_clock_get_ns (QEMU_CLOCK_REALTIME ); +int64_t now =qemu_clock_get_ns (clock_type);monitor_qapi_event_emit (evstate->event, evstate->qdict);QDECREF (evstate->qdict);
@@ -603,6 +606,10 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)static void monitor_qapi_event_init (void ) { +if (qtest_enabled ()) { + clock_type =QEMU_CLOCK_VIRTUAL ; + } + monitor_qapi_event_state =g_hash_table_new (qapi_event_throttle_hash, qapi_event_throttle_equal);qmp_event_set_func_emit (monitor_qapi_event_queue);
Signature · 1 line
2.7.0
cover Separate QUORUM_REPORT_BAD events according to their node name
Hi,
this was proposed by Eric in a recent email, but I'll summarize it here:
QUORUM_REPORT_BAD events are limited to a maximum rate of 1 per second. While this is not a problem in itself, this means that an error in one a Quorum child will mask errors in the other children if they happen within the same 1 second interval.
This series fixes that problem by separating these events in different queues if they come from different nodes. Once we add the 'type' field to QUORUM_REPORT_BAD we will also be able to classify them according to the type if we want.
In addition to the above, this series also fixes a crash that happens if there's an I/O error in one of the children. This is serious enough so I'll send the patch to fix this crash to qemu-stable as well.
Regards,
Beto
Alberto Garcia (4):
quorum: Fix crash in quorum_aio_cb()
monitor: Separate QUORUM_REPORT_BAD events according to the node name
monitor: Use QEMU_CLOCK_VIRTUAL for the event queue in qtest mode
iotests: Add test for QMP event rates
block/quorum.c | 12 +++-- monitor.c | 22 ++++++-- tests/qemu-iotests/146 | 129 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/146.out | 5 ++ tests/qemu-iotests/group | 1 + 5 files changed, 161 insertions(+), 8 deletions(-) create mode 100644 tests/qemu-iotests/146 create mode 100644 tests/qemu-iotests/146.out
Signature · 1 line
2.7.0
PATCH 1/4 quorum: Fix crash in quorum_aio_cb()
quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's an I/O error in a Quorum child. However sacb->aiocb must be correctly initialized for this to happen. read_quorum_children() and read_fifo_child() are not doing this, which results in a QEMU crash.
--- block/quorum.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
block/quorum.c +7 -5
@@ -648,8 +648,9 @@ static BlockAIOCB *read_quorum_children(QuorumAIOCB *acb) }for (i =0 ; i < s->num_children; i++) { -bdrv_aio_readv (s->children[i]->bs, acb->sector_num, &acb->qcrs[i].qiov, - acb->nb_sectors, quorum_aio_cb, &acb->qcrs[i]); + acb->qcrs[i].aiocb =bdrv_aio_readv (s->children[i]->bs, acb->sector_num, + &acb->qcrs[i].qiov, acb->nb_sectors, + quorum_aio_cb, &acb->qcrs[i]); }return &acb->common;
@@ -664,9 +665,10 @@ static BlockAIOCB *read_fifo_child(QuorumAIOCB *acb)qemu_iovec_init (&acb->qcrs[acb->child_iter].qiov, acb->qiov->niov);qemu_iovec_clone (&acb->qcrs[acb->child_iter].qiov, acb->qiov, acb->qcrs[acb->child_iter].buf); -bdrv_aio_readv (s->children[acb->child_iter]->bs, acb->sector_num, - &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors, - quorum_aio_cb, &acb->qcrs[acb->child_iter]); + acb->qcrs[acb->child_iter].aiocb = +bdrv_aio_readv (s->children[acb->child_iter]->bs, acb->sector_num, + &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors, + quorum_aio_cb, &acb->qcrs[acb->child_iter]);return &acb->common; }
Signature · 1 line
2.7.0
Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] quorum: Fix crash in quorum_aio_cb()
Quoted 9 lines · Alberto Garcia
quorum_aio_cb()emits theQUORUM_REPORT_BADevent if there's an I/O error in a Quorum child. Howeversacb->aiocbmust be correctly initialized for this to happen.read_quorum_children()andread_fifo_child()are not doing this, which results in a QEMU crash.Signed-off-by Alberto Garcia <[email protected]>--- block/quorum.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Although I'm wondering whether we could have just used acb->common.bs instead of sacb->aiocb->bs in quorum_aio_cb(). I guess that sacb->aiocb is supposed to be equal to &acb->common.
Max
Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] monitor: Separate QUORUM_REPORT_BAD events according to the node name
In reply to Alberto GarciaQuoted 6 lines · Alberto Garcia
The
QUORUM_REPORT_BADevent is emitted whenever there's an I/O error in a child of a Quorum device. This event is emitted at a maximum rate of 1 per second. This means that an error in one of the children will mask errors in the other children if they happen within the same 1 second interval.
Quoted 7 lines
This patch modifies
qapi_event_throttle_equal()soQUORUM_REPORT_BADevents are kept separately if they come from different children.Signed-off-by Alberto Garcia <[email protected]>--- monitor.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Re: [Qemu-devel] [Qemu-block] [PATCH 3/4] monitor: Use QEMU_CLOCK_VIRTUAL for the event queue in qtest mode
In reply to Alberto GarciaQuoted 21 lines · Alberto Garcia
This allows us to perform tests on the monitor queues to verify that the rate limits are enforced.
Signed-off-by Alberto Garcia <[email protected]>--- monitor.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)monitor.c +1
-0@@ -76,6 +76,7 @@#include "qapi-event.h" #include "qmp-introspect.h" #include "sysemu/block-backend.h" +#include "sysemu/qtest.h" /* for hmp_info_irq/pic */ #if defined(TARGET_SPARC )
monitor.c static const mon_cmd_t qmp_cmds[]; · line 236
Quoted 4 lines
monitor.c +1
-0@@ -232,6 +233,8 @@ static const mon_cmd_t qmp_cmds[];Monitor *cur_mon; +static QEMUClockType clock_type =QEMU_CLOCK_REALTIME ;
Maybe event_clock_type would be a better name.
Regardless:
Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] iotests: Add test for QMP event rates
In reply to Alberto GarciaQuoted 21 lines · Alberto Garcia
This test verifies that the rate-limited QMP events are emitted at a maximum rate of 1 per second as defined in
monitor_qapi_event_confinmonitor.cIt also checks that
QUORUM_REPORT_BADevents generated from different nodes are kept in separate queues so they don't mask each other.Signed-off-by Alberto Garcia <[email protected]>--- tests/qemu-iotests/146 | 129 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/146.out | 5 ++ tests/qemu-iotests/group | 1 + 3 files changed, 135 insertions(+) create mode 100644 tests/qemu-iotests/146 create mode 100644 tests/qemu-iotests/146.outtests/qemu-iotests/146 new file +0
-0
[...]
tests/qemu-iotests/146 line 124
Quoted 8 lines
tests/qemu-iotests/146 new file +8
-0+ # I/O errors in different children: all events are emitted + delay = 2 * event_rate + for i in range(len(imgs)): + self.vm.hmp_qemu_io("drive0", "aio_read %d %d" % + ((offset + i) * sector_size, sector_size)) + self.vm.qtest("clock_step %d" % delay) + self.do_check_event('img%d' % i, offset + i) +
A self.vm.qtest("clock_step %d" % (2 * event_rate)) may be useful here, but it's not necessary.
Quoted 5 lines
tests/qemu-iotests/146 new file +5
-0+ # No more pending events + self.do_check_event(None) + +if __name__ == '__main__': + iotests.main(supported_fmts=["raw"])
Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] quorum: Fix crash in quorum_aio_cb()
In reply to Max ReitzQuoted 11 lines · Max Reitz
Quoted 9 lines · Alberto Garcia
quorum_aio_cb()emits theQUORUM_REPORT_BADevent if there's an I/O error in a Quorum child. Howeversacb->aiocbmust be correctly initialized for this to happen.read_quorum_children()andread_fifo_child()are not doing this, which results in a QEMU crash.Signed-off-by Alberto Garcia <[email protected]>--- block/quorum.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Quoted 5 lines
Reviewed-by Max Reitz <[email protected]>Although I'm wondering whether we could have just used
acb->common.bsinstead ofsacb->aiocb->bsinquorum_aio_cb(). I guess thatsacb->aiocbis supposed to be equal to&acb->common.
acb->common.bs is the Quorum BDS, sacb->aiocb->bs is the child BDS.
Berto
Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] quorum: Fix crash in quorum_aio_cb()
Quoted 12 lines · Alberto Garcia
Quoted 11 lines · Max Reitz
Quoted 9 lines · Alberto Garcia
quorum_aio_cb()emits theQUORUM_REPORT_BADevent if there's an I/O error in a Quorum child. Howeversacb->aiocbmust be correctly initialized for this to happen.read_quorum_children()andread_fifo_child()are not doing this, which results in a QEMU crash.Signed-off-by Alberto Garcia <[email protected]>--- block/quorum.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Quoted 7 lines
Quoted 5 lines
Reviewed-by Max Reitz <[email protected]>Although I'm wondering whether we could have just used
acb->common.bsinstead ofsacb->aiocb->bsinquorum_aio_cb(). I guess thatsacb->aiocbis supposed to be equal to&acb->common.
acb->common.bsis the Quorum BDS,sacb->aiocb->bsis the child BDS.
You're right, thanks for explaining.
Max