[PATCH hci-8.1 0/5] scsi-disk: fix UNMAP and WRITE SAME racing with inactivation #VSTOR-151532

[PATCH hci-8.1 0/5] scsi-disk: fix UNMAP and WRITE SAME racing with inactivation #VSTOR-151532

[Denis V. Lunev]

scsi-disk splits UNMAP into one discard per descriptor and a non-zero
WRITE SAME into 512 KiB writes, submitting each step from the
completion callback of the previous one. A drain in between only waits
for the step in flight; the next one is queued by
blk_wait_while_drained() and resumed by drain_end. When the drain is
the one in bdrv_inactivate_all() (end of live migration,
blockdev-set-active), the resumed request hits an inactive node:

  bdrv_co_write_req_prepare: Assertion
  `!(bs->open_flags & BDRV_O_INACTIVE)' failed.

We hit this in production on a guest UNMAP with 38 descriptors.

The fix follows the one Kevin did for IDE TRIM:
https://lore.kernel.org/qemu-devel/20260421161132.99878-1-kwolf@redhat.com/

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: Fabiano Rosas <farosas@suse.de>
Cc: Laurent Vivier <lvivier@redhat.com>
Cc: qemu-stable@nongnu.org

Denis V. Lunev (5):
  scsi-disk: check all UNMAP descriptors before discarding any
    #VSTOR-151532
  scsi-disk: fix UNMAP racing with inactivation of block nodes
    #VSTOR-151532
  scsi-disk: fix WRITE SAME racing with inactivation of block nodes
    #VSTOR-151532
  virtio-scsi-test: split virtio_scsi_do_command() into submit and wait
    #VSTOR-151532
  virtio-scsi-test: test UNMAP and WRITE SAME during inactivation
    #VSTOR-151532

 hw/scsi/scsi-disk.c            | 278 +++++++++++++++++++--------------
 tests/qtest/virtio-scsi-test.c | 173 +++++++++++++++++---
 2 files changed, 309 insertions(+), 142 deletions(-)

base-commit: 81aea91c7f71cba39b4c3340579d45f0256a91d8

-- 
2.53.0

[PATCH hci-8.1 1/5] scsi-disk: check all UNMAP descriptors before discarding any #VSTOR-151532

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

UNMAP checks each descriptor against the capacity only when its turn
comes, so a parameter list with an out-of-range descriptor in the
middle discards everything before it and then fails with LOGICAL BLOCK
ADDRESS OUT OF RANGE. Validate the whole list before the first discard,
so the command either fails without touching the medium or runs to the
end.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/scsi/scsi-disk.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a501c7ffa4a..c608fc56036 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1763,13 +1763,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
         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);
-            scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
-            goto done;
-        }
-
         block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
                          r->sector_count * BDRV_SECTOR_SIZE,
                          BLOCK_ACCT_UNMAP);
@@ -1784,8 +1777,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
     }
 
     scsi_req_complete(&r->req, GOOD);
-
-done:
     scsi_req_unref(&r->req);
     g_free(data);
 }
@@ -1814,6 +1805,8 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
     uint8_t *p = inbuf;
     int len = r->req.cmd.xfer;
     UnmapCBData *data;
+    int count;
+    int i;
 
     /* Reject ANCHOR=1.  */
     if (r->req.cmd.buf[1] & 0x1) {
@@ -1839,10 +1832,21 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
         return;
     }
 
+    count = lduw_be_p(&p[2]) >> 4;
+    for (i = 0; i < count; i++) {
+        if (!check_lba_range(s, ldq_be_p(&p[8 + i * 16]),
+                             ldl_be_p(&p[8 + i * 16 + 8]))) {
+            block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
+                               BLOCK_ACCT_UNMAP);
+            scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
+            return;
+        }
+    }
+
     data = g_new0(UnmapCBData, 1);
     data->r = r;
     data->inbuf = &p[8];
-    data->count = lduw_be_p(&p[2]) >> 4;
+    data->count = count;
 
     /* The matching unref is in scsi_unmap_complete, before data is freed.  */
     scsi_req_ref(&r->req);
-- 
2.53.0

[PATCH hci-8.1 2/5] scsi-disk: fix UNMAP racing with inactivation of block nodes #VSTOR-151532

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

scsi-disk turns a multi-descriptor UNMAP into one discard per
descriptor and submits the next one from the completion callback of
the previous. Nothing holds the BlockBackend in-flight counter between
two descriptors, so a drain only waits for the discard currently in
flight. When that discard completes inside drained_begin, the callback
submits the next one, blk_wait_while_drained() queues it, and the drain
returns with the UNMAP half done.

bdrv_inactivate_all() drains, inactivates the nodes and ends the drain,
which resumes the queued discard on an inactive node:

  bdrv_co_write_req_prepare: Assertion
  `!(bs->open_flags & BDRV_O_INACTIVE)' failed.

