From nobody Mon Sep 28 02:08:01 2026 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists1p.gnu.org (lists1p.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1785162819420743.212470188425; Mon, 27 Jul 2026 07:33:39 -0700 (PDT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists1p.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1woMOD-0000G9-TH; Mon, 27 Jul 2026 10:33:09 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists1p.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1woMO3-0000FN-2L; Mon, 27 Jul 2026 10:32:59 -0400 Received: from [115.124.28.45] (helo=out28-45.mail.aliyun.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1woMNy-0001xA-RJ; Mon, 27 Jul 2026 10:32:58 -0400 Received: from Moon.localdomain(mailfrom:mail@jiesong.me fp:SMTPD_---.iWYIJgj_1785162500 cluster:ay29) by smtp.aliyun-inc.com; Mon, 27 Jul 2026 22:28:26 +0800 X-Alimail-AntiSpam: AC=CONTINUE; BC=0.07183095|-1; CH=green; DM=|CONTINUE|false|; DS=CONTINUE|ham_system_inform|0.00530952-0.00409479-0.990596; FP=11345540682718118353|0|0|0|0|-1|-1|-1; HT=maildocker-contentspam011083013073; MF=mail@jiesong.me; NM=1; PH=DS; RN=7; RT=7; SR=0; TI=SMTPD_---.iWYIJgj_1785162500; From: Jie Song To: qemu-devel@nongnu.org Cc: pbonzini@redhat.com, fam@euphon.net, kwolf@redhat.com, farosas@suse.de, qemu-stable@nongnu.org, songjie_yewu@cmss.chinamobile.com Subject: [PATCH v2] scsi-disk: Fix UNMAP drain race with migration Date: Mon, 27 Jul 2026 22:28:15 +0800 Message-ID: <20260727142815.6869-1-mail@jiesong.me> X-Mailer: git-send-email 2.48.1 In-Reply-To: <20260707152037.11352-1-mail@jiesong.me> References: <20260707152037.11352-1-mail@jiesong.me> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Host-Lookup-Failed: Reverse DNS lookup failed for 115.124.28.45 (deferred) Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists1p.gnu.org; Received-SPF: pass client-ip=115.124.28.45; envelope-from=mail@jiesong.me; helo=out28-45.mail.aliyun.com X-Spam_score_int: -10 X-Spam_score: -1.1 X-Spam_bar: - X-Spam_report: (-1.1 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_DNSWL_NONE=-0.0001, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: qemu development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer=patchew.org@nongnu.org X-ZM-MESSAGEID: 1785162823926158500 Content-Type: text/plain; charset="utf-8" From: Jie Song 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 =3D=3D 0 and be queued as a n= ew 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: qemu-stable@nongnu.org Signed-off-by: Jie Song --- 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 <=3D s->qdev.max_lba + 1); } =20 -typedef struct UnmapCBData { +typedef struct UnmapAIOCB { + BlockAIOCB common; SCSIDiskReq *r; uint8_t *inbuf; int count; -} UnmapCBData; + bool canceled; +} UnmapAIOCB; =20 -static void scsi_unmap_complete(void *opaque, int ret); +static void scsi_unmap_cancel(BlockAIOCB *acb) +{ + UnmapAIOCB *data =3D container_of(acb, UnmapAIOCB, common); + + data->canceled =3D true; +} + +static const AIOCBInfo scsi_unmap_aiocb_info =3D { + .aiocb_size =3D sizeof(UnmapAIOCB), + .cancel_async =3D scsi_unmap_cancel, +}; =20 -static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) +static void coroutine_fn scsi_unmap_co_entry(void *opaque) { + UnmapAIOCB *data =3D opaque; SCSIDiskReq *r =3D data->r; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); + BlockBackend *blk =3D s->qdev.conf.blk; =20 - assert(r->req.aiocb =3D=3D NULL); + /* Paired with blk_end_request() below. */ + blk_co_start_request(blk); =20 - if (data->count > 0) { + while (data->count > 0) { uint64_t sector_num =3D ldq_be_p(&data->inbuf[0]); uint32_t nb_sectors =3D ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; + int ret; + + if (data->canceled) { + r->req.aiocb =3D NULL; + scsi_req_cancel_complete(&r->req); + goto done; + } + r->sector =3D sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE); r->sector_count =3D nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_= SIZE); =20 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 =3D NULL; + block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_UNMAP); scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE)); goto done; } =20 - 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); =20 - r->req.aiocb =3D blk_aio_pdiscard(s->qdev.conf.blk, - r->sector * BDRV_SECTOR_SIZE, - r->sector_count * BDRV_SECTOR_SIZE, - scsi_unmap_complete, data); + ret =3D blk_co_pdiscard(blk, r->sector * BDRV_SECTOR_SIZE, + r->sector_count * BDRV_SECTOR_SIZE, + BDRV_REQ_NO_QUEUE); + r->req.aiocb =3D 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 +=3D 16; - return; + r->req.aiocb =3D &data->common; } =20 + r->req.aiocb =3D NULL; scsi_req_complete(&r->req, GOOD); =20 done: + qemu_aio_unref(data); scsi_req_unref(&r->req); - g_free(data); -} - -static void scsi_unmap_complete(void *opaque, int ret) -{ - UnmapCBData *data =3D opaque; - SCSIDiskReq *r =3D data->r; - SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); - - assert(r->req.aiocb !=3D NULL); - r->req.aiocb =3D 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); } =20 static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf) @@ -1812,7 +1824,8 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, u= int8_t *inbuf) SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); uint8_t *p =3D inbuf; int len =3D r->req.cmd.xfer; - UnmapCBData *data; + UnmapAIOCB *data; + Coroutine *co; =20 /* Reject ANCHOR=3D1. */ if (r->req.cmd.buf[1] & 0x1) { @@ -1838,14 +1851,17 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r,= uint8_t *inbuf) return; } =20 - data =3D g_new0(UnmapCBData, 1); + data =3D blk_aio_get(&scsi_unmap_aiocb_info, s->qdev.conf.blk, NULL, N= ULL); data->r =3D r; data->inbuf =3D &p[8]; data->count =3D lduw_be_p(&p[2]) >> 4; + data->canceled =3D false; =20 - /* 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 =3D &data->common; + co =3D qemu_coroutine_create(scsi_unmap_co_entry, data); + aio_co_enter(qemu_get_current_aio_context(), co); return; =20 invalid_param_len: --=20 2.48.1