[PATCH hci-dev 0/6] block, scsi-disk: honour a size change on child replacement #VSTOR-130961

[PATCH hci-dev 0/6] block, scsi-disk: honour a size change on child replacement #VSTOR-130961

[Denis V. Lunev]

This is the QEMU side of VSTOR-130961. disk_replace puts the real image
under the device with vz-x-blockdev-replace, which ends up in
bdrv_replace_child_bs(), and that swaps the node a BdrvChild points to
without telling the parent anything about it. If the placeholder had a
different size, the device never learns about it.

scsi-disk is the one really hurt by this. check_lba_range() validates
every guest request against SCSIDevice.max_lba, filled in by
scsi_disk_reset() and updated only by the READ CAPACITY(10) and (16)
handlers, which a migrated guest never reissues. Every request past the
end of the old node is refused with ILLEGAL REQUEST / LOGICAL BLOCK
ADDRESS OUT OF RANGE for the whole life of the device, which Linux turns
into EREMOTEIO. It is silent on the host: no BLOCK_IO_ERROR event, and
io-status stays 'ok'. This is ASUP-3466, ACSSP-965 and ASUP-3678.

Upstream merged the series as 8b267376df8..28fafb3147f, from
<20260812121500.1034178-1-den@openvz.org>. Differences:

- Patch 1 is Fiona's 08736e7584c, which exports
  bdrv_co_parent_cb_resize(); our tree predates it.
- Patch 2: our bdrv_replace_child_bs() only adds its steps to the
  caller's transaction, under the graph write lock, so the length check
  and the notification move to a .commit action. Reading both lengths
  there also covers Kevin's review remark that the upstream version reads
  the old length before draining. His other remark, notifying only the
  replaced BdrvChild, will come with the upstream follow-up.
- Patch 6 is new: it covers vz-x-blockdev-replace, the path disk_replace
  takes, which the upstream test does not.

Tested on dev-cl04: without patches 1-3 the qtest fails at both the
qom-set and the vz-x-blockdev-replace step with ILLEGAL REQUEST; with
them it passes. qos-test quick passes; iotests -g quick for qcow2 and raw
give the same result with and without the series.

Denis V. Lunev (5):
  block: notify the parent when a replaced child has a different size
    #VSTOR-130961
  scsi-disk: refresh the cached capacity from the resize callback
    #VSTOR-130961
  tests/qtest/virtio-scsi: cover a replacement that changes the size
    #VSTOR-130961
  tests/qemu-iotests/qom-set-drive: replace with a differently sized
    node #VSTOR-130961
  tests/qtest/virtio-scsi: cover vz-x-blockdev-replace changing the size
    #VSTOR-130961

Fiona Ebner (1):
  block: make bdrv_co_parent_cb_resize() a proper IO API function
    #VSTOR-130961

 block.c                                    | 27 +++++++
 block/io.c                                 |  9 +--
 hw/scsi/scsi-disk.c                        | 16 +++-
 include/block/block_int-io.h               |  9 +++
 tests/qemu-iotests/tests/qom-set-drive     | 14 +++-
 tests/qemu-iotests/tests/qom-set-drive.out |  2 +
 tests/qtest/virtio-scsi-test.c             | 85 ++++++++++++++++++++++
 7 files changed, 149 insertions(+), 13 deletions(-)


base-commit: 2ac36f063996449791d0db63fc2c9a67864631d1
-- 
2.53.0

[PATCH hci-dev 1/6] block: make bdrv_co_parent_cb_resize() a proper IO API function #VSTOR-130961

[Denis V. Lunev]

From: Fiona Ebner <f.ebner@proxmox.com>

In preparation for calling it via the bdrv_child_cb_resize() callback
that will be added by the next commit. Rename it to include the "_co_"
part while at it.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Message-ID: <20250917115509.401015-3-f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
(cherry picked from commit 08736e7584c41b75e734f412e1fea5ae949afac1)
---
 block/io.c                   | 9 +++------
 include/block/block_int-io.h | 6 ++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/io.c b/block/io.c
index 5efe17b2515..ae8b474d0b0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -51,9 +51,6 @@
 /* Maximum number of copy-on-read aio tasks inside one read request */
 #define COR_MAX_IN_FLIGHT 16
 
-static void coroutine_fn GRAPH_RDLOCK
-bdrv_parent_cb_resize(BlockDriverState *bs);
-
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int64_t bytes, BdrvRequestFlags flags);
 
@@ -2172,7 +2169,7 @@ bdrv_co_write_req_finish(BdrvChild *child, int64_t offset, int64_t bytes,
          end_sector > bs->total_sectors) &&
         req->type != BDRV_TRACKED_DISCARD) {
         bs->total_sectors = end_sector;
-        bdrv_parent_cb_resize(bs);
+        bdrv_co_parent_cb_resize(bs);
         bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS);
     }
     if (req->bytes) {
@@ -4114,11 +4111,11 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
                                    bytes, read_flags, write_flags);
 }
 
