1 | The following changes since commit 508ba0f7e2092d3ca56e3f75e894d52d8b94818e: | 1 | The following changes since commit 6c769690ac845fa62642a5f93b4e4bd906adab95: |
---|---|---|---|
2 | 2 | ||
3 | Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20171109' into staging (2017-11-13 11:41:47 +0000) | 3 | Merge remote-tracking branch 'remotes/vsementsov/tags/pull-simplebench-2021-05-04' into staging (2021-05-21 12:02:34 +0100) |
4 | 4 | ||
5 | are available in the git repository at: | 5 | are available in the Git repository at: |
6 | 6 | ||
7 | git://github.com/stefanha/qemu.git tags/block-pull-request | 7 | https://gitlab.com/stefanha/qemu.git tags/block-pull-request |
8 | 8 | ||
9 | for you to fetch changes up to 0761562687e0d8135310a94b1d3e08376387c027: | 9 | for you to fetch changes up to 0a6f0c76a030710780ce10d6347a70f098024d21: |
10 | 10 | ||
11 | qemu-iotests: Test I/O limits with removable media (2017-11-13 15:46:26 +0000) | 11 | coroutine-sleep: introduce qemu_co_sleep (2021-05-21 18:22:33 +0100) |
12 | 12 | ||
13 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
14 | Pull request | 14 | Pull request |
15 | 15 | ||
16 | The following disk I/O throttling fixes solve recent bugs. | 16 | (Resent due to an email preparation mistake.) |
17 | 17 | ||
18 | ---------------------------------------------------------------- | 18 | ---------------------------------------------------------------- |
19 | 19 | ||
20 | Alberto Garcia (3): | 20 | Paolo Bonzini (6): |
21 | block: Check for inserted BlockDriverState in blk_io_limits_disable() | 21 | coroutine-sleep: use a stack-allocated timer |
22 | block: Leave valid throttle timers when removing a BDS from a backend | 22 | coroutine-sleep: disallow NULL QemuCoSleepState** argument |
23 | qemu-iotests: Test I/O limits with removable media | 23 | coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing |
24 | coroutine-sleep: move timer out of QemuCoSleepState | ||
25 | coroutine-sleep: replace QemuCoSleepState pointer with struct in the | ||
26 | API | ||
27 | coroutine-sleep: introduce qemu_co_sleep | ||
24 | 28 | ||
25 | Stefan Hajnoczi (1): | 29 | Philippe Mathieu-Daudé (1): |
26 | throttle-groups: drain before detaching ThrottleState | 30 | bitops.h: Improve find_xxx_bit() documentation |
27 | 31 | ||
28 | Zhengui (1): | 32 | Zenghui Yu (1): |
29 | block: all I/O should be completed before removing throttle timers. | 33 | multi-process: Initialize variables declared with g_auto* |
30 | 34 | ||
31 | block/block-backend.c | 36 ++++++++++++++++++--------- | 35 | include/qemu/bitops.h | 15 ++++++-- |
32 | block/throttle-groups.c | 6 +++++ | 36 | include/qemu/coroutine.h | 27 ++++++++----- |
33 | tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ | 37 | block/block-copy.c | 10 ++--- |
34 | tests/qemu-iotests/093.out | 4 +-- | 38 | block/nbd.c | 14 +++---- |
35 | 4 files changed, 94 insertions(+), 14 deletions(-) | 39 | hw/remote/memory.c | 5 +-- |
40 | hw/remote/proxy.c | 3 +- | ||
41 | util/qemu-coroutine-sleep.c | 75 +++++++++++++++++++------------------ | ||
42 | 7 files changed, 79 insertions(+), 70 deletions(-) | ||
36 | 43 | ||
37 | -- | 44 | -- |
38 | 2.13.6 | 45 | 2.31.1 |
39 | 46 | ||
40 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | From: Zenghui Yu <yuzenghui@huawei.com> | ||
1 | 2 | ||
3 | Quote docs/devel/style.rst (section "Automatic memory deallocation"): | ||
4 | |||
5 | * Variables declared with g_auto* MUST always be initialized, | ||
6 | otherwise the cleanup function will use uninitialized stack memory | ||
7 | |||
8 | Initialize @name properly to get rid of the compilation error (using | ||
9 | gcc-7.3.0 on CentOS): | ||
10 | |||
11 | ../hw/remote/proxy.c: In function 'pci_proxy_dev_realize': | ||
12 | /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized] | ||
13 | g_free (*pp); | ||
14 | ^~~~~~~~~~~~ | ||
15 | ../hw/remote/proxy.c:350:30: note: 'name' was declared here | ||
16 | g_autofree char *name; | ||
17 | ^~~~ | ||
18 | |||
19 | Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> | ||
20 | Reviewed-by: Jagannathan Raman <jag.raman@oracle.com> | ||
21 | Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
22 | Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com> | ||
23 | Message-id: 20210312112143.1369-1-yuzenghui@huawei.com | ||
24 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
25 | --- | ||
26 | hw/remote/memory.c | 5 ++--- | ||
27 | hw/remote/proxy.c | 3 +-- | ||
28 | 2 files changed, 3 insertions(+), 5 deletions(-) | ||
29 | |||
30 | diff --git a/hw/remote/memory.c b/hw/remote/memory.c | ||
31 | index XXXXXXX..XXXXXXX 100644 | ||
32 | --- a/hw/remote/memory.c | ||
33 | +++ b/hw/remote/memory.c | ||
34 | @@ -XXX,XX +XXX,XX @@ void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp) | ||
35 | |||
36 | remote_sysmem_reset(); | ||
37 | |||
38 | - for (region = 0; region < msg->num_fds; region++) { | ||
39 | - g_autofree char *name; | ||
40 | + for (region = 0; region < msg->num_fds; region++, suffix++) { | ||
41 | + g_autofree char *name = g_strdup_printf("remote-mem-%u", suffix); | ||
42 | subregion = g_new(MemoryRegion, 1); | ||
43 | - name = g_strdup_printf("remote-mem-%u", suffix++); | ||
44 | memory_region_init_ram_from_fd(subregion, NULL, | ||
45 | name, sysmem_info->sizes[region], | ||
46 | true, msg->fds[region], | ||
47 | diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c | ||
48 | index XXXXXXX..XXXXXXX 100644 | ||
49 | --- a/hw/remote/proxy.c | ||
50 | +++ b/hw/remote/proxy.c | ||
51 | @@ -XXX,XX +XXX,XX @@ static void probe_pci_info(PCIDevice *dev, Error **errp) | ||
52 | PCI_BASE_ADDRESS_SPACE_IO : PCI_BASE_ADDRESS_SPACE_MEMORY; | ||
53 | |||
54 | if (size) { | ||
55 | - g_autofree char *name; | ||
56 | + g_autofree char *name = g_strdup_printf("bar-region-%d", i); | ||
57 | pdev->region[i].dev = pdev; | ||
58 | pdev->region[i].present = true; | ||
59 | if (type == PCI_BASE_ADDRESS_SPACE_MEMORY) { | ||
60 | pdev->region[i].memory = true; | ||
61 | } | ||
62 | - name = g_strdup_printf("bar-region-%d", i); | ||
63 | memory_region_init_io(&pdev->region[i].mr, OBJECT(pdev), | ||
64 | &proxy_mr_ops, &pdev->region[i], | ||
65 | name, size); | ||
66 | -- | ||
67 | 2.31.1 | ||
68 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | From: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
1 | 2 | ||
3 | Document the following functions return the bitmap size | ||
4 | if no matching bit is found: | ||
5 | |||
6 | - find_first_bit | ||
7 | - find_next_bit | ||
8 | - find_last_bit | ||
9 | - find_first_zero_bit | ||
10 | - find_next_zero_bit | ||
11 | |||
12 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
13 | Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> | ||
14 | Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
15 | Message-id: 20210510200758.2623154-2-philmd@redhat.com | ||
16 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
17 | --- | ||
18 | include/qemu/bitops.h | 15 ++++++++++++--- | ||
19 | 1 file changed, 12 insertions(+), 3 deletions(-) | ||
20 | |||
21 | diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h | ||
22 | index XXXXXXX..XXXXXXX 100644 | ||
23 | --- a/include/qemu/bitops.h | ||
24 | +++ b/include/qemu/bitops.h | ||
25 | @@ -XXX,XX +XXX,XX @@ static inline int test_bit(long nr, const unsigned long *addr) | ||
26 | * @addr: The address to start the search at | ||
27 | * @size: The maximum size to search | ||
28 | * | ||
29 | - * Returns the bit number of the first set bit, or size. | ||
30 | + * Returns the bit number of the last set bit, | ||
31 | + * or @size if there is no set bit in the bitmap. | ||
32 | */ | ||
33 | unsigned long find_last_bit(const unsigned long *addr, | ||
34 | unsigned long size); | ||
35 | @@ -XXX,XX +XXX,XX @@ unsigned long find_last_bit(const unsigned long *addr, | ||
36 | * @addr: The address to base the search on | ||
37 | * @offset: The bitnumber to start searching at | ||
38 | * @size: The bitmap size in bits | ||
39 | + * | ||
40 | + * Returns the bit number of the next set bit, | ||
41 | + * or @size if there are no further set bits in the bitmap. | ||
42 | */ | ||
43 | unsigned long find_next_bit(const unsigned long *addr, | ||
44 | unsigned long size, | ||
45 | @@ -XXX,XX +XXX,XX @@ unsigned long find_next_bit(const unsigned long *addr, | ||
46 | * @addr: The address to base the search on | ||
47 | * @offset: The bitnumber to start searching at | ||
48 | * @size: The bitmap size in bits | ||
49 | + * | ||
50 | + * Returns the bit number of the next cleared bit, | ||
51 | + * or @size if there are no further clear bits in the bitmap. | ||
52 | */ | ||
53 | |||
54 | unsigned long find_next_zero_bit(const unsigned long *addr, | ||
55 | @@ -XXX,XX +XXX,XX @@ unsigned long find_next_zero_bit(const unsigned long *addr, | ||
56 | * @addr: The address to start the search at | ||
57 | * @size: The maximum size to search | ||
58 | * | ||
59 | - * Returns the bit number of the first set bit. | ||
60 | + * Returns the bit number of the first set bit, | ||
61 | + * or @size if there is no set bit in the bitmap. | ||
62 | */ | ||
63 | static inline unsigned long find_first_bit(const unsigned long *addr, | ||
64 | unsigned long size) | ||
65 | @@ -XXX,XX +XXX,XX @@ static inline unsigned long find_first_bit(const unsigned long *addr, | ||
66 | * @addr: The address to start the search at | ||
67 | * @size: The maximum size to search | ||
68 | * | ||
69 | - * Returns the bit number of the first cleared bit. | ||
70 | + * Returns the bit number of the first cleared bit, | ||
71 | + * or @size if there is no clear bit in the bitmap. | ||
72 | */ | ||
73 | static inline unsigned long find_first_zero_bit(const unsigned long *addr, | ||
74 | unsigned long size) | ||
75 | -- | ||
76 | 2.31.1 | ||
77 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | From: Paolo Bonzini <pbonzini@redhat.com> | ||
1 | 2 | ||
3 | The lifetime of the timer is well-known (it cannot outlive | ||
4 | qemu_co_sleep_ns_wakeable, because it's deleted by the time the | ||
5 | coroutine resumes), so it is not necessary to place it on the heap. | ||
6 | |||
7 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> | ||
8 | Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> | ||
9 | Message-id: 20210517100548.28806-2-pbonzini@redhat.com | ||
10 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
11 | --- | ||
12 | util/qemu-coroutine-sleep.c | 9 ++++----- | ||
13 | 1 file changed, 4 insertions(+), 5 deletions(-) | ||
14 | |||
15 | diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c | ||
16 | index XXXXXXX..XXXXXXX 100644 | ||
17 | --- a/util/qemu-coroutine-sleep.c | ||
18 | +++ b/util/qemu-coroutine-sleep.c | ||
19 | @@ -XXX,XX +XXX,XX @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns"; | ||
20 | |||
21 | struct QemuCoSleepState { | ||
22 | Coroutine *co; | ||
23 | - QEMUTimer *ts; | ||
24 | + QEMUTimer ts; | ||
25 | QemuCoSleepState **user_state_pointer; | ||
26 | }; | ||
27 | |||
28 | @@ -XXX,XX +XXX,XX @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) | ||
29 | if (sleep_state->user_state_pointer) { | ||
30 | *sleep_state->user_state_pointer = NULL; | ||
31 | } | ||
32 | - timer_del(sleep_state->ts); | ||
33 | + timer_del(&sleep_state->ts); | ||
34 | aio_co_wake(sleep_state->co); | ||
35 | } | ||
36 | |||
37 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
38 | AioContext *ctx = qemu_get_current_aio_context(); | ||
39 | QemuCoSleepState state = { | ||
40 | .co = qemu_coroutine_self(), | ||
41 | - .ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &state), | ||
42 | .user_state_pointer = sleep_state, | ||
43 | }; | ||
44 | |||
45 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
46 | abort(); | ||
47 | } | ||
48 | |||
49 | + aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state); | ||
50 | if (sleep_state) { | ||
51 | *sleep_state = &state; | ||
52 | } | ||
53 | - timer_mod(state.ts, qemu_clock_get_ns(type) + ns); | ||
54 | + timer_mod(&state.ts, qemu_clock_get_ns(type) + ns); | ||
55 | qemu_coroutine_yield(); | ||
56 | if (sleep_state) { | ||
57 | /* | ||
58 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
59 | */ | ||
60 | assert(*sleep_state == NULL); | ||
61 | } | ||
62 | - timer_free(state.ts); | ||
63 | } | ||
64 | -- | ||
65 | 2.31.1 | ||
66 | diff view generated by jsdifflib |
1 | From: Alberto Garcia <berto@igalia.com> | 1 | From: Paolo Bonzini <pbonzini@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | This test hotplugs a CD drive to a VM and checks that I/O limits can | 3 | Simplify the code by removing conditionals. qemu_co_sleep_ns |
4 | be set only when the drive has media inserted and that they are kept | 4 | can simply point the argument to an on-stack temporary. |
5 | when the media is replaced. | ||
6 | 5 | ||
7 | This also tests the removal of a device with valid I/O limits set but | 6 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
8 | no media inserted. This involves deleting and disabling the limits | 7 | Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> |
9 | of a BlockBackend without BlockDriverState, a scenario that has been | 8 | Message-id: 20210517100548.28806-3-pbonzini@redhat.com |
10 | crashing until the fixes from the last couple of patches. | ||
11 | |||
12 | [Python PEP8 fixup: "Don't use spaces are the = sign when used to | ||
13 | indicate a keyword argument or a default parameter value" | ||
14 | --Stefan] | ||
15 | |||
16 | Signed-off-by: Alberto Garcia <berto@igalia.com> | ||
17 | Reviewed-by: Max Reitz <mreitz@redhat.com> | ||
18 | Message-id: 071eb397118ed207c5a7f01d58766e415ee18d6a.1510339534.git.berto@igalia.com | ||
19 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 9 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
20 | --- | 10 | --- |
21 | tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++ | 11 | include/qemu/coroutine.h | 5 +++-- |
22 | tests/qemu-iotests/093.out | 4 +-- | 12 | util/qemu-coroutine-sleep.c | 18 +++++------------- |
23 | 2 files changed, 64 insertions(+), 2 deletions(-) | 13 | 2 files changed, 8 insertions(+), 15 deletions(-) |
24 | 14 | ||
25 | diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093 | 15 | diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h |
26 | index XXXXXXX..XXXXXXX 100755 | 16 | index XXXXXXX..XXXXXXX 100644 |
27 | --- a/tests/qemu-iotests/093 | 17 | --- a/include/qemu/coroutine.h |
28 | +++ b/tests/qemu-iotests/093 | 18 | +++ b/include/qemu/coroutine.h |
29 | @@ -XXX,XX +XXX,XX @@ class ThrottleTestGroupNames(iotests.QMPTestCase): | 19 | @@ -XXX,XX +XXX,XX @@ typedef struct QemuCoSleepState QemuCoSleepState; |
30 | groupname = "group%d" % i | 20 | |
31 | self.verify_name(devname, groupname) | 21 | /** |
32 | 22 | * Yield the coroutine for a given duration. During this yield, @sleep_state | |
33 | +class ThrottleTestRemovableMedia(iotests.QMPTestCase): | 23 | - * (if not NULL) is set to an opaque pointer, which may be used for |
34 | + def setUp(self): | 24 | + * is set to an opaque pointer, which may be used for |
35 | + self.vm = iotests.VM() | 25 | * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the |
36 | + if iotests.qemu_default_machine == 's390-ccw-virtio': | 26 | * timer fires. Don't save the obtained value to other variables and don't call |
37 | + self.vm.add_device("virtio-scsi-ccw,id=virtio-scsi") | 27 | * qemu_co_sleep_wake from another aio context. |
38 | + else: | 28 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, |
39 | + self.vm.add_device("virtio-scsi-pci,id=virtio-scsi") | 29 | QemuCoSleepState **sleep_state); |
40 | + self.vm.launch() | 30 | static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) |
31 | { | ||
32 | - qemu_co_sleep_ns_wakeable(type, ns, NULL); | ||
33 | + QemuCoSleepState *unused = NULL; | ||
34 | + qemu_co_sleep_ns_wakeable(type, ns, &unused); | ||
35 | } | ||
36 | |||
37 | /** | ||
38 | diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c | ||
39 | index XXXXXXX..XXXXXXX 100644 | ||
40 | --- a/util/qemu-coroutine-sleep.c | ||
41 | +++ b/util/qemu-coroutine-sleep.c | ||
42 | @@ -XXX,XX +XXX,XX @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) | ||
43 | qemu_co_sleep_ns__scheduled, NULL); | ||
44 | |||
45 | assert(scheduled == qemu_co_sleep_ns__scheduled); | ||
46 | - if (sleep_state->user_state_pointer) { | ||
47 | - *sleep_state->user_state_pointer = NULL; | ||
48 | - } | ||
49 | + *sleep_state->user_state_pointer = NULL; | ||
50 | timer_del(&sleep_state->ts); | ||
51 | aio_co_wake(sleep_state->co); | ||
52 | } | ||
53 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
54 | } | ||
55 | |||
56 | aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state); | ||
57 | - if (sleep_state) { | ||
58 | - *sleep_state = &state; | ||
59 | - } | ||
60 | + *sleep_state = &state; | ||
61 | timer_mod(&state.ts, qemu_clock_get_ns(type) + ns); | ||
62 | qemu_coroutine_yield(); | ||
63 | - if (sleep_state) { | ||
64 | - /* | ||
65 | - * Note that *sleep_state is cleared during qemu_co_sleep_wake | ||
66 | - * before resuming this coroutine. | ||
67 | - */ | ||
68 | - assert(*sleep_state == NULL); | ||
69 | - } | ||
41 | + | 70 | + |
42 | + def tearDown(self): | 71 | + /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine. */ |
43 | + self.vm.shutdown() | 72 | + assert(*sleep_state == NULL); |
44 | + | 73 | } |
45 | + def test_removable_media(self): | ||
46 | + # Add a couple of dummy nodes named cd0 and cd1 | ||
47 | + result = self.vm.qmp("blockdev-add", driver="null-aio", | ||
48 | + node_name="cd0") | ||
49 | + self.assert_qmp(result, 'return', {}) | ||
50 | + result = self.vm.qmp("blockdev-add", driver="null-aio", | ||
51 | + node_name="cd1") | ||
52 | + self.assert_qmp(result, 'return', {}) | ||
53 | + | ||
54 | + # Attach a CD drive with cd0 inserted | ||
55 | + result = self.vm.qmp("device_add", driver="scsi-cd", | ||
56 | + id="dev0", drive="cd0") | ||
57 | + self.assert_qmp(result, 'return', {}) | ||
58 | + | ||
59 | + # Set I/O limits | ||
60 | + args = { "id": "dev0", "iops": 100, "iops_rd": 0, "iops_wr": 0, | ||
61 | + "bps": 50, "bps_rd": 0, "bps_wr": 0 } | ||
62 | + result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **args) | ||
63 | + self.assert_qmp(result, 'return', {}) | ||
64 | + | ||
65 | + # Check that the I/O limits have been set | ||
66 | + result = self.vm.qmp("query-block") | ||
67 | + self.assert_qmp(result, 'return[0]/inserted/iops', 100) | ||
68 | + self.assert_qmp(result, 'return[0]/inserted/bps', 50) | ||
69 | + | ||
70 | + # Now eject cd0 and insert cd1 | ||
71 | + result = self.vm.qmp("blockdev-open-tray", id='dev0') | ||
72 | + self.assert_qmp(result, 'return', {}) | ||
73 | + result = self.vm.qmp("x-blockdev-remove-medium", id='dev0') | ||
74 | + self.assert_qmp(result, 'return', {}) | ||
75 | + result = self.vm.qmp("x-blockdev-insert-medium", id='dev0', node_name='cd1') | ||
76 | + self.assert_qmp(result, 'return', {}) | ||
77 | + | ||
78 | + # Check that the I/O limits are still the same | ||
79 | + result = self.vm.qmp("query-block") | ||
80 | + self.assert_qmp(result, 'return[0]/inserted/iops', 100) | ||
81 | + self.assert_qmp(result, 'return[0]/inserted/bps', 50) | ||
82 | + | ||
83 | + # Eject cd1 | ||
84 | + result = self.vm.qmp("x-blockdev-remove-medium", id='dev0') | ||
85 | + self.assert_qmp(result, 'return', {}) | ||
86 | + | ||
87 | + # Check that we can't set limits if the device has no medium | ||
88 | + result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **args) | ||
89 | + self.assert_qmp(result, 'error/class', 'GenericError') | ||
90 | + | ||
91 | + # Remove the CD drive | ||
92 | + result = self.vm.qmp("device_del", id='dev0') | ||
93 | + self.assert_qmp(result, 'return', {}) | ||
94 | + | ||
95 | |||
96 | if __name__ == '__main__': | ||
97 | iotests.main(supported_fmts=["raw"]) | ||
98 | diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out | ||
99 | index XXXXXXX..XXXXXXX 100644 | ||
100 | --- a/tests/qemu-iotests/093.out | ||
101 | +++ b/tests/qemu-iotests/093.out | ||
102 | @@ -XXX,XX +XXX,XX @@ | ||
103 | -....... | ||
104 | +........ | ||
105 | ---------------------------------------------------------------------- | ||
106 | -Ran 7 tests | ||
107 | +Ran 8 tests | ||
108 | |||
109 | OK | ||
110 | -- | 74 | -- |
111 | 2.13.6 | 75 | 2.31.1 |
112 | 76 | ||
113 | diff view generated by jsdifflib |
1 | From: Alberto Garcia <berto@igalia.com> | 1 | From: Paolo Bonzini <pbonzini@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | When you set I/O limits using block_set_io_throttle or the command | 3 | All callers of qemu_co_sleep_wake are checking whether they are passing |
4 | line throttling.* options they are kept in the BlockBackend regardless | 4 | a NULL argument inside the pointer-to-pointer: do the check in |
5 | of whether a BlockDriverState is attached to the backend or not. | 5 | qemu_co_sleep_wake itself. |
6 | 6 | ||
7 | Therefore when removing the limits using blk_io_limits_disable() we | 7 | As a side effect, qemu_co_sleep_wake can be called more than once and |
8 | need to check if there's a BDS before attempting to drain it, else it | 8 | it will only wake the coroutine once; after the first time, the argument |
9 | will crash QEMU. This can be reproduced very easily using HMP: | 9 | will be set to NULL via *sleep_state->user_state_pointer. However, this |
10 | would not be safe unless co_sleep_cb keeps using the QemuCoSleepState* | ||
11 | directly, so make it go through the pointer-to-pointer instead. | ||
10 | 12 | ||
11 | (qemu) drive_add 0 if=none,throttling.iops-total=5000 | 13 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
12 | (qemu) drive_del none0 | 14 | Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> |
13 | 15 | Message-id: 20210517100548.28806-4-pbonzini@redhat.com | |
14 | Reported-by: sochin jiang <sochin.jiang@huawei.com> | ||
15 | Signed-off-by: Alberto Garcia <berto@igalia.com> | ||
16 | Reviewed-by: Max Reitz <mreitz@redhat.com> | ||
17 | Message-id: 0d3a67ce8d948bb33e08672564714dcfb76a3d8c.1510339534.git.berto@igalia.com | ||
18 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 16 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
19 | --- | 17 | --- |
20 | block/block-backend.c | 14 ++++++++++---- | 18 | block/block-copy.c | 4 +--- |
21 | 1 file changed, 10 insertions(+), 4 deletions(-) | 19 | block/nbd.c | 8 ++------ |
20 | util/qemu-coroutine-sleep.c | 21 ++++++++++++--------- | ||
21 | 3 files changed, 15 insertions(+), 18 deletions(-) | ||
22 | 22 | ||
23 | diff --git a/block/block-backend.c b/block/block-backend.c | 23 | diff --git a/block/block-copy.c b/block/block-copy.c |
24 | index XXXXXXX..XXXXXXX 100644 | 24 | index XXXXXXX..XXXXXXX 100644 |
25 | --- a/block/block-backend.c | 25 | --- a/block/block-copy.c |
26 | +++ b/block/block-backend.c | 26 | +++ b/block/block-copy.c |
27 | @@ -XXX,XX +XXX,XX @@ void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg) | 27 | @@ -XXX,XX +XXX,XX @@ out: |
28 | 28 | ||
29 | void blk_io_limits_disable(BlockBackend *blk) | 29 | void block_copy_kick(BlockCopyCallState *call_state) |
30 | { | 30 | { |
31 | - assert(blk->public.throttle_group_member.throttle_state); | 31 | - if (call_state->sleep_state) { |
32 | - bdrv_drained_begin(blk_bs(blk)); | 32 | - qemu_co_sleep_wake(call_state->sleep_state); |
33 | - throttle_group_unregister_tgm(&blk->public.throttle_group_member); | 33 | - } |
34 | - bdrv_drained_end(blk_bs(blk)); | 34 | + qemu_co_sleep_wake(call_state->sleep_state); |
35 | + BlockDriverState *bs = blk_bs(blk); | 35 | } |
36 | + ThrottleGroupMember *tgm = &blk->public.throttle_group_member; | 36 | |
37 | + assert(tgm->throttle_state); | 37 | /* |
38 | + if (bs) { | 38 | diff --git a/block/nbd.c b/block/nbd.c |
39 | + bdrv_drained_begin(bs); | 39 | index XXXXXXX..XXXXXXX 100644 |
40 | + } | 40 | --- a/block/nbd.c |
41 | + throttle_group_unregister_tgm(tgm); | 41 | +++ b/block/nbd.c |
42 | + if (bs) { | 42 | @@ -XXX,XX +XXX,XX @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs) |
43 | + bdrv_drained_end(bs); | 43 | BDRVNBDState *s = (BDRVNBDState *)bs->opaque; |
44 | |||
45 | s->drained = true; | ||
46 | - if (s->connection_co_sleep_ns_state) { | ||
47 | - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); | ||
48 | - } | ||
49 | + qemu_co_sleep_wake(s->connection_co_sleep_ns_state); | ||
50 | |||
51 | nbd_co_establish_connection_cancel(bs, false); | ||
52 | |||
53 | @@ -XXX,XX +XXX,XX @@ static void nbd_teardown_connection(BlockDriverState *bs) | ||
54 | |||
55 | s->state = NBD_CLIENT_QUIT; | ||
56 | if (s->connection_co) { | ||
57 | - if (s->connection_co_sleep_ns_state) { | ||
58 | - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); | ||
59 | - } | ||
60 | + qemu_co_sleep_wake(s->connection_co_sleep_ns_state); | ||
61 | nbd_co_establish_connection_cancel(bs, true); | ||
62 | } | ||
63 | if (qemu_in_coroutine()) { | ||
64 | diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c | ||
65 | index XXXXXXX..XXXXXXX 100644 | ||
66 | --- a/util/qemu-coroutine-sleep.c | ||
67 | +++ b/util/qemu-coroutine-sleep.c | ||
68 | @@ -XXX,XX +XXX,XX @@ struct QemuCoSleepState { | ||
69 | |||
70 | void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) | ||
71 | { | ||
72 | - /* Write of schedule protected by barrier write in aio_co_schedule */ | ||
73 | - const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled, | ||
74 | - qemu_co_sleep_ns__scheduled, NULL); | ||
75 | + if (sleep_state) { | ||
76 | + /* Write of schedule protected by barrier write in aio_co_schedule */ | ||
77 | + const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled, | ||
78 | + qemu_co_sleep_ns__scheduled, NULL); | ||
79 | |||
80 | - assert(scheduled == qemu_co_sleep_ns__scheduled); | ||
81 | - *sleep_state->user_state_pointer = NULL; | ||
82 | - timer_del(&sleep_state->ts); | ||
83 | - aio_co_wake(sleep_state->co); | ||
84 | + assert(scheduled == qemu_co_sleep_ns__scheduled); | ||
85 | + *sleep_state->user_state_pointer = NULL; | ||
86 | + timer_del(&sleep_state->ts); | ||
87 | + aio_co_wake(sleep_state->co); | ||
44 | + } | 88 | + } |
45 | } | 89 | } |
46 | 90 | ||
47 | /* should be called before blk_set_io_limits if a limit is set */ | 91 | static void co_sleep_cb(void *opaque) |
92 | { | ||
93 | - qemu_co_sleep_wake(opaque); | ||
94 | + QemuCoSleepState **sleep_state = opaque; | ||
95 | + qemu_co_sleep_wake(*sleep_state); | ||
96 | } | ||
97 | |||
98 | void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
99 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
100 | abort(); | ||
101 | } | ||
102 | |||
103 | - aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state); | ||
104 | + aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state); | ||
105 | *sleep_state = &state; | ||
106 | timer_mod(&state.ts, qemu_clock_get_ns(type) + ns); | ||
107 | qemu_coroutine_yield(); | ||
48 | -- | 108 | -- |
49 | 2.13.6 | 109 | 2.31.1 |
50 | 110 | ||
51 | diff view generated by jsdifflib |
1 | From: Alberto Garcia <berto@igalia.com> | 1 | From: Paolo Bonzini <pbonzini@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | If a BlockBackend has I/O limits set then its ThrottleGroupMember | 3 | This simplification is enabled by the previous patch. Now aio_co_wake |
4 | structure uses the AioContext from its attached BlockDriverState. | 4 | will only be called once, therefore we do not care about a spurious |
5 | Those two contexts must be kept in sync manually. This is not | 5 | firing of the timer after a qemu_co_sleep_wake. |
6 | ideal and will be fixed in the future by removing the throttling | ||
7 | configuration from the BlockBackend and storing it in an implicit | ||
8 | filter node instead, but for now we have to live with this. | ||
9 | 6 | ||
10 | When you remove the BlockDriverState from the backend then the | 7 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
11 | throttle timers are destroyed. If a new BlockDriverState is later | 8 | Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> |
12 | inserted then they are created again using the new AioContext. | 9 | Message-id: 20210517100548.28806-5-pbonzini@redhat.com |
13 | |||
14 | There are a couple of problems with this: | ||
15 | |||
16 | a) The code manipulates the timers directly, leaving the | ||
17 | ThrottleGroupMember.aio_context field in an inconsisent state. | ||
18 | |||
19 | b) If you remove the I/O limits (e.g by destroying the backend) | ||
20 | when the timers are gone then throttle_group_unregister_tgm() | ||
21 | will attempt to destroy them again, crashing QEMU. | ||
22 | |||
23 | While b) could be fixed easily by allowing the timers to be freed | ||
24 | twice, this would result in a situation in which we can no longer | ||
25 | guarantee that a valid ThrottleState has a valid AioContext and | ||
26 | timers. | ||
27 | |||
28 | This patch ensures that the timers and AioContext are always valid | ||
29 | when I/O limits are set, regardless of whether the BlockBackend has a | ||
30 | BlockDriverState inserted or not. | ||
31 | |||
32 | [Fixed "There'a" typo as suggested by Max Reitz <mreitz@redhat.com> | ||
33 | --Stefan] | ||
34 | |||
35 | Reported-by: sochin jiang <sochin.jiang@huawei.com> | ||
36 | Signed-off-by: Alberto Garcia <berto@igalia.com> | ||
37 | Reviewed-by: Max Reitz <mreitz@redhat.com> | ||
38 | Message-id: e089c66e7c20289b046d782cea4373b765c5bc1d.1510339534.git.berto@igalia.com | ||
39 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 10 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
40 | --- | 11 | --- |
41 | block/block-backend.c | 16 ++++++++-------- | 12 | util/qemu-coroutine-sleep.c | 8 ++++---- |
42 | 1 file changed, 8 insertions(+), 8 deletions(-) | 13 | 1 file changed, 4 insertions(+), 4 deletions(-) |
43 | 14 | ||
44 | diff --git a/block/block-backend.c b/block/block-backend.c | 15 | diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c |
45 | index XXXXXXX..XXXXXXX 100644 | 16 | index XXXXXXX..XXXXXXX 100644 |
46 | --- a/block/block-backend.c | 17 | --- a/util/qemu-coroutine-sleep.c |
47 | +++ b/block/block-backend.c | 18 | +++ b/util/qemu-coroutine-sleep.c |
48 | @@ -XXX,XX +XXX,XX @@ BlockBackend *blk_by_public(BlockBackendPublic *public) | 19 | @@ -XXX,XX +XXX,XX @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns"; |
49 | */ | 20 | |
50 | void blk_remove_bs(BlockBackend *blk) | 21 | struct QemuCoSleepState { |
22 | Coroutine *co; | ||
23 | - QEMUTimer ts; | ||
24 | QemuCoSleepState **user_state_pointer; | ||
25 | }; | ||
26 | |||
27 | @@ -XXX,XX +XXX,XX @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) | ||
28 | |||
29 | assert(scheduled == qemu_co_sleep_ns__scheduled); | ||
30 | *sleep_state->user_state_pointer = NULL; | ||
31 | - timer_del(&sleep_state->ts); | ||
32 | aio_co_wake(sleep_state->co); | ||
33 | } | ||
34 | } | ||
35 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
36 | QemuCoSleepState **sleep_state) | ||
51 | { | 37 | { |
52 | + ThrottleGroupMember *tgm = &blk->public.throttle_group_member; | 38 | AioContext *ctx = qemu_get_current_aio_context(); |
53 | BlockDriverState *bs; | 39 | + QEMUTimer ts; |
54 | - ThrottleTimers *tt; | 40 | QemuCoSleepState state = { |
55 | 41 | .co = qemu_coroutine_self(), | |
56 | notifier_list_notify(&blk->remove_bs_notifiers, blk); | 42 | .user_state_pointer = sleep_state, |
57 | - if (blk->public.throttle_group_member.throttle_state) { | 43 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, |
58 | - tt = &blk->public.throttle_group_member.throttle_timers; | 44 | abort(); |
59 | + if (tgm->throttle_state) { | ||
60 | bs = blk_bs(blk); | ||
61 | bdrv_drained_begin(bs); | ||
62 | - throttle_timers_detach_aio_context(tt); | ||
63 | + throttle_group_detach_aio_context(tgm); | ||
64 | + throttle_group_attach_aio_context(tgm, qemu_get_aio_context()); | ||
65 | bdrv_drained_end(bs); | ||
66 | } | 45 | } |
67 | 46 | ||
68 | @@ -XXX,XX +XXX,XX @@ void blk_remove_bs(BlockBackend *blk) | 47 | - aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state); |
69 | */ | 48 | + aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, sleep_state); |
70 | int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp) | 49 | *sleep_state = &state; |
71 | { | 50 | - timer_mod(&state.ts, qemu_clock_get_ns(type) + ns); |
72 | + ThrottleGroupMember *tgm = &blk->public.throttle_group_member; | 51 | + timer_mod(&ts, qemu_clock_get_ns(type) + ns); |
73 | blk->root = bdrv_root_attach_child(bs, "root", &child_root, | 52 | qemu_coroutine_yield(); |
74 | blk->perm, blk->shared_perm, blk, errp); | 53 | + timer_del(&ts); |
75 | if (blk->root == NULL) { | 54 | |
76 | @@ -XXX,XX +XXX,XX @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp) | 55 | /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine. */ |
77 | bdrv_ref(bs); | 56 | assert(*sleep_state == NULL); |
78 | |||
79 | notifier_list_notify(&blk->insert_bs_notifiers, blk); | ||
80 | - if (blk->public.throttle_group_member.throttle_state) { | ||
81 | - throttle_timers_attach_aio_context( | ||
82 | - &blk->public.throttle_group_member.throttle_timers, | ||
83 | - bdrv_get_aio_context(bs)); | ||
84 | + if (tgm->throttle_state) { | ||
85 | + throttle_group_detach_aio_context(tgm); | ||
86 | + throttle_group_attach_aio_context(tgm, bdrv_get_aio_context(bs)); | ||
87 | } | ||
88 | |||
89 | return 0; | ||
90 | -- | 57 | -- |
91 | 2.13.6 | 58 | 2.31.1 |
92 | 59 | ||
93 | diff view generated by jsdifflib |
1 | I/O requests hang after stop/cont commands at least since QEMU 2.10.0 | 1 | From: Paolo Bonzini <pbonzini@redhat.com> |
---|---|---|---|
2 | with -drive iops=100: | 2 | |
3 | 3 | Right now, users of qemu_co_sleep_ns_wakeable are simply passing | |
4 | (guest)$ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000 | 4 | a pointer to QemuCoSleepState by reference to the function. But |
5 | (qemu) stop | 5 | QemuCoSleepState really is just a Coroutine*; making the |
6 | (qemu) cont | 6 | content of the struct public is just as efficient and lets us |
7 | ...I/O is stuck... | 7 | skip the user_state_pointer indirection. |
8 | 8 | ||
9 | This happens because blk_set_aio_context() detaches the ThrottleState | 9 | Since the usage is changed, take the occasion to rename the |
10 | while requests may still be in flight: | 10 | struct to QemuCoSleep. |
11 | 11 | ||
12 | if (tgm->throttle_state) { | 12 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
13 | throttle_group_detach_aio_context(tgm); | 13 | Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> |
14 | throttle_group_attach_aio_context(tgm, new_context); | 14 | Message-id: 20210517100548.28806-6-pbonzini@redhat.com |
15 | } | ||
16 | |||
17 | This patch encloses the detach/attach calls in a drained region so no | ||
18 | I/O request is left hanging. Also add assertions so we don't make the | ||
19 | same mistake again in the future. | ||
20 | |||
21 | Reported-by: Yongxue Hong <yhong@redhat.com> | ||
22 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
23 | Reviewed-by: Alberto Garcia <berto@igalia.com> | ||
24 | Message-id: 20171110151934.16883-1-stefanha@redhat.com | ||
25 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 15 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
26 | --- | 16 | --- |
27 | block/block-backend.c | 2 ++ | 17 | include/qemu/coroutine.h | 23 +++++++++++---------- |
28 | block/throttle-groups.c | 6 ++++++ | 18 | block/block-copy.c | 8 ++++---- |
29 | 2 files changed, 8 insertions(+) | 19 | block/nbd.c | 10 ++++----- |
30 | 20 | util/qemu-coroutine-sleep.c | 41 ++++++++++++++++--------------------- | |
31 | diff --git a/block/block-backend.c b/block/block-backend.c | 21 | 4 files changed, 39 insertions(+), 43 deletions(-) |
32 | index XXXXXXX..XXXXXXX 100644 | 22 | |
33 | --- a/block/block-backend.c | 23 | diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h |
34 | +++ b/block/block-backend.c | 24 | index XXXXXXX..XXXXXXX 100644 |
35 | @@ -XXX,XX +XXX,XX @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context) | 25 | --- a/include/qemu/coroutine.h |
36 | 26 | +++ b/include/qemu/coroutine.h | |
37 | if (bs) { | 27 | @@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_wrlock(CoRwlock *lock); |
38 | if (tgm->throttle_state) { | 28 | */ |
39 | + bdrv_drained_begin(bs); | 29 | void qemu_co_rwlock_unlock(CoRwlock *lock); |
40 | throttle_group_detach_aio_context(tgm); | 30 | |
41 | throttle_group_attach_aio_context(tgm, new_context); | 31 | -typedef struct QemuCoSleepState QemuCoSleepState; |
42 | + bdrv_drained_end(bs); | 32 | +typedef struct QemuCoSleep { |
43 | } | 33 | + Coroutine *to_wake; |
44 | bdrv_set_aio_context(bs, new_context); | 34 | +} QemuCoSleep; |
35 | |||
36 | /** | ||
37 | - * Yield the coroutine for a given duration. During this yield, @sleep_state | ||
38 | - * is set to an opaque pointer, which may be used for | ||
39 | - * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the | ||
40 | - * timer fires. Don't save the obtained value to other variables and don't call | ||
41 | - * qemu_co_sleep_wake from another aio context. | ||
42 | + * Yield the coroutine for a given duration. Initializes @w so that, | ||
43 | + * during this yield, it can be passed to qemu_co_sleep_wake() to | ||
44 | + * terminate the sleep. | ||
45 | */ | ||
46 | -void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
47 | - QemuCoSleepState **sleep_state); | ||
48 | +void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, | ||
49 | + QEMUClockType type, int64_t ns); | ||
50 | + | ||
51 | static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) | ||
52 | { | ||
53 | - QemuCoSleepState *unused = NULL; | ||
54 | - qemu_co_sleep_ns_wakeable(type, ns, &unused); | ||
55 | + QemuCoSleep w = { 0 }; | ||
56 | + qemu_co_sleep_ns_wakeable(&w, type, ns); | ||
57 | } | ||
58 | |||
59 | /** | ||
60 | @@ -XXX,XX +XXX,XX @@ static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) | ||
61 | * qemu_co_sleep_ns() and should be checked to be non-NULL before calling | ||
62 | * qemu_co_sleep_wake(). | ||
63 | */ | ||
64 | -void qemu_co_sleep_wake(QemuCoSleepState *sleep_state); | ||
65 | +void qemu_co_sleep_wake(QemuCoSleep *w); | ||
66 | |||
67 | /** | ||
68 | * Yield until a file descriptor becomes readable | ||
69 | diff --git a/block/block-copy.c b/block/block-copy.c | ||
70 | index XXXXXXX..XXXXXXX 100644 | ||
71 | --- a/block/block-copy.c | ||
72 | +++ b/block/block-copy.c | ||
73 | @@ -XXX,XX +XXX,XX @@ typedef struct BlockCopyCallState { | ||
74 | /* State */ | ||
75 | int ret; | ||
76 | bool finished; | ||
77 | - QemuCoSleepState *sleep_state; | ||
78 | + QemuCoSleep sleep; | ||
79 | bool cancelled; | ||
80 | |||
81 | /* OUT parameters */ | ||
82 | @@ -XXX,XX +XXX,XX @@ block_copy_dirty_clusters(BlockCopyCallState *call_state) | ||
83 | if (ns > 0) { | ||
84 | block_copy_task_end(task, -EAGAIN); | ||
85 | g_free(task); | ||
86 | - qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, ns, | ||
87 | - &call_state->sleep_state); | ||
88 | + qemu_co_sleep_ns_wakeable(&call_state->sleep, | ||
89 | + QEMU_CLOCK_REALTIME, ns); | ||
90 | continue; | ||
91 | } | ||
92 | } | ||
93 | @@ -XXX,XX +XXX,XX @@ out: | ||
94 | |||
95 | void block_copy_kick(BlockCopyCallState *call_state) | ||
96 | { | ||
97 | - qemu_co_sleep_wake(call_state->sleep_state); | ||
98 | + qemu_co_sleep_wake(&call_state->sleep); | ||
99 | } | ||
100 | |||
101 | /* | ||
102 | diff --git a/block/nbd.c b/block/nbd.c | ||
103 | index XXXXXXX..XXXXXXX 100644 | ||
104 | --- a/block/nbd.c | ||
105 | +++ b/block/nbd.c | ||
106 | @@ -XXX,XX +XXX,XX @@ typedef struct BDRVNBDState { | ||
107 | CoQueue free_sema; | ||
108 | Coroutine *connection_co; | ||
109 | Coroutine *teardown_co; | ||
110 | - QemuCoSleepState *connection_co_sleep_ns_state; | ||
111 | + QemuCoSleep reconnect_sleep; | ||
112 | bool drained; | ||
113 | bool wait_drained_end; | ||
114 | int in_flight; | ||
115 | @@ -XXX,XX +XXX,XX @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs) | ||
116 | BDRVNBDState *s = (BDRVNBDState *)bs->opaque; | ||
117 | |||
118 | s->drained = true; | ||
119 | - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); | ||
120 | + qemu_co_sleep_wake(&s->reconnect_sleep); | ||
121 | |||
122 | nbd_co_establish_connection_cancel(bs, false); | ||
123 | |||
124 | @@ -XXX,XX +XXX,XX @@ static void nbd_teardown_connection(BlockDriverState *bs) | ||
125 | |||
126 | s->state = NBD_CLIENT_QUIT; | ||
127 | if (s->connection_co) { | ||
128 | - qemu_co_sleep_wake(s->connection_co_sleep_ns_state); | ||
129 | + qemu_co_sleep_wake(&s->reconnect_sleep); | ||
130 | nbd_co_establish_connection_cancel(bs, true); | ||
45 | } | 131 | } |
46 | diff --git a/block/throttle-groups.c b/block/throttle-groups.c | 132 | if (qemu_in_coroutine()) { |
47 | index XXXXXXX..XXXXXXX 100644 | 133 | @@ -XXX,XX +XXX,XX @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState *s) |
48 | --- a/block/throttle-groups.c | 134 | } |
49 | +++ b/block/throttle-groups.c | 135 | bdrv_inc_in_flight(s->bs); |
50 | @@ -XXX,XX +XXX,XX @@ void throttle_group_attach_aio_context(ThrottleGroupMember *tgm, | 136 | } else { |
51 | void throttle_group_detach_aio_context(ThrottleGroupMember *tgm) | 137 | - qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, timeout, |
52 | { | 138 | - &s->connection_co_sleep_ns_state); |
53 | ThrottleTimers *tt = &tgm->throttle_timers; | 139 | + qemu_co_sleep_ns_wakeable(&s->reconnect_sleep, |
54 | + | 140 | + QEMU_CLOCK_REALTIME, timeout); |
55 | + /* Requests must have been drained */ | 141 | if (s->drained) { |
56 | + assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0); | 142 | continue; |
57 | + assert(qemu_co_queue_empty(&tgm->throttled_reqs[0])); | 143 | } |
58 | + assert(qemu_co_queue_empty(&tgm->throttled_reqs[1])); | 144 | diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c |
59 | + | 145 | index XXXXXXX..XXXXXXX 100644 |
60 | throttle_timers_detach_aio_context(tt); | 146 | --- a/util/qemu-coroutine-sleep.c |
61 | tgm->aio_context = NULL; | 147 | +++ b/util/qemu-coroutine-sleep.c |
148 | @@ -XXX,XX +XXX,XX @@ | ||
149 | |||
150 | static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns"; | ||
151 | |||
152 | -struct QemuCoSleepState { | ||
153 | +void qemu_co_sleep_wake(QemuCoSleep *w) | ||
154 | +{ | ||
155 | Coroutine *co; | ||
156 | - QemuCoSleepState **user_state_pointer; | ||
157 | -}; | ||
158 | |||
159 | -void qemu_co_sleep_wake(QemuCoSleepState *sleep_state) | ||
160 | -{ | ||
161 | - if (sleep_state) { | ||
162 | + co = w->to_wake; | ||
163 | + w->to_wake = NULL; | ||
164 | + if (co) { | ||
165 | /* Write of schedule protected by barrier write in aio_co_schedule */ | ||
166 | - const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled, | ||
167 | + const char *scheduled = qatomic_cmpxchg(&co->scheduled, | ||
168 | qemu_co_sleep_ns__scheduled, NULL); | ||
169 | |||
170 | assert(scheduled == qemu_co_sleep_ns__scheduled); | ||
171 | - *sleep_state->user_state_pointer = NULL; | ||
172 | - aio_co_wake(sleep_state->co); | ||
173 | + aio_co_wake(co); | ||
174 | } | ||
175 | } | ||
176 | |||
177 | static void co_sleep_cb(void *opaque) | ||
178 | { | ||
179 | - QemuCoSleepState **sleep_state = opaque; | ||
180 | - qemu_co_sleep_wake(*sleep_state); | ||
181 | + QemuCoSleep *w = opaque; | ||
182 | + qemu_co_sleep_wake(w); | ||
183 | } | ||
184 | |||
185 | -void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
186 | - QemuCoSleepState **sleep_state) | ||
187 | +void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, | ||
188 | + QEMUClockType type, int64_t ns) | ||
189 | { | ||
190 | + Coroutine *co = qemu_coroutine_self(); | ||
191 | AioContext *ctx = qemu_get_current_aio_context(); | ||
192 | QEMUTimer ts; | ||
193 | - QemuCoSleepState state = { | ||
194 | - .co = qemu_coroutine_self(), | ||
195 | - .user_state_pointer = sleep_state, | ||
196 | - }; | ||
197 | |||
198 | - const char *scheduled = qatomic_cmpxchg(&state.co->scheduled, NULL, | ||
199 | - qemu_co_sleep_ns__scheduled); | ||
200 | + const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL, | ||
201 | + qemu_co_sleep_ns__scheduled); | ||
202 | if (scheduled) { | ||
203 | fprintf(stderr, | ||
204 | "%s: Co-routine was already scheduled in '%s'\n", | ||
205 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns, | ||
206 | abort(); | ||
207 | } | ||
208 | |||
209 | - aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, sleep_state); | ||
210 | - *sleep_state = &state; | ||
211 | + w->to_wake = co; | ||
212 | + aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w), | ||
213 | timer_mod(&ts, qemu_clock_get_ns(type) + ns); | ||
214 | qemu_coroutine_yield(); | ||
215 | timer_del(&ts); | ||
216 | |||
217 | - /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine. */ | ||
218 | - assert(*sleep_state == NULL); | ||
219 | + /* w->to_wake is cleared before resuming this coroutine. */ | ||
220 | + assert(w->to_wake == NULL); | ||
62 | } | 221 | } |
63 | -- | 222 | -- |
64 | 2.13.6 | 223 | 2.31.1 |
65 | 224 | ||
66 | diff view generated by jsdifflib |
1 | From: Zhengui <lizhengui@huawei.com> | 1 | From: Paolo Bonzini <pbonzini@redhat.com> |
---|---|---|---|
2 | 2 | ||
3 | In blk_remove_bs, all I/O should be completed before removing throttle | 3 | Allow using QemuCoSleep to sleep forever until woken by qemu_co_sleep_wake. |
4 | timers. If there has inflight I/O, removing throttle timers here will | 4 | This makes the logic of qemu_co_sleep_ns_wakeable easy to understand. |
5 | cause the inflight I/O never return. | ||
6 | This patch add bdrv_drained_begin before throttle_timers_detach_aio_context | ||
7 | to let all I/O completed before removing throttle timers. | ||
8 | 5 | ||
9 | [Moved declaration of bs as suggested by Alberto Garcia | 6 | In the future we will introduce an API that can work even if the |
10 | <berto@igalia.com>. | 7 | sleep and wake happen from different threads. For now, initializing |
11 | --Stefan] | 8 | w->to_wake after timer_mod is fine because the timer can only fire in |
9 | the same AioContext. | ||
12 | 10 | ||
13 | Signed-off-by: Zhengui <lizhengui@huawei.com> | 11 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
14 | Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> | 12 | Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> |
15 | Reviewed-by: Alberto Garcia <berto@igalia.com> | 13 | Message-id: 20210517100548.28806-7-pbonzini@redhat.com |
16 | Message-id: 1508564040-120700-1-git-send-email-lizhengui@huawei.com | ||
17 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 14 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
18 | --- | 15 | --- |
19 | block/block-backend.c | 4 ++++ | 16 | include/qemu/coroutine.h | 5 +++++ |
20 | 1 file changed, 4 insertions(+) | 17 | util/qemu-coroutine-sleep.c | 26 +++++++++++++++++++------- |
18 | 2 files changed, 24 insertions(+), 7 deletions(-) | ||
21 | 19 | ||
22 | diff --git a/block/block-backend.c b/block/block-backend.c | 20 | diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h |
23 | index XXXXXXX..XXXXXXX 100644 | 21 | index XXXXXXX..XXXXXXX 100644 |
24 | --- a/block/block-backend.c | 22 | --- a/include/qemu/coroutine.h |
25 | +++ b/block/block-backend.c | 23 | +++ b/include/qemu/coroutine.h |
26 | @@ -XXX,XX +XXX,XX @@ BlockBackend *blk_by_public(BlockBackendPublic *public) | 24 | @@ -XXX,XX +XXX,XX @@ typedef struct QemuCoSleep { |
27 | */ | 25 | void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, |
28 | void blk_remove_bs(BlockBackend *blk) | 26 | QEMUClockType type, int64_t ns); |
27 | |||
28 | +/** | ||
29 | + * Yield the coroutine until the next call to qemu_co_sleep_wake. | ||
30 | + */ | ||
31 | +void coroutine_fn qemu_co_sleep(QemuCoSleep *w); | ||
32 | + | ||
33 | static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) | ||
29 | { | 34 | { |
30 | + BlockDriverState *bs; | 35 | QemuCoSleep w = { 0 }; |
31 | ThrottleTimers *tt; | 36 | diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c |
32 | 37 | index XXXXXXX..XXXXXXX 100644 | |
33 | notifier_list_notify(&blk->remove_bs_notifiers, blk); | 38 | --- a/util/qemu-coroutine-sleep.c |
34 | if (blk->public.throttle_group_member.throttle_state) { | 39 | +++ b/util/qemu-coroutine-sleep.c |
35 | tt = &blk->public.throttle_group_member.throttle_timers; | 40 | @@ -XXX,XX +XXX,XX @@ static void co_sleep_cb(void *opaque) |
36 | + bs = blk_bs(blk); | 41 | qemu_co_sleep_wake(w); |
37 | + bdrv_drained_begin(bs); | 42 | } |
38 | throttle_timers_detach_aio_context(tt); | 43 | |
39 | + bdrv_drained_end(bs); | 44 | -void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, |
45 | - QEMUClockType type, int64_t ns) | ||
46 | +void coroutine_fn qemu_co_sleep(QemuCoSleep *w) | ||
47 | { | ||
48 | Coroutine *co = qemu_coroutine_self(); | ||
49 | - AioContext *ctx = qemu_get_current_aio_context(); | ||
50 | - QEMUTimer ts; | ||
51 | |||
52 | const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL, | ||
53 | qemu_co_sleep_ns__scheduled); | ||
54 | @@ -XXX,XX +XXX,XX @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, | ||
40 | } | 55 | } |
41 | 56 | ||
42 | blk_update_root_state(blk); | 57 | w->to_wake = co; |
58 | - aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w), | ||
59 | - timer_mod(&ts, qemu_clock_get_ns(type) + ns); | ||
60 | qemu_coroutine_yield(); | ||
61 | - timer_del(&ts); | ||
62 | |||
63 | /* w->to_wake is cleared before resuming this coroutine. */ | ||
64 | assert(w->to_wake == NULL); | ||
65 | } | ||
66 | + | ||
67 | +void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w, | ||
68 | + QEMUClockType type, int64_t ns) | ||
69 | +{ | ||
70 | + AioContext *ctx = qemu_get_current_aio_context(); | ||
71 | + QEMUTimer ts; | ||
72 | + | ||
73 | + aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w); | ||
74 | + timer_mod(&ts, qemu_clock_get_ns(type) + ns); | ||
75 | + | ||
76 | + /* | ||
77 | + * The timer will fire in the current AiOContext, so the callback | ||
78 | + * must happen after qemu_co_sleep yields and there is no race | ||
79 | + * between timer_mod and qemu_co_sleep. | ||
80 | + */ | ||
81 | + qemu_co_sleep(w); | ||
82 | + timer_del(&ts); | ||
83 | +} | ||
43 | -- | 84 | -- |
44 | 2.13.6 | 85 | 2.31.1 |
45 | 86 | ||
46 | diff view generated by jsdifflib |