Skip to contentSkip to conversation

Separate QUORUM_REPORT_BAD events according to their node name

PATCH In review Alberto Garcia 3 participants 8 messages Mar 10, 2016 – Mar 14, 2016 Block layer

lore
Share & tools
lore
#0 lore raw

v2 cover Separate QUORUM_REPORT_BAD events according to their node name

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,

Berto

v2:
- Patch 3: Rename clock_type to event_clock_type [Max]

v1: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02161.html
- Original version

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 v2 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
#2 lore raw

PATCH v2 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
Reviewed-by on v2 2/4
#3 lore raw

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

Quoted 20 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.

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 +0 -0
index e99ca8c..c9fe862 100644

monitor.c static unsigned int qapi_event_throttle_hash(const void *key) · line 576

Quoted 5 lines
monitor.c +2 -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"));

Is ^= any better than += when computing hashes, so that carry bits aren't weakening the distribution of bits? But as long as the computation is consistent, I'm not too worried, since you were copying the line above.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#4 lore raw

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

Quoted 11 lines · Eric Blake

monitor.c static unsigned int qapi_event_throttle_hash(const void *key) · line 576

Quoted 6 lines
monitor.c +2 -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"));

Is ^= any better than += when computing hashes, so that carry bits aren't weakening the distribution of bits? But as long as the computation is consistent, I'm not too worried, since you were copying the line above.

I think the chances of having collisions here are so low (and their effect so negligible) that I wouldn't worry too much about it.

Berto

#5 lore raw

PATCH v2 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..894f862 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 event_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(event_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(event_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(event_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()) {
+        event_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
#6 lore raw

PATCH v2 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
#7 lore raw
In reply to Alberto Garcia
Quoted 30 lines · Alberto Garcia

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,

Berto

v2:
- Patch 3: Rename clock_type to event_clock_type [Max]

v1: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02161.html
- Original version

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

Quoted 8 lines
 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

Thanks, applied to my block tree (with the test renamed to 148):

https://github.com/XanClic/qemu/commits/block

Max