A guest that issues UNMAP at the end of a live migration, or around
blockdev-set-active, takes QEMU down this way.

Handle UNMAP the way commit 095c08a7ba handles IDE TRIM: process the
descriptors in a coroutine between blk_co_start_request() and
blk_end_request() and submit them with BDRV_REQ_NO_QUEUE. A drain now
waits for the whole command instead of a single descriptor, the same
trade-off IDE makes. The coroutine is wrapped in an AIOCB that stays in
r->req.aiocb, so cancellation keeps working as before.

A failed descriptor ends the loop, and the completion applies the
rerror/werror policy as before. werror=ignore is handled inside the
loop, so the remaining descriptors are still discarded as the callback
chain did.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/scsi/scsi-disk.c | 160 +++++++++++++++++++++++++++++++-------------
 1 file changed, 113 insertions(+), 47 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index c608fc56036..8383b7cbe93 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1742,61 +1742,135 @@ static inline bool check_lba_range(SCSIDiskState *s,
             sector_num + nb_sectors <= s->qdev.max_lba + 1);
 }
 
-typedef struct UnmapCBData {
+typedef struct SCSIDiskCoAIOCB SCSIDiskCoAIOCB;
+
+typedef int coroutine_fn SCSIDiskCoFunc(SCSIDiskCoAIOCB *iocb);
+
+struct SCSIDiskCoAIOCB {
+    BlockAIOCB common;
     SCSIDiskReq *r;
-    uint8_t *inbuf;
-    int count;
-} UnmapCBData;
+    SCSIDiskCoFunc *fn;
+    int ret;
+    bool canceled;
+};
+
+static void scsi_disk_co_aio_cancel(BlockAIOCB *acb)
+{
+    SCSIDiskCoAIOCB *iocb = container_of(acb, SCSIDiskCoAIOCB, common);
+
+    iocb->canceled = true;
+}
 
-static void scsi_unmap_complete(void *opaque, int ret);
+static const AIOCBInfo scsi_disk_co_aiocb_info = {
+    .aiocb_size         = sizeof(SCSIDiskCoAIOCB),
+    .cancel_async       = scsi_disk_co_aio_cancel,
+};
 
-static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
+static void scsi_disk_co_aio_bh(void *opaque)
 {
-    SCSIDiskReq *r = data->r;
-    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    SCSIDiskCoAIOCB *iocb = opaque;
+    BlockBackend *blk = iocb->r->req.dev->conf.blk;
 
-    assert(r->req.aiocb == NULL);
+    iocb->common.cb(iocb->common.opaque, iocb->ret);
+    qemu_aio_unref(iocb);
 
-    if (data->count > 0) {
-        uint64_t sector_num = ldq_be_p(&data->inbuf[0]);
-        uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
-        r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
-        r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+    /* Paired with blk_co_start_request in scsi_disk_co_aio_entry() */
+    blk_end_request(blk);
+}
 
-        block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
-                         r->sector_count * BDRV_SECTOR_SIZE,
-                         BLOCK_ACCT_UNMAP);
+static void coroutine_fn scsi_disk_co_aio_entry(void *opaque)
+{
+    SCSIDiskCoAIOCB *iocb = opaque;
 
-        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);
-        data->count--;
-        data->inbuf += 16;
-        return;
-    }
+    /* Paired with blk_end_request in scsi_disk_co_aio_bh() */
+    blk_co_start_request(iocb->r->req.dev->conf.blk);
+    iocb->ret = iocb->fn(iocb);
+    aio_bh_schedule_oneshot(qemu_get_current_aio_context(),
+                            scsi_disk_co_aio_bh, iocb);
+}
 
