]>
Commit | Line | Data |
---|---|---|
58659169 FE |
1 | From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 |
2 | From: Fiona Ebner <f.ebner@proxmox.com> | |
3 | Date: Tue, 7 Mar 2023 15:03:02 +0100 | |
4 | Subject: [PATCH] ide: avoid potential deadlock when draining during trim | |
5 | MIME-Version: 1.0 | |
6 | Content-Type: text/plain; charset=UTF-8 | |
7 | Content-Transfer-Encoding: 8bit | |
8 | ||
9 | The deadlock can happen as follows: | |
10 | 1. ide_issue_trim is called, and increments the in_flight counter. | |
11 | 2. ide_issue_trim_cb calls blk_aio_pdiscard. | |
12 | 3. Somebody else starts draining (e.g. backup to insert the cbw node). | |
13 | 4. ide_issue_trim_cb is called as the completion callback for | |
14 | blk_aio_pdiscard. | |
15 | 5. ide_issue_trim_cb issues yet another blk_aio_pdiscard request. | |
16 | 6. The request is added to the wait queue via blk_wait_while_drained, | |
17 | because draining has been started. | |
18 | 7. Nobody ever decrements the in_flight counter and draining can't | |
19 | finish. This would be done by ide_trim_bh_cb, which is called after | |
20 | ide_issue_trim_cb has issued its last request, but | |
21 | ide_issue_trim_cb is not called anymore, because it's the | |
22 | completion callback of blk_aio_pdiscard, which waits on draining. | |
23 | ||
24 | Quoting Hanna Czenczek: | |
25 | > The point of 7e5cdb345f was that we need any in-flight count to | |
26 | > accompany a set s->bus->dma->aiocb. While blk_aio_pdiscard() is | |
27 | > happening, we don’t necessarily need another count. But we do need | |
28 | > it while there is no blk_aio_pdiscard(). | |
29 | > ide_issue_trim_cb() returns in two cases (and, recursively through | |
30 | > its callers, leaves s->bus->dma->aiocb set): | |
31 | > 1. After calling blk_aio_pdiscard(), which will keep an in-flight | |
32 | > count, | |
33 | > 2. After calling replay_bh_schedule_event() (i.e. | |
34 | > qemu_bh_schedule()), which does not keep an in-flight count. | |
35 | ||
36 | Thus, even after moving the blk_inc_in_flight to above the | |
37 | replay_bh_schedule_event call, the invariant "ide_issue_trim_cb | |
38 | returns with an accompanying in-flight count" is still satisfied. | |
39 | ||
3a94e1a1 FE |
40 | However, the issue 7e5cdb345f fixed for canceling resurfaces, because |
41 | ide_cancel_dma_sync assumes that it just needs to drain once. But now | |
42 | the in_flight count is not consistently > 0 during the trim operation. | |
43 | So, change it to drain until !s->bus->dma->aiocb, which means that the | |
44 | operation finished (s->bus->dma->aiocb is cleared by ide_set_inactive | |
45 | via the ide_dma_cb when the end of the transfer is reached). | |
46 | ||
47 | Discussion here: | |
48 | https://lists.nongnu.org/archive/html/qemu-devel/2023-03/msg02506.html | |
49 | ||
58659169 FE |
50 | Fixes: 7e5cdb345f ("ide: Increment BB in-flight counter for TRIM BH") |
51 | Suggested-by: Hanna Czenczek <hreitz@redhat.com> | |
52 | Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> | |
53 | --- | |
3a94e1a1 FE |
54 | hw/ide/core.c | 12 ++++++------ |
55 | 1 file changed, 6 insertions(+), 6 deletions(-) | |
58659169 FE |
56 | |
57 | diff --git a/hw/ide/core.c b/hw/ide/core.c | |
10e10933 | 58 | index 07971c0218..6a74afe564 100644 |
58659169 FE |
59 | --- a/hw/ide/core.c |
60 | +++ b/hw/ide/core.c | |
bf251437 | 61 | @@ -444,7 +444,7 @@ static void ide_trim_bh_cb(void *opaque) |
58659169 FE |
62 | iocb->bh = NULL; |
63 | qemu_aio_unref(iocb); | |
64 | ||
65 | - /* Paired with an increment in ide_issue_trim() */ | |
66 | + /* Paired with an increment in ide_issue_trim_cb() */ | |
67 | blk_dec_in_flight(blk); | |
68 | } | |
69 | ||
bf251437 | 70 | @@ -504,6 +504,8 @@ static void ide_issue_trim_cb(void *opaque, int ret) |
58659169 FE |
71 | done: |
72 | iocb->aiocb = NULL; | |
73 | if (iocb->bh) { | |
74 | + /* Paired with a decrement in ide_trim_bh_cb() */ | |
75 | + blk_inc_in_flight(s->blk); | |
76 | replay_bh_schedule_event(iocb->bh); | |
77 | } | |
78 | } | |
10e10933 FE |
79 | @@ -516,9 +518,6 @@ BlockAIOCB *ide_issue_trim( |
80 | IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master; | |
58659169 FE |
81 | TrimAIOCB *iocb; |
82 | ||
83 | - /* Paired with a decrement in ide_trim_bh_cb() */ | |
84 | - blk_inc_in_flight(s->blk); | |
85 | - | |
86 | iocb = blk_aio_get(&trim_aiocb_info, s->blk, cb, cb_opaque); | |
87 | iocb->s = s; | |
10e10933 FE |
88 | iocb->bh = qemu_bh_new_guarded(ide_trim_bh_cb, iocb, |
89 | @@ -742,8 +741,9 @@ void ide_cancel_dma_sync(IDEState *s) | |
3a94e1a1 FE |
90 | */ |
91 | if (s->bus->dma->aiocb) { | |
92 | trace_ide_cancel_dma_sync_remaining(); | |
93 | - blk_drain(s->blk); | |
94 | - assert(s->bus->dma->aiocb == NULL); | |
95 | + while (s->bus->dma->aiocb) { | |
96 | + blk_drain(s->blk); | |
97 | + } | |
98 | } | |
99 | } | |
100 |