Skip to contentSkip to conversation

Separate QUORUM_REPORT_BAD events according to their node name

PATCH Superseded Alberto Garcia 2 participants 11 messages Mar 9, 2016 – Mar 11, 2016 Block layer

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#9 lore raw

PATCH 4/4 iotests: Add test for QMP event rates

In reply to 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_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
new file mode 100644
index 0000000..30bc379
@@ -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
new file mode 100644
index 0000000..ae1213e
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
tests/qemu-iotests/group +1 -0
index 47fd40c..54e7acc 100644
@@ -148,3 +148,4 @@
 143 auto quick
 144 rw auto quick
 145 auto quick
+146 rw auto quick
Signature · 1 line
2.7.0
#5 lore raw

PATCH 2/4 monitor: Separate QUORUM_REPORT_BAD events according to the node name

In reply to Alberto Garcia

The 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
index e99ca8c..c9fe862 100644
@@ -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
#7 lore raw

PATCH 3/4 monitor: Use QEMU_CLOCK_VIRTUAL for the event queue in qtest mode

In reply to Alberto Garcia

This 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
index c9fe862..d689b83 100644
@@ -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
#0 lore raw

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
#1 lore raw

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
index 11cc60b..73ff309 100644
@@ -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
Reviewed-by on v1 1/4
#2 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] quorum: Fix crash in quorum_aio_cb()

Quoted 9 lines · Alberto Garcia

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

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

Reviewed-by on v1 2/4
#6 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] monitor: Separate QUORUM_REPORT_BAD events according to the node name

In reply to Alberto Garcia
Quoted 6 lines · Alberto Garcia

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

Quoted 7 lines

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(+)
Reviewed-by on v1 3/4
#8 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 3/4] monitor: Use QEMU_CLOCK_VIRTUAL for the event queue in qtest mode

In reply to Alberto Garcia
Quoted 21 lines · Alberto Garcia

This 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 +1 -0
index c9fe862..d689b83 100644
@@ -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:

Reviewed-by on v1 4/4
#10 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] iotests: Add test for QMP event rates

In reply to Alberto Garcia
Quoted 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_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 +0 -0
new file mode 100644
index 0000000..30bc379

[...]

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"])
#3 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] quorum: Fix crash in quorum_aio_cb()

In reply to Max Reitz
Quoted 11 lines · Max Reitz
Quoted 9 lines · Alberto Garcia

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(-)
Quoted 5 lines

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.

acb->common.bs is the Quorum BDS, sacb->aiocb->bs is the child BDS.

Berto

#4 lore raw

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 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(-)
Quoted 7 lines
Quoted 5 lines

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.

acb->common.bs is the Quorum BDS, sacb->aiocb->bs is the child BDS.

You're right, thanks for explaining.

Max