From d9dd3f8dd3918252024700b88a65067d4daf609f Mon Sep 17 00:00:00 2001 From: "Danilo C. L. de Paula" Date: Tue, 29 Jan 2019 14:28:22 +0000 Subject: [PATCH] * Tue Jan 29 2019 Danilo Cesar Lemes de Paula - 3.1.0-10.el8 - kvm-throttle-groups-fix-restart-coroutine-iothread-race.patch [bz#1655947] - kvm-iotests-add-238-for-throttling-tgm-unregister-iothre.patch [bz#1655947] - Resolves: bz#1655947 (qemu-kvm core dumped after unplug the device which was set io throttling parameters) --- ...for-throttling-tgm-unregister-iothre.patch | 122 ++++++++++++++++++ ...-fix-restart-coroutine-iothread-race.patch | 120 +++++++++++++++++ qemu-kvm.spec | 12 +- 3 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 kvm-iotests-add-238-for-throttling-tgm-unregister-iothre.patch create mode 100644 kvm-throttle-groups-fix-restart-coroutine-iothread-race.patch diff --git a/kvm-iotests-add-238-for-throttling-tgm-unregister-iothre.patch b/kvm-iotests-add-238-for-throttling-tgm-unregister-iothre.patch new file mode 100644 index 0000000..6da2540 --- /dev/null +++ b/kvm-iotests-add-238-for-throttling-tgm-unregister-iothre.patch @@ -0,0 +1,122 @@ +From 91ae068923b70fc62c8504f7c77e42829b4c2e18 Mon Sep 17 00:00:00 2001 +From: Stefan Hajnoczi +Date: Tue, 29 Jan 2019 07:02:50 +0000 +Subject: [PATCH 2/2] iotests: add 238 for throttling tgm unregister iothread + segfault + +RH-Author: Stefan Hajnoczi +Message-id: <20190129070250.22709-3-stefanha@redhat.com> +Patchwork-id: 84138 +O-Subject: [RHEL-8.0/AV qemu-kvm PATCH 2/2] iotests: add 238 for throttling tgm unregister iothread segfault +Bugzilla: 1655947 +RH-Acked-by: Stefano Garzarella +RH-Acked-by: Thomas Huth +RH-Acked-by: Laurent Vivier + +Hot-unplug a scsi-hd using an iothread. The previous patch fixes a +segfault in this scenario. + +This patch adds a regression test. + +Suggested-by: Alberto Garcia +Suggested-by: Kevin Wolf +Signed-off-by: Stefan Hajnoczi +Reviewed-by: Alberto Garcia +Message-id: 20190114133257.30299-3-stefanha@redhat.com +Signed-off-by: Stefan Hajnoczi +(cherry picked from commit 202277f43d544779b7a63123a51c54c3a16b74ad) +Signed-off-by: Stefan Hajnoczi +Signed-off-by: Danilo C. L. de Paula + +Conflicts: + tests/qemu-iotests/group + + Context conflict because downstream doesn't have 236. This patch adds + 238 and doesn't depend on 236. + +Signed-off-by: Danilo C. L. de Paula +--- + tests/qemu-iotests/238 | 47 ++++++++++++++++++++++++++++++++++++++++++++++ + tests/qemu-iotests/238.out | 6 ++++++ + tests/qemu-iotests/group | 1 + + 3 files changed, 54 insertions(+) + create mode 100755 tests/qemu-iotests/238 + create mode 100644 tests/qemu-iotests/238.out + +diff --git a/tests/qemu-iotests/238 b/tests/qemu-iotests/238 +new file mode 100755 +index 0000000..f81ee11 +--- /dev/null ++++ b/tests/qemu-iotests/238 +@@ -0,0 +1,47 @@ ++#!/usr/bin/env python ++# ++# Regression test for throttle group member unregister segfault with iothread ++# ++# Copyright (c) 2019 Red Hat, Inc. ++# ++# 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 . ++# ++ ++import sys ++import os ++import iotests ++from iotests import log ++ ++sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts')) ++ ++from qemu import QEMUMachine ++ ++if iotests.qemu_default_machine == 's390-ccw-virtio': ++ virtio_scsi_device = 'virtio-scsi-ccw' ++else: ++ virtio_scsi_device = 'virtio-scsi-pci' ++ ++vm = QEMUMachine(iotests.qemu_prog) ++vm.add_args('-machine', 'accel=kvm') ++vm.launch() ++ ++log(vm.qmp('blockdev-add', node_name='hd0', driver='null-co')) ++log(vm.qmp('object-add', qom_type='iothread', id='iothread0')) ++log(vm.qmp('device_add', id='scsi0', driver=virtio_scsi_device, iothread='iothread0')) ++log(vm.qmp('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0')) ++log(vm.qmp('block_set_io_throttle', id='scsi-hd0', bps=0, bps_rd=0, bps_wr=0, ++ iops=1000, iops_rd=0, iops_wr=0, conv_keys=False)) ++log(vm.qmp('device_del', id='scsi-hd0')) ++ ++vm.shutdown() +diff --git a/tests/qemu-iotests/238.out b/tests/qemu-iotests/238.out +new file mode 100644 +index 0000000..4de840b +--- /dev/null ++++ b/tests/qemu-iotests/238.out +@@ -0,0 +1,6 @@ ++{"return": {}} ++{"return": {}} ++{"return": {}} ++{"return": {}} ++{"return": {}} ++{"return": {}} +diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group +index 05996ae..268fefa 100644 +--- a/tests/qemu-iotests/group ++++ b/tests/qemu-iotests/group +@@ -233,3 +233,4 @@ + 233 auto quick + 234 auto quick migration + 235 auto quick ++238 auto quick +-- +1.8.3.1 + diff --git a/kvm-throttle-groups-fix-restart-coroutine-iothread-race.patch b/kvm-throttle-groups-fix-restart-coroutine-iothread-race.patch new file mode 100644 index 0000000..42ff2e7 --- /dev/null +++ b/kvm-throttle-groups-fix-restart-coroutine-iothread-race.patch @@ -0,0 +1,120 @@ +From 02287430957782ffb1db0d7d17693a73925ea02f Mon Sep 17 00:00:00 2001 +From: Stefan Hajnoczi +Date: Tue, 29 Jan 2019 07:02:49 +0000 +Subject: [PATCH 1/2] throttle-groups: fix restart coroutine iothread race + +RH-Author: Stefan Hajnoczi +Message-id: <20190129070250.22709-2-stefanha@redhat.com> +Patchwork-id: 84139 +O-Subject: [RHEL-8.0/AV qemu-kvm PATCH 1/2] throttle-groups: fix restart coroutine iothread race +Bugzilla: 1655947 +RH-Acked-by: Stefano Garzarella +RH-Acked-by: Thomas Huth +RH-Acked-by: Laurent Vivier + +The following QMP command leads to a crash when iothreads are used: + + { 'execute': 'device_del', 'arguments': {'id': 'data'} } + +The backtrace involves the queue restart coroutine where +tgm->throttle_state is a NULL pointer because +throttle_group_unregister_tgm() has already been called: + + (gdb) bt full + #0 0x00005585a7a3b378 in qemu_mutex_lock_impl (mutex=0xffffffffffffffd0, file=0x5585a7bb3d54 "block/throttle-groups.c", line=412) at util/qemu-thread-posix.c:64 + err = + __PRETTY_FUNCTION__ = "qemu_mutex_lock_impl" + __func__ = "qemu_mutex_lock_impl" + #1 0x00005585a79be074 in throttle_group_restart_queue_entry (opaque=0x5585a9de4eb0) at block/throttle-groups.c:412 + _f = + data = 0x5585a9de4eb0 + tgm = 0x5585a9079440 + ts = 0x0 + tg = 0xffffffffffffff98 + is_write = false + empty_queue = 255 + +This coroutine should not execute in the iothread after the throttle +group member has been unregistered! + +The root cause is that the device_del code path schedules the restart +coroutine in the iothread while holding the AioContext lock. Therefore +the iothread cannot execute the coroutine until after device_del +releases the lock - by this time it's too late. + +This patch adds a reference count to ThrottleGroupMember so we can +synchronously wait for restart coroutines to complete. Once they are +done it is safe to unregister the ThrottleGroupMember. + +Signed-off-by: Stefan Hajnoczi +Reviewed-by: Alberto Garcia +Message-id: 20190114133257.30299-2-stefanha@redhat.com +Signed-off-by: Stefan Hajnoczi +(cherry picked from commit bc19a0a6e4505390f99d3c593ebaf11b7962cc59) +Signed-off-by: Stefan Hajnoczi +Signed-off-by: Danilo C. L. de Paula +--- + block/throttle-groups.c | 9 +++++++++ + include/block/throttle-groups.h | 5 +++++ + 2 files changed, 14 insertions(+) + +diff --git a/block/throttle-groups.c b/block/throttle-groups.c +index 5d8213a..a5a2037 100644 +--- a/block/throttle-groups.c ++++ b/block/throttle-groups.c +@@ -415,6 +415,9 @@ static void coroutine_fn throttle_group_restart_queue_entry(void *opaque) + } + + g_free(data); ++ ++ atomic_dec(&tgm->restart_pending); ++ aio_wait_kick(); + } + + static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write) +@@ -430,6 +433,8 @@ static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write + * be no timer pending on this tgm at this point */ + assert(!timer_pending(tgm->throttle_timers.timers[is_write])); + ++ atomic_inc(&tgm->restart_pending); ++ + co = qemu_coroutine_create(throttle_group_restart_queue_entry, rd); + aio_co_enter(tgm->aio_context, co); + } +@@ -538,6 +543,7 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm, + + tgm->throttle_state = ts; + tgm->aio_context = ctx; ++ atomic_set(&tgm->restart_pending, 0); + + qemu_mutex_lock(&tg->lock); + /* If the ThrottleGroup is new set this ThrottleGroupMember as the token */ +@@ -584,6 +590,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm) + return; + } + ++ /* Wait for throttle_group_restart_queue_entry() coroutines to finish */ ++ AIO_WAIT_WHILE(tgm->aio_context, atomic_read(&tgm->restart_pending) > 0); ++ + qemu_mutex_lock(&tg->lock); + for (i = 0; i < 2; i++) { + assert(tgm->pending_reqs[i] == 0); +diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h +index e2fd051..712a8e6 100644 +--- a/include/block/throttle-groups.h ++++ b/include/block/throttle-groups.h +@@ -43,6 +43,11 @@ typedef struct ThrottleGroupMember { + */ + unsigned int io_limits_disabled; + ++ /* Number of pending throttle_group_restart_queue_entry() coroutines. ++ * Accessed with atomic operations. ++ */ ++ unsigned int restart_pending; ++ + /* The following fields are protected by the ThrottleGroup lock. + * See the ThrottleGroup documentation for details. + * throttle_state tells us if I/O limits are configured. */ +-- +1.8.3.1 + diff --git a/qemu-kvm.spec b/qemu-kvm.spec index 0729955..974abee 100644 --- a/qemu-kvm.spec +++ b/qemu-kvm.spec @@ -68,7 +68,7 @@ Obsoletes: %1-rhev Summary: QEMU is a machine emulator and virtualizer Name: qemu-kvm Version: 3.1.0 -Release: 9%{?dist} +Release: 10%{?dist} # Epoch because we pushed a qemu-1.0 package. AIUI this can't ever be dropped Epoch: 15 License: GPLv2 and GPLv2+ and CC-BY @@ -168,6 +168,10 @@ Patch41: kvm-migration-rdma-unregister-fd-handler.patch Patch42: kvm-s390x-tod-Properly-stop-the-KVM-TOD-while-the-guest-.patch # For bz#1659127 - Stress guest and stop it, then do live migration, guest hit call trace on destination end Patch43: kvm-hw-s390x-Fix-bad-mask-in-time2tod.patch +# For bz#1655947 - qemu-kvm core dumped after unplug the device which was set io throttling parameters +Patch44: kvm-throttle-groups-fix-restart-coroutine-iothread-race.patch +# For bz#1655947 - qemu-kvm core dumped after unplug the device which was set io throttling parameters +Patch45: kvm-iotests-add-238-for-throttling-tgm-unregister-iothre.patch BuildRequires: zlib-devel BuildRequires: glib2-devel @@ -1015,6 +1019,12 @@ useradd -r -u 107 -g qemu -G kvm -d / -s /sbin/nologin \ %changelog +* Tue Jan 29 2019 Danilo Cesar Lemes de Paula - 3.1.0-10.el8 +- kvm-throttle-groups-fix-restart-coroutine-iothread-race.patch [bz#1655947] +- kvm-iotests-add-238-for-throttling-tgm-unregister-iothre.patch [bz#1655947] +- Resolves: bz#1655947 + (qemu-kvm core dumped after unplug the device which was set io throttling parameters) + * Tue Jan 29 2019 Danilo Cesar Lemes de Paula - 3.1.0-9.el8 - kvm-migration-rdma-unregister-fd-handler.patch [bz#1666601] - kvm-s390x-tod-Properly-stop-the-KVM-TOD-while-the-guest-.patch [bz#1659127]