From: Jie Song <[email protected]>
Windows guests can send SCSI UNMAP requests with many descriptors when
running ReTrim. scsi-disk currently submits one blk_aio_pdiscard()
request per descriptor and chains the next descriptor from the completion
callback.
If migration stops the VM while this chain is running, the next discard
can enter blk_wait_while_drained() with flags == 0 and be queued as a new
BlockBackend request. This can make the BlockBackend look drained before
the whole UNMAP command has completed. The migration completion path can
then inactivate block nodes, and the queued discard later resumes and hits
this assertion in bdrv_co_write_req_prepare():
qemu-system-x86_64: ../block/io.c:1986:
bdrv_co_write_req_prepare: Assertion
`!(bs->open_flags & BDRV_O_INACTIVE)' failed.
The corresponding backtrace is:
bdrv_co_write_req_prepare
bdrv_co_pdiscard
blk_co_do_pdiscard
blk_aio_pdiscard_entry
coroutine_trampoline
Process the whole UNMAP descriptor list as a single BlockBackend macro
request instead. Use blk_co_start_request()/blk_end_request() around the
whole descriptor loop and submit the internal discards with
BDRV_REQ_NO_QUEUE. This follows the same block-layer model used by commit
095c08a7ba68 ("ide: Minimal fix for deadlock between TRIM and drain"),
which fixed chained IDE TRIM discards by running them from a coroutine and
using BDRV_REQ_NO_QUEUE under blk_co_start_request().
Because blk_co_pdiscard() does not return a BlockAIOCB, keep an outer
UnmapAIOCB attached to the SCSI request while the coroutine is running.
This preserves SCSI reset/TMF cancellation semantics: cancellation marks
the outer AIOCB as canceled, and the coroutine completes cancellation at a
safe point instead of letting the SCSI layer complete the request early.
In two stress-test runs with Windows ReTrim active, unpatched QEMU crashed
on the 15th and 18th live migration. With this change, corresponding runs
completed 100 and 50 consecutive migrations without a crash. GDB analysis
of the QEMU core dump from current master showed that the crashing discard
AIO had scsi_unmap_complete registered as its completion callback, with 26
UNMAP descriptors still pending, while the block node receiving the discard
had already been marked BDRV_O_INACTIVE.
Cc: [email protected]
Signed-off-by: Jie Song <[email protected]>
---
Changes in v2:
- Drop the multiple-descriptor qtest because it did not exercise the
drain/inactivation race.
- Add stress-test results and core dump evidence to the commit message.
- Rebase onto QEMU master at 6333226c2a.
v1: https://lists.gnu.org/archive/html/qemu-devel/2026-07/msg02176.html
hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++++++++-------------------
1 file changed, 53 insertions(+), 37 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 1b0cce128c..e24b61f186 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1741,70 +1741,82 @@ static inline bool check_lba_range(SCSIDiskState *s,
sector_num + nb_sectors <= s->qdev.max_lba + 1);
}
-typedef struct UnmapCBData {
+typedef struct UnmapAIOCB {
+ BlockAIOCB common;
SCSIDiskReq *r;
uint8_t *inbuf;
int count;
-} UnmapCBData;
+ bool canceled;
+} UnmapAIOCB;
-static void scsi_unmap_complete(void *opaque, int ret);
+static void scsi_unmap_cancel(BlockAIOCB *acb)
+{
+ UnmapAIOCB *data = container_of(acb, UnmapAIOCB, common);
+
+ data->canceled = true;
+}
+
+static const AIOCBInfo scsi_unmap_aiocb_info = {
+ .aiocb_size = sizeof(UnmapAIOCB),
+ .cancel_async = scsi_unmap_cancel,
+};
-static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
+static void coroutine_fn scsi_unmap_co_entry(void *opaque)
{
+ UnmapAIOCB *data = opaque;
SCSIDiskReq *r = data->r;
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+ BlockBackend *blk = s->qdev.conf.blk;
- assert(r->req.aiocb == NULL);
+ /* Paired with blk_end_request() below. */
+ blk_co_start_request(blk);
- if (data->count > 0) {
+ while (data->count > 0) {
uint64_t sector_num = ldq_be_p(&data->inbuf[0]);
uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
+ int ret;
+
+ if (data->canceled) {
+ r->req.aiocb = NULL;
+ scsi_req_cancel_complete(&r->req);
+ goto done;
+ }
+
r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
if (!check_lba_range(s, sector_num, nb_sectors)) {
- block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
- BLOCK_ACCT_UNMAP);
+ r->req.aiocb = NULL;
+ block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_UNMAP);
scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
goto done;
}
- block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+ block_acct_start(blk_get_stats(blk), &r->acct,
r->sector_count * BDRV_SECTOR_SIZE,
BLOCK_ACCT_UNMAP);
- r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
- r->sector * BDRV_SECTOR_SIZE,
- r->sector_count * BDRV_SECTOR_SIZE,
- scsi_unmap_complete, data);
+ ret = blk_co_pdiscard(blk, r->sector * BDRV_SECTOR_SIZE,
+ r->sector_count * BDRV_SECTOR_SIZE,
+ BDRV_REQ_NO_QUEUE);
+ r->req.aiocb = NULL;
+ if (scsi_disk_req_check_error(r, ret, true)) {
+ goto done;
+ }
+
+ block_acct_done(blk_get_stats(blk), &r->acct);
data->count--;
data->inbuf += 16;
- return;
+ r->req.aiocb = &data->common;
}
+ r->req.aiocb = NULL;
scsi_req_complete(&r->req, GOOD);
done:
+ qemu_aio_unref(data);
scsi_req_unref(&r->req);
- g_free(data);
-}
-
-static void scsi_unmap_complete(void *opaque, int ret)
-{
- UnmapCBData *data = opaque;
- SCSIDiskReq *r = data->r;
- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-
- assert(r->req.aiocb != NULL);
- r->req.aiocb = NULL;
-
- if (scsi_disk_req_check_error(r, ret, true)) {
- scsi_req_unref(&r->req);
- g_free(data);
- } else {
- block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
- scsi_unmap_complete_noio(data, ret);
- }
+ blk_end_request(blk);
}
static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
@@ -1812,7 +1824,8 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r,
uint8_t *inbuf)
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
uint8_t *p = inbuf;
int len = r->req.cmd.xfer;
- UnmapCBData *data;
+ UnmapAIOCB *data;
+ Coroutine *co;
/* Reject ANCHOR=1. */
if (r->req.cmd.buf[1] & 0x1) {
@@ -1838,14 +1851,17 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r,
uint8_t *inbuf)
return;
}
- data = g_new0(UnmapCBData, 1);
+ data = blk_aio_get(&scsi_unmap_aiocb_info, s->qdev.conf.blk, NULL, NULL);
data->r = r;
data->inbuf = &p[8];
data->count = lduw_be_p(&p[2]) >> 4;
+ data->canceled = false;
- /* The matching unref is in scsi_unmap_complete, before data is freed. */
+ /* The matching unref is in scsi_unmap_co_entry(). */
scsi_req_ref(&r->req);
- scsi_unmap_complete_noio(data, 0);
+ r->req.aiocb = &data->common;
+ co = qemu_coroutine_create(scsi_unmap_co_entry, data);
+ aio_co_enter(qemu_get_current_aio_context(), co);
return;
invalid_param_len:
--
2.48.1