-static void coroutine_fn GRAPH_RDLOCK
-bdrv_parent_cb_resize(BlockDriverState *bs)
+void coroutine_fn bdrv_co_parent_cb_resize(BlockDriverState *bs)
 {
     BdrvChild *c;
 
+    IO_CODE();
     assert_bdrv_graph_readable();
 
     QLIST_FOREACH(c, &bs->parents, next_parent) {
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index fd92bab25f5..e462b33b515 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -194,4 +194,10 @@ void bdrv_bsc_invalidate_range(BlockDriverState *bs,
  */
 void bdrv_bsc_fill(BlockDriverState *bs, int64_t offset, int64_t bytes);
 
+/*
+ * Notify all parents that the size of the child changed.
+ */
+void coroutine_fn GRAPH_RDLOCK
+bdrv_co_parent_cb_resize(BlockDriverState *bs);
+
 #endif /* BLOCK_INT_IO_H */
-- 
2.53.0

[PATCH hci-dev 2/6] block: notify the parent when a replaced child has a different size #VSTOR-130961

[Denis V. Lunev]

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

bdrv_replace_child_bs() swaps the node a BdrvChild points to without
telling the parent anything about it, so a parent which tracks the size of
its child never learns that it changed. Every other way the size of a node
changes goes through bdrv_co_truncate(), which does notify.

The device models which care all register the same BlockDevOps hook:

  virtio-blk  resize_cb = virtio_blk_resize(), ending up in
              virtio_notify_config()
  ide/ahci    resize_cb = ide_resize_cb(), refreshing the cached size
              and the IDENTIFY data
  scsi-disk   resize_cb = scsi_disk_resize_cb(), reporting
              CAPACITY DATA HAS CHANGED
  xen-block   resize_cb = xen_block_resize_cb()

Only scsi-disk is really hurt by the missing notification, because it is
the one which validates guest requests against a cached size.
virtio_blk_sect_range_ok() and ide_sect_range_ok() both call
blk_get_geometry() for every request, so a missing notification leaves
those guests with a stale idea of the size but never refuses I/O which the
node underneath can serve. check_lba_range() compares against
SCSIDevice.max_lba, filled in by scsi_disk_reset() and updated only by the
READ CAPACITY(10) and (16) handlers, so with nothing to make the guest
re-read the capacity every request past the end of the old node is refused
for the whole life of the device.

This is reachable with qom-set of the 'drive' property, which is allowed
on a realized device, and it is silent on the host: out-of-range requests
are answered by scsi_check_condition() and never reach
scsi_handle_rw_error(), so there is no BLOCK_IO_ERROR event and io-status
stays 'ok'. The guest sees ILLEGAL REQUEST / LOGICAL BLOCK ADDRESS OUT OF
RANGE, which Linux turns into EREMOTEIO for reads as well as writes.

Emit the notification from bdrv_replace_child_bs(), so that every device
model gets it through the path it already implements rather than any of
them being special cased. Send it only when the size really changed, which
keeps the callback out of the common case of a replacement by an equally
sized node.

[ Ported to hci-dev: the downstream bdrv_replace_child_bs() only adds
  its steps to the caller's Transaction, and the callers hold the graph
  write lock while it runs, so neither the length query nor the
  notification can be done there. Both happen in a .commit action
  instead: tran_finalize() runs after the lock is dropped, and both
  nodes stay referenced and drained by bdrv_drained() until .clean. ]

Signed-off-by: Denis V. Lunev <den@openvz.org>
Message-ID: <20260812121500.1034178-2-den@openvz.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 8b267376df877681d0ac9fbe24035d06bf6ed47f)
---
 block.c                      | 27 +++++++++++++++++++++++++++
 include/block/block_int-io.h |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/block.c b/block.c
index 7b27cb4edf3..1dc7e47431c 100644
--- a/block.c
+++ b/block.c
@@ -5605,6 +5605,28 @@ static void bdrv_drained(BlockDriverState *bs, Transaction *tran)
     tran_add(tran, &bdrv_drained_drv, bs);
 }
 
+typedef struct BdrvReplaceResizeState {
+    BlockDriverState *old_bs;
+    BlockDriverState *new_bs;
+} BdrvReplaceResizeState;
+
+static void bdrv_replace_resize_commit(void *opaque)
+{
+    BdrvReplaceResizeState *s = opaque;
+    int64_t old_len = bdrv_getlength(s->old_bs);
+    int64_t new_len = bdrv_getlength(s->new_bs);
+
+    /* A parent caching the child size has to learn that it changed */
+    if (old_len >= 0 && new_len >= 0 && new_len != old_len) {
+        bdrv_parent_cb_resize(s->new_bs);
+    }
+}
+
+static TransactionActionDrv bdrv_replace_resize_drv = {
+    .commit = bdrv_replace_resize_commit,
+    .clean = g_free,
+};
+
 /* Not for empty child */
 int TSA_NO_TSA
 bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
@@ -5612,9 +5634,14 @@ bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
 {
     g_autoptr(GSList) refresh_list = NULL;
     BlockDriverState *old_bs = child->bs;
+    BdrvReplaceResizeState *s;
 
     GLOBAL_STATE_CODE();
 
+    s = g_new(BdrvReplaceResizeState, 1);
+    *s = (BdrvReplaceResizeState) { .old_bs = old_bs, .new_bs = new_bs };
+    tran_add(tran, &bdrv_replace_resize_drv, s);
+
     bdrv_drained(old_bs, tran);
     bdrv_drained(new_bs, tran);
 
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index e462b33b515..6f0c278bbd4 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -200,4 +200,7 @@ void bdrv_bsc_fill(BlockDriverState *bs, int64_t offset, int64_t bytes);
 void coroutine_fn GRAPH_RDLOCK
 bdrv_co_parent_cb_resize(BlockDriverState *bs);
 
+void co_wrapper_bdrv_rdlock
+bdrv_parent_cb_resize(BlockDriverState *bs);
+
 #endif /* BLOCK_INT_IO_H */
-- 
2.53.0

[PATCH hci-dev 3/6] scsi-disk: refresh the cached capacity from the resize callback #VSTOR-130961

[Denis V. Lunev]

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

scsi_disk_resize_cb() reports CAPACITY DATA HAS CHANGED but leaves
SCSIDevice.max_lba alone, and max_lba is what check_lba_range() compares
every request against. Until the guest reacts to the unit attention and
issues READ CAPACITY, requests beyond the old size keep being refused with
LOGICAL BLOCK ADDRESS OUT OF RANGE, and a guest which never re-reads the
capacity never recovers at all. A guest migrated onto a device whose size
changed underneath it is exactly such a guest.

ide_resize_cb() refreshes its cached size along with the IDENTIFY data it
reports to the guest. Do the same here, so that the notification is what
tells the guest about the new capacity rather than what unblocks the
device.

Factor the computation out of scsi_disk_reset(), which already had it.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Message-ID: <20260812121500.1034178-3-den@openvz.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit f26eb3c2770ea6a85ffb306a6ca5602f0d2f79ec)
---
 hw/scsi/scsi-disk.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a501c7ffa4a..80a65c20005 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2388,13 +2388,10 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
     }
 }
 