-    scsi_req_complete(&r->req, GOOD);
+static void scsi_disk_co_aio_complete(void *opaque, int ret)
+{
+    SCSIDiskReq *r = opaque;
+
+    assert(r->req.ctx == qemu_get_current_aio_context());
+    assert(r->req.aiocb != NULL);
+    r->req.aiocb = NULL;
+
+    if (!scsi_disk_req_check_error(r, ret, true)) {
+        scsi_req_complete(&r->req, GOOD);
+    }
     scsi_req_unref(&r->req);
-    g_free(data);
 }
 
-static void scsi_unmap_complete(void *opaque, int ret)
+/* fn must submit with BDRV_REQ_NO_QUEUE and stop once iocb->canceled */
+static void scsi_disk_co_aio_start(SCSIDiskReq *r, SCSIDiskCoFunc *fn)
 {
-    UnmapCBData *data = opaque;
-    SCSIDiskReq *r = data->r;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    SCSIDiskCoAIOCB *iocb;
 
-    assert(r->req.aiocb != NULL);
-    r->req.aiocb = NULL;
+    iocb = blk_aio_get(&scsi_disk_co_aiocb_info, s->qdev.conf.blk,
+                       scsi_disk_co_aio_complete, r);
+    iocb->r = r;
+    iocb->fn = fn;
+    iocb->ret = 0;
+    iocb->canceled = false;
 
-    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);
+    /* The matching unref is in scsi_disk_co_aio_complete */
+    scsi_req_ref(&r->req);
+    r->req.aiocb = &iocb->common;
+    aio_co_enter(qemu_get_current_aio_context(),
+                 qemu_coroutine_create(scsi_disk_co_aio_entry, iocb));
+}
+
+/* Report like scsi_handle_rw_error(), but let werror=ignore go on */
+static bool scsi_disk_ignore_error(SCSIDiskReq *r, int ret)
+{
+    BlockBackend *blk = r->req.dev->conf.blk;
+    BlockErrorAction action = blk_get_error_action(blk, false, -ret);
+
+    if (action != BLOCK_ERROR_ACTION_IGNORE) {
+        return false;
+    }
+    blk_error_action(blk, action, false, r->sector << BDRV_SECTOR_BITS,
+                     r->sector_count << BDRV_SECTOR_BITS, -ret);
+    return true;
+}
+
+static int coroutine_fn scsi_unmap_co(SCSIDiskCoAIOCB *iocb)
+{
+    SCSIDiskReq *r = iocb->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    BlockBackend *blk = s->qdev.conf.blk;
+    uint8_t *p = r->iov.iov_base;
+    int count = lduw_be_p(&p[2]) >> 4;
+    int i, ret;
+
+    for (i = 0; i < count; i++) {
+        uint64_t sector_num = ldq_be_p(&p[8 + i * 16]);
+        uint32_t nb_sectors = ldl_be_p(&p[8 + i * 16 + 8]);
+
+        if (iocb->canceled) {
+            return -ECANCELED;
+        }
+
+        r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+        r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+
+        block_acct_start(blk_get_stats(blk), &r->acct,
+                         r->sector_count * BDRV_SECTOR_SIZE,
+                         BLOCK_ACCT_UNMAP);
+
+        ret = blk_co_pdiscard(blk, r->sector * BDRV_SECTOR_SIZE,
+                              r->sector_count * BDRV_SECTOR_SIZE,
+                              BDRV_REQ_NO_QUEUE);
+        if (ret < 0 && !scsi_disk_ignore_error(r, ret)) {
+            return ret;
+        }
+        block_acct_done(blk_get_stats(blk), &r->acct);
     }
+
+    return 0;
 }
 
 static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
@@ -1804,7 +1878,6 @@ 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;
     int count;
     int i;
 
@@ -1843,14 +1916,7 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
         }
     }
 
-    data = g_new0(UnmapCBData, 1);
-    data->r = r;
-    data->inbuf = &p[8];
-    data->count = count;
-
-    /* The matching unref is in scsi_unmap_complete, before data is freed.  */
-    scsi_req_ref(&r->req);
-    scsi_unmap_complete_noio(data, 0);
+    scsi_disk_co_aio_start(r, scsi_unmap_co);
     return;
 
 invalid_param_len:
