Kevin Wolf <kw...@redhat.com> writes: > Am 19.07.2024 um 06:54 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 09.01.2024 um 14:13 hat Vladimir Sementsov-Ogievskiy geschrieben: >> >> From: Leonid Kaplan <x...@yandex-team.ru> >> >> >> >> BLOCK_IO_ERROR events comes from guest, so we must throttle them. >> >> We still want per-device throttling, so let's use device id as a key. >> >> >> >> Signed-off-by: Leonid Kaplan <x...@yandex-team.ru> >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru> >> >> --- >> >> >> >> v2: add Note: to QAPI doc >> >> >> >> monitor/monitor.c | 10 ++++++++++ >> >> qapi/block-core.json | 2 ++ >> >> 2 files changed, 12 insertions(+) >> >> >> >> diff --git a/monitor/monitor.c b/monitor/monitor.c >> >> index 01ede1babd..ad0243e9d7 100644 >> >> --- a/monitor/monitor.c >> >> +++ b/monitor/monitor.c >> >> @@ -309,6 +309,7 @@ int error_printf_unless_qmp(const char *fmt, ...) >> >> static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = { >> >> /* Limit guest-triggerable events to 1 per second */ >> >> [QAPI_EVENT_RTC_CHANGE] = { 1000 * SCALE_MS }, >> >> + [QAPI_EVENT_BLOCK_IO_ERROR] = { 1000 * SCALE_MS }, >> >> [QAPI_EVENT_WATCHDOG] = { 1000 * SCALE_MS }, >> >> [QAPI_EVENT_BALLOON_CHANGE] = { 1000 * SCALE_MS }, >> >> [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS }, >> >> @@ -498,6 +499,10 @@ static unsigned int qapi_event_throttle_hash(const >> >> void *key) >> >> hash += g_str_hash(qdict_get_str(evstate->data, "qom-path")); >> >> } >> >> >> >> + if (evstate->event == QAPI_EVENT_BLOCK_IO_ERROR) { >> >> + hash += g_str_hash(qdict_get_str(evstate->data, "device")); >> >> + } >> > >> > Using "device" only works with -drive, i.e. when the BlockBackend >> > actually has a name. In modern configurations with a -blockdev >> > referenced by -device, the BlockBackend doesn't have a name any more. >> > >> > Maybe we should be using the qdev id (or more generally, QOM path) here, >> > but that's something the event doesn't even contain yet. >> >> Uh, does the event reliably identify the I/O error's node or not? If >> not, then that's a serious design defect. >> >> There's @node-name. Several commands use "either @device or @node-name" >> to identify a node. Is that sufficient here? > > Possibly. The QAPI event is sent by a device, not by the backend, and > the commit message claims per-device throttling. That's what made me > think that we should base it on the device id.
This is an argument for having the event point at the device. Events that already point at the device carry a mandatory @qom-path, and some also carry an optional qdev @id for backward compatibility. As far as I can tell, not all devices implement this event. If that's true, then documentation should spell it out. > But it's true that the error does originate in the backend (and it's > unlikely that two devices are attached to the same node anyway), so the > node-name could be good enough if we don't have a BlockBackend name. We > should claim per-block-node rather then per-device throttling then. Yes.