[PATCH 4/4] iotests: switch to AQMP
All that's left is to import type definitions from the new library
instead.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
---
tests/qemu-iotests/iotests.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8cdb381f2a..46e51ff07d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -38,7 +38,7 @@
from contextlib import contextmanager
from qemu.machine import qtest
-from qemu.qmp import QMPMessage
+from qemu.aqmp.legacy import QMPMessage
# Use this logger for logging messages directly from the iotests module
logger = logging.getLogger('qemu.iotests')
--
2.31.1
[PATCH 3/4] iotests/mirror-top-perms: switch to AQMP
We don't have to maintain compatibility with both QMP libraries anymore,
so we can just remove the old exception. While we're here, take
advantage of the extra fields present in the VMLaunchFailure exception
that machine.py now raises.
(Note: I'm leaving the logging suppression here unchanged. I had
suggested previously we use filters to scrub the PID out of the logging
information so it could just be diffed as part of the iotest output, but
that meant *always* scrubbing PID from logger output, which defeated the
point of even offering that information in the output to begin with.
Ultimately, I decided it's fine to just suppress the logger temporarily.)
Signed-off-by: John Snow
---
tests/qemu-iotests/tests/mirror-top-perms | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index b5849978c4..223f3c1620 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -22,7 +22,6 @@
import os
from qemu.machine import machine
-from qemu.qmp import QMPConnectError
import iotests
from iotests import change_log_level, qemu_img
@@ -99,15 +98,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
try:
-# Silence AQMP errors temporarily.
-# TODO: Remove this and just allow the errors to be logged when
-# AQMP fully replaces QMP.
+# Silence AQMP logging errors temporarily.
with change_log_level('qemu.aqmp'):
self.vm_b.launch()
print('ERROR: VM B launched successfully, '
'this should not have happened')
-except (QMPConnectError, machine.VMLaunchFailure):
-assert 'Is another process using the image' in self.vm_b.get_log()
+except machine.VMLaunchFailure as exc:
+assert 'Is another process using the image' in exc.output
result = self.vm.qmp('block-job-cancel',
device='mirror')
--
2.31.1
[PATCH 2/4] scripts/bench-block-job: switch to AQMP
For this commit, we only need to remove accommodations for the
synchronous QMP library.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
---
scripts/simplebench/bench_block_job.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index a403c35b08..af9d1646a4 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -27,7 +27,6 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
-from qemu.qmp import QMPConnectError
from qemu.aqmp import ConnectError
@@ -50,7 +49,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.launch()
except OSError as e:
return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, ConnectError, socket.timeout):
+except (ConnectError, socket.timeout):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
--
2.31.1
[PATCH 1/4] python/machine: permanently switch to AQMP
Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
switch permanent. Update Exceptions and import paths as necessary.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
---
python/qemu/machine/machine.py | 18 +++---
python/qemu/machine/qtest.py | 2 +-
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a5972fab4d..41be025ac7 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,21 +40,16 @@
TypeVar,
)
-from qemu.qmp import ( # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
+from qemu.aqmp.legacy import (
+QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
-SocketAddrT,
)
from . import console_socket
-if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
-from qemu.qmp import QEMUMonitorProtocol
-else:
-from qemu.aqmp.legacy import QEMUMonitorProtocol
-
-
LOG = logging.getLogger(__name__)
@@ -743,8 +738,9 @@ def events_wait(self,
:param timeout: Optional timeout, in seconds.
See QEMUMonitorProtocol.pull_event.
-:raise QMPTimeoutError: If timeout was non-zero and no matching events
-were found.
+:raise asyncio.TimeoutError:
+If timeout was non-zero and no matching events were found.
+
:return: A QMP event matching the filter criteria.
If timeout was 0 and no event matched, None.
"""
@@ -767,7 +763,7 @@ def _match(event: QMPMessage) -> bool:
event = self._qmp.pull_event(wait=timeout)
if event is None:
# NB: None is only returned when timeout is false-ish.
-# Timeouts raise QMPTimeoutError instead!
+# Timeouts raise asyncio.TimeoutError instead!
break
if _match(event):
return event
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index f2f9aaa5e5..13e0aaff84 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
TextIO,
)
-from qemu.qmp import SocketAddrT # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
from .machine import QEMUMachine
--
2.31.1
[PATCH 0/4] iotests: finalize switch to async QMP
Based-on: <20220203015946.1330386-1-js...@redhat.com> [PULL 0/4] Python patches GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch-pt1b This tiny series is a spiritual v4 to: "[PATCH v3 00/31] Python: delete synchronous qemu.qmp package". I've isolated just the bits that touch iotests, and that's these four patches. If this series is approved, I'll send the series that renames "qemu.aqmp" to "qemu.qmp" separately. That series has a lot of churn, but it doesn't meaningfully alter anything -- so I'll avoid cluttering up qemu-block list with those. (Just be aware that I plan to finalize the switch soon!) John Snow (4): python/machine: permanently switch to AQMP scripts/bench-block-job: switch to AQMP iotests/mirror-top-perms: switch to AQMP iotests: switch to AQMP python/qemu/machine/machine.py| 18 +++--- python/qemu/machine/qtest.py | 2 +- scripts/simplebench/bench_block_job.py| 3 +-- tests/qemu-iotests/iotests.py | 2 +- tests/qemu-iotests/tests/mirror-top-perms | 9 +++-- 5 files changed, 13 insertions(+), 21 deletions(-) -- 2.31.1
[PULL 4/4] python/aqmp: add socket bind step to legacy.py
The synchronous QMP library would bind to the server address during
__init__(). The new library delays this to the accept() call, because
binding occurs inside of the call to start_[unix_]server(), which is an
async method -- so it cannot happen during __init__ anymore.
Python 3.7+ adds the ability to create the server (and thus the bind()
call) and begin the active listening in separate steps, but we don't
have that functionality in 3.6, our current minimum.
Therefore ... Add a temporary workaround that allows the synchronous
version of the client to bind the socket in advance, guaranteeing that
there will be a UNIX socket in the filesystem ready for the QEMU client
to connect to without a race condition.
(Yes, it's a bit ugly. Fixing it more nicely will have to wait until our
minimum Python version is 3.7+.)
Signed-off-by: John Snow
Reviewed-by: Kevin Wolf
Message-id: 20220201041134.1237016-5-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 3 +++
python/qemu/aqmp/protocol.py | 41 +---
2 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 0890f95b16..6baa5f3409 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -56,6 +56,9 @@ def __init__(self, address: SocketAddrT,
self._address = address
self._timeout: Optional[float] = None
+if server:
+self._aqmp._bind_hack(address) # pylint: disable=protected-access
+
_T = TypeVar('_T')
def _sync(
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 50e973c2f2..33358f5cd7 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -15,6 +15,7 @@
from enum import Enum
from functools import wraps
import logging
+import socket
from ssl import SSLContext
from typing import (
Any,
@@ -238,6 +239,9 @@ def __init__(self, name: Optional[str] = None) -> None:
self._runstate = Runstate.IDLE
self._runstate_changed: Optional[asyncio.Event] = None
+# Workaround for bind()
+self._sock: Optional[socket.socket] = None
+
def __repr__(self) -> str:
cls_name = type(self).__name__
tokens = []
@@ -427,6 +431,34 @@ async def _establish_connection(
else:
await self._do_connect(address, ssl)
+def _bind_hack(self, address: Union[str, Tuple[str, int]]) -> None:
+"""
+Used to create a socket in advance of accept().
+
+This is a workaround to ensure that we can guarantee timing of
+precisely when a socket exists to avoid a connection attempt
+bouncing off of nothing.
+
+Python 3.7+ adds a feature to separate the server creation and
+listening phases instead, and should be used instead of this
+hack.
+"""
+if isinstance(address, tuple):
+family = socket.AF_INET
+else:
+family = socket.AF_UNIX
+
+sock = socket.socket(family, socket.SOCK_STREAM)
+sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+
+try:
+sock.bind(address)
+except:
+sock.close()
+raise
+
+self._sock = sock
+
@upper_half
async def _do_accept(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
@@ -464,24 +496,27 @@ async def _client_connected_cb(reader:
asyncio.StreamReader,
if isinstance(address, tuple):
coro = asyncio.start_server(
_client_connected_cb,
-host=address[0],
-port=address[1],
+host=None if self._sock else address[0],
+port=None if self._sock else address[1],
ssl=ssl,
backlog=1,
limit=self._limit,
+sock=self._sock,
)
else:
coro = asyncio.start_unix_server(
_client_connected_cb,
-path=address,
+path=None if self._sock else address,
ssl=ssl,
backlog=1,
limit=self._limit,
+sock=self._sock,
)
server = await coro # Starts listening
await connected.wait() # Waits for the callback to fire (and finish)
assert server is None
+self._sock = None
self.logger.debug("Connection accepted.")
--
2.31.1
[PULL 3/4] python: upgrade mypy to 0.780
We need a slightly newer version of mypy in order to use some features
of the asyncio server functions in the next commit.
(Note: pipenv is not really suited to upgrading individual packages; I
need to replace this tool with something better for the task. For now,
the miscellaneous updates not related to the mypy upgrade are simply
beyond my control. It's on my list to take care of soon.)
Signed-off-by: John Snow
Reviewed-by: Kevin Wolf
Message-id: 20220201041134.1237016-4-js...@redhat.com
Signed-off-by: John Snow
---
python/Pipfile.lock | 66 ++---
python/setup.cfg| 2 +-
2 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index d2a7dbd88b..ce46404ce0 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
{
"_meta": {
"hash": {
-"sha256":
"784b327272db32403d5a488507853b5afba850ba26a5948e5b6a90c1baef2d9c"
+"sha256":
"f1a25654d884a5b450e38d78b1f2e3ebb9073e421cc4358d4bbb83ac251a5670"
},
"pipfile-spec": 6,
"requires": {
@@ -34,7 +34,7 @@
"sha256:09bdb456e02564731f8b5957cdd0c98a7f01d2db5e90eb1d794c353c28bfd705",
"sha256:6a8a51f64dae307f6e0c9db752b66a7951e282389d8362cc1d39a56f3feeb31d"
],
-"markers": "python_version ~= '3.6'",
+"index": "pypi",
"version": "==2.6.0"
},
"avocado-framework": {
@@ -50,6 +50,7 @@
"sha256:106fef6dc37dd8c0e2c0a60d3fca3e77460a48907f335fa28420463a6f799736",
"sha256:23e223426b28491b1ced97dc3bbe183027419dfc7982b4fa2f05d5f3ff10711c"
],
+"index": "pypi",
"version": "==0.3.2"
},
"filelock": {
@@ -57,6 +58,7 @@
"sha256:18d82244ee114f543149c66a6e0c14e9c4f8a1044b5cdaadd0f82159d6a6ff59",
"sha256:929b7d63ec5b7d6b71b0fa5ac14e030b3f70b75747cef1b10da9b879fef15836"
],
+"index": "pypi",
"version": "==3.0.12"
},
"flake8": {
@@ -88,7 +90,7 @@
"sha256:54161657e8ffc76596c4ede7080ca68cb02962a2e074a2586b695a93a925d36e",
"sha256:e962bff7440364183203d179d7ae9ad90cb1f2b74dcb84300e88ecc42dca3351"
],
-"markers": "python_version < '3.7'",
+"index": "pypi",
"version": "==5.1.4"
},
"isort": {
@@ -124,7 +126,7 @@
"sha256:ed361bb83436f117f9917d282a456f9e5009ea12fd6de8742d1a4752c3017e93",
"sha256:f5144c75445ae3ca2057faac03fda5a902eff196702b0a24daf1d6ce0650514b"
],
-"markers": "python_version >= '2.7' and python_version not in
'3.0, 3.1, 3.2, 3.3, 3.4, 3.5'",
+"index": "pypi",
"version": "==1.6.0"
},
"mccabe": {
@@ -136,23 +138,23 @@
},
"mypy": {
"hashes": [
-
"sha256:15b948e1302682e3682f11f50208b726a246ab4e6c1b39f9264a8796bb416aa2",
-
"sha256:219a3116ecd015f8dca7b5d2c366c973509dfb9a8fc97ef044a36e3da66144a1",
-
"sha256:3b1fc683fb204c6b4403a1ef23f0b1fac8e4477091585e0c8c54cbdf7d7bb164",
-
"sha256:3beff56b453b6ef94ecb2996bea101a08f1f8a9771d3cbf4988a61e4d9973761",
-
"sha256:7687f6455ec3ed7649d1ae574136835a4272b65b3ddcf01ab8704ac65616c5ce",
-
"sha256:7ec45a70d40ede1ec7ad7f95b3c94c9cf4c186a32f6bacb1795b60abd2f9ef27",
-
"sha256:86c857510a9b7c3104cf4cde1568f4921762c8f9842e987bc03ed4f160925754",
-
"sha256:8a627507ef9b307b46a1fea9513d5c98680ba09591253082b4c48697ba05a4ae",
-
"sha256:8dfb69fbf9f3aeed18afffb15e319ca7f8da9642336348ddd6cab2713ddcf8f9",
-
"sha256:a34b577cdf6313bf24755f7a0e3f3c326d5c1f4fe7422d1d06498eb25ad0c600",
-
"sha256:a8ffcd53cb5dfc131850851cc09f1c44689c2812d0beb954d8138d4f5fc17f65",
-
"sha256:b90928f2d9eb2f33162405f32dde9f6dcead63a0971ca8a1b50eb4ca3e35ceb8",
-
"sha256:c56ffe22faa2e51054c5f7a3bc70a370939c2ed4de308c690e7949230c995913",
-
"sha256:f91c7ae919bbc3f96cd5e5b2e786b2b108343d1d7972ea130f7de27fdd547cf3"
+
"sha256:00cb1964a7476e871d6108341ac9c1a857d6bd20bf5877f4773ac5e9d92cd3cd",
+
"sha256:127de5a9b817a03a98c5ae8a0c46a20dc2af6dcfa2ae7f96cb519b312efa",
+
"sha256:1f3976a945ad7f0a0727aafdc5651c2d3278e3c88dee94e2bf75cd3386b7b2f4",
+
"sha256:2f8c098f12b402c19b735aec724cc9105cc1a9eea405d08814eb4b14a6fb1a41",
+
"sha256:4ef13b619a289aa025f2273e05e755f8049bb4eaba6d703a425de37d495d178d",
+
"sha256:5d142f219bf8c7894dfa79ebfb7d352c4c63a325e75f10dfb4c3db9417dcd135",
+
"sha256:62eb5dd4ea86bda8ce386f26684f7f26e4bfe6283c9f2b6ca6d17faf704dcfad",
+
[PULL 0/4] Python patches
The following changes since commit 47cc1a3655135b89fa75c2824fbddd29df874612: Merge remote-tracking branch 'remotes/kwolf-gitlab/tags/for-upstream' into staging (2022-02-01 19:48:15 +) are available in the Git repository at: https://gitlab.com/jsnow/qemu.git tags/python-pull-request for you to fetch changes up to b0b662bb2b340d63529672b5bdae596a6243c4d0: python/aqmp: add socket bind step to legacy.py (2022-02-02 14:12:22 -0500) Python patches Peter: I expect this to address the iotest 040,041 failures you observed on NetBSD. If it doesn't, let me know. John Snow (4): python/aqmp: Fix negotiation with pre-"oob" QEMU python/machine: raise VMLaunchFailure exception from launch() python: upgrade mypy to 0.780 python/aqmp: add socket bind step to legacy.py python/Pipfile.lock | 66 +-- python/qemu/aqmp/legacy.py| 3 ++ python/qemu/aqmp/protocol.py | 41 -- python/qemu/aqmp/qmp_client.py| 4 +- python/qemu/machine/machine.py| 45 +--- python/setup.cfg | 2 +- tests/qemu-iotests/tests/mirror-top-perms | 3 +- 7 files changed, 123 insertions(+), 41 deletions(-) -- 2.31.1
[PULL 2/4] python/machine: raise VMLaunchFailure exception from launch()
This allows us to pack in some extra information about the failure,
which guarantees that if the caller did not *intentionally* cause a
failure (by capturing this Exception), some pretty good clues will be
printed at the bottom of the traceback information.
This will help make failures in the event of a non-negative return code
more obvious when they go unhandled; the current behavior in
_post_shutdown() is to print a warning message only in the event of
signal-based terminations (for negative return codes).
(Note: In Python, catching BaseException instead of Exception catches a
broader array of Exception events, including SystemExit and
KeyboardInterrupt. We do not want to "wrap" such exceptions as a
VMLaunchFailure, because that will 'downgrade' the exception from a
BaseException to a regular Exception. We do, however, want to perform
cleanup in either case, so catch on the broadest scope and
wrap-and-re-raise only in the more targeted scope.)
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Message-id: 20220201041134.1237016-3-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py| 45 ---
tests/qemu-iotests/tests/mirror-top-perms | 3 +-
2 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 67ab06ca2b..a5972fab4d 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -74,6 +74,35 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
"""
+class VMLaunchFailure(QEMUMachineError):
+"""
+Exception raised when a VM launch was attempted, but failed.
+"""
+def __init__(self, exitcode: Optional[int],
+ command: str, output: Optional[str]):
+super().__init__(exitcode, command, output)
+self.exitcode = exitcode
+self.command = command
+self.output = output
+
+def __str__(self) -> str:
+ret = ''
+if self.__cause__ is not None:
+name = type(self.__cause__).__name__
+reason = str(self.__cause__)
+if reason:
+ret += f"{name}: {reason}"
+else:
+ret += f"{name}"
+ret += '\n'
+
+if self.exitcode is not None:
+ret += f"\tExit code: {self.exitcode}\n"
+ret += f"\tCommand: {self.command}\n"
+ret += f"\tOutput: {self.output}\n"
+return ret
+
+
class AbnormalShutdown(QEMUMachineError):
"""
Exception raised when a graceful shutdown was requested, but not performed.
@@ -397,7 +426,7 @@ def launch(self) -> None:
try:
self._launch()
-except:
+except BaseException as exc:
# We may have launched the process but it may
# have exited before we could connect via QMP.
# Assume the VM didn't launch or is exiting.
@@ -408,11 +437,15 @@ def launch(self) -> None:
else:
self._post_shutdown()
-LOG.debug('Error launching VM')
-if self._qemu_full_args:
-LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
-if self._iolog:
-LOG.debug('Output: %r', self._iolog)
+if isinstance(exc, Exception):
+raise VMLaunchFailure(
+exitcode=self.exitcode(),
+command=' '.join(self._qemu_full_args),
+output=self._iolog
+) from exc
+
+# Don't wrap 'BaseException'; doing so would downgrade
+# that exception. However, we still want to clean up.
raise
def _launch(self) -> None:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index 0a51a613f3..b5849978c4 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,7 +21,6 @@
import os
-from qemu.aqmp import ConnectError
from qemu.machine import machine
from qemu.qmp import QMPConnectError
@@ -107,7 +106,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.launch()
print('ERROR: VM B launched successfully, '
'this should not have happened')
-except (QMPConnectError, ConnectError):
+except (QMPConnectError, machine.VMLaunchFailure):
assert 'Is another process using the image' in self.vm_b.get_log()
result = self.vm.qmp('block-job-cancel',
--
2.31.1
[PULL 1/4] python/aqmp: Fix negotiation with pre-"oob" QEMU
QEMU versions prior to the "oob" capability *also* can't accept the
"enable" keyword argument at all. Fix the handshake process with older
QEMU versions.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Reviewed-by: Kevin Wolf
Message-id: 20220201041134.1237016-2-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/qmp_client.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index f1a845cc82..90a8737f03 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -292,9 +292,9 @@ async def _negotiate(self) -> None:
"""
self.logger.debug("Negotiating capabilities ...")
-arguments: Dict[str, List[str]] = {'enable': []}
+arguments: Dict[str, List[str]] = {}
if self._greeting and 'oob' in self._greeting.QMP.capabilities:
-arguments['enable'].append('oob')
+arguments.setdefault('enable', []).append('oob')
msg = self.make_execute_msg('qmp_capabilities', arguments=arguments)
# It's not safe to use execute() here, because the reader/writers
--
2.31.1
[RFC] thread-pool: Add option to fix the pool size
The thread pool regulates itself: when idle, it kills threads until
empty, when in demand, it creates new threads until full. This behaviour
doesn't play well with latency sensitive workloads where the price of
creating a new thread is too high. For example, when paired with qemu's
'-mlock', or using safety features like SafeStack, creating a new thread
has been measured take multiple milliseconds.
In order to mitigate this let's introduce a new option to set a fixed
pool size. The threads will be created during the pool's initialization,
remain available during its lifetime regardless of demand, and destroyed
upon freeing it. A properly characterized workload will then be able to
configure the pool to avoid any latency spike.
Signed-off-by: Nicolas Saenz Julienne
---
The fix I propose here works for my specific use-case, but I'm pretty
sure it'll need to be a bit more versatile to accommodate other
use-cases.
Some questions:
- Is unanimously setting these parameters for any pool instance too
limiting? It'd make sense to move the options into the AioContext the
pool belongs to. IIUC, for the general block use-case, this would be
'qemu_aio_context' as initialized in qemu_init_main_loop().
- Currently I'm setting two pool properties through a single qemu
option. The pool's size and dynamic behaviour, or lack thereof. I
think it'd be better to split them into separate options. I thought of
different ways of expressing this (min/max-size where static happens
when min-size=max-size, size and static/dynamic, etc..), but you might
have ideas on what could be useful to other use-cases.
Some background on my workload: I'm using IDE emulation, the guest is an
old RTOS that doesn't support virtio, using 'aio=native' isn't possible
either (unaligned IO accesses).
Thanks!
include/block/thread-pool.h | 2 ++
qemu-options.hx | 21 +
softmmu/vl.c| 28
util/thread-pool.c | 20 +---
4 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h
index 7dd7d730a0..3337971669 100644
--- a/include/block/thread-pool.h
+++ b/include/block/thread-pool.h
@@ -23,6 +23,8 @@
typedef int ThreadPoolFunc(void *opaque);
typedef struct ThreadPool ThreadPool;
+extern int thread_pool_max_threads;
+extern bool thread_pool_fixed_size;
ThreadPool *thread_pool_new(struct AioContext *ctx);
void thread_pool_free(ThreadPool *pool);
diff --git a/qemu-options.hx b/qemu-options.hx
index ba3ae6a42a..cb8f50db66 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4627,6 +4627,27 @@ SRST
user-provided config files on sysconfdir.
ERST
+DEF("thread-pool", HAS_ARG, QEMU_OPTION_threadpool,
+"-thread-pool fixed-size=[n]\n"
+" Sets the number of threads always available in the
pool.\n",
+QEMU_ARCH_ALL)
+SRST
+``-thread-pool fixed-size=[n]``
+The ``fixed-size=value`` option sets the number of readily available
+threads in the pool. When set, the pool will create the threads during
+initialization and will abstain from growing or shrinking during runtime.
+This moves the burden of properly sizing the pool to the user in exchange
+for a more deterministic thread pool behaviour. The number of threads has
+to be greater than 0.
+
+When not used, the thread pool size will change dynamically based on
+demand: converging to being empty when idle and maxing out at 64 threads.
+
+This option targets real-time systems sensitive to the latency introduced
+by creating new threads during runtime. Performance sensitive use-cases are
+better-off not using this.
+ERST
+
DEF("trace", HAS_ARG, QEMU_OPTION_trace,
"-trace [[enable=]][,events=][,file=]\n"
"specify tracing options\n",
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5e1b35ba48..6a44cc1818 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -72,6 +72,7 @@
#include "qemu/log.h"
#include "sysemu/blockdev.h"
#include "hw/block/block.h"
+#include "block/thread-pool.h"
#include "hw/i386/x86.h"
#include "hw/i386/pc.h"
#include "migration/misc.h"
@@ -496,6 +497,19 @@ static QemuOptsList qemu_action_opts = {
},
};
+static QemuOptsList qemu_thread_pool_opts = {
+.name = "thread-pool",
+.head = QTAILQ_HEAD_INITIALIZER(qemu_thread_pool_opts.head),
+.desc = {
+{
+.name = "fixed-size",
+.type = QEMU_OPT_NUMBER,
+.help = "Sets the number of threads available in the pool",
+},
+{ /* end of list */ }
+},
+};
+
const char *qemu_get_vm_name(void)
{
return qemu_name;
@@ -2809,6 +2823,7 @@ void qemu_init(int argc, char **argv, char **envp)
qemu_add_opts(_semihosting_config_opts);
qemu_add_opts(_fw_cfg_opts);
qemu_add_opts(_action_opts);
+qemu_add_opts(_thread_pool_opts);
module_call_init(MODULE_INIT_OPTS);
Re: [PATCH v4 4/4] python/aqmp: add socket bind step to legacy.py
On Tue, Feb 1, 2022 at 2:46 PM Kevin Wolf wrote: > > Am 01.02.2022 um 19:32 hat John Snow geschrieben: > > On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf wrote: > > > > > > Am 01.02.2022 um 05:11 hat John Snow geschrieben: > > > > The synchronous QMP library would bind to the server address during > > > > __init__(). The new library delays this to the accept() call, because > > > > binding occurs inside of the call to start_[unix_]server(), which is an > > > > async method -- so it cannot happen during __init__ anymore. > > > > > > > > Python 3.7+ adds the ability to create the server (and thus the bind() > > > > call) and begin the active listening in separate steps, but we don't > > > > have that functionality in 3.6, our current minimum. > > > > > > > > Therefore ... Add a temporary workaround that allows the synchronous > > > > version of the client to bind the socket in advance, guaranteeing that > > > > there will be a UNIX socket in the filesystem ready for the QEMU client > > > > to connect to without a race condition. > > > > > > > > (Yes, it's a bit ugly. Fixing it more nicely will have to wait until our > > > > minimum Python version is 3.7+.) > > > > > > > > Signed-off-by: John Snow > > > > --- > > > > python/qemu/aqmp/legacy.py | 3 +++ > > > > python/qemu/aqmp/protocol.py | 41 +--- > > > > 2 files changed, 41 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py > > > > index 0890f95b16..6baa5f3409 100644 > > > > --- a/python/qemu/aqmp/legacy.py > > > > +++ b/python/qemu/aqmp/legacy.py > > > > @@ -56,6 +56,9 @@ def __init__(self, address: SocketAddrT, > > > > self._address = address > > > > self._timeout: Optional[float] = None > > > > > > > > +if server: > > > > +self._aqmp._bind_hack(address) # pylint: > > > > disable=protected-access > > > > > > I feel that this is the only part that really makes it ugly. Do you > > > really think this way is so bad that we can't make it an official public > > > interface in the library? > > > > > > Kevin > > > > > > > Good question. > > > > I felt like I'd rather use the 'start_serving' parameter of > > loop.create_server(...), added in python 3.7; see > > https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_server > > Python 3.6 is already EOL, but we still depend on it for our build and > > I wasn't prepared to write the series that forces us on to 3.7, > > because RHEL uses 3.6 as its native python. I'll have to update the > > docker files, etc -- and I'm sure people will be kind of unhappy with > > this, so I am putting it off. People were unhappy enough with the move > > to Python 3.6. > > Yes, I understand. In theory - I haven't really thought much about it - > it feels like whether you create a separate socket in a first step and > pass it to the server that is created in the second step (for 3.6) or > you start an idle server in the first step and then let it start serving > in the second step (for 3.7+) should be an implementation detail if we > get the external API right. > > > I also felt as though the async version has no real need for a > > separate bind step -- you can just start the server in a coroutine and > > wait until it yields, then proceed to launch QEMU. It's easy in that > > paradigm. If this bind step is only for the benefit of the legacy.py > > interface, I thought maybe it wasn't wise to commit to supporting it > > if it was something I wanted to get rid of soon anyway. There's also > > the ugliness that if you use the early bind step, the arguments passed > > to accept() are now ignored, which is kind of ugly, too. It's not a > > *great* interface. It doesn't spark joy. > > Right, that's probably a sign that the interfaces aren't completely > right. I'm not sure which interfaces. As long as it's just _bind_hack() > and it's only used in an API that is going away in the future, that's no > problem. But it could also be a sign that the public interfaces aren't > flexible enough. > Yeah, I agree. It's not flexible enough. I think the sync.py development will force me to understand where I have come to rest on the conveniences of asyncio and force the design to be more flexible overall. > > I have some patches that are building a "sync.py" module that's meant > > to replace "legacy.py", and it's in the development of that module > > that I expect to either remove the bits I am unhappy with, or commit > > to supporting necessary infrastructure that's just simply required for > > a functional synchronous interface to work. I planned to start > > versioning the "qemu.qmp" package at 0.0.1, and the version that drops > > legacy.py I intend to version at 0.1.0. > > If I understand these version numbers correctly, this also implies that > there is no compatibility promise yet. So maybe what to make a public > interface isn't even a big concern yet. Right. (Still, I didn't want
Re: [PATCH v6 21/33] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate
Am 02.02.2022 um 18:27 hat Paolo Bonzini geschrieben:
> On 1/27/22 12:03, Kevin Wolf wrote:
> > > +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error
> > > **errp)
> > > +{
> > > +Error *local_err = NULL;
> > > +
> > > +if (bs->drv->bdrv_co_invalidate_cache) {
> > > +bs->drv->bdrv_co_invalidate_cache(bs, _err);
> > > +if (local_err) {
> > > +bs->open_flags |= BDRV_O_INACTIVE;
> >
> > This doesn't feel like the right place. The flag is cleared by the
> > caller, so it should also be set again on failure by the caller and not
> > by this function.
> >
> > What bdrv_co_invalidate_cache() could do is assert that BDRV_O_INACTIVE
> > is cleared when it's called.
>
> Do you think this would be handled more easily into its own series?
>
> In general, the work in this series is more incremental than its size
> suggests. Perhaps it should be flushed out in smaller pieces.
Smaller pieces are always easier to handle, so if things make sense
independently, splitting them off is usually a good idea.
Kevin
Re: [PATCH v6 21/33] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate
On 1/27/22 12:03, Kevin Wolf wrote:
+int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
+{
+Error *local_err = NULL;
+
+if (bs->drv->bdrv_co_invalidate_cache) {
+bs->drv->bdrv_co_invalidate_cache(bs, _err);
+if (local_err) {
+bs->open_flags |= BDRV_O_INACTIVE;
This doesn't feel like the right place. The flag is cleared by the
caller, so it should also be set again on failure by the caller and not
by this function.
What bdrv_co_invalidate_cache() could do is assert that BDRV_O_INACTIVE
is cleared when it's called.
Do you think this would be handled more easily into its own series?
In general, the work in this series is more incremental than its size
suggests. Perhaps it should be flushed out in smaller pieces.
Paolo
Re: [PATCH 10/12] block.c: add subtree_drains where needed
On 2/2/22 16:37, Emanuele Giuseppe Esposito wrote: So we have disk B with backing file C, and new disk A that wants to have backing file C. I think I understand what you mean, so in theory the operation would be - create new child - add child to A->children list - add child to C->parents list So in theory we need to * drain A (without subtree), because it can't happen that child nodes of A have in-flight requests that look at A status (children list), right? In other words, if A has another node X, can a request in X inspect A->children * drain C, as parents can inspect C status (like B). Same assumption here, C->children[x]->bs cannot have requests inspecting C->parents list? In that case (i.e. if parents have to be drained, but children need not) bdrv_drained_begin_unlocked would be enough, right? That would mean that ->children is I/O state but ->parents is global state. I think it's quite a bit more complicated to analyze and to understand. Paolo
Re: [PATCH 10/12] block.c: add subtree_drains where needed
On 01/02/2022 15:47, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2022 19:27, Emanuele Giuseppe Esposito wrote:
>> Protect bdrv_replace_child_noperm, as it modifies the
>> graph by adding/removing elements to .children and .parents
>> list of a bs. Use the newly introduced
>> bdrv_subtree_drained_{begin/end}_unlocked drains to achieve
>> that and be free from the aiocontext lock.
>>
>> One important criteria to keep in mind is that if the caller of
>> bdrv_replace_child_noperm creates a transaction, we need to make sure
>> that the
>> whole transaction is under the same drain block. This is imperative,
>> as having
>> multiple drains also in the .abort() class of functions causes
>> discrepancies
>> in the drained counters (as nodes are put back into the original
>> positions),
>> making it really hard to retourn all to zero and leaving the code very
>> buggy.
>> See https://patchew.org/QEMU/20211213104014.69858-1-eespo...@redhat.com/
>> for more explanations.
>>
>> Unfortunately we still need to have bdrv_subtree_drained_begin/end
>> in bdrv_detach_child() releasing and then holding the AioContext
>> lock, since it later invokes bdrv_try_set_aio_context() that is
>> not safe yet. Once all is cleaned up, we can also remove the
>> acquire/release locks in job_unref, artificially added because of this.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> block.c | 50 --
>> 1 file changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index fcc44a49a0..6196c95aae 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3114,8 +3114,22 @@ static void bdrv_detach_child(BdrvChild **childp)
>> BlockDriverState *old_bs = (*childp)->bs;
>> assert(qemu_in_main_thread());
>> + if (old_bs) {
>> + /*
>> + * TODO: this is called by job_unref with lock held, because
>> + * afterwards it calls bdrv_try_set_aio_context.
>> + * Once all of this is fixed, take care of removing
>> + * the aiocontext lock and make this function _unlocked.
>> + */
>> + bdrv_subtree_drained_begin(old_bs);
>> + }
>> +
>> bdrv_replace_child_noperm(childp, NULL, true);
>> + if (old_bs) {
>> + bdrv_subtree_drained_end(old_bs);
>> + }
>> +
>> if (old_bs) {
>> /*
>> * Update permissions for old node. We're just taking a
>> parent away, so
>> @@ -3154,6 +3168,7 @@ BdrvChild
>> *bdrv_root_attach_child(BlockDriverState *child_bs,
>> Transaction *tran = tran_new();
>> assert(qemu_in_main_thread());
>> + bdrv_subtree_drained_begin_unlocked(child_bs);
>> ret = bdrv_attach_child_common(child_bs, child_name, child_class,
>> child_role, perm, shared_perm,
>> opaque,
>> @@ -3168,6 +3183,7 @@ out:
>> tran_finalize(tran, ret);
>> /* child is unset on failure by bdrv_attach_child_common_abort() */
>> assert((ret < 0) == !child);
>> + bdrv_subtree_drained_end_unlocked(child_bs);
>> bdrv_unref(child_bs);
>> return child;
>> @@ -3197,6 +3213,9 @@ BdrvChild *bdrv_attach_child(BlockDriverState
>> *parent_bs,
>> assert(qemu_in_main_thread());
>> + bdrv_subtree_drained_begin_unlocked(parent_bs);
>> + bdrv_subtree_drained_begin_unlocked(child_bs);
>> +
>> ret = bdrv_attach_child_noperm(parent_bs, child_bs, child_name,
>> child_class,
>> child_role, , tran, errp);
>> if (ret < 0) {
>> @@ -3211,6 +3230,9 @@ BdrvChild *bdrv_attach_child(BlockDriverState
>> *parent_bs,
>> out:
>> tran_finalize(tran, ret);
>> /* child is unset on failure by bdrv_attach_child_common_abort() */
>> + bdrv_subtree_drained_end_unlocked(child_bs);
>> + bdrv_subtree_drained_end_unlocked(parent_bs);
>> +
>> assert((ret < 0) == !child);
>> bdrv_unref(child_bs);
>> @@ -3456,6 +3478,11 @@ int bdrv_set_backing_hd(BlockDriverState *bs,
>> BlockDriverState *backing_hd,
>> assert(qemu_in_main_thread());
>> + bdrv_subtree_drained_begin_unlocked(bs);
>> + if (backing_hd) {
>> + bdrv_subtree_drained_begin_unlocked(backing_hd);
>> + }
>> +
>> ret = bdrv_set_backing_noperm(bs, backing_hd, tran, errp);
>> if (ret < 0) {
>> goto out;
>> @@ -3464,6 +3491,10 @@ int bdrv_set_backing_hd(BlockDriverState *bs,
>> BlockDriverState *backing_hd,
>> ret = bdrv_refresh_perms(bs, errp);
>> out:
>> tran_finalize(tran, ret);
>> + if (backing_hd) {
>> + bdrv_subtree_drained_end_unlocked(backing_hd);
>> + }
>> + bdrv_subtree_drained_end_unlocked(bs);
>> return ret;
>> }
>> @@ -5266,7 +5297,8 @@ static int
>> bdrv_replace_node_common(BlockDriverState *from,
>> assert(qemu_get_current_aio_context() == qemu_get_aio_context());
>> assert(bdrv_get_aio_context(from) == bdrv_get_aio_context(to));
>> - bdrv_drained_begin(from);
>> +
Re: [PULL 18/20] block/nbd: drop connection_co
On 02.02.22 14:53, Eric Blake wrote:
On Wed, Feb 02, 2022 at 12:49:36PM +0100, Fabian Ebner wrote:
Am 27.09.21 um 23:55 schrieb Eric Blake:
From: Vladimir Sementsov-Ogievskiy
OK, that's a big rewrite of the logic.
Pre-patch we have an always running coroutine - connection_co. It does
reply receiving and reconnecting. And it leads to a lot of difficult
and unobvious code around drained sections and context switch. We also
abuse bs->in_flight counter which is increased for connection_co and
temporary decreased in points where we want to allow drained section to
begin. One of these place is in another file: in nbd_read_eof() in
nbd/client.c.
We also cancel reconnect and requests waiting for reconnect on drained
begin which is not correct. And this patch fixes that.
Let's finally drop this always running coroutine and go another way:
do both reconnect and receiving in request coroutines.
Hi,
while updating our stack to 6.2, one of our live-migration tests stopped
working (backtrace is below) and bisecting led me to this patch.
The VM has a single qcow2 disk (converting to raw doesn't make a
difference) and the issue only appears when using iothread (for both
virtio-scsi-pci and virtio-block-pci).
Reverting 1af7737871fb3b66036f5e520acb0a98fc2605f7 (which lives on top)
and 4ddb5d2fde6f22b2cf65f314107e890a7ca14fcf (the commit corresponding
to this patch) in v6.2.0 makes the migration work again.
Backtrace:
Thread 1 (Thread 0x7f9d93458fc0 (LWP 56711) "kvm"):
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x7f9d9d6bc537 in __GI_abort () at abort.c:79
#2 0x7f9d9d6bc40f in __assert_fail_base (fmt=0x7f9d9d825128
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5579153763f8
"qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)",
file=0x5579153764f9 "../io/channel.c", line=483, function=) at assert.c:92
Given that this assertion is about which aio context is set, I wonder
if the conversation at
https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg00096.html is
relevant; if so, Vladimir may already be working on the patch.
It should be exactly that patch:
https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06222.html
(From the discussion it appears that for v1 I need to ensure the
reconnection timer is deleted immediately once reconnecting succeeds,
and then that should be good to move out of the RFC state.)
Basically, I expect qemu to crash every time that you try to use an NBD
block device in an I/O thread (unless you don’t do any I/O), for example
this is the simplest reproducer I know of:
$ qemu-nbd --fork -k /tmp/nbd.sock -f raw null-co://
$ qemu-system-x86_64 \
-object iothread,id=iothr0 \
-device virtio-scsi,id=vscsi,iothread=iothr0 \
-blockdev '{
"driver": "nbd",
"node-name": "nbd",
"server": {
"type": "unix",
"path": "/tmp/nbd.sock"
} }' \
-device scsi-hd,bus=vscsi.0,drive=nbd
qemu-system-x86_64: ../qemu-6.2.0/io/channel.c:483:
qio_channel_restart_read: Assertion `qemu_get_current_aio_context() ==
qemu_coroutine_get_aio_context(co)' failed.
qemu-nbd: Disconnect client, due to: Unable to read from socket:
Connection reset by peer
[1] 108747 abort (core dumped) qemu-system-x86_64 -object
iothread,id=iothr0 -device -blockdev -device
Re: multiple connections per block device with NBD?
On Wed, Feb 02, 2022 at 02:42:02AM +0530, Sam wrote: > Hello, > > I am wondering whether NBD integration of qemu is able to establish multiple > connections to remote NBD server for a single block device. nbd-client has > this option: > > -C, --connections > Use num connections to the server, to allow speeding up request handling, at > the cost of higher resource usage on the server. Use of this option requires > kernel support available first with Linux 4.9. > > I am looking for equivalent functionality in qemu's direct NBD access as > multiple connections in nbd-client does increase speeds many folds. At the moment, qemu does not support multiple NBD client connections to a single NBD server. There is certainly room for improvement to implement this, but it may not be trivial, and I am not aware of anyone that has tried doing it yet. For comparison, 'qemu-img convert' with an NBD destination is an example of a single NBD connection to the server, while 'nbdcopy' (from the libnbd project) opens multiple client connections; you can indeed see performance improvements with nbdcopy taking advantage of the parallelism made possible by multiple sockets. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PULL 18/20] block/nbd: drop connection_co
On Wed, Feb 02, 2022 at 12:49:36PM +0100, Fabian Ebner wrote: > Am 27.09.21 um 23:55 schrieb Eric Blake: > > From: Vladimir Sementsov-Ogievskiy > > > > OK, that's a big rewrite of the logic. > > > > Pre-patch we have an always running coroutine - connection_co. It does > > reply receiving and reconnecting. And it leads to a lot of difficult > > and unobvious code around drained sections and context switch. We also > > abuse bs->in_flight counter which is increased for connection_co and > > temporary decreased in points where we want to allow drained section to > > begin. One of these place is in another file: in nbd_read_eof() in > > nbd/client.c. > > > > We also cancel reconnect and requests waiting for reconnect on drained > > begin which is not correct. And this patch fixes that. > > > > Let's finally drop this always running coroutine and go another way: > > do both reconnect and receiving in request coroutines. > > > > Hi, > > while updating our stack to 6.2, one of our live-migration tests stopped > working (backtrace is below) and bisecting led me to this patch. > > The VM has a single qcow2 disk (converting to raw doesn't make a > difference) and the issue only appears when using iothread (for both > virtio-scsi-pci and virtio-block-pci). > > Reverting 1af7737871fb3b66036f5e520acb0a98fc2605f7 (which lives on top) > and 4ddb5d2fde6f22b2cf65f314107e890a7ca14fcf (the commit corresponding > to this patch) in v6.2.0 makes the migration work again. > > Backtrace: > > Thread 1 (Thread 0x7f9d93458fc0 (LWP 56711) "kvm"): > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > #1 0x7f9d9d6bc537 in __GI_abort () at abort.c:79 > #2 0x7f9d9d6bc40f in __assert_fail_base (fmt=0x7f9d9d825128 > "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5579153763f8 > "qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)", > file=0x5579153764f9 "../io/channel.c", line=483, function= out>) at assert.c:92 Given that this assertion is about which aio context is set, I wonder if the conversation at https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg00096.html is relevant; if so, Vladimir may already be working on the patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PULL 18/20] block/nbd: drop connection_co
Am 27.09.21 um 23:55 schrieb Eric Blake: > From: Vladimir Sementsov-Ogievskiy > > OK, that's a big rewrite of the logic. > > Pre-patch we have an always running coroutine - connection_co. It does > reply receiving and reconnecting. And it leads to a lot of difficult > and unobvious code around drained sections and context switch. We also > abuse bs->in_flight counter which is increased for connection_co and > temporary decreased in points where we want to allow drained section to > begin. One of these place is in another file: in nbd_read_eof() in > nbd/client.c. > > We also cancel reconnect and requests waiting for reconnect on drained > begin which is not correct. And this patch fixes that. > > Let's finally drop this always running coroutine and go another way: > do both reconnect and receiving in request coroutines. > Hi, while updating our stack to 6.2, one of our live-migration tests stopped working (backtrace is below) and bisecting led me to this patch. The VM has a single qcow2 disk (converting to raw doesn't make a difference) and the issue only appears when using iothread (for both virtio-scsi-pci and virtio-block-pci). Reverting 1af7737871fb3b66036f5e520acb0a98fc2605f7 (which lives on top) and 4ddb5d2fde6f22b2cf65f314107e890a7ca14fcf (the commit corresponding to this patch) in v6.2.0 makes the migration work again. Backtrace: Thread 1 (Thread 0x7f9d93458fc0 (LWP 56711) "kvm"): #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x7f9d9d6bc537 in __GI_abort () at abort.c:79 #2 0x7f9d9d6bc40f in __assert_fail_base (fmt=0x7f9d9d825128 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5579153763f8 "qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)", file=0x5579153764f9 "../io/channel.c", line=483, function=) at assert.c:92 #3 0x7f9d9d6cb662 in __GI___assert_fail (assertion=assertion@entry=0x5579153763f8 "qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)", file=file@entry=0x5579153764f9 "../io/channel.c", line=line@entry=483, function=function@entry=0x557915376570 <__PRETTY_FUNCTION__.2> "qio_channel_restart_read") at assert.c:101 #4 0x5579150c351c in qio_channel_restart_read (opaque=) at ../io/channel.c:483 #5 qio_channel_restart_read (opaque=) at ../io/channel.c:477 #6 0x55791520182a in aio_dispatch_handler (ctx=ctx@entry=0x557916908c60, node=0x7f9d8400f800) at ../util/aio-posix.c:329 #7 0x557915201f62 in aio_dispatch_handlers (ctx=0x557916908c60) at ../util/aio-posix.c:372 #8 aio_dispatch (ctx=0x557916908c60) at ../util/aio-posix.c:382 #9 0x5579151ea74e in aio_ctx_dispatch (source=, callback=, user_data=) at ../util/async.c:311 #10 0x7f9d9e647e6b in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #11 0x557915203030 in glib_pollfds_poll () at ../util/main-loop.c:232 #12 os_host_main_loop_wait (timeout=992816) at ../util/main-loop.c:255 #13 main_loop_wait (nonblocking=nonblocking@entry=0) at ../util/main-loop.c:531 #14 0x5579150539c1 in qemu_main_loop () at ../softmmu/runstate.c:726 #15 0x557914ce8ebe in main (argc=, argv=, envp=) at ../softmmu/main.c:50
Re: [PULL 00/10] Block layer patches
On Tue, 1 Feb 2022 at 15:21, Kevin Wolf wrote: > > The following changes since commit 804b30d25f8d70dc2dea951883ea92235274a50c: > > Merge remote-tracking branch 'remotes/legoater/tags/pull-ppc-20220130' into > staging (2022-01-31 11:10:08 +) > > are available in the Git repository at: > > https://gitlab.com/kmwolf/qemu.git tags/for-upstream > > for you to fetch changes up to fc176116cdea816ceb8dd969080b2b95f58edbc0: > > block/rbd: workaround for ceph issue #53784 (2022-02-01 15:16:32 +0100) > > > Block layer patches > > - rbd: fix handling of holes in .bdrv_co_block_status > - Fix potential crash in bdrv_set_backing_hd() > - vhost-user-blk export: Fix shutdown with requests in flight > - FUSE export: Fix build failure on FreeBSD > - Documentation improvements > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/7.0 for any user-visible changes. -- PMM
