1 | The following changes since commit 6157b0e19721aadb4c7fdcfe57b2924af6144b14: | 1 | The following changes since commit 887cba855bb6ff4775256f7968409281350b568c: |
---|---|---|---|
2 | 2 | ||
3 | Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.0-pull-= | 3 | configure: Fix cross-building for RISCV host (v5) (2023-07-11 17:56:09 +0100) |
4 | request' into staging (2021-03-14 17:47:49 +0000) | ||
5 | 4 | ||
6 | are available in the Git repository at: | 5 | are available in the Git repository at: |
7 | 6 | ||
8 | https://gitlab.com/stefanha/qemu.git tags/block-pull-request | 7 | https://gitlab.com/stefanha/qemu.git tags/block-pull-request |
9 | 8 | ||
10 | for you to fetch changes up to fb0b154c801e3447e505de420195fb7038695941: | 9 | for you to fetch changes up to 75dcb4d790bbe5327169fd72b185960ca58e2fa6: |
11 | 10 | ||
12 | virtio-blk: Respect discard granularity (2021-03-15 09:48:53 +0000) | 11 | virtio-blk: fix host notifier issues during dataplane start/stop (2023-07-12 15:20:32 -0400) |
13 | 12 | ||
14 | ---------------------------------------------------------------- | 13 | ---------------------------------------------------------------- |
15 | Pull request | 14 | Pull request |
16 | 15 | ||
17 | ---------------------------------------------------------------- | 16 | ---------------------------------------------------------------- |
18 | 17 | ||
19 | Akihiko Odaki (1): | 18 | Stefan Hajnoczi (1): |
20 | virtio-blk: Respect discard granularity | 19 | virtio-blk: fix host notifier issues during dataplane start/stop |
21 | 20 | ||
22 | include/hw/virtio/virtio-blk.h | 1 + | 21 | hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++-------------- |
23 | hw/block/virtio-blk.c | 8 +++++++- | 22 | 1 file changed, 38 insertions(+), 29 deletions(-) |
24 | hw/core/machine.c | 1 + | ||
25 | 3 files changed, 9 insertions(+), 1 deletion(-) | ||
26 | 23 | ||
27 | --=20 | 24 | -- |
28 | 2.29.2 | 25 | 2.40.1 |
29 | diff view generated by jsdifflib |
1 | From: Akihiko Odaki <akihiko.odaki@gmail.com> | 1 | The main loop thread can consume 100% CPU when using --device |
---|---|---|---|
2 | virtio-blk-pci,iothread=<iothread>. ppoll() constantly returns but | ||
3 | reading virtqueue host notifiers fails with EAGAIN. The file descriptors | ||
4 | are stale and remain registered with the AioContext because of bugs in | ||
5 | the virtio-blk dataplane start/stop code. | ||
2 | 6 | ||
3 | Report the configured granularity for discard operation to the | 7 | The problem is that the dataplane start/stop code involves drain |
4 | guest. If this is not set use the block size. | 8 | operations, which call virtio_blk_drained_begin() and |
9 | virtio_blk_drained_end() at points where the host notifier is not | ||
10 | operational: | ||
11 | - In virtio_blk_data_plane_start(), blk_set_aio_context() drains after | ||
12 | vblk->dataplane_started has been set to true but the host notifier has | ||
13 | not been attached yet. | ||
14 | - In virtio_blk_data_plane_stop(), blk_drain() and blk_set_aio_context() | ||
15 | drain after the host notifier has already been detached but with | ||
16 | vblk->dataplane_started still set to true. | ||
5 | 17 | ||
6 | Since until now we have ignored the configured discard granularity | 18 | I would like to simplify ->ioeventfd_start/stop() to avoid interactions |
7 | and always reported the block size, let's add | 19 | with drain entirely, but couldn't find a way to do that. Instead, this |
8 | 'report-discard-granularity' property and disable it for older | 20 | patch accepts the fragile nature of the code and reorders it so that |
9 | machine types to avoid migration issues. | 21 | vblk->dataplane_started is false during drain operations. This way the |
22 | virtio_blk_drained_begin() and virtio_blk_drained_end() calls don't | ||
23 | touch the host notifier. The result is that | ||
24 | virtio_blk_data_plane_start() and virtio_blk_data_plane_stop() have | ||
25 | complete control over the host notifier and stale file descriptors are | ||
26 | no longer left in the AioContext. | ||
10 | 27 | ||
11 | Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com> | 28 | This patch fixes the 100% CPU consumption in the main loop thread and |
12 | Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> | 29 | correctly moves host notifier processing to the IOThread. |
30 | |||
31 | Fixes: 1665d9326fd2 ("virtio-blk: implement BlockDevOps->drained_begin()") | ||
32 | Reported-by: Lukáš Doktor <ldoktor@redhat.com> | ||
13 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | 33 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> |
14 | Message-Id: <20210225001239.47046-1-akihiko.odaki@gmail.com> | 34 | Tested-by: Lukas Doktor <ldoktor@redhat.com> |
35 | Message-id: 20230704151527.193586-1-stefanha@redhat.com | ||
36 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | ||
15 | --- | 37 | --- |
16 | include/hw/virtio/virtio-blk.h | 1 + | 38 | hw/block/dataplane/virtio-blk.c | 67 +++++++++++++++++++-------------- |
17 | hw/block/virtio-blk.c | 8 +++++++- | 39 | 1 file changed, 38 insertions(+), 29 deletions(-) |
18 | hw/core/machine.c | 1 + | ||
19 | 3 files changed, 9 insertions(+), 1 deletion(-) | ||
20 | 40 | ||
21 | diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h | 41 | diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c |
22 | index XXXXXXX..XXXXXXX 100644 | 42 | index XXXXXXX..XXXXXXX 100644 |
23 | --- a/include/hw/virtio/virtio-blk.h | 43 | --- a/hw/block/dataplane/virtio-blk.c |
24 | +++ b/include/hw/virtio/virtio-blk.h | 44 | +++ b/hw/block/dataplane/virtio-blk.c |
25 | @@ -XXX,XX +XXX,XX @@ struct VirtIOBlkConf | 45 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) |
26 | uint16_t num_queues; | 46 | |
27 | uint16_t queue_size; | 47 | memory_region_transaction_commit(); |
28 | bool seg_max_adjust; | 48 | |
29 | + bool report_discard_granularity; | 49 | - /* |
30 | uint32_t max_discard_sectors; | 50 | - * These fields are visible to the IOThread so we rely on implicit barriers |
31 | uint32_t max_write_zeroes_sectors; | 51 | - * in aio_context_acquire() on the write side and aio_notify_accept() on |
32 | bool x_enable_wce_if_config_wce; | 52 | - * the read side. |
33 | diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c | 53 | - */ |
34 | index XXXXXXX..XXXXXXX 100644 | 54 | - s->starting = false; |
35 | --- a/hw/block/virtio-blk.c | 55 | - vblk->dataplane_started = true; |
36 | +++ b/hw/block/virtio-blk.c | 56 | trace_virtio_blk_data_plane_start(s); |
37 | @@ -XXX,XX +XXX,XX @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) | 57 | |
38 | blkcfg.wce = blk_enable_write_cache(s->blk); | 58 | old_context = blk_get_aio_context(s->conf->conf.blk); |
39 | virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues); | 59 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) |
40 | if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD)) { | 60 | event_notifier_set(virtio_queue_get_host_notifier(vq)); |
41 | + uint32_t discard_granularity = conf->discard_granularity; | 61 | } |
42 | + if (discard_granularity == -1 || !s->conf.report_discard_granularity) { | 62 | |
43 | + discard_granularity = blk_size; | 63 | + /* |
44 | + } | 64 | + * These fields must be visible to the IOThread when it processes the |
45 | virtio_stl_p(vdev, &blkcfg.max_discard_sectors, | 65 | + * virtqueue, otherwise it will think dataplane has not started yet. |
46 | s->conf.max_discard_sectors); | 66 | + * |
47 | virtio_stl_p(vdev, &blkcfg.discard_sector_alignment, | 67 | + * Make sure ->dataplane_started is false when blk_set_aio_context() is |
48 | - blk_size >> BDRV_SECTOR_BITS); | 68 | + * called above so that draining does not cause the host notifier to be |
49 | + discard_granularity >> BDRV_SECTOR_BITS); | 69 | + * detached/attached prematurely. |
50 | /* | 70 | + */ |
51 | * We support only one segment per request since multiple segments | 71 | + s->starting = false; |
52 | * are not widely used and there are no userspace APIs that allow | 72 | + vblk->dataplane_started = true; |
53 | @@ -XXX,XX +XXX,XX @@ static Property virtio_blk_properties[] = { | 73 | + smp_wmb(); /* paired with aio_notify_accept() on the read side */ |
54 | IOThread *), | 74 | + |
55 | DEFINE_PROP_BIT64("discard", VirtIOBlock, host_features, | 75 | /* Get this show started by hooking up our callbacks */ |
56 | VIRTIO_BLK_F_DISCARD, true), | 76 | if (!blk_in_drain(s->conf->conf.blk)) { |
57 | + DEFINE_PROP_BOOL("report-discard-granularity", VirtIOBlock, | 77 | aio_context_acquire(s->ctx); |
58 | + conf.report_discard_granularity, true), | 78 | @@ -XXX,XX +XXX,XX @@ int virtio_blk_data_plane_start(VirtIODevice *vdev) |
59 | DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features, | 79 | fail_guest_notifiers: |
60 | VIRTIO_BLK_F_WRITE_ZEROES, true), | 80 | vblk->dataplane_disabled = true; |
61 | DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock, | 81 | s->starting = false; |
62 | diff --git a/hw/core/machine.c b/hw/core/machine.c | 82 | - vblk->dataplane_started = true; |
63 | index XXXXXXX..XXXXXXX 100644 | 83 | return -ENOSYS; |
64 | --- a/hw/core/machine.c | 84 | } |
65 | +++ b/hw/core/machine.c | 85 | |
66 | @@ -XXX,XX +XXX,XX @@ | 86 | @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) |
67 | GlobalProperty hw_compat_5_2[] = { | 87 | aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s); |
68 | { "ICH9-LPC", "smm-compat", "on"}, | 88 | } |
69 | { "PIIX4_PM", "smm-compat", "on"}, | 89 | |
70 | + { "virtio-blk-device", "report-discard-granularity", "off" }, | 90 | + /* |
71 | }; | 91 | + * Batch all the host notifiers in a single transaction to avoid |
72 | const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2); | 92 | + * quadratic time complexity in address_space_update_ioeventfds(). |
73 | 93 | + */ | |
94 | + memory_region_transaction_begin(); | ||
95 | + | ||
96 | + for (i = 0; i < nvqs; i++) { | ||
97 | + virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false); | ||
98 | + } | ||
99 | + | ||
100 | + /* | ||
101 | + * The transaction expects the ioeventfds to be open when it | ||
102 | + * commits. Do it now, before the cleanup loop. | ||
103 | + */ | ||
104 | + memory_region_transaction_commit(); | ||
105 | + | ||
106 | + for (i = 0; i < nvqs; i++) { | ||
107 | + virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i); | ||
108 | + } | ||
109 | + | ||
110 | + /* | ||
111 | + * Set ->dataplane_started to false before draining so that host notifiers | ||
112 | + * are not detached/attached anymore. | ||
113 | + */ | ||
114 | + vblk->dataplane_started = false; | ||
115 | + | ||
116 | aio_context_acquire(s->ctx); | ||
117 | |||
118 | /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */ | ||
119 | @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev) | ||
120 | |||
121 | aio_context_release(s->ctx); | ||
122 | |||
123 | - /* | ||
124 | - * Batch all the host notifiers in a single transaction to avoid | ||
125 | - * quadratic time complexity in address_space_update_ioeventfds(). | ||
126 | - */ | ||
127 | - memory_region_transaction_begin(); | ||
128 | - | ||
129 | - for (i = 0; i < nvqs; i++) { | ||
130 | - virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false); | ||
131 | - } | ||
132 | - | ||
133 | - /* | ||
134 | - * The transaction expects the ioeventfds to be open when it | ||
135 | - * commits. Do it now, before the cleanup loop. | ||
136 | - */ | ||
137 | - memory_region_transaction_commit(); | ||
138 | - | ||
139 | - for (i = 0; i < nvqs; i++) { | ||
140 | - virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i); | ||
141 | - } | ||
142 | - | ||
143 | qemu_bh_cancel(s->bh); | ||
144 | notify_guest_bh(s); /* final chance to notify guest */ | ||
145 | |||
146 | /* Clean up guest notifier (irq) */ | ||
147 | k->set_guest_notifiers(qbus->parent, nvqs, false); | ||
148 | |||
149 | - vblk->dataplane_started = false; | ||
150 | s->stopping = false; | ||
151 | } | ||
74 | -- | 152 | -- |
75 | 2.29.2 | 153 | 2.40.1 |
76 | 154 | ||
155 | diff view generated by jsdifflib |