-static void scsi_disk_reset(DeviceState *dev)
+static void scsi_disk_refresh_max_lba(SCSIDiskState *s)
 {
-    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev.qdev, dev);
     uint64_t nb_sectors;
 
-    scsi_device_purge_requests(&s->qdev, SENSE_CODE(RESET));
-
     blk_get_geometry(s->qdev.conf.blk, &nb_sectors);
 
     nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
@@ -2402,6 +2399,15 @@ static void scsi_disk_reset(DeviceState *dev)
         nb_sectors--;
     }
     s->qdev.max_lba = nb_sectors;
+}
+
+static void scsi_disk_reset(DeviceState *dev)
+{
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev.qdev, dev);
+
+    scsi_device_purge_requests(&s->qdev, SENSE_CODE(RESET));
+
+    scsi_disk_refresh_max_lba(s);
     /* reset tray statuses */
     s->tray_locked = 0;
     s->tray_open = 0;
@@ -2427,6 +2433,8 @@ static void scsi_disk_resize_cb(void *opaque)
 {
     SCSIDiskState *s = opaque;
 
+    scsi_disk_refresh_max_lba(s);
+
     /* SPC lists this sense code as available only for
      * direct-access devices.
      */
-- 
2.53.0

[PATCH hci-dev 4/6] tests/qtest/virtio-scsi: cover a replacement that changes the size #VSTOR-130961

[Denis V. Lunev]

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

Realize a scsi-hd on a 1 MiB node, replace it with a 128 MiB one using
qom-set, and check that the device reports CAPACITY DATA HAS CHANGED and
then serves a read which only fits in the new node.

Both halves regress on their own. Without the notification from
bdrv_replace_child_bs() the first command after the replacement fails with
ILLEGAL REQUEST / LOGICAL BLOCK ADDRESS OUT OF RANGE instead of the unit
attention. Without the refresh in scsi_disk_resize_cb() the read after it
is refused the same way, since nothing here issues READ CAPACITY.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Message-ID: <20260812121500.1034178-4-den@openvz.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit e0856654911f3730cc71ab5c3a3a272cdbaff148)
---
 tests/qtest/virtio-scsi-test.c | 63 ++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
