[Qemu-devel] [PATCH v2 0/4] Separate QUORUM_REPORT_BAD events according to their node name

[Qemu-devel] [PATCH v2 0/4] Separate QUORUM_REPORT_BAD events according to their node name

[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

 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

-- 
2.7.0

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

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

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/quorum.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 11cc60b..73ff309 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -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;
 }
-- 
2.7.0

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

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

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 monitor.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/monitor.c b/monitor.c
index e99ca8c..c9fe862 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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;
 }
 
-- 
2.7.0

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

On 03/10/2016 04:55 AM, Alberto Garcia wrote:
> 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.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  monitor.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/monitor.c b/monitor.c
> index e99ca8c..c9fe862 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Alberto Garcia]

On Thu 10 Mar 2016 09:35:32 PM CET, Eric Blake wrote:
>> @@ -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

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

[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 <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 monitor.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index c9fe862..894f862 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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);
-- 
2.7.0

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

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

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 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

diff --git a/tests/qemu-iotests/146 b/tests/qemu-iotests/146
new file mode 100644
index 0000000..30bc379
--- /dev/null
+++ b/tests/qemu-iotests/146
@@ -0,0 +1,129 @@
+#!/usr/bin/env python
+#
+# Test the rate limit of QMP events
+#
+# Copyright (C) 2016 Igalia, S.L.
+# Author: Alberto Garcia <berto@igalia.com>
+#
+# 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"])
diff --git a/tests/qemu-iotests/146.out b/tests/qemu-iotests/146.out
new file mode 100644
index 0000000..ae1213e
--- /dev/null
+++ b/tests/qemu-iotests/146.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 47fd40c..54e7acc 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -148,3 +148,4 @@
 143 auto quick
 144 rw auto quick
 145 auto quick
+146 rw auto quick
-- 
2.7.0

Re: [Qemu-devel] [PATCH v2 0/4] Separate QUORUM_REPORT_BAD events according to their node name

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1762 bytes --]

On 10.03.2016 12:55, Alberto Garcia wrote:
> 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

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

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

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]