[PATCH 0/4] block, scsi-disk: honour a size change on child replacement

[PATCH 0/4] block, scsi-disk: honour a size change on child replacement

[Denis V. Lunev]

qom-set of the 'drive' property is allowed on a realized device and ends
up in bdrv_replace_child_bs(), which swaps the node a BdrvChild points to
without telling the parent anything about it. Every other way the size of
a node changes goes through bdrv_co_truncate(), which does notify, so a
device model caching the size of its child has no way to learn that it
changed.

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. Point a scsi-hd at a larger node and every request past the end
of the old one is refused with ILLEGAL REQUEST / LOGICAL BLOCK ADDRESS
OUT OF RANGE for the whole life of the device, which Linux turns into
EREMOTEIO for reads as well as writes. 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'. virtio-blk and ide call blk_get_geometry() for
every request, so they end up with a stale idea of the size but never
refuse I/O the node underneath can serve.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Fam Zheng <fam@euphon.net>

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

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


base-commit: 3e3ccab106f879b1512f8e0d51a827dd4de30e22
-- 
2.53.0

[PATCH 1/4] block: notify the parent when a replaced child has a different size

[Denis V. Lunev]

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.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block.c                      | 13 +++++++++++++
 include/block/block_int-io.h |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/block.c b/block.c
index f0a6042e61..1fe66caf8d 100644
--- a/block.c
+++ b/block.c
@@ -5524,9 +5524,12 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
     Transaction *tran = tran_new();
     g_autoptr(GSList) refresh_list = NULL;
     BlockDriverState *old_bs = child->bs;
+    int64_t old_len;
 
     GLOBAL_STATE_CODE();
 
+    old_len = bdrv_getlength(old_bs);
+
     bdrv_ref(old_bs);
     bdrv_drained_begin(old_bs);
     bdrv_drained_begin(new_bs);
@@ -5542,6 +5545,16 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
     tran_finalize(tran, ret);
 
     bdrv_graph_wrunlock();
+
+    /* A parent caching the child size has to learn that it changed */
+    if (ret == 0) {
+        int64_t new_len = bdrv_getlength(new_bs);
+
+        if (new_len >= 0 && new_len != old_len) {
+            bdrv_parent_cb_resize(new_bs);
+        }
+    }
+
     bdrv_drained_end(old_bs);
     bdrv_drained_end(new_bs);
     bdrv_unref(old_bs);
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index ed8b5657d6..95d0dd7a05 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -197,4 +197,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 2/4] scsi-disk: refresh the cached capacity from the resize callback

[Denis V. Lunev]

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>
---
 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 1b0cce128c..3d404a8651 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2386,13 +2386,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;
@@ -2400,6 +2397,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;
@@ -2425,6 +2431,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 3/4] tests/qtest/virtio-scsi: cover a replacement that changes the size

[Denis V. Lunev]

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>
---
 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 e2350c52f6..f82b7e5de9 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)
 {
@@ -365,6 +412,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,
@@ -399,6 +458,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 4/4] tests/qemu-iotests/qom-set-drive: replace with a differently sized node

[Denis V. Lunev]

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>
---
 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 ec8ddac4fd..52d7eb96da 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 7fc243dca6..6f6e5e2b31 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

Re: [PATCH 0/4] block, scsi-disk: honour a size change on child replacement

[Denis V. Lunev]

On 8/12/26 14:14, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain it was sent from.
> Do not click links or open attachments unless it is an email you expected to receive.
> qom-set of the 'drive' property is allowed on a realized device and ends
> up in bdrv_replace_child_bs(), which swaps the node a BdrvChild points to
> without telling the parent anything about it. Every other way the size of
> a node changes goes through bdrv_co_truncate(), which does notify, so a
> device model caching the size of its child has no way to learn that it
> changed.
>
> 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. Point a scsi-hd at a larger node and every request past the end
> of the old one is refused with ILLEGAL REQUEST / LOGICAL BLOCK ADDRESS
> OUT OF RANGE for the whole life of the device, which Linux turns into
> EREMOTEIO for reads as well as writes. 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'. virtio-blk and ide call blk_get_geometry() for
> every request, so they end up with a stale idea of the size but never
> refuse I/O the node underneath can serve.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Fam Zheng <fam@euphon.net>
>
> Denis V. Lunev (4):
>   block: notify the parent when a replaced child has a different size
>   scsi-disk: refresh the cached capacity from the resize callback
>   tests/qtest/virtio-scsi: cover a replacement that changes the size
>   tests/qemu-iotests/qom-set-drive: replace with a differently sized
>     node
>
>  block.c                                    | 13 +++++
>  hw/scsi/scsi-disk.c                        | 16 ++++--
>  include/block/block_int-io.h               |  3 ++
>  tests/qemu-iotests/tests/qom-set-drive     | 14 +++--
>  tests/qemu-iotests/tests/qom-set-drive.out |  2 +
>  tests/qtest/virtio-scsi-test.c             | 63 ++++++++++++++++++++++
>  6 files changed, 104 insertions(+), 7 deletions(-)
>
>
> base-commit: 3e3ccab106f879b1512f8e0d51a827dd4de30e22
ping