index db10d572d0f..f1aaeb31559 100644
--- a/tests/qtest/virtio-scsi-test.c
+++ b/tests/qtest/virtio-scsi-test.c
@@ -236,6 +236,53 @@ static void test_unmap_large_lba(void *obj, void *data,
     qvirtio_scsi_pci_free(vs);
 }
 
+/* Test replacing the backing node with one of a different size */
+static void test_replaced_node_size(void *obj, void *data,
+                                    QGuestAllocator *t_alloc)
+{
+    QVirtioSCSI *scsi = obj;
+    QVirtioSCSIQueues *vs;
+    uint8_t buf[512] = { 0 };
+    /* READ(10), LBA 0, transfer length 1 */
+    const uint8_t read_low_cdb[VIRTIO_SCSI_CDB_SIZE] = {
+        0x28, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00
+    };
+    /* READ(10), LBA 0x1000 (2 MiB), transfer length 1 */
+    const uint8_t read_high_cdb[VIRTIO_SCSI_CDB_SIZE] = {
+        0x28, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x01, 0x00
+    };
+    struct virtio_scsi_cmd_resp resp;
+
+    alloc = t_alloc;
+    vs = qvirtio_scsi_init(scsi->vdev);
+
+    /* LBA 0 is inside the 1 MiB node the device was realized with */
+    virtio_scsi_do_command(vs, read_low_cdb, buf, sizeof(buf), NULL, 0, &resp);
+    g_assert_cmphex(resp.response, ==, 0);
+    g_assert_cmphex(resp.status, ==, GOOD);
+
+    /* Swap in a 128 MiB node; the device is not reset by this */
+    qtest_qmp_assert_success(global_qtest,
+                             "{'execute': 'qom-set', 'arguments': {"
+                             "  'path': '/machine/peripheral/scsi-hd0',"
+                             "  'property': 'drive', 'value': 'big'}}");
+
+    /* The guest has to be told that the capacity it knows is stale */
+    virtio_scsi_do_command(vs, read_high_cdb, buf, sizeof(buf), NULL, 0, &resp);
+    g_assert_cmphex(resp.response, ==, 0);
+    g_assert_cmphex(resp.status, ==, CHECK_CONDITION);
+    g_assert_cmphex(resp.sense[2], ==, UNIT_ATTENTION);
+    g_assert_cmphex(resp.sense[12], ==, 0x2a);
+    g_assert_cmphex(resp.sense[13], ==, 0x09); /* CAPACITY DATA HAS CHANGED */
+
+    /* LBA 0x1000 is past the old node but well inside the new one */
+    virtio_scsi_do_command(vs, read_high_cdb, buf, sizeof(buf), NULL, 0, &resp);
+    g_assert_cmphex(resp.response, ==, 0);
+    g_assert_cmphex(resp.status, ==, GOOD);
+
+    qvirtio_scsi_pci_free(vs);
+}
+
 static void test_write_to_cdrom(void *obj, void *data,
                                 QGuestAllocator *t_alloc)
 {
@@ -340,6 +387,18 @@ static void *virtio_scsi_setup_4k(GString *cmd_line, void *arg)
     return arg;
 }
 
