From nobody Sun Jul 26 07:56:19 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 1783438293210128.02201203765208; Tue, 7 Jul 2026 08:31:33 -0700 (PDT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists1p.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1wh7lA-00080G-UO; Tue, 07 Jul 2026 11:30:56 -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 1wh7l7-0007uV-TY; Tue, 07 Jul 2026 11:30:54 -0400 Received: from [115.124.28.83] (helo=out28-83.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 1wh7l3-0000Jm-0e; Tue, 07 Jul 2026 11:30:53 -0400 Received: from Moon.localdomain(mailfrom:mail@jiesong.me fp:SMTPD_---.iFWF8jx_1783437651 cluster:ay29) by smtp.aliyun-inc.com; Tue, 07 Jul 2026 23:21:05 +0800 X-Alimail-AntiSpam: AC=CONTINUE; BC=0.07436259|-1; CH=green; DM=|CONTINUE|false|; DS=CONTINUE|ham_system_inform|0.0083374-0.00179887-0.989864; FP=11273465595276321241|1|1|1|0|-1|-1|-1; HT=maildocker-contentspam033037022039; MF=mail@jiesong.me; NM=1; PH=DS; RN=8; RT=8; SR=0; TI=SMTPD_---.iFWF8jx_1783437651; From: Jie Song To: qemu-devel@nongnu.org Cc: pbonzini@redhat.com, fam@euphon.net, kwolf@redhat.com, farosas@suse.de, lvivier@redhat.com, qemu-stable@nongnu.org, Jie Song Subject: [PATCH] scsi-disk: Fix UNMAP drain race with migration Date: Tue, 7 Jul 2026 23:20:37 +0800 Message-ID: <20260707152037.11352-1-mail@jiesong.me> X-Mailer: git-send-email 2.48.1 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Host-Lookup-Failed: Reverse DNS lookup failed for 115.124.28.83 (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.83; envelope-from=mail@jiesong.me; helo=out28-83.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: 1783438297939158500 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. Add a virtio-scsi qtest that sends one UNMAP command with 64 descriptors, matching the kind of batched request that triggers the migration race. Cc: qemu-stable@nongnu.org Signed-off-by: Jie Song --- hw/scsi/scsi-disk.c | 90 ++++++++++++++++++++-------------- tests/qtest/virtio-scsi-test.c | 36 ++++++++++++++ 2 files changed, 89 insertions(+), 37 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 5ba5b46c4f..6cf1f60957 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -1736,70 +1736,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) @@ -1807,7 +1819,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) { @@ -1833,14 +1846,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: diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c index e2350c52f6..74616bf92d 100644 --- a/tests/qtest/virtio-scsi-test.c +++ b/tests/qtest/virtio-scsi-test.c @@ -9,6 +9,7 @@ */ =20 #include "qemu/osdep.h" +#include "qemu/bswap.h" #include "libqtest-single.h" #include "qemu/module.h" #include "scsi/constants.h" @@ -236,6 +237,37 @@ static void test_unmap_large_lba(void *obj, void *data, qvirtio_scsi_pci_free(vs); } =20 +static void test_unmap_multiple_descriptors(void *obj, void *data, + QGuestAllocator *t_alloc) +{ + enum { NUM_DESCRIPTORS =3D 64 }; + QVirtioSCSI *scsi =3D obj; + QVirtioSCSIQueues *vs; + uint8_t unmap[VIRTIO_SCSI_CDB_SIZE] =3D { 0x42 }; + uint8_t unmap_params[8 + NUM_DESCRIPTORS * 16] =3D { 0 }; + struct virtio_scsi_cmd_resp resp; + int i; + + stw_be_p(&unmap_params[0], sizeof(unmap_params) - 2); + stw_be_p(&unmap_params[2], NUM_DESCRIPTORS * 16); + for (i =3D 0; i < NUM_DESCRIPTORS; i++) { + stq_be_p(&unmap_params[8 + i * 16], i * 2); + stl_be_p(&unmap_params[16 + i * 16], 1); + } + unmap[7] =3D sizeof(unmap_params) >> 8; + unmap[8] =3D sizeof(unmap_params) & 0xff; + + alloc =3D t_alloc; + vs =3D qvirtio_scsi_init(scsi->vdev); + + virtio_scsi_do_command(vs, unmap, NULL, 0, unmap_params, + sizeof(unmap_params), &resp); + g_assert_cmphex(resp.response, =3D=3D, 0); + g_assert_cmphex(resp.status, !=3D, CHECK_CONDITION); + + qvirtio_scsi_pci_free(vs); +} + static void test_write_to_cdrom(void *obj, void *data, QGuestAllocator *t_alloc) { @@ -399,6 +431,10 @@ static void register_virtio_scsi_test(void) qos_add_test("large-lba-unmap", "virtio-scsi", test_unmap_large_lba, &opts); =20 + opts.before =3D virtio_scsi_setup; + qos_add_test("multiple-descriptor-unmap", "virtio-scsi", + test_unmap_multiple_descriptors, &opts); + opts.before =3D virtio_scsi_setup_cd; qos_add_test("write-to-cdrom", "virtio-scsi", test_write_to_cdrom, &op= ts); =20 --=20 2.48.1