-- 
2.53.0

[PATCH hci-8.1 3/5] scsi-disk: fix WRITE SAME racing with inactivation of block nodes #VSTOR-151532

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

WRITE SAME with a non-zero pattern is written in SCSI_WRITE_SAME_MAX
chunks, and each chunk is submitted from the completion callback of the
previous one. As with UNMAP, a drain only waits for the chunk in
flight, the next one is queued by blk_wait_while_drained(), and
bdrv_inactivate_all() resumes it on an inactive node:

  bdrv_co_write_req_prepare: Assertion
  `!(bs->open_flags & BDRV_O_INACTIVE)' failed.

Write the chunks from scsi_disk_co_aio_start() with BDRV_REQ_NO_QUEUE,
so a drain waits for the whole command. werror=ignore still goes on
with the next chunk.

The sector count becomes int64_t on the way: the old int overflowed for
a WRITE SAME(16) of 2^31 or more 512-byte sectors.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/scsi/scsi-disk.c | 102 +++++++++++++++++---------------------------
 1 file changed, 39 insertions(+), 63 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 8383b7cbe93..f287315fbc5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1929,52 +1929,52 @@ invalid_field:
     scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
 }
 
-typedef struct WriteSameCBData {
-    SCSIDiskReq *r;
-    int64_t sector;
-    int nb_sectors;
-    QEMUIOVector qiov;
-    struct iovec iov;
-} WriteSameCBData;
-
-static void scsi_write_same_complete(void *opaque, int ret)
+static int coroutine_fn scsi_write_same_co(SCSIDiskCoAIOCB *iocb)
 {
-    WriteSameCBData *data = opaque;
-    SCSIDiskReq *r = data->r;
+    SCSIDiskReq *r = iocb->r;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+    BlockBackend *blk = s->qdev.conf.blk;
+    uint8_t *inbuf = r->iov.iov_base;
+    uint32_t buflen = MIN(s->qdev.blocksize, r->buflen);
+    int64_t sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+    int64_t nb_sectors = (int64_t)scsi_data_cdb_xfer(r->req.cmd.buf) *
+                         (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+    size_t size = MIN(nb_sectors * BDRV_SECTOR_SIZE, SCSI_WRITE_SAME_MAX);
+    uint8_t *buf = blk_blockalign(blk, size);
+    int ret = 0;
+    size_t i, l;
+
+    for (i = 0; i < size; i += l) {
+        l = MIN(buflen, size - i);
+        memcpy(&buf[i], inbuf, l);
+    }
 
-    assert(r->req.aiocb != NULL);
-    r->req.aiocb = NULL;
+    while (nb_sectors > 0) {
+        size_t chunk = MIN(nb_sectors * BDRV_SECTOR_SIZE, size);
 
-    if (scsi_disk_req_check_error(r, ret, true)) {
-        goto done;
-    }
+        if (iocb->canceled) {
+            ret = -ECANCELED;
+            break;
+        }
 
-    block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
+        block_acct_start(blk_get_stats(blk), &r->acct, chunk,
+                         BLOCK_ACCT_WRITE);
+        ret = blk_co_pwrite(blk, sector << BDRV_SECTOR_BITS, chunk, buf,
+                            BDRV_REQ_NO_QUEUE);
+        if (ret < 0) {
+            if (!scsi_disk_ignore_error(r, ret)) {
+                break;
+            }
+            ret = 0;
+        }
+        block_acct_done(blk_get_stats(blk), &r->acct);
 
-    data->nb_sectors -= data->iov.iov_len / BDRV_SECTOR_SIZE;
-    data->sector += data->iov.iov_len / BDRV_SECTOR_SIZE;
-    data->iov.iov_len = MIN(data->nb_sectors * BDRV_SECTOR_SIZE,
-                            data->iov.iov_len);
-    if (data->iov.iov_len) {
-        block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
-                         data->iov.iov_len, BLOCK_ACCT_WRITE);
-        /* Reinitialize qiov, to handle unaligned WRITE SAME request
-         * where final qiov may need smaller size */
-        qemu_iovec_init_external(&data->qiov, &data->iov, 1);
-        r->req.aiocb = blk_aio_pwritev(s->qdev.conf.blk,
-                                       data->sector << BDRV_SECTOR_BITS,
-                                       &data->qiov, 0,
-                                       scsi_write_same_complete, data);
-        return;
+        sector += chunk / BDRV_SECTOR_SIZE;
+        nb_sectors -= chunk / BDRV_SECTOR_SIZE;
     }
 
-    scsi_req_complete(&r->req, GOOD);
-
-done:
-    scsi_req_unref(&r->req);
-    qemu_vfree(data->iov.iov_base);
-    g_free(data);
+    qemu_vfree(buf);
+    return ret;
 }
 
 static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
@@ -1983,9 +1983,6 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     uint32_t nb_sectors = scsi_data_cdb_xfer(r->req.cmd.buf);
     uint32_t buflen = MIN(s->qdev.blocksize, r->buflen);
-    WriteSameCBData *data;
-    uint8_t *buf;
-    int i, l;
 
     /* Fail if PBDATA=1 or LBDATA=1 or ANCHOR=1.  */
     if (nb_sectors == 0 || (req->cmd.buf[1] & 0x16)) {
@@ -2017,28 +2014,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
         return;
     }
 
-    data = g_new0(WriteSameCBData, 1);
-    data->r = r;
-    data->sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
-    data->nb_sectors = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
-    data->iov.iov_len = MIN(data->nb_sectors * BDRV_SECTOR_SIZE,
-                            SCSI_WRITE_SAME_MAX);
-    data->iov.iov_base = buf = blk_blockalign(s->qdev.conf.blk,
-                                              data->iov.iov_len);
-    qemu_iovec_init_external(&data->qiov, &data->iov, 1);
-
-    for (i = 0; i < data->iov.iov_len; i += l) {
-        l = MIN(buflen, data->iov.iov_len - i);
-        memcpy(&buf[i], inbuf, l);
-    }
-
-    scsi_req_ref(&r->req);
-    block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
-                     data->iov.iov_len, BLOCK_ACCT_WRITE);
-    r->req.aiocb = blk_aio_pwritev(s->qdev.conf.blk,
-                                   data->sector << BDRV_SECTOR_BITS,
-                                   &data->qiov, 0,
-                                   scsi_write_same_complete, data);
+    scsi_disk_co_aio_start(r, scsi_write_same_co);
 }
 
 static void scsi_disk_emulate_write_data(SCSIRequest *req)
-- 
2.53.0

[PATCH hci-8.1 4/5] virtio-scsi-test: split virtio_scsi_do_command() into submit and wait #VSTOR-151532

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

Add virtio_scsi_submit_command() and virtio_scsi_wait_command() so that
a test can act on the device while a command is still in flight.
virtio_scsi_do_command() becomes a wrapper around the two and its
callers are unchanged.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: Fabiano Rosas <farosas@suse.de>
Cc: Laurent Vivier <lvivier@redhat.com>
---
 tests/qtest/virtio-scsi-test.c | 85 +++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 26 deletions(-)

diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
index db10d572d0f..bc1edf046c2 100644
--- a/tests/qtest/virtio-scsi-test.c
+++ b/tests/qtest/virtio-scsi-test.c
@@ -59,22 +59,28 @@ static uint64_t qvirtio_scsi_alloc(QVirtioSCSIQueues *vs, size_t alloc_size,
     return addr;
 }
 
-static uint8_t virtio_scsi_do_command(QVirtioSCSIQueues *vs,
-                                      const uint8_t *cdb,
-                                      const uint8_t *data_in,
-                                      size_t data_in_len,
-                                      uint8_t *data_out, size_t data_out_len,
-                                      struct virtio_scsi_cmd_resp *resp_out)
+typedef struct {
+    uint32_t free_head;
+    uint64_t req_addr;
+    uint64_t resp_addr;
+    uint64_t data_in_addr;
+    uint64_t data_out_addr;
+} VirtioSCSICmd;
+
+static void virtio_scsi_submit_command(QVirtioSCSIQueues *vs,
+                                       const uint8_t *cdb,
+                                       const uint8_t *data_in,
+                                       size_t data_in_len,
+                                       uint8_t *data_out, size_t data_out_len,
+                                       VirtioSCSICmd *cmd)
 {
     QVirtQueue *vq;
     struct virtio_scsi_cmd_req req = { { 0 } };
     struct virtio_scsi_cmd_resp resp = { .response = 0xff, .status = 0xff };
-    uint64_t req_addr, resp_addr, data_in_addr = 0, data_out_addr = 0;
-    uint8_t response;
-    uint32_t free_head;
     QTestState *qts = global_qtest;
 
     vq = vs->vq[2];
+    *cmd = (VirtioSCSICmd) { 0 };
 
     req.lun[0] = 1; /* Select LUN */
     req.lun[1] = 1; /* Select target 1 */
@@ -83,41 +89,68 @@ static uint8_t virtio_scsi_do_command(QVirtioSCSIQueues *vs,
     /* XXX: Fix endian if any multi-byte field in req/resp is used */
 
     /* Add request header */
-    req_addr = qvirtio_scsi_alloc(vs, sizeof(req), &req);
-    free_head = qvirtqueue_add(qts, vq, req_addr, sizeof(req), false, true);
+    cmd->req_addr = qvirtio_scsi_alloc(vs, sizeof(req), &req);
+    cmd->free_head = qvirtqueue_add(qts, vq, cmd->req_addr, sizeof(req),
+                                    false, true);
 
     if (data_out_len) {
-        data_out_addr = qvirtio_scsi_alloc(vs, data_out_len, data_out);
-        qvirtqueue_add(qts, vq, data_out_addr, data_out_len, false, true);
+        cmd->data_out_addr = qvirtio_scsi_alloc(vs, data_out_len, data_out);
+        qvirtqueue_add(qts, vq, cmd->data_out_addr, data_out_len, false, true);
     }
 
     /* Add response header */
-    resp_addr = qvirtio_scsi_alloc(vs, sizeof(resp), &resp);
-    qvirtqueue_add(qts, vq, resp_addr, sizeof(resp), true, !!data_in_len);
+    cmd->resp_addr = qvirtio_scsi_alloc(vs, sizeof(resp), &resp);
+    qvirtqueue_add(qts, vq, cmd->resp_addr, sizeof(resp), true,
+                   !!data_in_len);
 
     if (data_in_len) {
-        data_in_addr = qvirtio_scsi_alloc(vs, data_in_len, data_in);
-        qvirtqueue_add(qts, vq, data_in_addr, data_in_len, true, false);
+        cmd->data_in_addr = qvirtio_scsi_alloc(vs, data_in_len, data_in);
+        qvirtqueue_add(qts, vq, cmd->data_in_addr, data_in_len, true, false);
     }
 
-    qvirtqueue_kick(qts, vs->dev, vq, free_head);
-    qvirtio_wait_used_elem(qts, vs->dev, vq, free_head, NULL,
-                           QVIRTIO_SCSI_TIMEOUT_US);
+    qvirtqueue_kick(qts, vs->dev, vq, cmd->free_head);
+}
+
+static uint8_t virtio_scsi_wait_command(QVirtioSCSIQueues *vs,
+                                        VirtioSCSICmd *cmd,
+                                        struct virtio_scsi_cmd_resp *resp_out,
+                                        gint64 timeout_us)
+{
+    uint8_t response;
+    QTestState *qts = global_qtest;
+
+    qvirtio_wait_used_elem(qts, vs->dev, vs->vq[2], cmd->free_head, NULL,
+                           timeout_us);
 
-    response = readb(resp_addr +
+    response = readb(cmd->resp_addr +
                      offsetof(struct virtio_scsi_cmd_resp, response));
 
     if (resp_out) {
-        memread(resp_addr, resp_out, sizeof(*resp_out));
+        memread(cmd->resp_addr, resp_out, sizeof(*resp_out));
     }
 
-    guest_free(alloc, req_addr);
-    guest_free(alloc, resp_addr);
-    guest_free(alloc, data_in_addr);
-    guest_free(alloc, data_out_addr);
+    guest_free(alloc, cmd->req_addr);
+    guest_free(alloc, cmd->resp_addr);
+    guest_free(alloc, cmd->data_in_addr);
+    guest_free(alloc, cmd->data_out_addr);
     return response;
 }
 
+static uint8_t virtio_scsi_do_command(QVirtioSCSIQueues *vs,
+                                      const uint8_t *cdb,
+                                      const uint8_t *data_in,
+                                      size_t data_in_len,
+                                      uint8_t *data_out, size_t data_out_len,
+                                      struct virtio_scsi_cmd_resp *resp_out)
+{
+    VirtioSCSICmd cmd;
+
+    virtio_scsi_submit_command(vs, cdb, data_in, data_in_len,
+                               data_out, data_out_len, &cmd);
+    return virtio_scsi_wait_command(vs, &cmd, resp_out,
+                                    QVIRTIO_SCSI_TIMEOUT_US);
+}
+
 static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev)
 {
     QVirtioSCSIQueues *vs;
-- 
2.53.0

[PATCH hci-8.1 5/5] virtio-scsi-test: test UNMAP and WRITE SAME during inactivation #VSTOR-151532

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

Inactivate all nodes while a multi-descriptor UNMAP, or a WRITE SAME
spanning several chunks, is in flight on a slow node and check that the
command completes. QEMU used to abort when the drain in
bdrv_inactivate_all() ended and resumed the request queued for the next
descriptor or chunk on an inactive node.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: Fabiano Rosas <farosas@suse.de>
Cc: Laurent Vivier <lvivier@redhat.com>
---
 tests/qtest/virtio-scsi-test.c | 88 ++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
index bc1edf046c2..16153c6e9ec 100644
--- a/tests/qtest/virtio-scsi-test.c
+++ b/tests/qtest/virtio-scsi-test.c
@@ -269,6 +269,68 @@ static void test_unmap_large_lba(void *obj, void *data,
     qvirtio_scsi_pci_free(vs);
 }
 
+static void inactivate_during_command(QVirtioSCSIPCI *scsi_pci,
+                                      QGuestAllocator *t_alloc,
+                                      const uint8_t *cdb,
+                                      uint8_t *data_out, size_t data_out_len)
+{
+    QVirtioSCSIQueues *vs;
+    VirtioSCSICmd cmd;
+    struct virtio_scsi_cmd_resp resp;
+
+    alloc = t_alloc;
+    vs = qvirtio_scsi_init(scsi_pci->scsi.vdev);
+
+    virtio_scsi_submit_command(vs, cdb, NULL, 0, data_out, data_out_len,
+                               &cmd);
+    qtest_qmp_assert_success(scsi_pci->pci_vdev.pdev->bus->qts,
+                             "{'execute': 'blockdev-set-active',"
+                             " 'arguments': {'active': false}}");
+
+    /* The drain must have waited for the whole command */
+    virtio_scsi_wait_command(vs, &cmd, &resp, 0);
+    g_assert_cmphex(resp.response, ==, 0);
+    g_assert_cmphex(resp.status, ==, GOOD);
+
+    qvirtio_scsi_pci_free(vs);
+}
+
+static void test_unmap_inactivate(void *obj, void *data,
+                                  QGuestAllocator *t_alloc)
+{
+    const uint8_t unmap[VIRTIO_SCSI_CDB_SIZE] = {
+        0x42, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x48, 0x00
+    };
+    uint8_t unmap_params[0x48] = {
+        0x00, 0x46, /* unmap data length */
+        0x00, 0x40, /* unmap block descriptor data length */
+    };
+    int i;
+
+    for (i = 0; i < 4; i++) {
+        uint8_t *desc = &unmap_params[8 + i * 16];
+
+        desc[7] = i * 8;    /* LBA */
+        desc[11] = 1;       /* sector count */
+    }
+
+    inactivate_during_command(obj, t_alloc, unmap, unmap_params,
+                              sizeof(unmap_params));
+}
+
+static void test_write_same_inactivate(void *obj, void *data,
+                                       QGuestAllocator *t_alloc)
+{
+    /* 2 MiB of non-zero data is written in 512 KiB chunks */
+    const uint8_t write_same[VIRTIO_SCSI_CDB_SIZE] = {
+        0x41, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00
+    };
+    uint8_t buf[512];
+
+    memset(buf, 0xa5, sizeof(buf));
+    inactivate_during_command(obj, t_alloc, write_same, buf, sizeof(buf));
+}
+
 static void test_write_to_cdrom(void *obj, void *data,
                                 QGuestAllocator *t_alloc)
 {
@@ -373,6 +435,20 @@ static void *virtio_scsi_setup_4k(GString *cmd_line, void *arg)
     return arg;
 }
 
+/* Each request must outlast the kick-to-QMP round trip on a loaded host */
+static void *virtio_scsi_setup_slow(GString *cmd_line, void *arg)
+{
+    g_string_append_printf(cmd_line,
+                           " -blockdev null-co,node-name=null0,read-zeroes=on"
+                           " -blockdev blkdebug,image=null0,node-name=dbg,"
+                           "discard=unmap,inject-error.0.event=none,"
+                           "inject-error.0.iotype=%s,inject-error.0.errno=0,"
+                           "inject-error.0.delay-ns=100000000"
+                           " -device scsi-hd,drive=dbg,lun=0,scsi-id=1",
+                           (const char *)arg);
+    return arg;
+}
+
 static void *virtio_scsi_setup_cd(GString *cmd_line, void *arg)
 {
     g_string_append(cmd_line,
@@ -410,6 +486,18 @@ static void register_virtio_scsi_test(void)
     opts.before = virtio_scsi_setup_cd;
     qos_add_test("write-to-cdrom", "virtio-scsi", test_write_to_cdrom, &opts);
 
+    /* ioeventfd=off starts the command before the kick returns */
+    opts.before = virtio_scsi_setup_slow;
+    opts.edge = (QOSGraphEdgeOptions) {
+        .extra_device_opts = "ioeventfd=off",
+    };
+    opts.arg = (void *)"discard";
+    qos_add_test("unmap-inactivate", "virtio-scsi-pci",
+                 test_unmap_inactivate, &opts);
+    opts.arg = (void *)"write";
+    qos_add_test("write-same-inactivate", "virtio-scsi-pci",
+                 test_write_same_inactivate, &opts);
+
     opts.before = virtio_scsi_setup_iothread;
     opts.edge = (QOSGraphEdgeOptions) {
         .extra_device_opts = "iothread=thread0",
-- 
2.53.0

Re: [PATCH hci-8.1 0/5] scsi-disk: fix UNMAP and WRITE SAME racing with inactivation #VSTOR-151532

[Andrey Drobyshev]

Let's hear what the upstream block maintainers have to say about this first:

https://lore.kernel.org/qemu-devel/20261026174220.674460-1-den@openvz.org

Andrey

On 10/26/26 7:49 PM, Denis V. Lunev wrote:
> scsi-disk splits UNMAP into one discard per descriptor and a non-zero
> WRITE SAME into 512 KiB writes, submitting each step from the
> completion callback of the previous one. A drain in between only waits
> for the step in flight; the next one is queued by
> blk_wait_while_drained() and resumed by drain_end. When the drain is
> the one in bdrv_inactivate_all() (end of live migration,
> blockdev-set-active), the resumed request hits an inactive node:
> 
>   bdrv_co_write_req_prepare: Assertion
>   `!(bs->open_flags & BDRV_O_INACTIVE)' failed.
> 
> We hit this in production on a guest UNMAP with 38 descriptors.
> 
> The fix follows the one Kevin did for IDE TRIM:
> https://lore.kernel.org/qemu-devel/20260421161132.99878-1-kwolf@redhat.com/
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Laurent Vivier <lvivier@redhat.com>
> Cc: qemu-stable@nongnu.org
> 
> Denis V. Lunev (5):
>   scsi-disk: check all UNMAP descriptors before discarding any
>     #VSTOR-151532
>   scsi-disk: fix UNMAP racing with inactivation of block nodes
>     #VSTOR-151532
>   scsi-disk: fix WRITE SAME racing with inactivation of block nodes
>     #VSTOR-151532
>   virtio-scsi-test: split virtio_scsi_do_command() into submit and wait
>     #VSTOR-151532
>   virtio-scsi-test: test UNMAP and WRITE SAME during inactivation
>     #VSTOR-151532
> 
>  hw/scsi/scsi-disk.c            | 278 +++++++++++++++++++--------------
>  tests/qtest/virtio-scsi-test.c | 173 +++++++++++++++++---
>  2 files changed, 309 insertions(+), 142 deletions(-)
> 
> base-commit: 81aea91c7f71cba39b4c3340579d45f0256a91d8
>