+static void *virtio_scsi_setup_replace(GString *cmd_line, void *arg)
+{
+    g_string_append(cmd_line,
+                    " -blockdev driver=null-co,read-zeroes=on,"
+                    "size=1048576,node-name=small"
+                    " -blockdev driver=null-co,read-zeroes=on,"
+                    "size=134217728,node-name=big"
+                    " -device scsi-hd,id=scsi-hd0,drive=small,"
+                    "lun=0,scsi-id=1");
+    return arg;
+}
+
 static void *virtio_scsi_setup_cd(GString *cmd_line, void *arg)
 {
     g_string_append(cmd_line,
@@ -374,6 +433,10 @@ static void register_virtio_scsi_test(void)
     qos_add_test("large-lba-unmap", "virtio-scsi",
                  test_unmap_large_lba, &opts);
 
+    opts.before = virtio_scsi_setup_replace;
+    qos_add_test("replaced-node-size", "virtio-scsi",
+                 test_replaced_node_size, &opts);
+
     opts.before = virtio_scsi_setup_cd;
     qos_add_test("write-to-cdrom", "virtio-scsi", test_write_to_cdrom, &opts);
 
-- 
2.53.0

[PATCH hci-dev 5/6] tests/qemu-iotests/qom-set-drive: replace with a differently sized node #VSTOR-130961

[Denis V. Lunev]

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

All images in this test have the same size, so the test never reaches
the size change path of bdrv_replace_child_bs(). Add a node of a
different size and point a device at it.

The notification itself is not visible over QMP, so this only covers the
block layer side. What the device does with it is checked by
tests/qtest/virtio-scsi-test.c.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Message-ID: <20260812121500.1034178-5-den@openvz.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 28fafb3147f54291c384a305796384051882067d)
---
 tests/qemu-iotests/tests/qom-set-drive     | 14 +++++++++++---
 tests/qemu-iotests/tests/qom-set-drive.out |  2 ++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/tests/qom-set-drive b/tests/qemu-iotests/tests/qom-set-drive
index ec8ddac4fdd..52d7eb96da8 100755
--- a/tests/qemu-iotests/tests/qom-set-drive
+++ b/tests/qemu-iotests/tests/qom-set-drive
@@ -24,12 +24,15 @@ import iotests
 from iotests import imgfmt, log, qemu_img_create, QMPTestCase
 
 image_size = 1 * 1024 * 1024
-images = [os.path.join(iotests.test_dir, f'{i}.img') for i in range(0, 4)]
+images = [os.path.join(iotests.test_dir, f'{i}.img') for i in range(0, 5)]
+
+# node4 is deliberately larger, to cover a replacement which changes the size
+image_sizes = [image_size] * 4 + [2 * image_size]
 
 class TestQOMSetDrive(QMPTestCase):
     def setUp(self) -> None:
-        for image in images:
-            qemu_img_create('-f', imgfmt, image, str(image_size))
+        for image, size in zip(images, image_sizes):
+            qemu_img_create('-f', imgfmt, image, str(size))
 
         self.vm = iotests.VM()
         for i, image in enumerate(images):
@@ -68,6 +71,11 @@ class TestQOMSetDrive(QMPTestCase):
         log(self.vm.qmp('qom-get', path='/machine/peripheral/no-iot',
                         property='drive'))
 
+        log(self.vm.qmp('qom-set', path='/machine/peripheral/no-iot',
+                        property='drive', value='node4'))
+        log(self.vm.qmp('qom-get', path='/machine/peripheral/no-iot',
+                        property='drive'))
+
 if __name__ == '__main__':
     iotests.activate_logging()
     # LUKS would require special key-secret handling in add_blockdevs()
diff --git a/tests/qemu-iotests/tests/qom-set-drive.out b/tests/qemu-iotests/tests/qom-set-drive.out
index 7fc243dca6c..6f6e5e2b31c 100644
--- a/tests/qemu-iotests/tests/qom-set-drive.out
+++ b/tests/qemu-iotests/tests/qom-set-drive.out
@@ -4,6 +4,8 @@
 {"return": "node1"}
 {"return": {}}
 {"return": "node3"}
+{"return": {}}
+{"return": "node4"}
 .
 ----------------------------------------------------------------------
 Ran 1 tests
-- 
2.53.0

[PATCH hci-dev 6/6] tests/qtest/virtio-scsi: cover vz-x-blockdev-replace changing the size #VSTOR-130961

[Denis V. Lunev]

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

replaced-node-size reaches bdrv_replace_child_bs() through qom-set and
blk_replace_bs(). The disk_replace migration uses vz-x-blockdev-replace
instead, which gets there from blockdev_replace() inside a transaction
of its own. Swap in a third, larger node that way as well and check that
the guest is told about the new capacity and can read past the old end.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 tests/qtest/virtio-scsi-test.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
index f1aaeb31559..4e2bcd7b485 100644
--- a/tests/qtest/virtio-scsi-test.c
+++ b/tests/qtest/virtio-scsi-test.c
@@ -251,6 +251,10 @@ static void test_replaced_node_size(void *obj, void *data,
     const uint8_t read_high_cdb[VIRTIO_SCSI_CDB_SIZE] = {
         0x28, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x01, 0x00
     };
+    /* READ(10), LBA 0x40000 (128 MiB), transfer length 1 */
+    const uint8_t read_huge_cdb[VIRTIO_SCSI_CDB_SIZE] = {
+        0x28, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00
+    };
     struct virtio_scsi_cmd_resp resp;
 
     alloc = t_alloc;
@@ -280,6 +284,22 @@ static void test_replaced_node_size(void *obj, void *data,
     g_assert_cmphex(resp.response, ==, 0);
     g_assert_cmphex(resp.status, ==, GOOD);
 
+    qtest_qmp_assert_success(global_qtest,
+                             "{'execute': 'vz-x-blockdev-replace', "
+                             "'arguments': {'parent-type': 'qdev',"
+                             "  'qdev-id': 'scsi-hd0', 'new-child': 'huge'}}");
+
+    virtio_scsi_do_command(vs, read_huge_cdb, buf, sizeof(buf), NULL, 0, &resp);
+    g_assert_cmphex(resp.response, ==, 0);
+    g_assert_cmphex(resp.status, ==, CHECK_CONDITION);
+    g_assert_cmphex(resp.sense[2], ==, UNIT_ATTENTION);
+    g_assert_cmphex(resp.sense[12], ==, 0x2a);
+    g_assert_cmphex(resp.sense[13], ==, 0x09);
+
+    virtio_scsi_do_command(vs, read_huge_cdb, buf, sizeof(buf), NULL, 0, &resp);
+    g_assert_cmphex(resp.response, ==, 0);
+    g_assert_cmphex(resp.status, ==, GOOD);
+
     qvirtio_scsi_pci_free(vs);
 }
 
@@ -394,6 +414,8 @@ static void *virtio_scsi_setup_replace(GString *cmd_line, void *arg)
                     "size=1048576,node-name=small"
                     " -blockdev driver=null-co,read-zeroes=on,"
                     "size=134217728,node-name=big"
+                    " -blockdev driver=null-co,read-zeroes=on,"
+                    "size=268435456,node-name=huge"
                     " -device scsi-hd,id=scsi-hd0,drive=small,"
                     "lun=0,scsi-id=1");
     return arg;
-- 
2.53.0

Re: [PATCH hci-dev 1/6] block: make bdrv_co_parent_cb_resize() a proper IO API function #VSTOR-130961

[no-reply]

> From: Fiona Ebner <f.ebner@proxmox.com>
> 
> In preparation for calling it via the bdrv_child_cb_resize() callback
> that will be added by the next commit. Rename it to include the "_co_"
> part while at it.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
> Message-ID: <20250917115509.401015-3-f.ebner@proxmox.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261104134539.1519657-1-den@openvz.org?part=1

Re: [PATCH hci-dev 3/6] scsi-disk: refresh the cached capacity from the resize callback #VSTOR-130961

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] scsi-disk: scsi_disk_refresh_max_lba() leaves max_lba 0 for a zero-size backend
--

commit 0d24fb2b136b4040fa253df9cd5bb8b0c4064335
Author: Denis V. Lunev <den@openvz.org>

scsi-disk: refresh the cached capacity from the resize callback

This commit moves the max_lba computation out of scsi_disk_reset() into a
new helper, scsi_disk_refresh_max_lba(). It then calls the helper from
scsi_disk_resize_cb() so the cached capacity follows a backend resize.

Message-ID: <20260812121500.1034178-3-den@openvz.org>

> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c

[ ... ]

> +static void scsi_disk_refresh_max_lba(SCSIDiskState *s)
>  {
>      uint64_t nb_sectors;
>  
>      blk_get_geometry(s->qdev.conf.blk, &nb_sectors);
>  
>      nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
>      if (nb_sectors) {
>          nb_sectors--;
>      }
>      s->qdev.max_lba = nb_sectors;
> +}

[Severity: Low]
Can max_lba end up describing a one-block device when the backend has zero
sectors?

blk_get_geometry() maps a negative blk_nb_sectors() to 0. The helper only
decrements when nb_sectors is non-zero, so max_lba stays 0 in that case.

check_lba_range() then evaluates a request for LBA 0 with length 1 as:

    sector_num + nb_sectors <= s->qdev.max_lba + 1

which is 0 + 1 <= 0 + 1, so the request is accepted. It is not refused
with LOGICAL BLOCK ADDRESS OUT OF RANGE.

The guard in scsi_disk_dma_command() does not cover this:

    if (!blk_is_available(s->qdev.conf.blk)) {
        scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
        return 0;
    }

It only tests for a missing medium, not a present medium that was resized
to 0 sectors.

The READ CAPACITY handling in scsi_disk_emulate_command_complete() takes a
different approach for a zero-size backend:

    blk_get_geometry(s->qdev.conf.blk, &nb_sectors);
    if (!nb_sectors) {
        scsi_check_condition(r, SENSE_CODE(LUN_NOT_READY));
        return 0;
    }

That path returns LUN_NOT_READY and does not touch max_lba. The new helper
has no equivalent check.

The zero-size handling was already in scsi_disk_reset(). This patch newly
applies it at runtime from scsi_disk_resize_cb(), for example after a
resize to 0 sectors or a failed size query during a replace.

I did not check how the block layer treats the request after it passes
check_lba_range(). The result is at least a wrong sense code, and possibly
an error from the backend.

Should the helper handle the zero-sector case separately, so that
check_lba_range() refuses every request in that state?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261104134539.1519657-1-den@openvz.org?part=3

Re: [PATCH hci-dev 2/6] block: notify the parent when a replaced child has a different size #VSTOR-130961

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] block: bdrv_replace_resize_commit() notifies all parents of new_bs
--

commit 1388b90801798d7d441e56e99ea5745089939495
Author: Denis V. Lunev <den@openvz.org>

block: notify the parent when a replaced child has a different size #VSTOR-130961

This commit makes bdrv_replace_child_bs() register a transaction action. When the transaction commits, the action compares the lengths of the old and new nodes. If they differ, it calls bdrv_parent_cb_resize() on the new node, so parents that cache the child size learn about the change.

> diff --git a/block.c b/block.c
> --- a/block.c
> +++ b/block.c

[ ... ]

> +static void bdrv_replace_resize_commit(void *opaque)
> +{
> +    BdrvReplaceResizeState *s = opaque;
> +    int64_t old_len = bdrv_getlength(s->old_bs);
> +    int64_t new_len = bdrv_getlength(s->new_bs);
> +
> +    /* A parent caching the child size has to learn that it changed */
> +    if (old_len >= 0 && new_len >= 0 && new_len != old_len) {
> +        bdrv_parent_cb_resize(s->new_bs);
> +    }
> +}

[Severity: Low]
Is the notification broader than the commit message describes? The subject and body talk about notifying "the parent" of the replaced child. However, bdrv_replace_resize_commit() calls bdrv_parent_cb_resize(s->new_bs), which wraps bdrv_co_parent_cb_resize(). That function walks every entry in bs->parents:

block/io.c:bdrv_co_parent_cb_resize() {
    ...
    QLIST_FOREACH(c, &bs->parents, next_parent) {
        if (c->klass->resize) {
            c->klass->resize(c);
        }
    }
}

BdrvReplaceResizeState records only old_bs and new_bs, not the replaced BdrvChild. The old_len versus new_len check says nothing about the other parents of new_bs.

When the commit action runs, the replaced child is already a parent of new_bs. Any other BlockBackend, export or device model attached to new_bs also gets resize(). Their view of the size did not change.

Can this send redundant notifications to those parents? For example, virtio-blk would get an extra config-change interrupt. scsi-disk would report a spurious CAPACITY DATA HAS CHANGED unit attention.

Would it be better to notify only the parent that was replaced, or to describe the broader behavior in the commit message?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261104134539.1519657-1-den@openvz.org?part=2

Re: [PATCH hci-dev 5/6] tests/qemu-iotests/qom-set-drive: replace with a differently sized node #VSTOR-130961

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> All images in this test have the same size, so the test never reaches
> the size change path of bdrv_replace_child_bs(). Add a node of a
> different size and point a device at it.
> 
> The notification itself is not visible over QMP, so this only covers the
> block layer side. What the device does with it is checked by
> tests/qtest/virtio-scsi-test.c.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Message-ID: <20260812121500.1034178-5-den@openvz.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261104134539.1519657-1-den@openvz.org?part=5

Re: [PATCH hci-dev 6/6] tests/qtest/virtio-scsi: cover vz-x-blockdev-replace changing the size #VSTOR-130961

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> replaced-node-size reaches bdrv_replace_child_bs() through qom-set and
> blk_replace_bs(). The disk_replace migration uses vz-x-blockdev-replace
> instead, which gets there from blockdev_replace() inside a transaction
> of its own. Swap in a third, larger node that way as well and check that
> the guest is told about the new capacity and can read past the old end.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261104134539.1519657-1-den@openvz.org?part=6

Re: [PATCH hci-dev 4/6] tests/qtest/virtio-scsi: cover a replacement that changes the size #VSTOR-130961

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] qtest: replaced-node tests never check equal-size or multi-parent notification
--

Subject: Re: [PATCH] tests/qtest/virtio-scsi: cover a replacement that changes the size #VSTOR-130961

commit dfa745e49f7f99a718150defda33d68b4cb092a9
Author: Denis V. Lunev <den@openvz.org>

tests/qtest/virtio-scsi: cover a replacement that changes the size #VSTOR-130961

This adds a qtest that realizes a scsi-hd on a 1 MiB null-co node, replaces
it with a 128 MiB node via qom-set, and checks for the CAPACITY DATA HAS
CHANGED unit attention followed by a successful read that only fits in the
new node.

Message-ID: <20260812121500.1034178-4-den@openvz.org>

> diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
> --- a/tests/qtest/virtio-scsi-test.c
> +++ b/tests/qtest/virtio-scsi-test.c

[ ... ]

> +/* Test replacing the backing node with one of a different size */
> +static void test_replaced_node_size(void *obj, void *data,
> +                                    QGuestAllocator *t_alloc)
> +{

[ ... ]

> +    /* Swap in a 128 MiB node; the device is not reset by this */
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{'execute': 'qom-set', 'arguments': {"
> +                             "  'path': '/machine/peripheral/scsi-hd0',"
> +                             "  'property': 'drive', 'value': 'big'}}");

[Severity: Low]
This isn't a bug, but is the coverage here complete enough?

test_replaced_node_size() only exercises a size-changing replacement
(small at 1 MiB to big at 128 MiB) with a single parent, scsi-hd0.

The notification is issued from bdrv_replace_resize_commit() in block.c:

    if (old_len >= 0 && new_len >= 0 && new_len != old_len) {
        bdrv_parent_cb_resize(s->new_bs);
    }

Would this test still pass if the new_len != old_len check were dropped?
An equal-size replacement would then send a spurious capacity-changed unit
attention, and nothing here would catch it.

Also, bdrv_parent_cb_resize() goes through bdrv_co_parent_cb_resize(),
which notifies every parent of new_bs, not only the replaced child.  Would
it be worth adding a case where new_bs has a second parent, to check that
behaviour?

Looking at the final state of the series, the later patches only add
further size-changing replacements (node4 at 2 MiB in the qom-set-drive
iotest, and the huge node via vz-x-blockdev-replace).  I did not see an
equal-size case or a multi-parent case added anywhere in the series.

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261104134539.1519657-1-den@openvz.org?part=4

Re: [PATCH hci-dev 0/6] block, scsi-disk: honour a size change on child replacement #VSTOR-130961

[Andrey Drobyshev]

On 11/4/26 3:45 PM, Denis V. Lunev wrote:
> This is the QEMU side of VSTOR-130961. disk_replace puts the real image
> under the device with vz-x-blockdev-replace, which ends up in
> bdrv_replace_child_bs(), and that swaps the node a BdrvChild points to
> without telling the parent anything about it. If the placeholder had a
> different size, the device never learns about it.
> 
> scsi-disk is the one really hurt by this. check_lba_range() validates
> every guest request against SCSIDevice.max_lba, filled in by
> scsi_disk_reset() and updated only by the READ CAPACITY(10) and (16)
> handlers, which a migrated guest never reissues. Every request past the
> end of the old node is refused with ILLEGAL REQUEST / LOGICAL BLOCK
> ADDRESS OUT OF RANGE for the whole life of the device, which Linux turns
> into EREMOTEIO. It is silent on the host: no BLOCK_IO_ERROR event, and
> io-status stays 'ok'. This is ASUP-3466, ACSSP-965 and ASUP-3678.
> 
> Upstream merged the series as 8b267376df8..28fafb3147f, from
> <20260812121500.1034178-1-den@openvz.org>. Differences:
> 
> - Patch 1 is Fiona's 08736e7584c, which exports
>   bdrv_co_parent_cb_resize(); our tree predates it.
> - Patch 2: our bdrv_replace_child_bs() only adds its steps to the
>   caller's transaction, under the graph write lock, so the length check
>   and the notification move to a .commit action. Reading both lengths
>   there also covers Kevin's review remark that the upstream version reads
>   the old length before draining. His other remark, notifying only the
>   replaced BdrvChild, will come with the upstream follow-up.
> - Patch 6 is new: it covers vz-x-blockdev-replace, the path disk_replace
>   takes, which the upstream test does not.
> 
> Tested on dev-cl04: without patches 1-3 the qtest fails at both the
> qom-set and the vz-x-blockdev-replace step with ILLEGAL REQUEST; with
> them it passes. qos-test quick passes; iotests -g quick for qcow2 and raw
> give the same result with and without the series.
> 
> Denis V. Lunev (5):
>   block: notify the parent when a replaced child has a different size
>     #VSTOR-130961
>   scsi-disk: refresh the cached capacity from the resize callback
>     #VSTOR-130961
>   tests/qtest/virtio-scsi: cover a replacement that changes the size
>     #VSTOR-130961
>   tests/qemu-iotests/qom-set-drive: replace with a differently sized
>     node #VSTOR-130961
>   tests/qtest/virtio-scsi: cover vz-x-blockdev-replace changing the size
>     #VSTOR-130961
> 
> Fiona Ebner (1):
>   block: make bdrv_co_parent_cb_resize() a proper IO API function
>     #VSTOR-130961
> 
>  block.c                                    | 27 +++++++
>  block/io.c                                 |  9 +--
>  hw/scsi/scsi-disk.c                        | 16 +++-
>  include/block/block_int-io.h               |  9 +++
>  tests/qemu-iotests/tests/qom-set-drive     | 14 +++-
>  tests/qemu-iotests/tests/qom-set-drive.out |  2 +
>  tests/qtest/virtio-scsi-test.c             | 85 ++++++++++++++++++++++
>  7 files changed, 149 insertions(+), 13 deletions(-)
> 
> 
> base-commit: 2ac36f063996449791d0db63fc2c9a67864631d1

Ack
Applied to hci-dev