The following changes since commit 1b8c45899715d292398152ba97ef755ccaf84680: Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20200507a' into staging (2020-05-07 18:43:20 +0100) are available in the Git repository at: git://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 47e0b38a13935cb666f88964c3096654092f42d6: block: Drop unused .bdrv_has_zero_init_truncate (2020-05-08 13:26:35 +0200) ---------------------------------------------------------------- Block layer patches: - qcow2: Fix preallocation on block devices - backup: Make sure that source and target size match - vmdk: Fix zero cluster handling - Follow-up cleanups and fixes for the truncate changes - iotests: Skip more tests if required drivers are missing ---------------------------------------------------------------- Alberto Garcia (1): qcow2: Avoid integer wraparound in qcow2_co_truncate() Eric Blake (9): gluster: Drop useless has_zero_init callback file-win32: Support BDRV_REQ_ZERO_WRITE for truncate nfs: Support BDRV_REQ_ZERO_WRITE for truncate rbd: Support BDRV_REQ_ZERO_WRITE for truncate sheepdog: Support BDRV_REQ_ZERO_WRITE for truncate ssh: Support BDRV_REQ_ZERO_WRITE for truncate parallels: Rework truncation logic vhdx: Rework truncation logic block: Drop unused .bdrv_has_zero_init_truncate Kevin Wolf (11): vmdk: Rename VmdkMetaData.valid to new_allocation vmdk: Fix zero cluster allocation vmdk: Fix partial overwrite of zero cluster vmdk: Don't update L2 table for zero write on zero cluster vmdk: Flush only once in vmdk_L2update() iotests: vmdk: Enable zeroed_grained=on by default iotests/283: Use consistent size for source and target backup: Improve error for bdrv_getlength() failure backup: Make sure that source and target size match iotests: Backup with different source/target size iotests/055: Use cache.no-flush for vmdk target Max Reitz (1): qcow2: Fix preallocation on block devices Vladimir Sementsov-Ogievskiy (8): iotests: handle tmpfs iotests/082: require bochs iotests/148: use skip_if_unsupported iotests/041: drop self.assert_no_active_block_jobs() iotests/055: refactor compressed backup to vmdk iotests/055: skip vmdk target tests if vmdk is not whitelisted iotests/109: mark required formats as required to support whitelisting iotests/113: mark bochs as required to support whitelisting include/block/block.h | 1 - include/block/block_int.h | 7 --- block.c | 21 -------- block/backup-top.c | 14 +++-- block/backup.c | 18 +++++-- block/file-posix.c | 1 - block/file-win32.c | 4 +- block/gluster.c | 14 ----- block/nfs.c | 4 +- block/parallels.c | 25 +++++---- block/qcow2.c | 23 ++++++--- block/qed.c | 1 - block/raw-format.c | 6 --- block/rbd.c | 4 +- block/sheepdog.c | 4 +- block/ssh.c | 5 +- block/vhdx.c | 89 ++++++++++++++++++-------------- block/vmdk.c | 47 ++++++++++------- tests/qemu-iotests/041 | 8 --- tests/qemu-iotests/055 | 120 ++++++++++++++++++++++++++++++------------- tests/qemu-iotests/055.out | 4 +- tests/qemu-iotests/059 | 6 +-- tests/qemu-iotests/082 | 1 + tests/qemu-iotests/091 | 2 +- tests/qemu-iotests/109 | 1 + tests/qemu-iotests/113 | 4 +- tests/qemu-iotests/148 | 1 + tests/qemu-iotests/283 | 6 ++- tests/qemu-iotests/283.out | 2 +- tests/qemu-iotests/292 | 73 ++++++++++++++++++++++++++ tests/qemu-iotests/292.out | 24 +++++++++ tests/qemu-iotests/check | 3 ++ tests/qemu-iotests/common.rc | 37 ++++++++++++- tests/qemu-iotests/group | 1 + 34 files changed, 386 insertions(+), 195 deletions(-) create mode 100755 tests/qemu-iotests/292 create mode 100644 tests/qemu-iotests/292.out From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Some tests requires O_DIRECT, or want it by default. Introduce smarter O_DIRECT handling: - Check O_DIRECT in common.rc, if it is requested by selected cache-mode. - Support second fall-through argument in _default_cache_mode Inspired-by: Max's 23e1d054112cec1e Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200430124713.3067-2-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/091 | 2 +- tests/qemu-iotests/common.rc | 37 ++++++++++++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/091 +++ b/tests/qemu-iotests/091 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow2 _supported_proto file _supported_os Linux -_default_cache_mode none _supported_cache_modes writethrough none writeback +_default_cache_mode none writeback size=1G diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -XXX,XX +XXX,XX @@ _supported_cache_modes() _notrun "not suitable for cache mode: $CACHEMODE" } +# Check whether the filesystem supports O_DIRECT +_check_o_direct() +{ + $QEMU_IMG create -f raw "$TEST_IMG".test_o_direct 1M > /dev/null + out=$($QEMU_IO -f raw -t none -c quit "$TEST_IMG".test_o_direct 2>&1) + rm -f "$TEST_IMG".test_o_direct + + [[ "$out" != *"O_DIRECT"* ]] +} + +_require_o_direct() +{ + if ! _check_o_direct; then + _notrun "file system on $TEST_DIR does not support O_DIRECT" + fi +} + +_check_cache_mode() +{ + if [ $CACHEMODE == "none" ] || [ $CACHEMODE == "directsync" ]; then + _require_o_direct + fi +} + +_check_cache_mode + +# $1 - cache mode to use by default +# $2 - (optional) cache mode to use by default if O_DIRECT is not supported _default_cache_mode() { if $CACHEMODE_IS_DEFAULT; then - CACHEMODE="$1" - QEMU_IO="$QEMU_IO --cache $1" + if [ -z "$2" ] || _check_o_direct; then + CACHEMODE="$1" + else + CACHEMODE="$2" + fi + QEMU_IO="$QEMU_IO --cache $CACHEMODE" + _check_cache_mode return fi } -- 2.25.3 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Test fails if bochs not whitelisted, so, skip it in this case. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200430124713.3067-3-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/082 | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/082 +++ b/tests/qemu-iotests/082 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow2 _supported_proto file nfs +_require_drivers bochs run_qemu_img() { -- 2.25.3 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Skip test-case with quorum if quorum is not whitelisted. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200430124713.3067-4-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/148 | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemu-iotests/148 b/tests/qemu-iotests/148 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/148 +++ b/tests/qemu-iotests/148 @@ -XXX,XX +XXX,XX @@ sector = "%d" ''' % bad_sector) file.close() + @iotests.skip_if_unsupported(['quorum']) def setUp(self): driveopts = ['driver=quorum', 'vote-threshold=2'] driveopts.append('read-pattern=%s' % self.read_pattern) -- 2.25.3 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Drop check for no block-jobs: it's obvious that there no jobs immediately after vm.launch(). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200430124713.3067-5-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/041 | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -XXX,XX +XXX,XX @@ class TestRepairQuorum(iotests.QMPTestCase): pass def test_complete(self): - self.assert_no_active_block_jobs() - result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', sync='full', node_name="repair0", replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) @@ -XXX,XX +XXX,XX @@ class TestRepairQuorum(iotests.QMPTestCase): 'target image does not match source after mirroring') def test_cancel(self): - self.assert_no_active_block_jobs() - result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', sync='full', node_name="repair0", replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) @@ -XXX,XX +XXX,XX @@ class TestRepairQuorum(iotests.QMPTestCase): self.assert_has_block_node(None, quorum_img3) def test_cancel_after_ready(self): - self.assert_no_active_block_jobs() - result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', sync='full', node_name="repair0", replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) @@ -XXX,XX +XXX,XX @@ class TestRepairQuorum(iotests.QMPTestCase): 'target image does not match source after mirroring') def test_pause(self): - self.assert_no_active_block_jobs() - result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0', sync='full', node_name="repair0", replaces="img1", target=quorum_repair_img, format=iotests.imgfmt) -- 2.25.3 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Instead of looping in each test, let's better refactor vmdk target case as a subclass. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200430124713.3067-6-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/055 | 70 ++++++++++++++++++++------------------ tests/qemu-iotests/055.out | 4 +-- 2 files changed, 39 insertions(+), 35 deletions(-) diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -XXX,XX +XXX,XX @@ class TestSingleTransaction(iotests.QMPTestCase): self.assert_no_active_block_jobs() -class TestDriveCompression(iotests.QMPTestCase): +class TestCompressedToQcow2(iotests.QMPTestCase): image_len = 64 * 1024 * 1024 # MB - fmt_supports_compression = [{'type': 'qcow2', 'args': ()}, - {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')}] + target_fmt = {'type': 'qcow2', 'args': ()} def tearDown(self): self.vm.shutdown() @@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase): except OSError: pass - def do_prepare_drives(self, fmt, args, attach_target): + def do_prepare_drives(self, attach_target): self.vm = iotests.VM().add_drive('blkdebug::' + test_img) - qemu_img('create', '-f', fmt, blockdev_target_img, - str(TestDriveCompression.image_len), *args) + qemu_img('create', '-f', self.target_fmt['type'], blockdev_target_img, + str(self.image_len), *self.target_fmt['args']) if attach_target: self.vm.add_drive(blockdev_target_img, - img_format=fmt, interface="none") + img_format=self.target_fmt['type'], + interface="none") self.vm.launch() - def do_test_compress_complete(self, cmd, format, attach_target, **args): - self.do_prepare_drives(format['type'], format['args'], attach_target) + def do_test_compress_complete(self, cmd, attach_target, **args): + self.do_prepare_drives(attach_target) self.assert_no_active_block_jobs() @@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase): self.vm.shutdown() self.assertTrue(iotests.compare_images(test_img, blockdev_target_img, - iotests.imgfmt, format['type']), + iotests.imgfmt, + self.target_fmt['type']), 'target image does not match source after backup') def test_complete_compress_drive_backup(self): - for format in TestDriveCompression.fmt_supports_compression: - self.do_test_compress_complete('drive-backup', format, False, - target=blockdev_target_img, mode='existing') + self.do_test_compress_complete('drive-backup', False, + target=blockdev_target_img, + mode='existing') def test_complete_compress_blockdev_backup(self): - for format in TestDriveCompression.fmt_supports_compression: - self.do_test_compress_complete('blockdev-backup', format, True, - target='drive1') + self.do_test_compress_complete('blockdev-backup', + True, target='drive1') - def do_test_compress_cancel(self, cmd, format, attach_target, **args): - self.do_prepare_drives(format['type'], format['args'], attach_target) + def do_test_compress_cancel(self, cmd, attach_target, **args): + self.do_prepare_drives(attach_target) self.assert_no_active_block_jobs() @@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase): self.vm.shutdown() def test_compress_cancel_drive_backup(self): - for format in TestDriveCompression.fmt_supports_compression: - self.do_test_compress_cancel('drive-backup', format, False, - target=blockdev_target_img, mode='existing') + self.do_test_compress_cancel('drive-backup', False, + target=blockdev_target_img, + mode='existing') def test_compress_cancel_blockdev_backup(self): - for format in TestDriveCompression.fmt_supports_compression: - self.do_test_compress_cancel('blockdev-backup', format, True, - target='drive1') + self.do_test_compress_cancel('blockdev-backup', True, + target='drive1') - def do_test_compress_pause(self, cmd, format, attach_target, **args): - self.do_prepare_drives(format['type'], format['args'], attach_target) + def do_test_compress_pause(self, cmd, attach_target, **args): + self.do_prepare_drives(attach_target) self.assert_no_active_block_jobs() @@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase): self.vm.shutdown() self.assertTrue(iotests.compare_images(test_img, blockdev_target_img, - iotests.imgfmt, format['type']), + iotests.imgfmt, + self.target_fmt['type']), 'target image does not match source after backup') def test_compress_pause_drive_backup(self): - for format in TestDriveCompression.fmt_supports_compression: - self.do_test_compress_pause('drive-backup', format, False, - target=blockdev_target_img, mode='existing') + self.do_test_compress_pause('drive-backup', False, + target=blockdev_target_img, + mode='existing') def test_compress_pause_blockdev_backup(self): - for format in TestDriveCompression.fmt_supports_compression: - self.do_test_compress_pause('blockdev-backup', format, True, - target='drive1') + self.do_test_compress_pause('blockdev-backup', True, + target='drive1') + + +class TestCompressedToVmdk(TestCompressedToQcow2): + target_fmt = {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')} + if __name__ == '__main__': iotests.main(supported_fmts=['raw', 'qcow2'], diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/055.out +++ b/tests/qemu-iotests/055.out @@ -XXX,XX +XXX,XX @@ -.............................. +.................................... ---------------------------------------------------------------------- -Ran 30 tests +Ran 36 tests OK -- 2.25.3 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200430124713.3067-7-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/055 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -XXX,XX +XXX,XX @@ class TestCompressedToQcow2(iotests.QMPTestCase): class TestCompressedToVmdk(TestCompressedToQcow2): target_fmt = {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')} + @iotests.skip_if_unsupported(['vmdk']) + def setUp(self): + pass + if __name__ == '__main__': iotests.main(supported_fmts=['raw', 'qcow2'], -- 2.25.3 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200430124713.3067-8-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/109 | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/109 +++ b/tests/qemu-iotests/109 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt raw _supported_proto file _supported_os Linux +_require_drivers qcow qcow2 qed vdi vmdk vpc qemu_comm_method=qmp -- 2.25.3 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200430124713.3067-9-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/113 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/113 b/tests/qemu-iotests/113 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/113 +++ b/tests/qemu-iotests/113 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.rc . ./common.filter -# Some of these test cases use bochs, but others do use raw, so this -# is only half a lie. +# Some of these test cases use bochs, but others do use raw +_require_drivers bochs _supported_fmt raw _supported_proto file _supported_os Linux -- 2.25.3 From: Alberto Garcia <berto@igalia.com> After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are zeroized. The code however does not detect correctly situations when the old and the new end of the image are within the same cluster. The problem can be reproduced with these steps: qemu-img create -f qcow2 backing.qcow2 1M qemu-img create -f qcow2 -F qcow2 -b backing.qcow2 top.qcow2 qemu-img resize --shrink top.qcow2 520k qemu-img resize top.qcow2 567k In the last step offset - zero_start causes an integer wraparound. Signed-off-by: Alberto Garcia <berto@igalia.com> Message-Id: <20200504155217.10325-1-berto@igalia.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.c | 12 ++++--- tests/qemu-iotests/292 | 73 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/292.out | 24 +++++++++++++ tests/qemu-iotests/group | 1 + 4 files changed, 105 insertions(+), 5 deletions(-) create mode 100755 tests/qemu-iotests/292 create mode 100644 tests/qemu-iotests/292.out diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, * requires a cluster-aligned start. The end may be unaligned if it is * at the end of the image (which it is here). */ - ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0); - if (ret < 0) { - error_setg_errno(errp, -ret, "Failed to zero out new clusters"); - goto fail; + if (offset > zero_start) { + ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to zero out new clusters"); + goto fail; + } } /* Write explicit zeros for the unaligned head */ if (zero_start > old_length) { - uint64_t len = zero_start - old_length; + uint64_t len = MIN(zero_start, offset) - old_length; uint8_t *buf = qemu_blockalign0(bs, len); QEMUIOVector qiov; qemu_iovec_init_buf(&qiov, buf, len); diff --git a/tests/qemu-iotests/292 b/tests/qemu-iotests/292 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/292 @@ -XXX,XX +XXX,XX @@ +#!/usr/bin/env bash +# +# Test resizing a qcow2 image with a backing file +# +# Copyright (C) 2020 Igalia, S.L. +# Author: Alberto Garcia <berto@igalia.com> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +# creator +owner=berto@igalia.com + +seq=`basename $0` +echo "QA output created by $seq" + +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt qcow2 +_supported_proto file +_supported_os Linux + +echo '### Create the backing image' +BACKING_IMG="$TEST_IMG.base" +TEST_IMG="$BACKING_IMG" _make_test_img 1M + +echo '### Fill the backing image with data (0x11)' +$QEMU_IO -c 'write -P 0x11 0 1M' "$BACKING_IMG" | _filter_qemu_io + +echo '### Create the top image' +_make_test_img -F "$IMGFMT" -b "$BACKING_IMG" + +echo '### Fill the top image with data (0x22)' +$QEMU_IO -c 'write -P 0x22 0 1M' "$TEST_IMG" | _filter_qemu_io + +# Both offsets are part of the same cluster. +echo '### Shrink the image to 520k' +$QEMU_IMG resize --shrink "$TEST_IMG" 520k +echo '### Grow the image to 567k' +$QEMU_IMG resize "$TEST_IMG" 567k + +echo '### Check that the tail of the image reads as zeroes' +$QEMU_IO -c 'read -P 0x22 0 520k' "$TEST_IMG" | _filter_qemu_io +$QEMU_IO -c 'read -P 0x00 520k 47k' "$TEST_IMG" | _filter_qemu_io + +echo '### Show output of qemu-img map' +$QEMU_IMG map "$TEST_IMG" | _filter_testdir + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/292.out b/tests/qemu-iotests/292.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/292.out @@ -XXX,XX +XXX,XX @@ +QA output created by 292 +### Create the backing image +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576 +### Fill the backing image with data (0x11) +wrote 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +### Create the top image +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT +### Fill the top image with data (0x22) +wrote 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +### Shrink the image to 520k +Image resized. +### Grow the image to 567k +Image resized. +### Check that the tail of the image reads as zeroes +read 532480/532480 bytes at offset 0 +520 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 48128/48128 bytes at offset 532480 +47 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +### Show output of qemu-img map +Offset Length Mapped to File +0 0x8dc00 0x50000 TEST_DIR/t.qcow2 +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -XXX,XX +XXX,XX @@ 288 quick 289 rw quick 290 rw auto quick +292 rw auto quick -- 2.25.3 m_data is used for zero clusters even though valid == 0. It really only means that a new cluster was allocated in the image file. Rename it to reflect this. While at it, change it from int to bool, too. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430133007.170335-2-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vmdk.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ typedef struct VmdkMetaData { unsigned int l1_index; unsigned int l2_index; unsigned int l2_offset; - int valid; + bool new_allocation; uint32_t *l2_cache_entry; } VmdkMetaData; @@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs, unsigned int l2_size_bytes = extent->l2_size * extent->entry_size; if (m_data) { - m_data->valid = 0; + m_data->new_allocation = false; } if (extent->flat) { *cluster_offset = extent->flat_start_offset; @@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs, return ret; } if (m_data) { - m_data->valid = 1; + m_data->new_allocation = true; m_data->l1_index = l1_index; m_data->l2_index = l2_index; m_data->l2_offset = l2_offset; @@ -XXX,XX +XXX,XX @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset, if (ret) { return ret; } - if (m_data.valid) { + if (m_data.new_allocation) { /* update L2 tables */ if (vmdk_L2update(extent, &m_data, cluster_offset >> BDRV_SECTOR_BITS) -- 2.25.3 m_data must contain valid data even for zero clusters when no cluster was allocated in the image file. Without this, zero writes segfault with images that have zeroed_grain=on. For zero writes, we don't want to allocate a cluster in the image file even in compressed files. Fixes: 524089bce43fd1cd3daaca979872451efa2cf7c6 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430133007.170335-3-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vmdk.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs, extent->l2_cache_counts[min_index] = 1; found: l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size; + if (m_data) { + m_data->l1_index = l1_index; + m_data->l2_index = l2_index; + m_data->l2_offset = l2_offset; + m_data->l2_cache_entry = ((uint32_t *)l2_table) + l2_index; + } if (extent->sesparse) { cluster_sector = le64_to_cpu(((uint64_t *)l2_table)[l2_index]); @@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs, } if (m_data) { m_data->new_allocation = true; - m_data->l1_index = l1_index; - m_data->l2_index = l2_index; - m_data->l2_offset = l2_offset; - m_data->l2_cache_entry = ((uint32_t *)l2_table) + l2_index; } } *cluster_offset = cluster_sector << BDRV_SECTOR_BITS; @@ -XXX,XX +XXX,XX @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset, error_report("Could not write to allocated cluster" " for streamOptimized"); return -EIO; - } else { + } else if (!zeroed) { /* allocate */ ret = get_cluster_offset(bs, extent, &m_data, offset, true, &cluster_offset, 0, 0); -- 2.25.3 When overwriting a zero cluster, we must not perform copy-on-write from the backing file, but from a zeroed buffer. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430133007.170335-4-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vmdk.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp) * get_whole_cluster * * Copy backing file's cluster that covers @sector_num, otherwise write zero, - * to the cluster at @cluster_sector_num. + * to the cluster at @cluster_sector_num. If @zeroed is true, we're overwriting + * a zeroed cluster in the current layer and must not copy data from the + * backing file. * * If @skip_start_sector < @skip_end_sector, the relative range * [@skip_start_sector, @skip_end_sector) is not copied or written, and leave @@ -XXX,XX +XXX,XX @@ static int get_whole_cluster(BlockDriverState *bs, uint64_t cluster_offset, uint64_t offset, uint64_t skip_start_bytes, - uint64_t skip_end_bytes) + uint64_t skip_end_bytes, + bool zeroed) { int ret = VMDK_OK; int64_t cluster_bytes; uint8_t *whole_grain; + bool copy_from_backing; /* For COW, align request sector_num to cluster start */ cluster_bytes = extent->cluster_sectors << BDRV_SECTOR_BITS; offset = QEMU_ALIGN_DOWN(offset, cluster_bytes); whole_grain = qemu_blockalign(bs, cluster_bytes); + copy_from_backing = bs->backing && !zeroed; - if (!bs->backing) { + if (!copy_from_backing) { memset(whole_grain, 0, skip_start_bytes); memset(whole_grain + skip_end_bytes, 0, cluster_bytes - skip_end_bytes); } @@ -XXX,XX +XXX,XX @@ static int get_whole_cluster(BlockDriverState *bs, /* Read backing data before skip range */ if (skip_start_bytes > 0) { - if (bs->backing) { + if (copy_from_backing) { /* qcow2 emits this on bs->file instead of bs->backing */ BLKDBG_EVENT(extent->file, BLKDBG_COW_READ); ret = bdrv_pread(bs->backing, offset, whole_grain, @@ -XXX,XX +XXX,XX @@ static int get_whole_cluster(BlockDriverState *bs, } /* Read backing data after skip range */ if (skip_end_bytes < cluster_bytes) { - if (bs->backing) { + if (copy_from_backing) { /* qcow2 emits this on bs->file instead of bs->backing */ BLKDBG_EVENT(extent->file, BLKDBG_COW_READ); ret = bdrv_pread(bs->backing, offset + skip_end_bytes, @@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs, * or inappropriate VM shutdown. */ ret = get_whole_cluster(bs, extent, cluster_sector * BDRV_SECTOR_SIZE, - offset, skip_start_bytes, skip_end_bytes); + offset, skip_start_bytes, skip_end_bytes, + zeroed); if (ret) { return ret; } -- 2.25.3 If a cluster is already zeroed, we don't have to call vmdk_L2update(), which is rather slow because it flushes the image file. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430133007.170335-5-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vmdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset, offset_in_cluster == 0 && n_bytes >= extent->cluster_sectors * BDRV_SECTOR_SIZE) { n_bytes = extent->cluster_sectors * BDRV_SECTOR_SIZE; - if (!zero_dry_run) { + if (!zero_dry_run && ret != VMDK_ZEROED) { /* update L2 tables */ if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED) != VMDK_OK) { -- 2.25.3 If we have a backup L2 table, we currently flush once after writing to the active L2 table and again after writing to the backup table. A single flush is enough and makes things a little less slow. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430133007.170335-6-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vmdk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data, offset = cpu_to_le32(offset); /* update L2 table */ BLKDBG_EVENT(extent->file, BLKDBG_L2_UPDATE); - if (bdrv_pwrite_sync(extent->file, + if (bdrv_pwrite(extent->file, ((int64_t)m_data->l2_offset * 512) + (m_data->l2_index * sizeof(offset)), &offset, sizeof(offset)) < 0) { @@ -XXX,XX +XXX,XX @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data, /* update backup L2 table */ if (extent->l1_backup_table_offset != 0) { m_data->l2_offset = extent->l1_backup_table[m_data->l1_index]; - if (bdrv_pwrite_sync(extent->file, + if (bdrv_pwrite(extent->file, ((int64_t)m_data->l2_offset * 512) + (m_data->l2_index * sizeof(offset)), &offset, sizeof(offset)) < 0) { return VMDK_ERROR; } } + if (bdrv_flush(extent->file->bs) < 0) { + return VMDK_ERROR; + } if (m_data->l2_cache_entry) { *m_data->l2_cache_entry = offset; } -- 2.25.3 In order to avoid bitrot in the zero cluster code in VMDK, enable zeroed_grain=on by default for the tests. 059 now unsets the default options because zeroed_grain=on works only with some subformats and the test case tests many different subformats, including those for which it doesn't work. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430133007.170335-7-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/059 | 6 +++--- tests/qemu-iotests/check | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/059 +++ b/tests/qemu-iotests/059 @@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt vmdk _supported_proto file _supported_os Linux -_unsupported_imgopts "subformat=monolithicFlat" \ - "subformat=twoGbMaxExtentFlat" \ - "subformat=twoGbMaxExtentSparse" + +# We test all kinds of VMDK options here, so ignore user-specified options +IMGOPTS="" capacity_offset=16 granularity_offset=20 diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -XXX,XX +XXX,XX @@ fi if [ "$IMGFMT" == "luks" ] && ! (echo "$IMGOPTS" | grep "iter-time=" > /dev/null); then IMGOPTS=$(_optstr_add "$IMGOPTS" "iter-time=10") fi +if [ "$IMGFMT" == "vmdk" ] && ! (echo "$IMGOPTS" | grep "zeroed_grain=" > /dev/null); then + IMGOPTS=$(_optstr_add "$IMGOPTS" "zeroed_grain=on") +fi if [ -z "$SAMPLE_IMG_DIR" ]; then SAMPLE_IMG_DIR="$source_iotests/sample_images" -- 2.25.3 The test case forgot to specify the null-co size for the target node. When adding a check to backup that both sizes match, this would fail because of the size mismatch and not the behaviour that the test really wanted to test. Fixes: a541fcc27c98b96da187c7d4573f3270f3ddd283 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430142755.315494-2-kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/283 | 6 +++++- tests/qemu-iotests/283.out | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283 index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/283 +++ b/tests/qemu-iotests/283 @@ -XXX,XX +XXX,XX @@ to check that crash is fixed :) vm = iotests.VM() vm.launch() -vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'}) +vm.qmp_log('blockdev-add', **{ + 'node-name': 'target', + 'driver': 'null-co', + 'size': size, +}) vm.qmp_log('blockdev-add', **{ 'node-name': 'source', diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -XXX,XX +XXX,XX @@ -{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target"}} +{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target", "size": 1048576}} {"return": {}} {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}} {"return": {}} -- 2.25.3 bdrv_get_device_name() will be an empty string with modern management tools that don't use -drive. Use bdrv_get_device_or_node_name() instead so that the node name is used if the BlockBackend is anonymous. While at it, start with upper case to make the message consistent with the rest of the function. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Message-Id: <20200430142755.315494-3-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/backup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, len = bdrv_getlength(bs); if (len < 0) { - error_setg_errno(errp, -len, "unable to get length for '%s'", - bdrv_get_device_name(bs)); + error_setg_errno(errp, -len, "Unable to get length for '%s'", + bdrv_get_device_or_node_name(bs)); goto error; } -- 2.25.3 Since the introduction of a backup filter node in commit 00e30f05d, the backup block job crashes when the target image is smaller than the source image because it will try to write after the end of the target node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer would have caught this and errored out gracefully.) We can fix this and even do better than the old behaviour: Check that source and target have the same image size at the start of the block job and unshare BLK_PERM_RESIZE. (This permission was already unshared before the same commit 00e30f05d, but the BlockBackend that was used to make the restriction was removed without a replacement.) This will immediately error out when starting the job instead of only when writing to a block that doesn't exist in the target. Longer target than source would technically work because we would never write to blocks that don't exist, but semantically these are invalid, too, because a backup is supposed to create a copy, not just an image that starts with a copy. Fixes: 00e30f05de1d19586345ec373970ef4c192c6270 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593 Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430142755.315494-4-kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/backup-top.c | 14 +++++++++----- block/backup.c | 14 +++++++++++++- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/block/backup-top.c b/block/backup-top.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup-top.c +++ b/block/backup-top.c @@ -XXX,XX +XXX,XX @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c, * * Share write to target (child_file), to not interfere * with guest writes to its disk which may be in target backing chain. + * Can't resize during a backup block job because we check the size + * only upfront. */ - *nshared = BLK_PERM_ALL; + *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE; *nperm = BLK_PERM_WRITE; } else { /* Source child */ @@ -XXX,XX +XXX,XX @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c, if (perm & BLK_PERM_WRITE) { *nperm = *nperm | BLK_PERM_CONSISTENT_READ; } - *nshared &= ~BLK_PERM_WRITE; + *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE); } } @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, { Error *local_err = NULL; BDRVBackupTopState *state; - BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter, - filter_node_name, - BDRV_O_RDWR, errp); + BlockDriverState *top; bool appended = false; + assert(source->total_sectors == target->total_sectors); + + top = bdrv_new_open_driver(&bdrv_backup_top_filter, filter_node_name, + BDRV_O_RDWR, errp); if (!top) { return NULL; } diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque, JobTxn *txn, Error **errp) { - int64_t len; + int64_t len, target_len; BackupBlockJob *job = NULL; int64_t cluster_size; BdrvRequestFlags write_flags; @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, goto error; } + target_len = bdrv_getlength(target); + if (target_len < 0) { + error_setg_errno(errp, -target_len, "Unable to get length for '%s'", + bdrv_get_device_or_node_name(bs)); + goto error; + } + + if (target_len != len) { + error_setg(errp, "Source and target image have different sizes"); + goto error; + } + cluster_size = backup_calculate_cluster_size(target, errp); if (cluster_size < 0) { goto error; -- 2.25.3 This tests that the backup job catches situations where the target node has a different size than the source node. It must also forbid resize operations when the job is already running. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200430142755.315494-5-kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/055 | 41 ++++++++++++++++++++++++++++++++++++-- tests/qemu-iotests/055.out | 4 ++-- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -XXX,XX +XXX,XX @@ class TestSingleDrive(iotests.QMPTestCase): def setUp(self): qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len)) - self.vm = iotests.VM().add_drive('blkdebug::' + test_img) - self.vm.add_drive(blockdev_target_img, interface="none") + self.vm = iotests.VM() + self.vm.add_drive('blkdebug::' + test_img, 'node-name=source') + self.vm.add_drive(blockdev_target_img, 'node-name=target', + interface="none") if iotests.qemu_default_machine == 'pc': self.vm.add_drive(None, 'media=cdrom', 'ide') self.vm.launch() @@ -XXX,XX +XXX,XX @@ class TestSingleDrive(iotests.QMPTestCase): def test_pause_blockdev_backup(self): self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img) + def do_test_resize_blockdev_backup(self, device, node): + def pre_finalize(): + result = self.vm.qmp('block_resize', device=device, size=65536) + self.assert_qmp(result, 'error/class', 'GenericError') + + result = self.vm.qmp('block_resize', node_name=node, size=65536) + self.assert_qmp(result, 'error/class', 'GenericError') + + result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0', + target='drive1', sync='full', auto_finalize=False, + auto_dismiss=False) + self.assert_qmp(result, 'return', {}) + + self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize) + + def test_source_resize_blockdev_backup(self): + self.do_test_resize_blockdev_backup('drive0', 'source') + + def test_target_resize_blockdev_backup(self): + self.do_test_resize_blockdev_backup('drive1', 'target') + + def do_test_target_size(self, size): + result = self.vm.qmp('block_resize', device='drive1', size=size) + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0', + target='drive1', sync='full') + self.assert_qmp(result, 'error/class', 'GenericError') + + def test_small_target(self): + self.do_test_target_size(image_len // 2) + + def test_large_target(self): + self.do_test_target_size(image_len * 2) + def test_medium_not_found(self): if iotests.qemu_default_machine != 'pc': return diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/055.out +++ b/tests/qemu-iotests/055.out @@ -XXX,XX +XXX,XX @@ -.................................... +........................................ ---------------------------------------------------------------------- -Ran 36 tests +Ran 40 tests OK -- 2.25.3 055 uses the backup block job to create a compressed backup of an $IMGFMT image with both qcow2 and vmdk targets. However, cluster allocation in vmdk is very slow because it flushes the image file after each L2 update. There is no reason why we need this level of safety in this test, so let's disable flushes for vmdk. For the blockdev-backup tests this is achieved by simply adding the cache.no-flush=on to the drive_add() for the target. For drive-backup, the caching flags are copied from the source node, so we'll also add the flag to the source node, even though it is not vmdk. This can make the test run significantly faster (though it doesn't make a difference on tmpfs). In my usual setup it goes from ~45s to ~15s. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200505064618.16267-1-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/055 | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -XXX,XX +XXX,XX @@ class TestSingleTransaction(iotests.QMPTestCase): class TestCompressedToQcow2(iotests.QMPTestCase): image_len = 64 * 1024 * 1024 # MB - target_fmt = {'type': 'qcow2', 'args': ()} + target_fmt = {'type': 'qcow2', 'args': (), 'drive-opts': ''} def tearDown(self): self.vm.shutdown() @@ -XXX,XX +XXX,XX @@ class TestCompressedToQcow2(iotests.QMPTestCase): pass def do_prepare_drives(self, attach_target): - self.vm = iotests.VM().add_drive('blkdebug::' + test_img) + self.vm = iotests.VM().add_drive('blkdebug::' + test_img, + opts=self.target_fmt['drive-opts']) qemu_img('create', '-f', self.target_fmt['type'], blockdev_target_img, str(self.image_len), *self.target_fmt['args']) if attach_target: self.vm.add_drive(blockdev_target_img, img_format=self.target_fmt['type'], - interface="none") + interface="none", + opts=self.target_fmt['drive-opts']) self.vm.launch() @@ -XXX,XX +XXX,XX @@ class TestCompressedToQcow2(iotests.QMPTestCase): class TestCompressedToVmdk(TestCompressedToQcow2): - target_fmt = {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')} + target_fmt = {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized'), + 'drive-opts': 'cache.no-flush=on'} @iotests.skip_if_unsupported(['vmdk']) def setUp(self): -- 2.25.3 From: Max Reitz <mreitz@redhat.com> Calling bdrv_getlength() to get the pre-truncate file size will not really work on block devices, because they have always the same length, and trying to write beyond it will fail with a rather cryptic error message. Instead, we should use qcow2_get_last_cluster() and bdrv_getlength() only as a fallback. Before this patch: $ truncate -s 1G test.img $ sudo losetup -f --show test.img /dev/loop0 $ sudo qemu-img create -f qcow2 -o preallocation=full /dev/loop0 64M Formatting '/dev/loop0', fmt=qcow2 size=67108864 cluster_size=65536 preallocation=full lazy_refcounts=off refcount_bits=16 qemu-img: /dev/loop0: Could not resize image: Failed to resize refcount structures: No space left on device With this patch: $ sudo qemu-img create -f qcow2 -o preallocation=full /dev/loop0 64M Formatting '/dev/loop0', fmt=qcow2 size=67108864 cluster_size=65536 preallocation=full lazy_refcounts=off refcount_bits=16 qemu-img: /dev/loop0: Could not resize image: Failed to resize underlying file: Preallocation mode 'full' unsupported for this non-regular file So as you can see, it still fails, but now the problem is missing support on the block device level, so we at least get a better error message. Note that we cannot preallocate block devices on truncate by design, because we do not know what area to preallocate. Their length is always the same, the truncate operation does not change it. Signed-off-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200505141801.1096763-1-mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, { int64_t allocation_start, host_offset, guest_offset; int64_t clusters_allocated; - int64_t old_file_size, new_file_size; + int64_t old_file_size, last_cluster, new_file_size; uint64_t nb_new_data_clusters, nb_new_l2_tables; /* With a data file, preallocation means just allocating the metadata @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, ret = old_file_size; goto fail; } - old_file_size = ROUND_UP(old_file_size, s->cluster_size); + + last_cluster = qcow2_get_last_cluster(bs, old_file_size); + if (last_cluster >= 0) { + old_file_size = (last_cluster + 1) * s->cluster_size; + } else { + old_file_size = ROUND_UP(old_file_size, s->cluster_size); + } nb_new_data_clusters = DIV_ROUND_UP(offset - old_length, s->cluster_size); -- 2.25.3 From: Eric Blake <eblake@redhat.com> block.c already defaults to 0 if we don't provide a callback; there's no need to write a callback that always fails. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Message-Id: <20200428202905.770727-2-eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/gluster.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -XXX,XX +XXX,XX @@ static int64_t qemu_gluster_allocated_file_size(BlockDriverState *bs) } } -static int qemu_gluster_has_zero_init(BlockDriverState *bs) -{ - /* GlusterFS volume could be backed by a block device */ - return 0; -} - /* * Find allocation range in @bs around offset @start. * May change underlying file descriptor's file offset. @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_gluster = { .bdrv_co_readv = qemu_gluster_co_readv, .bdrv_co_writev = qemu_gluster_co_writev, .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, #ifdef CONFIG_GLUSTERFS_DISCARD .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, #endif @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_gluster_tcp = { .bdrv_co_readv = qemu_gluster_co_readv, .bdrv_co_writev = qemu_gluster_co_writev, .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, #ifdef CONFIG_GLUSTERFS_DISCARD .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, #endif @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_gluster_unix = { .bdrv_co_readv = qemu_gluster_co_readv, .bdrv_co_writev = qemu_gluster_co_writev, .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, #ifdef CONFIG_GLUSTERFS_DISCARD .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, #endif @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_gluster_rdma = { .bdrv_co_readv = qemu_gluster_co_readv, .bdrv_co_writev = qemu_gluster_co_writev, .bdrv_co_flush_to_disk = qemu_gluster_co_flush_to_disk, - .bdrv_has_zero_init = qemu_gluster_has_zero_init, - .bdrv_has_zero_init_truncate = qemu_gluster_has_zero_init, #ifdef CONFIG_GLUSTERFS_DISCARD .bdrv_co_pdiscard = qemu_gluster_co_pdiscard, #endif -- 2.25.3 From: Eric Blake <eblake@redhat.com> When using bdrv_file, .bdrv_has_zero_init_truncate always returns 1; therefore, we can behave just like file-posix, and always implement BDRV_REQ_ZERO_WRITE by ignoring it since the OS gives it to us for free (note that file-posix.c had to use an 'if' because it shared code between regular files and block devices, but in file-win32.c, bdrv_host_device uses a separate .bdrv_file_open). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428202905.770727-3-eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/file-win32.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/file-win32.c b/block/file-win32.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-win32.c +++ b/block/file-win32.c @@ -XXX,XX +XXX,XX @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, win32_aio_attach_aio_context(s->aio, bdrv_get_aio_context(bs)); } + /* When extending regular files, we get zeros from the OS */ + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; + ret = 0; fail: qemu_opts_del(opts); -- 2.25.3 From: Eric Blake <eblake@redhat.com> Our .bdrv_has_zero_init_truncate returns 1 if we detect that the OS always 0-fills; we can use that same knowledge to implement BDRV_REQ_ZERO_WRITE by ignoring it when the OS gives it to us for free. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428202905.770727-4-eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/nfs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/nfs.c b/block/nfs.c index XXXXXXX..XXXXXXX 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -XXX,XX +XXX,XX @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags, } bs->total_sectors = ret; + if (client->has_zero_init) { + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; + } return 0; } -- 2.25.3 From: Eric Blake <eblake@redhat.com> Our .bdrv_has_zero_init_truncate always returns 1 because rbd always 0-fills; we can use that same knowledge to implement BDRV_REQ_ZERO_WRITE by ignoring it. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428202905.770727-5-eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/rbd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/rbd.c b/block/rbd.c index XXXXXXX..XXXXXXX 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -XXX,XX +XXX,XX @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, } } + /* When extending regular files, we get zeros from the OS */ + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; + r = 0; goto out; -- 2.25.3 From: Eric Blake <eblake@redhat.com> Our .bdrv_has_zero_init_truncate always returns 1 because sheepdog always 0-fills; we can use that same knowledge to implement BDRV_REQ_ZERO_WRITE by ignoring it. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428202905.770727-6-eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/sheepdog.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/sheepdog.c b/block/sheepdog.c index XXXXXXX..XXXXXXX 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -XXX,XX +XXX,XX @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, memcpy(&s->inode, buf, sizeof(s->inode)); bs->total_sectors = s->inode.vdi_size / BDRV_SECTOR_SIZE; + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; pstrcpy(s->name, sizeof(s->name), vdi); qemu_co_mutex_init(&s->lock); qemu_co_mutex_init(&s->queue_lock); -- 2.25.3 From: Eric Blake <eblake@redhat.com> Our .bdrv_has_zero_init_truncate can detect when the remote side always zero fills; we can reuse that same knowledge to implement BDRV_REQ_ZERO_WRITE by ignoring it when the server gives it to us for free. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428202905.770727-7-eblake@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/ssh.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/block/ssh.c b/block/ssh.c index XXXXXXX..XXXXXXX 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -XXX,XX +XXX,XX @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, /* Go non-blocking. */ ssh_set_blocking(s->session, 0); + if (s->attrs->type == SSH_FILEXFER_TYPE_REGULAR) { + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; + } + qapi_free_BlockdevOptionsSsh(opts); return 0; -- 2.25.3 From: Eric Blake <eblake@redhat.com> The parallels driver tries to use truncation for image growth, but can only do so when reads are guaranteed as zero. Now that we have a way to request zero contents from truncation, we can defer the decision to actual allocation attempts rather than up front, reducing the number of places that still use bdrv_has_zero_init_truncate. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428202905.770727-8-eblake@redhat.com> Reviewed-by: Denis V. Lunev <den@openvz.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/parallels.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index XXXXXXX..XXXXXXX 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -XXX,XX +XXX,XX @@ static int64_t block_status(BDRVParallelsState *s, int64_t sector_num, static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) { - int ret; + int ret = 0; BDRVParallelsState *s = bs->opaque; int64_t pos, space, idx, to_allocate, i, len; @@ -XXX,XX +XXX,XX @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, } if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) { space += s->prealloc_size; + /* + * We require the expanded size to read back as zero. If the + * user permitted truncation, we try that; but if it fails, we + * force the safer-but-slower fallocate. + */ + if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) { + ret = bdrv_truncate(bs->file, + (s->data_end + space) << BDRV_SECTOR_BITS, + false, PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE, + NULL); + if (ret == -ENOTSUP) { + s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; + } + } if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) { ret = bdrv_pwrite_zeroes(bs->file, s->data_end << BDRV_SECTOR_BITS, space << BDRV_SECTOR_BITS, 0); - } else { - ret = bdrv_truncate(bs->file, - (s->data_end + space) << BDRV_SECTOR_BITS, - false, PREALLOC_MODE_OFF, 0, NULL); } if (ret < 0) { return ret; @@ -XXX,XX +XXX,XX @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0); s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS); buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE); + /* prealloc_mode can be downgraded later during allocate_clusters */ s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf, PRL_PREALLOC_MODE_FALLOCATE, &local_err); @@ -XXX,XX +XXX,XX @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, goto fail_options; } - if (!bdrv_has_zero_init_truncate(bs->file->bs)) { - s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; - } - if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) { s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC); ret = parallels_update_header(bs); -- 2.25.3 From: Eric Blake <eblake@redhat.com> The vhdx driver uses truncation for image growth, with a special case for blocks that already read as zero but which are only being partially written. But with a bit of rearranging, it's just as easy to defer the decision on whether truncation resulted in zeroes to the actual allocation attempt, reducing the number of places that still use bdrv_has_zero_init_truncate. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428202905.770727-9-eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vhdx.c | 89 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 51 insertions(+), 38 deletions(-) diff --git a/block/vhdx.c b/block/vhdx.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -XXX,XX +XXX,XX @@ exit: /* * Allocate a new payload block at the end of the file. * - * Allocation will happen at 1MB alignment inside the file + * Allocation will happen at 1MB alignment inside the file. + * + * If @need_zero is set on entry but not cleared on return, then truncation + * could not guarantee that the new portion reads as zero, and the caller + * will take care of it instead. * * Returns the file offset start of the new payload block */ static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s, - uint64_t *new_offset) + uint64_t *new_offset, bool *need_zero) { int64_t current_len; @@ -XXX,XX +XXX,XX @@ static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s, return -EINVAL; } + if (*need_zero) { + int ret; + + ret = bdrv_truncate(bs->file, *new_offset + s->block_size, false, + PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE, NULL); + if (ret != -ENOTSUP) { + *need_zero = false; + return ret; + } + } + return bdrv_truncate(bs->file, *new_offset + s->block_size, false, PREALLOC_MODE_OFF, 0, NULL); } @@ -XXX,XX +XXX,XX @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num, /* in this case, we need to preserve zero writes for * data that is not part of this write, so we must pad * the rest of the buffer to zeroes */ - - /* if we are on a posix system with ftruncate() that extends - * a file, then it is zero-filled for us. On Win32, the raw - * layer uses SetFilePointer and SetFileEnd, which does not - * zero fill AFAIK */ - - /* Queue another write of zero buffers if the underlying file - * does not zero-fill on file extension */ - - if (bdrv_has_zero_init_truncate(bs->file->bs) == 0) { - use_zero_buffers = true; - + use_zero_buffers = true; + /* fall through */ + case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */ + case PAYLOAD_BLOCK_UNMAPPED: + case PAYLOAD_BLOCK_UNMAPPED_v095: + case PAYLOAD_BLOCK_UNDEFINED: + bat_prior_offset = sinfo.file_offset; + ret = vhdx_allocate_block(bs, s, &sinfo.file_offset, + &use_zero_buffers); + if (ret < 0) { + goto exit; + } + /* + * once we support differencing files, this may also be + * partially present + */ + /* update block state to the newly specified state */ + vhdx_update_bat_table_entry(bs, s, &sinfo, &bat_entry, + &bat_entry_offset, + PAYLOAD_BLOCK_FULLY_PRESENT); + bat_update = true; + /* + * Since we just allocated a block, file_offset is the + * beginning of the payload block. It needs to be the + * write address, which includes the offset into the + * block, unless the entire block needs to read as + * zeroes but truncation was not able to provide them, + * in which case we need to fill in the rest. + */ + if (!use_zero_buffers) { + sinfo.file_offset += sinfo.block_offset; + } else { /* zero fill the front, if any */ if (sinfo.block_offset) { iov1.iov_len = sinfo.block_offset; @@ -XXX,XX +XXX,XX @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num, } /* our actual data */ - qemu_iovec_concat(&hd_qiov, qiov, bytes_done, + qemu_iovec_concat(&hd_qiov, qiov, bytes_done, sinfo.bytes_avail); /* zero fill the back, if any */ @@ -XXX,XX +XXX,XX @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num, sectors_to_write += iov2.iov_len >> BDRV_SECTOR_BITS; } } - /* fall through */ - case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */ - case PAYLOAD_BLOCK_UNMAPPED: - case PAYLOAD_BLOCK_UNMAPPED_v095: - case PAYLOAD_BLOCK_UNDEFINED: - bat_prior_offset = sinfo.file_offset; - ret = vhdx_allocate_block(bs, s, &sinfo.file_offset); - if (ret < 0) { - goto exit; - } - /* once we support differencing files, this may also be - * partially present */ - /* update block state to the newly specified state */ - vhdx_update_bat_table_entry(bs, s, &sinfo, &bat_entry, - &bat_entry_offset, - PAYLOAD_BLOCK_FULLY_PRESENT); - bat_update = true; - /* since we just allocated a block, file_offset is the - * beginning of the payload block. It needs to be the - * write address, which includes the offset into the block */ - if (!use_zero_buffers) { - sinfo.file_offset += sinfo.block_offset; - } + /* fall through */ case PAYLOAD_BLOCK_FULLY_PRESENT: /* if the file offset address is in the header zone, -- 2.25.3 From: Eric Blake <eblake@redhat.com> Now that there are no clients of bdrv_has_zero_init_truncate, none of the drivers need to worry about providing it. What's more, this eliminates a source of some confusion: a literal reading of the documentation as written in ceaca56f and implemented in commit 1dcaf527 claims that a driver which returns 0 for bdrv_has_zero_init_truncate() must not return 1 for bdrv_has_zero_init(); this condition was violated for parallels, qcow, and sometimes for vdi, although in practice it did not matter since those drivers also lacked .bdrv_co_truncate. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428202905.770727-10-eblake@redhat.com> Acked-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/block.h | 1 - include/block/block_int.h | 7 ------- block.c | 21 --------------------- block/file-posix.c | 1 - block/file-win32.c | 1 - block/nfs.c | 1 - block/qcow2.c | 1 - block/qed.c | 1 - block/raw-format.c | 6 ------ block/rbd.c | 1 - block/sheepdog.c | 3 --- block/ssh.c | 1 - 12 files changed, 45 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -XXX,XX +XXX,XX @@ int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes); int bdrv_has_zero_init_1(BlockDriverState *bs); int bdrv_has_zero_init(BlockDriverState *bs); -int bdrv_has_zero_init_truncate(BlockDriverState *bs); bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs); bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs); int bdrv_block_status(BlockDriverState *bs, int64_t offset, diff --git a/include/block/block_int.h b/include/block/block_int.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -XXX,XX +XXX,XX @@ struct BlockDriver { /* * Returns 1 if newly created images are guaranteed to contain only * zeros, 0 otherwise. - * Must return 0 if .bdrv_has_zero_init_truncate() returns 0. */ int (*bdrv_has_zero_init)(BlockDriverState *bs); - /* - * Returns 1 if new areas added by growing the image with - * PREALLOC_MODE_OFF contain only zeros, 0 otherwise. - */ - int (*bdrv_has_zero_init_truncate)(BlockDriverState *bs); - /* Remove fd handlers, timers, and other event loop callbacks so the event * loop is no longer in use. Called with no in-flight requests and in * depth-first traversal order with parents before child nodes. diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ int bdrv_has_zero_init(BlockDriverState *bs) return 0; } -int bdrv_has_zero_init_truncate(BlockDriverState *bs) -{ - if (!bs->drv) { - return 0; - } - - if (bs->backing) { - /* Depends on the backing image length, but better safe than sorry */ - return 0; - } - if (bs->drv->bdrv_has_zero_init_truncate) { - return bs->drv->bdrv_has_zero_init_truncate(bs); - } - if (bs->file && bs->drv->is_filter) { - return bdrv_has_zero_init_truncate(bs->file->bs); - } - - /* safe default */ - return 0; -} - bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs) { BlockDriverInfo bdi; diff --git a/block/file-posix.c b/block/file-posix.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_file = { .bdrv_co_create = raw_co_create, .bdrv_co_create_opts = raw_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_co_block_status = raw_co_block_status, .bdrv_co_invalidate_cache = raw_co_invalidate_cache, .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes, diff --git a/block/file-win32.c b/block/file-win32.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-win32.c +++ b/block/file-win32.c @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_file = { .bdrv_close = raw_close, .bdrv_co_create_opts = raw_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_aio_preadv = raw_aio_preadv, .bdrv_aio_pwritev = raw_aio_pwritev, diff --git a/block/nfs.c b/block/nfs.c index XXXXXXX..XXXXXXX 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_nfs = { .create_opts = &nfs_create_opts, .bdrv_has_zero_init = nfs_has_zero_init, - .bdrv_has_zero_init_truncate = nfs_has_zero_init, .bdrv_get_allocated_file_size = nfs_get_allocated_file_size, .bdrv_co_truncate = nfs_file_co_truncate, diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_qcow2 = { .bdrv_co_create_opts = qcow2_co_create_opts, .bdrv_co_create = qcow2_co_create, .bdrv_has_zero_init = qcow2_has_zero_init, - .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_co_block_status = qcow2_co_block_status, .bdrv_co_preadv_part = qcow2_co_preadv_part, diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_qed = { .bdrv_co_create = bdrv_qed_co_create, .bdrv_co_create_opts = bdrv_qed_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_co_block_status = bdrv_qed_co_block_status, .bdrv_co_readv = bdrv_qed_co_readv, .bdrv_co_writev = bdrv_qed_co_writev, diff --git a/block/raw-format.c b/block/raw-format.c index XXXXXXX..XXXXXXX 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -XXX,XX +XXX,XX @@ static int raw_has_zero_init(BlockDriverState *bs) return bdrv_has_zero_init(bs->file->bs); } -static int raw_has_zero_init_truncate(BlockDriverState *bs) -{ - return bdrv_has_zero_init_truncate(bs->file->bs); -} - static int coroutine_fn raw_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts, @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_raw = { .bdrv_co_ioctl = &raw_co_ioctl, .create_opts = &raw_create_opts, .bdrv_has_zero_init = &raw_has_zero_init, - .bdrv_has_zero_init_truncate = &raw_has_zero_init_truncate, .strong_runtime_opts = raw_strong_runtime_opts, .mutable_opts = mutable_opts, }; diff --git a/block/rbd.c b/block/rbd.c index XXXXXXX..XXXXXXX 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_rbd = { .bdrv_co_create = qemu_rbd_co_create, .bdrv_co_create_opts = qemu_rbd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_get_info = qemu_rbd_getinfo, .create_opts = &qemu_rbd_create_opts, .bdrv_getlength = qemu_rbd_getlength, diff --git a/block/sheepdog.c b/block/sheepdog.c index XXXXXXX..XXXXXXX 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_sheepdog = { .bdrv_co_create = sd_co_create, .bdrv_co_create_opts = sd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size, .bdrv_co_truncate = sd_co_truncate, @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_sheepdog_tcp = { .bdrv_co_create = sd_co_create, .bdrv_co_create_opts = sd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size, .bdrv_co_truncate = sd_co_truncate, @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_sheepdog_unix = { .bdrv_co_create = sd_co_create, .bdrv_co_create_opts = sd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, .bdrv_get_allocated_file_size = sd_get_allocated_file_size, .bdrv_co_truncate = sd_co_truncate, diff --git a/block/ssh.c b/block/ssh.c index XXXXXXX..XXXXXXX 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_ssh = { .bdrv_co_create_opts = ssh_co_create_opts, .bdrv_close = ssh_close, .bdrv_has_zero_init = ssh_has_zero_init, - .bdrv_has_zero_init_truncate = ssh_has_zero_init, .bdrv_co_readv = ssh_co_readv, .bdrv_co_writev = ssh_co_writev, .bdrv_getlength = ssh_getlength, -- 2.25.3 The following changes since commit 825b96dbcee23d134b691fc75618b59c5f53da32: Merge tag 'migration-20250310-pull-request' of https://gitlab.com/farosas/qemu into staging (2025-03-11 09:32:07 +0800) are available in the Git repository at: https://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to a93c04f3cbe690877b3297a9df4767aa811fcd97: virtio-scsi: only expose cmd vqs via iothread-vq-mapping (2025-03-11 15:49:22 +0100) ---------------------------------------------------------------- Block layer patches - virtio-scsi: add iothread-vq-mapping parameter - Improve writethrough performance - Fix missing zero init in bdrv_snapshot_goto() - Code cleanup and iotests fixes ---------------------------------------------------------------- Kevin Wolf (8): block: Remove unused blk_op_is_blocked() block: Zero block driver state before reopening file-posix: Support FUA writes block/io: Ignore FUA with cache.no-flush=on aio: Create AioPolledEvent aio-posix: Factor out adjust_polling_time() aio-posix: Separate AioPolledEvent per AioHandler aio-posix: Adjust polling time also for new handlers Stefan Hajnoczi (13): scsi-disk: drop unused SCSIDiskState->bh field dma: use current AioContext for dma_blk_io() scsi: track per-SCSIRequest AioContext scsi: introduce requests_lock virtio-scsi: introduce event and ctrl virtqueue locks virtio-scsi: protect events_dropped field virtio-scsi: perform TMFs in appropriate AioContexts virtio-blk: extract cleanup_iothread_vq_mapping() function virtio-blk: tidy up iothread_vq_mapping functions virtio: extract iothread-vq-mapping.h API virtio-scsi: add iothread-vq-mapping parameter virtio-scsi: handle ctrl virtqueue in main loop virtio-scsi: only expose cmd vqs via iothread-vq-mapping Thomas Huth (1): iotests: Limit qsd-migrate to working formats include/block/aio.h | 5 +- include/block/raw-aio.h | 8 +- include/hw/scsi/scsi.h | 8 +- include/hw/virtio/iothread-vq-mapping.h | 45 +++ include/hw/virtio/virtio-scsi.h | 15 +- include/system/block-backend-global-state.h | 1 - include/system/dma.h | 3 +- util/aio-posix.h | 1 + block/block-backend.c | 12 - block/file-posix.c | 26 +- block/io.c | 4 + block/io_uring.c | 13 +- block/linux-aio.c | 24 +- block/snapshot.c | 1 + hw/block/virtio-blk.c | 132 +------- hw/ide/core.c | 3 +- hw/ide/macio.c | 3 +- hw/scsi/scsi-bus.c | 121 +++++-- hw/scsi/scsi-disk.c | 24 +- hw/scsi/virtio-scsi-dataplane.c | 103 ++++-- hw/scsi/virtio-scsi.c | 502 ++++++++++++++++------------ hw/virtio/iothread-vq-mapping.c | 131 ++++++++ system/dma-helpers.c | 8 +- util/aio-posix.c | 114 ++++--- util/async.c | 1 - hw/virtio/meson.build | 1 + meson.build | 4 + tests/qemu-iotests/tests/qsd-migrate | 2 +- 28 files changed, 803 insertions(+), 512 deletions(-) create mode 100644 include/hw/virtio/iothread-vq-mapping.h create mode 100644 hw/virtio/iothread-vq-mapping.c Commit fc4e394b28 removed the last caller of blk_op_is_blocked(). Remove the now unused function. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250206165331.379033-1-kwolf@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/system/block-backend-global-state.h | 1 - block/block-backend.c | 12 ------------ 2 files changed, 13 deletions(-) diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h index XXXXXXX..XXXXXXX 100644 --- a/include/system/block-backend-global-state.h +++ b/include/system/block-backend-global-state.h @@ -XXX,XX +XXX,XX @@ bool blk_supports_write_perm(BlockBackend *blk); bool blk_is_sg(BlockBackend *blk); void blk_set_enable_write_cache(BlockBackend *blk, bool wce); int blk_get_flags(BlockBackend *blk); -bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp); int blk_set_aio_context(BlockBackend *blk, AioContext *new_context, Error **errp); void blk_add_aio_context_notifier(BlockBackend *blk, diff --git a/block/block-backend.c b/block/block-backend.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -XXX,XX +XXX,XX @@ void *blk_blockalign(BlockBackend *blk, size_t size) return qemu_blockalign(blk ? blk_bs(blk) : NULL, size); } -bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp) -{ - BlockDriverState *bs = blk_bs(blk); - GLOBAL_STATE_CODE(); - GRAPH_RDLOCK_GUARD_MAINLOOP(); - - if (!bs) { - return false; - } - - return bdrv_op_is_blocked(bs, op, errp); -} /** * Return BB's current AioContext. Note that this context may change -- 2.48.1 Block drivers assume in their .bdrv_open() implementation that their state in bs->opaque has been zeroed; it is initially allocated with g_malloc0() in bdrv_open_driver(). bdrv_snapshot_goto() needs to make sure that it is zeroed again before calling drv->bdrv_open() to avoid that block drivers use stale values. One symptom of this bug is VMDK running into a double free when the user tries to apply an internal snapshot like 'qemu-img snapshot -a test test.vmdk'. This should be a graceful error because VMDK doesn't support internal snapshots. ==25507== Invalid free() / delete / delete[] / realloc() ==25507== at 0x484B347: realloc (vg_replace_malloc.c:1801) ==25507== by 0x54B592A: g_realloc (gmem.c:171) ==25507== by 0x1B221D: vmdk_add_extent (../block/vmdk.c:570) ==25507== by 0x1B1084: vmdk_open_sparse (../block/vmdk.c:1059) ==25507== by 0x1AF3D8: vmdk_open (../block/vmdk.c:1371) ==25507== by 0x1A2AE0: bdrv_snapshot_goto (../block/snapshot.c:299) ==25507== by 0x205C77: img_snapshot (../qemu-img.c:3500) ==25507== by 0x58FA087: (below main) (libc_start_call_main.h:58) ==25507== Address 0x832f3e0 is 0 bytes inside a block of size 272 free'd ==25507== at 0x4846B83: free (vg_replace_malloc.c:989) ==25507== by 0x54AEAC4: g_free (gmem.c:208) ==25507== by 0x1AF629: vmdk_close (../block/vmdk.c:2889) ==25507== by 0x1A2A9C: bdrv_snapshot_goto (../block/snapshot.c:290) ==25507== by 0x205C77: img_snapshot (../qemu-img.c:3500) ==25507== by 0x58FA087: (below main) (libc_start_call_main.h:58) This error was discovered by fuzzing qemu-img. Cc: qemu-stable@nongnu.org Closes: https://gitlab.com/qemu-project/qemu/-/issues/2853 Closes: https://gitlab.com/qemu-project/qemu/-/issues/2851 Reported-by: Denis Rastyogin <gerben@altlinux.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250310104858.28221-1-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/snapshot.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/snapshot.c b/block/snapshot.c index XXXXXXX..XXXXXXX 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -XXX,XX +XXX,XX @@ int bdrv_snapshot_goto(BlockDriverState *bs, bdrv_graph_wrunlock(); ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp); + memset(bs->opaque, 0, drv->instance_size); open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err); qobject_unref(options); if (open_ret < 0) { -- 2.48.1 Until now, FUA was always emulated with a separate flush after the write for file-posix. The overhead of processing a second request can reduce performance significantly for a guest disk that has disabled the write cache, especially if the host disk is already write through, too, and the flush isn't actually doing anything. Advertise support for REQ_FUA in write requests and implement it for Linux AIO and io_uring using the RWF_DSYNC flag for write requests. The thread pool still performs a separate fdatasync() call. This can be improved later by using the pwritev2() syscall if available. As an example, this is how fio numbers can be improved in some scenarios with this patch (all using virtio-blk with cache=directsync on an nvme block device for the VM, fio with ioengine=libaio,direct=1,sync=1): | old | with FUA support ------------------------------+---------------+------------------- bs=4k, iodepth=1, numjobs=1 | 45.6k iops | 56.1k iops bs=4k, iodepth=1, numjobs=16 | 183.3k iops | 236.0k iops bs=4k, iodepth=16, numjobs=1 | 258.4k iops | 311.1k iops However, not all scenarios are clear wins. On another slower disk I saw little to no improvment. In fact, in two corner case scenarios, I even observed a regression, which I however consider acceptable: 1. On slow host disks in a write through cache mode, when the guest is using virtio-blk in a separate iothread so that polling can be enabled, and each completion is quickly followed up with a new request (so that polling gets it), it can happen that enabling FUA makes things slower - the additional very fast no-op flush we used to have gave the adaptive polling algorithm a success so that it kept polling. Without it, we only have the slow write request, which disables polling. This is a problem in the polling algorithm that will be fixed later in this series. 2. With a high queue depth, it can be beneficial to have flush requests for another reason: The optimisation in bdrv_co_flush() that flushes only once per write generation acts as a synchronisation mechanism that lets all requests complete at the same time. This can result in better batching and if the disk is very fast (I only saw this with a null_blk backend), this can make up for the overhead of the flush and improve throughput. In theory, we could optionally introduce a similar artificial latency in the normal completion path to achieve the same kind of completion batching. This is not implemented in this series. Compatibility is not a concern for io_uring, it has supported RWF_DSYNC from the start. Linux AIO started supporting it in Linux 4.13 and libaio 0.3.111. The kernel is not a problem for any supported build platform, so it's not necessary to add runtime checks. However, openSUSE is still stuck with an older libaio version that would break the build. We must detect this at build time to avoid build failures. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250307221634.71951-2-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/raw-aio.h | 8 ++++++-- block/file-posix.c | 26 ++++++++++++++++++-------- block/io_uring.c | 13 ++++++++----- block/linux-aio.c | 24 +++++++++++++++++++++--- meson.build | 4 ++++ 5 files changed, 57 insertions(+), 18 deletions(-) diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/raw-aio.h +++ b/include/block/raw-aio.h @@ -XXX,XX +XXX,XX @@ #define QEMU_RAW_AIO_H #include "block/aio.h" +#include "block/block-common.h" #include "qemu/iov.h" /* AIO request types */ @@ -XXX,XX +XXX,XX @@ void laio_cleanup(LinuxAioState *s); /* laio_co_submit: submit I/O requests in the thread's current AioContext. */ int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov, - int type, uint64_t dev_max_batch); + int type, BdrvRequestFlags flags, + uint64_t dev_max_batch); bool laio_has_fdsync(int); +bool laio_has_fua(void); void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context); void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context); #endif @@ -XXX,XX +XXX,XX @@ void luring_cleanup(LuringState *s); /* luring_co_submit: submit I/O requests in the thread's current AioContext. */ int coroutine_fn luring_co_submit(BlockDriverState *bs, int fd, uint64_t offset, - QEMUIOVector *qiov, int type); + QEMUIOVector *qiov, int type, + BdrvRequestFlags flags); void luring_detach_aio_context(LuringState *s, AioContext *old_context); void luring_attach_aio_context(LuringState *s, AioContext *new_context); #endif diff --git a/block/file-posix.c b/block/file-posix.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -XXX,XX +XXX,XX @@ static int fd_open(BlockDriverState *bs) } static int64_t raw_getlength(BlockDriverState *bs); +static int coroutine_fn raw_co_flush_to_disk(BlockDriverState *bs); typedef struct RawPosixAIOData { BlockDriverState *bs; @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, #endif s->needs_alignment = raw_needs_alignment(bs); + if (!s->use_linux_aio || laio_has_fua()) { + bs->supported_write_flags = BDRV_REQ_FUA; + } + bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK; if (S_ISREG(st.st_mode)) { /* When extending regular files, we get zeros from the OS */ @@ -XXX,XX +XXX,XX @@ static inline bool raw_check_linux_aio(BDRVRawState *s) #endif static int coroutine_fn raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, - uint64_t bytes, QEMUIOVector *qiov, int type) + uint64_t bytes, QEMUIOVector *qiov, int type, + int flags) { BDRVRawState *s = bs->opaque; RawPosixAIOData acb; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, #ifdef CONFIG_LINUX_IO_URING } else if (raw_check_linux_io_uring(s)) { assert(qiov->size == bytes); - ret = luring_co_submit(bs, s->fd, offset, qiov, type); + ret = luring_co_submit(bs, s->fd, offset, qiov, type, flags); goto out; #endif #ifdef CONFIG_LINUX_AIO } else if (raw_check_linux_aio(s)) { assert(qiov->size == bytes); - ret = laio_co_submit(s->fd, offset, qiov, type, + ret = laio_co_submit(s->fd, offset, qiov, type, flags, s->aio_max_batch); goto out; #endif @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, assert(qiov->size == bytes); ret = raw_thread_pool_submit(handle_aiocb_rw, &acb); + if (ret == 0 && (flags & BDRV_REQ_FUA)) { + /* TODO Use pwritev2() instead if it's available */ + ret = raw_co_flush_to_disk(bs); + } goto out; /* Avoid the compiler err of unused label */ out: @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) { - return raw_co_prw(bs, &offset, bytes, qiov, QEMU_AIO_READ); + return raw_co_prw(bs, &offset, bytes, qiov, QEMU_AIO_READ, flags); } static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) { - return raw_co_prw(bs, &offset, bytes, qiov, QEMU_AIO_WRITE); + return raw_co_prw(bs, &offset, bytes, qiov, QEMU_AIO_WRITE, flags); } static int coroutine_fn raw_co_flush_to_disk(BlockDriverState *bs) @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_flush_to_disk(BlockDriverState *bs) #ifdef CONFIG_LINUX_IO_URING if (raw_check_linux_io_uring(s)) { - return luring_co_submit(bs, s->fd, 0, NULL, QEMU_AIO_FLUSH); + return luring_co_submit(bs, s->fd, 0, NULL, QEMU_AIO_FLUSH, 0); } #endif #ifdef CONFIG_LINUX_AIO if (s->has_laio_fdsync && raw_check_linux_aio(s)) { - return laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0); + return laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0, 0); } #endif return raw_thread_pool_submit(handle_aiocb_flush, &acb); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_zone_append(BlockDriverState *bs, } trace_zbd_zone_append(bs, *offset >> BDRV_SECTOR_BITS); - return raw_co_prw(bs, offset, len, qiov, QEMU_AIO_ZONE_APPEND); + return raw_co_prw(bs, offset, len, qiov, QEMU_AIO_ZONE_APPEND, 0); } #endif diff --git a/block/io_uring.c b/block/io_uring.c index XXXXXXX..XXXXXXX 100644 --- a/block/io_uring.c +++ b/block/io_uring.c @@ -XXX,XX +XXX,XX @@ static void luring_deferred_fn(void *opaque) * */ static int luring_do_submit(int fd, LuringAIOCB *luringcb, LuringState *s, - uint64_t offset, int type) + uint64_t offset, int type, BdrvRequestFlags flags) { int ret; struct io_uring_sqe *sqes = &luringcb->sqeq; + int luring_flags; switch (type) { case QEMU_AIO_WRITE: - io_uring_prep_writev(sqes, fd, luringcb->qiov->iov, - luringcb->qiov->niov, offset); + luring_flags = (flags & BDRV_REQ_FUA) ? RWF_DSYNC : 0; + io_uring_prep_writev2(sqes, fd, luringcb->qiov->iov, + luringcb->qiov->niov, offset, luring_flags); break; case QEMU_AIO_ZONE_APPEND: io_uring_prep_writev(sqes, fd, luringcb->qiov->iov, @@ -XXX,XX +XXX,XX @@ static int luring_do_submit(int fd, LuringAIOCB *luringcb, LuringState *s, } int coroutine_fn luring_co_submit(BlockDriverState *bs, int fd, uint64_t offset, - QEMUIOVector *qiov, int type) + QEMUIOVector *qiov, int type, + BdrvRequestFlags flags) { int ret; AioContext *ctx = qemu_get_current_aio_context(); @@ -XXX,XX +XXX,XX @@ int coroutine_fn luring_co_submit(BlockDriverState *bs, int fd, uint64_t offset, }; trace_luring_co_submit(bs, s, &luringcb, fd, offset, qiov ? qiov->size : 0, type); - ret = luring_do_submit(fd, &luringcb, s, offset, type); + ret = luring_do_submit(fd, &luringcb, s, offset, type, flags); if (ret < 0) { return ret; diff --git a/block/linux-aio.c b/block/linux-aio.c index XXXXXXX..XXXXXXX 100644 --- a/block/linux-aio.c +++ b/block/linux-aio.c @@ -XXX,XX +XXX,XX @@ static void laio_deferred_fn(void *opaque) } static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, - int type, uint64_t dev_max_batch) + int type, BdrvRequestFlags flags, + uint64_t dev_max_batch) { LinuxAioState *s = laiocb->ctx; struct iocb *iocbs = &laiocb->iocb; QEMUIOVector *qiov = laiocb->qiov; + int laio_flags; switch (type) { case QEMU_AIO_WRITE: +#ifdef HAVE_IO_PREP_PWRITEV2 + laio_flags = (flags & BDRV_REQ_FUA) ? RWF_DSYNC : 0; + io_prep_pwritev2(iocbs, fd, qiov->iov, qiov->niov, offset, laio_flags); +#else + assert(flags == 0); io_prep_pwritev(iocbs, fd, qiov->iov, qiov->niov, offset); +#endif break; case QEMU_AIO_ZONE_APPEND: io_prep_pwritev(iocbs, fd, qiov->iov, qiov->niov, offset); @@ -XXX,XX +XXX,XX @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset, } int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov, - int type, uint64_t dev_max_batch) + int type, BdrvRequestFlags flags, + uint64_t dev_max_batch) { int ret; AioContext *ctx = qemu_get_current_aio_context(); @@ -XXX,XX +XXX,XX @@ int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov, .qiov = qiov, }; - ret = laio_do_submit(fd, &laiocb, offset, type, dev_max_batch); + ret = laio_do_submit(fd, &laiocb, offset, type, flags, dev_max_batch); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ bool laio_has_fdsync(int fd) io_destroy(ctx); return (ret == -EINVAL) ? false : true; } + +bool laio_has_fua(void) +{ +#ifdef HAVE_IO_PREP_PWRITEV2 + return true; +#else + return false; +#endif +} diff --git a/meson.build b/meson.build index XXXXXXX..XXXXXXX 100644 --- a/meson.build +++ b/meson.build @@ -XXX,XX +XXX,XX @@ config_host_data.set('HAVE_OPTRESET', cc.has_header_symbol('getopt.h', 'optreset')) config_host_data.set('HAVE_IPPROTO_MPTCP', cc.has_header_symbol('netinet/in.h', 'IPPROTO_MPTCP')) +if libaio.found() + config_host_data.set('HAVE_IO_PREP_PWRITEV2', + cc.has_header_symbol('libaio.h', 'io_prep_pwritev2')) +endif # has_member config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID', -- 2.48.1 For block drivers that don't advertise FUA support, we already call bdrv_co_flush(), which considers BDRV_O_NO_FLUSH. However, drivers that do support FUA still see the FUA flag with BDRV_O_NO_FLUSH and get the associated performance penalty that cache.no-flush=on was supposed to avoid. Clear FUA for write requests if BDRV_O_NO_FLUSH is set. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250307221634.71951-3-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/io.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/block/io.c b/block/io.c index XXXXXXX..XXXXXXX 100644 --- a/block/io.c +++ b/block/io.c @@ -XXX,XX +XXX,XX @@ bdrv_driver_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes, return -ENOMEDIUM; } + if (bs->open_flags & BDRV_O_NO_FLUSH) { + flags &= ~BDRV_REQ_FUA; + } + if ((flags & BDRV_REQ_FUA) && (~bs->supported_write_flags & BDRV_REQ_FUA)) { flags &= ~BDRV_REQ_FUA; -- 2.48.1 As a preparation for having multiple adaptive polling states per AioContext, move the 'ns' field into a separate struct. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250307221634.71951-4-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/aio.h | 6 +++++- util/aio-posix.c | 31 ++++++++++++++++--------------- util/async.c | 3 ++- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/include/block/aio.h b/include/block/aio.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -XXX,XX +XXX,XX @@ struct BHListSlice { typedef QSLIST_HEAD(, AioHandler) AioHandlerSList; +typedef struct AioPolledEvent { + int64_t ns; /* current polling time in nanoseconds */ +} AioPolledEvent; + struct AioContext { GSource source; @@ -XXX,XX +XXX,XX @@ struct AioContext { int poll_disable_cnt; /* Polling mode parameters */ - int64_t poll_ns; /* current polling time in nanoseconds */ + AioPolledEvent poll; int64_t poll_max_ns; /* maximum polling time in nanoseconds */ int64_t poll_grow; /* polling time growth factor */ int64_t poll_shrink; /* polling time shrink factor */ diff --git a/util/aio-posix.c b/util/aio-posix.c index XXXXXXX..XXXXXXX 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -XXX,XX +XXX,XX @@ static bool try_poll_mode(AioContext *ctx, AioHandlerList *ready_list, return false; } - max_ns = qemu_soonest_timeout(*timeout, ctx->poll_ns); + max_ns = qemu_soonest_timeout(*timeout, ctx->poll.ns); if (max_ns && !ctx->fdmon_ops->need_wait(ctx)) { /* * Enable poll mode. It pairs with the poll_set_started() in @@ -XXX,XX +XXX,XX @@ bool aio_poll(AioContext *ctx, bool blocking) if (ctx->poll_max_ns) { int64_t block_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start; - if (block_ns <= ctx->poll_ns) { + if (block_ns <= ctx->poll.ns) { /* This is the sweet spot, no adjustment needed */ } else if (block_ns > ctx->poll_max_ns) { /* We'd have to poll for too long, poll less */ - int64_t old = ctx->poll_ns; + int64_t old = ctx->poll.ns; if (ctx->poll_shrink) { - ctx->poll_ns /= ctx->poll_shrink; + ctx->poll.ns /= ctx->poll_shrink; } else { - ctx->poll_ns = 0; + ctx->poll.ns = 0; } - trace_poll_shrink(ctx, old, ctx->poll_ns); - } else if (ctx->poll_ns < ctx->poll_max_ns && + trace_poll_shrink(ctx, old, ctx->poll.ns); + } else if (ctx->poll.ns < ctx->poll_max_ns && block_ns < ctx->poll_max_ns) { /* There is room to grow, poll longer */ - int64_t old = ctx->poll_ns; + int64_t old = ctx->poll.ns; int64_t grow = ctx->poll_grow; if (grow == 0) { grow = 2; } - if (ctx->poll_ns) { - ctx->poll_ns *= grow; + if (ctx->poll.ns) { + ctx->poll.ns *= grow; } else { - ctx->poll_ns = 4000; /* start polling at 4 microseconds */ + ctx->poll.ns = 4000; /* start polling at 4 microseconds */ } - if (ctx->poll_ns > ctx->poll_max_ns) { - ctx->poll_ns = ctx->poll_max_ns; + if (ctx->poll.ns > ctx->poll_max_ns) { + ctx->poll.ns = ctx->poll_max_ns; } - trace_poll_grow(ctx, old, ctx->poll_ns); + trace_poll_grow(ctx, old, ctx->poll.ns); } } @@ -XXX,XX +XXX,XX @@ void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns, /* No thread synchronization here, it doesn't matter if an incorrect value * is used once. */ + ctx->poll.ns = 0; + ctx->poll_max_ns = max_ns; - ctx->poll_ns = 0; ctx->poll_grow = grow; ctx->poll_shrink = shrink; diff --git a/util/async.c b/util/async.c index XXXXXXX..XXXXXXX 100644 --- a/util/async.c +++ b/util/async.c @@ -XXX,XX +XXX,XX @@ AioContext *aio_context_new(Error **errp) qemu_rec_mutex_init(&ctx->lock); timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx); - ctx->poll_ns = 0; + ctx->poll.ns = 0; + ctx->poll_max_ns = 0; ctx->poll_grow = 0; ctx->poll_shrink = 0; -- 2.48.1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250307221634.71951-5-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- util/aio-posix.c | 77 ++++++++++++++++++++++++++---------------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/util/aio-posix.c b/util/aio-posix.c index XXXXXXX..XXXXXXX 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -XXX,XX +XXX,XX @@ static bool try_poll_mode(AioContext *ctx, AioHandlerList *ready_list, return false; } +static void adjust_polling_time(AioContext *ctx, AioPolledEvent *poll, + int64_t block_ns) +{ + if (block_ns <= poll->ns) { + /* This is the sweet spot, no adjustment needed */ + } else if (block_ns > ctx->poll_max_ns) { + /* We'd have to poll for too long, poll less */ + int64_t old = poll->ns; + + if (ctx->poll_shrink) { + poll->ns /= ctx->poll_shrink; + } else { + poll->ns = 0; + } + + trace_poll_shrink(ctx, old, poll->ns); + } else if (poll->ns < ctx->poll_max_ns && + block_ns < ctx->poll_max_ns) { + /* There is room to grow, poll longer */ + int64_t old = poll->ns; + int64_t grow = ctx->poll_grow; + + if (grow == 0) { + grow = 2; + } + + if (poll->ns) { + poll->ns *= grow; + } else { + poll->ns = 4000; /* start polling at 4 microseconds */ + } + + if (poll->ns > ctx->poll_max_ns) { + poll->ns = ctx->poll_max_ns; + } + + trace_poll_grow(ctx, old, poll->ns); + } +} + bool aio_poll(AioContext *ctx, bool blocking) { AioHandlerList ready_list = QLIST_HEAD_INITIALIZER(ready_list); @@ -XXX,XX +XXX,XX @@ bool aio_poll(AioContext *ctx, bool blocking) /* Adjust polling time */ if (ctx->poll_max_ns) { int64_t block_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start; - - if (block_ns <= ctx->poll.ns) { - /* This is the sweet spot, no adjustment needed */ - } else if (block_ns > ctx->poll_max_ns) { - /* We'd have to poll for too long, poll less */ - int64_t old = ctx->poll.ns; - - if (ctx->poll_shrink) { - ctx->poll.ns /= ctx->poll_shrink; - } else { - ctx->poll.ns = 0; - } - - trace_poll_shrink(ctx, old, ctx->poll.ns); - } else if (ctx->poll.ns < ctx->poll_max_ns && - block_ns < ctx->poll_max_ns) { - /* There is room to grow, poll longer */ - int64_t old = ctx->poll.ns; - int64_t grow = ctx->poll_grow; - - if (grow == 0) { - grow = 2; - } - - if (ctx->poll.ns) { - ctx->poll.ns *= grow; - } else { - ctx->poll.ns = 4000; /* start polling at 4 microseconds */ - } - - if (ctx->poll.ns > ctx->poll_max_ns) { - ctx->poll.ns = ctx->poll_max_ns; - } - - trace_poll_grow(ctx, old, ctx->poll.ns); - } + adjust_polling_time(ctx, &ctx->poll, block_ns); } progress |= aio_bh_poll(ctx); -- 2.48.1 Adaptive polling has a big problem: It doesn't consider that an event loop can wait for many different events that may have very different typical latencies. For example, think of a guest that tends to send a new I/O request soon after the previous I/O request completes, but the storage on the host is rather slow. In this case, getting the new request from guest quickly means that polling is enabled, but the next thing is performing the I/O request on the backend, which is slow and disables polling again for the next guest request. This means that in such a scenario, polling could help for every other event, but is only ever enabled when it can't succeed. In order to fix this, keep a separate AioPolledEvent for each AioHandler. We will then know that the backend file descriptor always has a high latency and isn't worth polling for, but we also know that the guest is always fast and we should poll for it. This solves at least half of the problem, we can now keep polling for those cases where it makes sense and get the improved performance from it. Since the event loop doesn't know which event will be next, we still do some unnecessary polling while we're waiting for the slow disk. I made some attempts to be more clever than just randomly growing and shrinking the polling time, and even to let callers be explicit about when they expect a new event, but so far this hasn't resulted in improved performance or even caused performance regressions. For now, let's just fix the part that is easy enough to fix, we can revisit the rest later. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250307221634.71951-6-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/aio.h | 1 - util/aio-posix.h | 1 + util/aio-posix.c | 26 ++++++++++++++++++++++---- util/async.c | 2 -- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/include/block/aio.h b/include/block/aio.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -XXX,XX +XXX,XX @@ struct AioContext { int poll_disable_cnt; /* Polling mode parameters */ - AioPolledEvent poll; int64_t poll_max_ns; /* maximum polling time in nanoseconds */ int64_t poll_grow; /* polling time growth factor */ int64_t poll_shrink; /* polling time shrink factor */ diff --git a/util/aio-posix.h b/util/aio-posix.h index XXXXXXX..XXXXXXX 100644 --- a/util/aio-posix.h +++ b/util/aio-posix.h @@ -XXX,XX +XXX,XX @@ struct AioHandler { #endif int64_t poll_idle_timeout; /* when to stop userspace polling */ bool poll_ready; /* has polling detected an event? */ + AioPolledEvent poll; }; /* Add a handler to a ready list */ diff --git a/util/aio-posix.c b/util/aio-posix.c index XXXXXXX..XXXXXXX 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -XXX,XX +XXX,XX @@ static bool run_poll_handlers(AioContext *ctx, AioHandlerList *ready_list, static bool try_poll_mode(AioContext *ctx, AioHandlerList *ready_list, int64_t *timeout) { + AioHandler *node; int64_t max_ns; if (QLIST_EMPTY_RCU(&ctx->poll_aio_handlers)) { return false; } - max_ns = qemu_soonest_timeout(*timeout, ctx->poll.ns); + max_ns = 0; + QLIST_FOREACH(node, &ctx->poll_aio_handlers, node_poll) { + max_ns = MAX(max_ns, node->poll.ns); + } + max_ns = qemu_soonest_timeout(*timeout, max_ns); + if (max_ns && !ctx->fdmon_ops->need_wait(ctx)) { /* * Enable poll mode. It pairs with the poll_set_started() in @@ -XXX,XX +XXX,XX @@ bool aio_poll(AioContext *ctx, bool blocking) /* Adjust polling time */ if (ctx->poll_max_ns) { + AioHandler *node; int64_t block_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start; - adjust_polling_time(ctx, &ctx->poll, block_ns); + + QLIST_FOREACH(node, &ctx->poll_aio_handlers, node_poll) { + if (QLIST_IS_INSERTED(node, node_ready)) { + adjust_polling_time(ctx, &node->poll, block_ns); + } + } } progress |= aio_bh_poll(ctx); @@ -XXX,XX +XXX,XX @@ void aio_context_use_g_source(AioContext *ctx) void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns, int64_t grow, int64_t shrink, Error **errp) { + AioHandler *node; + + qemu_lockcnt_inc(&ctx->list_lock); + QLIST_FOREACH(node, &ctx->aio_handlers, node) { + node->poll.ns = 0; + } + qemu_lockcnt_dec(&ctx->list_lock); + /* No thread synchronization here, it doesn't matter if an incorrect value * is used once. */ - ctx->poll.ns = 0; - ctx->poll_max_ns = max_ns; ctx->poll_grow = grow; ctx->poll_shrink = shrink; diff --git a/util/async.c b/util/async.c index XXXXXXX..XXXXXXX 100644 --- a/util/async.c +++ b/util/async.c @@ -XXX,XX +XXX,XX @@ AioContext *aio_context_new(Error **errp) qemu_rec_mutex_init(&ctx->lock); timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx); - ctx->poll.ns = 0; - ctx->poll_max_ns = 0; ctx->poll_grow = 0; ctx->poll_shrink = 0; -- 2.48.1 aio_dispatch_handler() adds handlers to ctx->poll_aio_handlers if polling should be enabled. If we call adjust_polling_time() for all polling handlers before this, new polling handlers are still left at poll->ns = 0 and polling is only actually enabled after the next event. Move the adjust_polling_time() call after aio_dispatch_handler(). This fixes test-nested-aio-poll, which expects that polling becomes effective the first time around. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311141912.135657-1-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- util/aio-posix.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/util/aio-posix.c b/util/aio-posix.c index XXXXXXX..XXXXXXX 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -XXX,XX +XXX,XX @@ /* Stop userspace polling on a handler if it isn't active for some time */ #define POLL_IDLE_INTERVAL_NS (7 * NANOSECONDS_PER_SECOND) +static void adjust_polling_time(AioContext *ctx, AioPolledEvent *poll, + int64_t block_ns); + bool aio_poll_disabled(AioContext *ctx) { return qatomic_read(&ctx->poll_disable_cnt); @@ -XXX,XX +XXX,XX @@ static bool aio_dispatch_handler(AioContext *ctx, AioHandler *node) * scanning all handlers with aio_dispatch_handlers(). */ static bool aio_dispatch_ready_handlers(AioContext *ctx, - AioHandlerList *ready_list) + AioHandlerList *ready_list, + int64_t block_ns) { bool progress = false; AioHandler *node; @@ -XXX,XX +XXX,XX @@ static bool aio_dispatch_ready_handlers(AioContext *ctx, while ((node = QLIST_FIRST(ready_list))) { QLIST_REMOVE(node, node_ready); progress = aio_dispatch_handler(ctx, node) || progress; + + /* + * Adjust polling time only after aio_dispatch_handler(), which can + * add the handler to ctx->poll_aio_handlers. + */ + if (ctx->poll_max_ns && QLIST_IS_INSERTED(node, node_poll)) { + adjust_polling_time(ctx, &node->poll, block_ns); + } } return progress; @@ -XXX,XX +XXX,XX @@ bool aio_poll(AioContext *ctx, bool blocking) bool use_notify_me; int64_t timeout; int64_t start = 0; + int64_t block_ns = 0; /* * There cannot be two concurrent aio_poll calls for the same AioContext (or @@ -XXX,XX +XXX,XX @@ bool aio_poll(AioContext *ctx, bool blocking) aio_notify_accept(ctx); - /* Adjust polling time */ + /* Calculate blocked time for adaptive polling */ if (ctx->poll_max_ns) { - AioHandler *node; - int64_t block_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start; - - QLIST_FOREACH(node, &ctx->poll_aio_handlers, node_poll) { - if (QLIST_IS_INSERTED(node, node_ready)) { - adjust_polling_time(ctx, &node->poll, block_ns); - } - } + block_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - start; } progress |= aio_bh_poll(ctx); - progress |= aio_dispatch_ready_handlers(ctx, &ready_list); + progress |= aio_dispatch_ready_handlers(ctx, &ready_list, block_ns); aio_free_deleted_handlers(ctx); -- 2.48.1 From: Thomas Huth <thuth@redhat.com> qsd-migrate is currently only working for raw, qcow2 and qed. Other formats are failing, e.g. because they don't support migration. Thus let's limit this test to the three usable formats now. Suggested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Thomas Huth <thuth@redhat.com> Message-ID: <20250224214058.205889-1-thuth@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/tests/qsd-migrate | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/tests/qsd-migrate b/tests/qemu-iotests/tests/qsd-migrate index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/tests/qsd-migrate +++ b/tests/qemu-iotests/tests/qsd-migrate @@ -XXX,XX +XXX,XX @@ import iotests from iotests import filter_qemu_io, filter_qtest -iotests.script_initialize(supported_fmts=['generic'], +iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'], supported_protocols=['file'], supported_platforms=['linux']) -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> Commit 71544d30a6f8 ("scsi: push request restart to SCSIDevice") removed the only user of SCSIDiskState->bh. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-2-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/scsi/scsi-disk.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -XXX,XX +XXX,XX @@ struct SCSIDiskState { uint64_t max_unmap_size; uint64_t max_io_size; uint32_t quirks; - QEMUBH *bh; char *version; char *serial; char *vendor; -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> In the past a single AioContext was used for block I/O and it was fetched using blk_get_aio_context(). Nowadays the block layer supports running I/O from any AioContext and multiple AioContexts at the same time. Remove the dma_blk_io() AioContext argument and use the current AioContext instead. This makes calling the function easier and enables multiple IOThreads to use dma_blk_io() concurrently for the same block device. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-3-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/system/dma.h | 3 +-- hw/ide/core.c | 3 +-- hw/ide/macio.c | 3 +-- hw/scsi/scsi-disk.c | 6 ++---- system/dma-helpers.c | 8 ++++---- 5 files changed, 9 insertions(+), 14 deletions(-) diff --git a/include/system/dma.h b/include/system/dma.h index XXXXXXX..XXXXXXX 100644 --- a/include/system/dma.h +++ b/include/system/dma.h @@ -XXX,XX +XXX,XX @@ typedef BlockAIOCB *DMAIOFunc(int64_t offset, QEMUIOVector *iov, BlockCompletionFunc *cb, void *cb_opaque, void *opaque); -BlockAIOCB *dma_blk_io(AioContext *ctx, - QEMUSGList *sg, uint64_t offset, uint32_t align, +BlockAIOCB *dma_blk_io(QEMUSGList *sg, uint64_t offset, uint32_t align, DMAIOFunc *io_func, void *io_func_opaque, BlockCompletionFunc *cb, void *opaque, DMADirection dir); BlockAIOCB *dma_blk_read(BlockBackend *blk, diff --git a/hw/ide/core.c b/hw/ide/core.c index XXXXXXX..XXXXXXX 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -XXX,XX +XXX,XX @@ static void ide_dma_cb(void *opaque, int ret) BDRV_SECTOR_SIZE, ide_dma_cb, s); break; case IDE_DMA_TRIM: - s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk), - &s->sg, offset, BDRV_SECTOR_SIZE, + s->bus->dma->aiocb = dma_blk_io(&s->sg, offset, BDRV_SECTOR_SIZE, ide_issue_trim, s, ide_dma_cb, s, DMA_DIRECTION_TO_DEVICE); break; diff --git a/hw/ide/macio.c b/hw/ide/macio.c index XXXXXXX..XXXXXXX 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -XXX,XX +XXX,XX @@ static void pmac_ide_transfer_cb(void *opaque, int ret) pmac_ide_transfer_cb, io); break; case IDE_DMA_TRIM: - s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk), &s->sg, - offset, 0x1, ide_issue_trim, s, + s->bus->dma->aiocb = dma_blk_io(&s->sg, offset, 0x1, ide_issue_trim, s, pmac_ide_transfer_cb, io, DMA_DIRECTION_TO_DEVICE); break; diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -XXX,XX +XXX,XX @@ static void scsi_do_read(SCSIDiskReq *r, int ret) if (r->req.sg) { dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_READ); r->req.residual -= r->req.sg->size; - r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk), - r->req.sg, r->sector << BDRV_SECTOR_BITS, + r->req.aiocb = dma_blk_io(r->req.sg, r->sector << BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE, sdc->dma_readv, r, scsi_dma_complete, r, DMA_DIRECTION_FROM_DEVICE); @@ -XXX,XX +XXX,XX @@ static void scsi_write_data(SCSIRequest *req) if (r->req.sg) { dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_WRITE); r->req.residual -= r->req.sg->size; - r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk), - r->req.sg, r->sector << BDRV_SECTOR_BITS, + r->req.aiocb = dma_blk_io(r->req.sg, r->sector << BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE, sdc->dma_writev, r, scsi_dma_complete, r, DMA_DIRECTION_TO_DEVICE); diff --git a/system/dma-helpers.c b/system/dma-helpers.c index XXXXXXX..XXXXXXX 100644 --- a/system/dma-helpers.c +++ b/system/dma-helpers.c @@ -XXX,XX +XXX,XX @@ static const AIOCBInfo dma_aiocb_info = { .cancel_async = dma_aio_cancel, }; -BlockAIOCB *dma_blk_io(AioContext *ctx, +BlockAIOCB *dma_blk_io( QEMUSGList *sg, uint64_t offset, uint32_t align, DMAIOFunc *io_func, void *io_func_opaque, BlockCompletionFunc *cb, @@ -XXX,XX +XXX,XX @@ BlockAIOCB *dma_blk_io(AioContext *ctx, dbs->acb = NULL; dbs->sg = sg; - dbs->ctx = ctx; + dbs->ctx = qemu_get_current_aio_context(); dbs->offset = offset; dbs->align = align; dbs->sg_cur_index = 0; @@ -XXX,XX +XXX,XX @@ BlockAIOCB *dma_blk_read(BlockBackend *blk, QEMUSGList *sg, uint64_t offset, uint32_t align, void (*cb)(void *opaque, int ret), void *opaque) { - return dma_blk_io(blk_get_aio_context(blk), sg, offset, align, + return dma_blk_io(sg, offset, align, dma_blk_read_io_func, blk, cb, opaque, DMA_DIRECTION_FROM_DEVICE); } @@ -XXX,XX +XXX,XX @@ BlockAIOCB *dma_blk_write(BlockBackend *blk, QEMUSGList *sg, uint64_t offset, uint32_t align, void (*cb)(void *opaque, int ret), void *opaque) { - return dma_blk_io(blk_get_aio_context(blk), sg, offset, align, + return dma_blk_io(sg, offset, align, dma_blk_write_io_func, blk, cb, opaque, DMA_DIRECTION_TO_DEVICE); } -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> Until now, a SCSIDevice's I/O requests have run in a single AioContext. In order to support multiple IOThreads it will be necessary to move to the concept of a per-SCSIRequest AioContext. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-4-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/scsi/scsi.h | 1 + hw/scsi/scsi-bus.c | 1 + hw/scsi/scsi-disk.c | 17 ++++++----------- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -XXX,XX +XXX,XX @@ struct SCSIRequest { SCSIBus *bus; SCSIDevice *dev; const SCSIReqOps *ops; + AioContext *ctx; uint32_t refcount; uint32_t tag; uint32_t lun; diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -XXX,XX +XXX,XX @@ invalid_opcode: } } + req->ctx = qemu_get_current_aio_context(); req->cmd = cmd; req->residual = req->cmd.xfer; diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -XXX,XX +XXX,XX @@ static void scsi_aio_complete(void *opaque, int ret) SCSIDiskReq *r = (SCSIDiskReq *)opaque; SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); - /* The request must only run in the BlockBackend's AioContext */ - assert(blk_get_aio_context(s->qdev.conf.blk) == - qemu_get_current_aio_context()); + /* The request must run in its AioContext */ + assert(r->req.ctx == qemu_get_current_aio_context()); assert(r->req.aiocb != NULL); r->req.aiocb = NULL; @@ -XXX,XX +XXX,XX @@ static void scsi_dma_complete(void *opaque, int ret) static void scsi_read_complete_noio(SCSIDiskReq *r, int ret) { - SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); uint32_t n; - /* The request must only run in the BlockBackend's AioContext */ - assert(blk_get_aio_context(s->qdev.conf.blk) == - qemu_get_current_aio_context()); + /* The request must run in its AioContext */ + assert(r->req.ctx == qemu_get_current_aio_context()); assert(r->req.aiocb == NULL); if (scsi_disk_req_check_error(r, ret, ret > 0)) { @@ -XXX,XX +XXX,XX @@ static void scsi_read_data(SCSIRequest *req) static void scsi_write_complete_noio(SCSIDiskReq *r, int ret) { - SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); uint32_t n; - /* The request must only run in the BlockBackend's AioContext */ - assert(blk_get_aio_context(s->qdev.conf.blk) == - qemu_get_current_aio_context()); + /* The request must run in its AioContext */ + assert(r->req.ctx == qemu_get_current_aio_context()); assert (r->req.aiocb == NULL); if (scsi_disk_req_check_error(r, ret, ret > 0)) { -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> SCSIDevice keeps track of in-flight requests for device reset and Task Management Functions (TMFs). The request list requires protection so that multi-threaded SCSI emulation can be implemented in commits that follow. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-5-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/scsi/scsi.h | 7 ++- hw/scsi/scsi-bus.c | 120 +++++++++++++++++++++++++++++------------ 2 files changed, 88 insertions(+), 39 deletions(-) diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -XXX,XX +XXX,XX @@ struct SCSIRequest { bool dma_started; BlockAIOCB *aiocb; QEMUSGList *sg; + + /* Protected by SCSIDevice->requests_lock */ QTAILQ_ENTRY(SCSIRequest) next; }; @@ -XXX,XX +XXX,XX @@ struct SCSIDevice uint8_t sense[SCSI_SENSE_BUF_SIZE]; uint32_t sense_len; - /* - * The requests list is only accessed from the AioContext that executes - * requests or from the main loop when IOThread processing is stopped. - */ + QemuMutex requests_lock; /* protects the requests list */ QTAILQ_HEAD(, SCSIRequest) requests; uint32_t channel; diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -XXX,XX +XXX,XX @@ static void scsi_device_for_each_req_sync(SCSIDevice *s, assert(!runstate_is_running()); assert(qemu_in_main_thread()); - QTAILQ_FOREACH_SAFE(req, &s->requests, next, next_req) { - fn(req, opaque); + /* + * Locking is not necessary because the guest is stopped and no other + * threads can be accessing the requests list, but take the lock for + * consistency. + */ + WITH_QEMU_LOCK_GUARD(&s->requests_lock) { + QTAILQ_FOREACH_SAFE(req, &s->requests, next, next_req) { + fn(req, opaque); + } } } @@ -XXX,XX +XXX,XX @@ static void scsi_device_for_each_req_async_bh(void *opaque) { g_autofree SCSIDeviceForEachReqAsyncData *data = opaque; SCSIDevice *s = data->s; - AioContext *ctx; - SCSIRequest *req; - SCSIRequest *next; + g_autoptr(GList) reqs = NULL; /* - * The BB cannot have changed contexts between this BH being scheduled and - * now: BBs' AioContexts, when they have a node attached, can only be - * changed via bdrv_try_change_aio_context(), in a drained section. While - * we have the in-flight counter incremented, that drain must block. + * Build a list of requests in this AioContext so fn() can be invoked later + * outside requests_lock. */ - ctx = blk_get_aio_context(s->conf.blk); - assert(ctx == qemu_get_current_aio_context()); + WITH_QEMU_LOCK_GUARD(&s->requests_lock) { + AioContext *ctx = qemu_get_current_aio_context(); + SCSIRequest *req; + SCSIRequest *next; + + QTAILQ_FOREACH_SAFE(req, &s->requests, next, next) { + if (req->ctx == ctx) { + scsi_req_ref(req); /* dropped after calling fn() */ + reqs = g_list_prepend(reqs, req); + } + } + } - QTAILQ_FOREACH_SAFE(req, &s->requests, next, next) { - data->fn(req, data->fn_opaque); + /* Call fn() on each request */ + for (GList *elem = g_list_first(reqs); elem; elem = g_list_next(elem)) { + data->fn(elem->data, data->fn_opaque); + scsi_req_unref(elem->data); } /* Drop the reference taken by scsi_device_for_each_req_async() */ @@ -XXX,XX +XXX,XX @@ static void scsi_device_for_each_req_async_bh(void *opaque) blk_dec_in_flight(s->conf.blk); } +static void scsi_device_for_each_req_async_do_ctx(gpointer key, gpointer value, + gpointer user_data) +{ + AioContext *ctx = key; + SCSIDeviceForEachReqAsyncData *params = user_data; + SCSIDeviceForEachReqAsyncData *data; + + data = g_new(SCSIDeviceForEachReqAsyncData, 1); + data->s = params->s; + data->fn = params->fn; + data->fn_opaque = params->fn_opaque; + + /* + * Hold a reference to the SCSIDevice until + * scsi_device_for_each_req_async_bh() finishes. + */ + object_ref(OBJECT(data->s)); + + /* Paired with scsi_device_for_each_req_async_bh() */ + blk_inc_in_flight(data->s->conf.blk); + + aio_bh_schedule_oneshot(ctx, scsi_device_for_each_req_async_bh, data); +} + /* * Schedule @fn() to be invoked for each enqueued request in device @s. @fn() - * runs in the AioContext that is executing the request. + * must be thread-safe because it runs concurrently in each AioContext that is + * executing a request. + * * Keeps the BlockBackend's in-flight counter incremented until everything is * done, so draining it will settle all scheduled @fn() calls. */ @@ -XXX,XX +XXX,XX @@ static void scsi_device_for_each_req_async(SCSIDevice *s, { assert(qemu_in_main_thread()); - SCSIDeviceForEachReqAsyncData *data = - g_new(SCSIDeviceForEachReqAsyncData, 1); - - data->s = s; - data->fn = fn; - data->fn_opaque = opaque; - - /* - * Hold a reference to the SCSIDevice until - * scsi_device_for_each_req_async_bh() finishes. - */ - object_ref(OBJECT(s)); + /* The set of AioContexts where the requests are being processed */ + g_autoptr(GHashTable) aio_contexts = g_hash_table_new(NULL, NULL); + WITH_QEMU_LOCK_GUARD(&s->requests_lock) { + SCSIRequest *req; + QTAILQ_FOREACH(req, &s->requests, next) { + g_hash_table_add(aio_contexts, req->ctx); + } + } - /* Paired with blk_dec_in_flight() in scsi_device_for_each_req_async_bh() */ - blk_inc_in_flight(s->conf.blk); - aio_bh_schedule_oneshot(blk_get_aio_context(s->conf.blk), - scsi_device_for_each_req_async_bh, - data); + /* Schedule a BH for each AioContext */ + SCSIDeviceForEachReqAsyncData params = { + .s = s, + .fn = fn, + .fn_opaque = opaque, + }; + g_hash_table_foreach( + aio_contexts, + scsi_device_for_each_req_async_do_ctx, + &params + ); } static void scsi_device_realize(SCSIDevice *s, Error **errp) @@ -XXX,XX +XXX,XX @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp) dev->lun = lun; } + qemu_mutex_init(&dev->requests_lock); QTAILQ_INIT(&dev->requests); scsi_device_realize(dev, &local_err); if (local_err) { @@ -XXX,XX +XXX,XX @@ static void scsi_qdev_unrealize(DeviceState *qdev) scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE)); + qemu_mutex_destroy(&dev->requests_lock); + scsi_device_unrealize(dev); blockdev_mark_auto_del(dev->conf.blk); @@ -XXX,XX +XXX,XX @@ static void scsi_req_enqueue_internal(SCSIRequest *req) req->sg = NULL; } req->enqueued = true; - QTAILQ_INSERT_TAIL(&req->dev->requests, req, next); + + WITH_QEMU_LOCK_GUARD(&req->dev->requests_lock) { + QTAILQ_INSERT_TAIL(&req->dev->requests, req, next); + } } int32_t scsi_req_enqueue(SCSIRequest *req) @@ -XXX,XX +XXX,XX @@ static void scsi_req_dequeue(SCSIRequest *req) trace_scsi_req_dequeue(req->dev->id, req->lun, req->tag); req->retry = false; if (req->enqueued) { - QTAILQ_REMOVE(&req->dev->requests, req, next); + WITH_QEMU_LOCK_GUARD(&req->dev->requests_lock) { + QTAILQ_REMOVE(&req->dev->requests, req, next); + } req->enqueued = false; scsi_req_unref(req); } @@ -XXX,XX +XXX,XX @@ static void scsi_device_class_init(ObjectClass *klass, void *data) static void scsi_dev_instance_init(Object *obj) { - DeviceState *dev = DEVICE(obj); - SCSIDevice *s = SCSI_DEVICE(dev); + SCSIDevice *s = SCSI_DEVICE(obj); device_add_bootindex_property(obj, &s->conf.bootindex, "bootindex", NULL, -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> Virtqueues are not thread-safe. Until now this was not a major issue since all virtqueue processing happened in the same thread. The ctrl queue's Task Management Function (TMF) requests sometimes need the main loop, so a BH was used to schedule the virtqueue completion back in the thread that has virtqueue access. When IOThread Virtqueue Mapping is introduced in later commits, event and ctrl virtqueue accesses from other threads will become necessary. Introduce an optional per-virtqueue lock so the event and ctrl virtqueues can be protected in the commits that follow. The addition of the ctrl virtqueue lock makes virtio_scsi_complete_req_from_main_loop() and its BH unnecessary. Instead, take the ctrl virtqueue lock from the main loop thread. The cmd virtqueue does not have a lock because the entirety of SCSI command processing happens in one thread. Only one thread accesses the cmd virtqueue and a lock is unnecessary. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-6-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/virtio/virtio-scsi.h | 3 ++ hw/scsi/virtio-scsi.c | 84 ++++++++++++++++++--------------- 2 files changed, 49 insertions(+), 38 deletions(-) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -XXX,XX +XXX,XX @@ struct VirtIOSCSI { int resetting; /* written from main loop thread, read from any thread */ bool events_dropped; + QemuMutex ctrl_lock; /* protects ctrl_vq */ + QemuMutex event_lock; /* protects event_vq */ + /* * TMFs deferred to main loop BH. These fields are protected by * tmf_bh_lock. diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_free_req(VirtIOSCSIReq *req) g_free(req); } -static void virtio_scsi_complete_req(VirtIOSCSIReq *req) +static void virtio_scsi_complete_req(VirtIOSCSIReq *req, QemuMutex *vq_lock) { VirtIOSCSI *s = req->dev; VirtQueue *vq = req->vq; VirtIODevice *vdev = VIRTIO_DEVICE(s); qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size); + + if (vq_lock) { + qemu_mutex_lock(vq_lock); + } + virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size); if (s->dataplane_started && !s->dataplane_fenced) { virtio_notify_irqfd(vdev, vq); @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req) virtio_notify(vdev, vq); } + if (vq_lock) { + qemu_mutex_unlock(vq_lock); + } + if (req->sreq) { req->sreq->hba_private = NULL; scsi_req_unref(req->sreq); @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req) virtio_scsi_free_req(req); } -static void virtio_scsi_complete_req_bh(void *opaque) +static void virtio_scsi_bad_req(VirtIOSCSIReq *req, QemuMutex *vq_lock) { - VirtIOSCSIReq *req = opaque; + virtio_error(VIRTIO_DEVICE(req->dev), "wrong size for virtio-scsi headers"); - virtio_scsi_complete_req(req); -} + if (vq_lock) { + qemu_mutex_lock(vq_lock); + } -/* - * Called from virtio_scsi_do_one_tmf_bh() in main loop thread. The main loop - * thread cannot touch the virtqueue since that could race with an IOThread. - */ -static void virtio_scsi_complete_req_from_main_loop(VirtIOSCSIReq *req) -{ - VirtIOSCSI *s = req->dev; + virtqueue_detach_element(req->vq, &req->elem, 0); - if (!s->ctx || s->ctx == qemu_get_aio_context()) { - /* No need to schedule a BH when there is no IOThread */ - virtio_scsi_complete_req(req); - } else { - /* Run request completion in the IOThread */ - aio_wait_bh_oneshot(s->ctx, virtio_scsi_complete_req_bh, req); + if (vq_lock) { + qemu_mutex_unlock(vq_lock); } -} -static void virtio_scsi_bad_req(VirtIOSCSIReq *req) -{ - virtio_error(VIRTIO_DEVICE(req->dev), "wrong size for virtio-scsi headers"); - virtqueue_detach_element(req->vq, &req->elem, 0); virtio_scsi_free_req(req); } @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req, return 0; } -static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq) +static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq, QemuMutex *vq_lock) { VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s; VirtIOSCSIReq *req; + if (vq_lock) { + qemu_mutex_lock(vq_lock); + } + req = virtqueue_pop(vq, sizeof(VirtIOSCSIReq) + vs->cdb_size); + + if (vq_lock) { + qemu_mutex_unlock(vq_lock); + } + if (!req) { return NULL; } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_cancel_notify(Notifier *notifier, void *data) trace_virtio_scsi_tmf_resp(virtio_scsi_get_lun(req->req.tmf.lun), req->req.tmf.tag, req->resp.tmf.response); - virtio_scsi_complete_req(req); + virtio_scsi_complete_req(req, &req->dev->ctrl_lock); } g_free(n); } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req) out: object_unref(OBJECT(d)); - virtio_scsi_complete_req_from_main_loop(req); + virtio_scsi_complete_req(req, &s->ctrl_lock); } /* Some TMFs must be processed from the main loop thread */ @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s) /* SAM-6 6.3.2 Hard reset */ req->resp.tmf.response = VIRTIO_SCSI_S_TARGET_FAILURE; - virtio_scsi_complete_req(req); + virtio_scsi_complete_req(req, &req->dev->ctrl_lock); } } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req) if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0, &type, sizeof(type)) < sizeof(type)) { - virtio_scsi_bad_req(req); + virtio_scsi_bad_req(req, &s->ctrl_lock); return; } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req) if (type == VIRTIO_SCSI_T_TMF) { if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlTMFReq), sizeof(VirtIOSCSICtrlTMFResp)) < 0) { - virtio_scsi_bad_req(req); + virtio_scsi_bad_req(req, &s->ctrl_lock); return; } else { r = virtio_scsi_do_tmf(s, req); @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req) type == VIRTIO_SCSI_T_AN_SUBSCRIBE) { if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlANReq), sizeof(VirtIOSCSICtrlANResp)) < 0) { - virtio_scsi_bad_req(req); + virtio_scsi_bad_req(req, &s->ctrl_lock); return; } else { req->req.an.event_requested = @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req) type == VIRTIO_SCSI_T_AN_SUBSCRIBE) trace_virtio_scsi_an_resp(virtio_scsi_get_lun(req->req.an.lun), req->resp.an.response); - virtio_scsi_complete_req(req); + virtio_scsi_complete_req(req, &s->ctrl_lock); } else { assert(r == -EINPROGRESS); } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq) { VirtIOSCSIReq *req; - while ((req = virtio_scsi_pop_req(s, vq))) { + while ((req = virtio_scsi_pop_req(s, vq, &s->ctrl_lock))) { virtio_scsi_handle_ctrl_req(s, req); } } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req) * in virtio_scsi_command_complete. */ req->resp_size = sizeof(VirtIOSCSICmdResp); - virtio_scsi_complete_req(req); + virtio_scsi_complete_req(req, NULL); } static void virtio_scsi_command_failed(SCSIRequest *r) @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req) virtio_scsi_fail_cmd_req(req); return -ENOTSUP; } else { - virtio_scsi_bad_req(req); + virtio_scsi_bad_req(req, NULL); return -EINVAL; } } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq) virtio_queue_set_notification(vq, 0); } - while ((req = virtio_scsi_pop_req(s, vq))) { + while ((req = virtio_scsi_pop_req(s, vq, NULL))) { ret = virtio_scsi_handle_cmd_req_prepare(s, req); if (!ret) { QTAILQ_INSERT_TAIL(&reqs, req, next); @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_push_event(VirtIOSCSI *s, return; } - req = virtio_scsi_pop_req(s, vs->event_vq); + req = virtio_scsi_pop_req(s, vs->event_vq, &s->event_lock); if (!req) { s->events_dropped = true; return; @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_push_event(VirtIOSCSI *s, } if (virtio_scsi_parse_req(req, 0, sizeof(VirtIOSCSIEvent))) { - virtio_scsi_bad_req(req); + virtio_scsi_bad_req(req, &s->event_lock); return; } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_push_event(VirtIOSCSI *s, } trace_virtio_scsi_event(virtio_scsi_get_lun(evt->lun), event, reason); - virtio_scsi_complete_req(req); + virtio_scsi_complete_req(req, &s->event_lock); } static void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq) @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp) Error *err = NULL; QTAILQ_INIT(&s->tmf_bh_list); + qemu_mutex_init(&s->ctrl_lock); + qemu_mutex_init(&s->event_lock); qemu_mutex_init(&s->tmf_bh_lock); virtio_scsi_common_realize(dev, @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_device_unrealize(DeviceState *dev) qbus_set_hotplug_handler(BUS(&s->bus), NULL); virtio_scsi_common_unrealize(dev); qemu_mutex_destroy(&s->tmf_bh_lock); + qemu_mutex_destroy(&s->event_lock); + qemu_mutex_destroy(&s->ctrl_lock); } static const Property virtio_scsi_properties[] = { -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> The block layer can invoke the resize callback from any AioContext that is processing requests. The virtqueue is already protected but the events_dropped field also needs to be protected against races. Cover it using the event virtqueue lock because it is closely associated with accesses to the virtqueue. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-7-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/virtio/virtio-scsi.h | 3 ++- hw/scsi/virtio-scsi.c | 29 ++++++++++++++++++++--------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -XXX,XX +XXX,XX @@ struct VirtIOSCSI { SCSIBus bus; int resetting; /* written from main loop thread, read from any thread */ + + QemuMutex event_lock; /* protects event_vq and events_dropped */ bool events_dropped; QemuMutex ctrl_lock; /* protects ctrl_vq */ - QemuMutex event_lock; /* protects event_vq */ /* * TMFs deferred to main loop BH. These fields are protected by diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_reset(VirtIODevice *vdev) vs->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE; vs->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE; - s->events_dropped = false; + + WITH_QEMU_LOCK_GUARD(&s->event_lock) { + s->events_dropped = false; + } } typedef struct { @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_push_event(VirtIOSCSI *s, } req = virtio_scsi_pop_req(s, vs->event_vq, &s->event_lock); - if (!req) { - s->events_dropped = true; - return; - } + WITH_QEMU_LOCK_GUARD(&s->event_lock) { + if (!req) { + s->events_dropped = true; + return; + } - if (s->events_dropped) { - event |= VIRTIO_SCSI_T_EVENTS_MISSED; - s->events_dropped = false; + if (s->events_dropped) { + event |= VIRTIO_SCSI_T_EVENTS_MISSED; + s->events_dropped = false; + } } if (virtio_scsi_parse_req(req, 0, sizeof(VirtIOSCSIEvent))) { @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_push_event(VirtIOSCSI *s, static void virtio_scsi_handle_event_vq(VirtIOSCSI *s, VirtQueue *vq) { - if (s->events_dropped) { + bool events_dropped; + + WITH_QEMU_LOCK_GUARD(&s->event_lock) { + events_dropped = s->events_dropped; + } + + if (events_dropped) { VirtIOSCSIEventInfo info = { .event = VIRTIO_SCSI_T_NO_EVENT, }; -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> With IOThread Virtqueue Mapping there will be multiple AioContexts processing SCSI requests. scsi_req_cancel() and other SCSI request operations must be performed from the AioContext where the request is running. Introduce a virtio_scsi_defer_tmf_to_aio_context() function and the necessary VirtIOSCSIReq->remaining refcount infrastructure to move the TMF code into the AioContext where the request is running. For the time being there is still just one AioContext: the main loop or the IOThread. When the iothread-vq-mapping parameter is added in a later patch this will be changed to per-virtqueue AioContexts. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-8-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/scsi/virtio-scsi.c | 270 ++++++++++++++++++++++++++++++++---------- 1 file changed, 206 insertions(+), 64 deletions(-) diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -XXX,XX +XXX,XX @@ typedef struct VirtIOSCSIReq { /* Used for two-stage request submission and TMFs deferred to BH */ QTAILQ_ENTRY(VirtIOSCSIReq) next; - /* Used for cancellation of request during TMFs */ + /* Used for cancellation of request during TMFs. Atomic. */ int remaining; SCSIRequest *sreq; @@ -XXX,XX +XXX,XX @@ typedef struct { VirtIOSCSIReq *tmf_req; } VirtIOSCSICancelNotifier; +static void virtio_scsi_tmf_dec_remaining(VirtIOSCSIReq *tmf) +{ + if (qatomic_fetch_dec(&tmf->remaining) == 1) { + trace_virtio_scsi_tmf_resp(virtio_scsi_get_lun(tmf->req.tmf.lun), + tmf->req.tmf.tag, tmf->resp.tmf.response); + + virtio_scsi_complete_req(tmf, &tmf->dev->ctrl_lock); + } +} + static void virtio_scsi_cancel_notify(Notifier *notifier, void *data) { VirtIOSCSICancelNotifier *n = container_of(notifier, VirtIOSCSICancelNotifier, notifier); - if (--n->tmf_req->remaining == 0) { - VirtIOSCSIReq *req = n->tmf_req; - - trace_virtio_scsi_tmf_resp(virtio_scsi_get_lun(req->req.tmf.lun), - req->req.tmf.tag, req->resp.tmf.response); - virtio_scsi_complete_req(req, &req->dev->ctrl_lock); - } + virtio_scsi_tmf_dec_remaining(n->tmf_req); g_free(n); } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s) } } -static void virtio_scsi_defer_tmf_to_bh(VirtIOSCSIReq *req) +static void virtio_scsi_defer_tmf_to_main_loop(VirtIOSCSIReq *req) { VirtIOSCSI *s = req->dev; @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_defer_tmf_to_bh(VirtIOSCSIReq *req) } } +static void virtio_scsi_tmf_cancel_req(VirtIOSCSIReq *tmf, SCSIRequest *r) +{ + VirtIOSCSICancelNotifier *notifier; + + assert(r->ctx == qemu_get_current_aio_context()); + + /* Decremented in virtio_scsi_cancel_notify() */ + qatomic_inc(&tmf->remaining); + + notifier = g_new(VirtIOSCSICancelNotifier, 1); + notifier->notifier.notify = virtio_scsi_cancel_notify; + notifier->tmf_req = tmf; + scsi_req_cancel_async(r, &notifier->notifier); +} + +/* Execute a TMF on the requests in the current AioContext */ +static void virtio_scsi_do_tmf_aio_context(void *opaque) +{ + AioContext *ctx = qemu_get_current_aio_context(); + VirtIOSCSIReq *tmf = opaque; + VirtIOSCSI *s = tmf->dev; + SCSIDevice *d = virtio_scsi_device_get(s, tmf->req.tmf.lun); + SCSIRequest *r; + bool match_tag; + + if (!d) { + tmf->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET; + virtio_scsi_tmf_dec_remaining(tmf); + return; + } + + /* + * This function could handle other subtypes that need to be processed in + * the request's AioContext in the future, but for now only request + * cancelation subtypes are performed here. + */ + switch (tmf->req.tmf.subtype) { + case VIRTIO_SCSI_T_TMF_ABORT_TASK: + match_tag = true; + break; + case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET: + case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET: + match_tag = false; + break; + default: + g_assert_not_reached(); + } + + WITH_QEMU_LOCK_GUARD(&d->requests_lock) { + QTAILQ_FOREACH(r, &d->requests, next) { + VirtIOSCSIReq *cmd_req = r->hba_private; + assert(cmd_req); /* request has hba_private while enqueued */ + + if (r->ctx != ctx) { + continue; + } + if (match_tag && cmd_req->req.cmd.tag != tmf->req.tmf.tag) { + continue; + } + virtio_scsi_tmf_cancel_req(tmf, r); + } + } + + /* Incremented by virtio_scsi_do_tmf() */ + virtio_scsi_tmf_dec_remaining(tmf); + + object_unref(d); +} + +static void dummy_bh(void *opaque) +{ + /* Do nothing */ +} + +/* + * Wait for pending virtio_scsi_defer_tmf_to_aio_context() BHs. + */ +static void virtio_scsi_flush_defer_tmf_to_aio_context(VirtIOSCSI *s) +{ + GLOBAL_STATE_CODE(); + + assert(!s->dataplane_started); + + if (s->ctx) { + /* Our BH only runs after previously scheduled BHs */ + aio_wait_bh_oneshot(s->ctx, dummy_bh, NULL); + } +} + +/* + * Run the TMF in a specific AioContext, handling only requests in that + * AioContext. This is necessary because requests can run in different + * AioContext and it is only possible to cancel them from the AioContext where + * they are running. + */ +static void virtio_scsi_defer_tmf_to_aio_context(VirtIOSCSIReq *tmf, + AioContext *ctx) +{ + /* Decremented in virtio_scsi_do_tmf_aio_context() */ + qatomic_inc(&tmf->remaining); + + /* See virtio_scsi_flush_defer_tmf_to_aio_context() cleanup during reset */ + aio_bh_schedule_oneshot(ctx, virtio_scsi_do_tmf_aio_context, tmf); +} + +/* + * Returns the AioContext for a given TMF's tag field or NULL. Note that the + * request identified by the tag may have completed by the time you can execute + * a BH in the AioContext, so don't assume the request still exists in your BH. + */ +static AioContext *find_aio_context_for_tmf_tag(SCSIDevice *d, + VirtIOSCSIReq *tmf) +{ + WITH_QEMU_LOCK_GUARD(&d->requests_lock) { + SCSIRequest *r; + SCSIRequest *next; + + QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) { + VirtIOSCSIReq *cmd_req = r->hba_private; + + /* hba_private is non-NULL while the request is enqueued */ + assert(cmd_req); + + if (cmd_req->req.cmd.tag == tmf->req.tmf.tag) { + return r->ctx; + } + } + } + return NULL; +} + /* Return 0 if the request is ready to be completed and return to guest; * -EINPROGRESS if the request is submitted and will be completed later, in the * case of async cancellation. */ @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) { SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun); SCSIRequest *r, *next; + AioContext *ctx; int ret = 0; virtio_scsi_ctx_check(s, d); @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) req->req.tmf.tag, req->req.tmf.subtype); switch (req->req.tmf.subtype) { - case VIRTIO_SCSI_T_TMF_ABORT_TASK: - case VIRTIO_SCSI_T_TMF_QUERY_TASK: + case VIRTIO_SCSI_T_TMF_ABORT_TASK: { if (!d) { goto fail; } if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) { goto incorrect_lun; } - QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) { - VirtIOSCSIReq *cmd_req = r->hba_private; - if (cmd_req && cmd_req->req.cmd.tag == req->req.tmf.tag) { - break; - } + + ctx = find_aio_context_for_tmf_tag(d, req); + if (ctx) { + virtio_scsi_defer_tmf_to_aio_context(req, ctx); + ret = -EINPROGRESS; } - if (r) { - /* - * Assert that the request has not been completed yet, we - * check for it in the loop above. - */ - assert(r->hba_private); - if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK) { - /* "If the specified command is present in the task set, then - * return a service response set to FUNCTION SUCCEEDED". - */ - req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED; - } else { - VirtIOSCSICancelNotifier *notifier; - - req->remaining = 1; - notifier = g_new(VirtIOSCSICancelNotifier, 1); - notifier->tmf_req = req; - notifier->notifier.notify = virtio_scsi_cancel_notify; - scsi_req_cancel_async(r, &notifier->notifier); - ret = -EINPROGRESS; + break; + } + + case VIRTIO_SCSI_T_TMF_QUERY_TASK: + if (!d) { + goto fail; + } + if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) { + goto incorrect_lun; + } + + WITH_QEMU_LOCK_GUARD(&d->requests_lock) { + QTAILQ_FOREACH(r, &d->requests, next) { + VirtIOSCSIReq *cmd_req = r->hba_private; + assert(cmd_req); /* request has hba_private while enqueued */ + + if (cmd_req->req.cmd.tag == req->req.tmf.tag) { + /* + * "If the specified command is present in the task set, + * then return a service response set to FUNCTION + * SUCCEEDED". + */ + req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED; + } } } break; case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET: case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: - virtio_scsi_defer_tmf_to_bh(req); + virtio_scsi_defer_tmf_to_main_loop(req); ret = -EINPROGRESS; break; case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET: - case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET: + case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET: { + if (!d) { + goto fail; + } + if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) { + goto incorrect_lun; + } + + qatomic_inc(&req->remaining); + + ctx = s->ctx ?: qemu_get_aio_context(); + virtio_scsi_defer_tmf_to_aio_context(req, ctx); + + virtio_scsi_tmf_dec_remaining(req); + ret = -EINPROGRESS; + break; + } + case VIRTIO_SCSI_T_TMF_QUERY_TASK_SET: if (!d) { goto fail; @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) goto incorrect_lun; } - /* Add 1 to "remaining" until virtio_scsi_do_tmf returns. - * This way, if the bus starts calling back to the notifiers - * even before we finish the loop, virtio_scsi_cancel_notify - * will not complete the TMF too early. - */ - req->remaining = 1; - QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) { - if (r->hba_private) { - if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) { - /* "If there is any command present in the task set, then - * return a service response set to FUNCTION SUCCEEDED". - */ - req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED; - break; - } else { - VirtIOSCSICancelNotifier *notifier; - - req->remaining++; - notifier = g_new(VirtIOSCSICancelNotifier, 1); - notifier->notifier.notify = virtio_scsi_cancel_notify; - notifier->tmf_req = req; - scsi_req_cancel_async(r, &notifier->notifier); - } + WITH_QEMU_LOCK_GUARD(&d->requests_lock) { + QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) { + /* Request has hba_private while enqueued */ + assert(r->hba_private); + + /* + * "If there is any command present in the task set, then + * return a service response set to FUNCTION SUCCEEDED". + */ + req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED; + break; } } - if (--req->remaining > 0) { - ret = -EINPROGRESS; - } break; case VIRTIO_SCSI_T_TMF_CLEAR_ACA: @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_reset(VirtIODevice *vdev) assert(!s->dataplane_started); virtio_scsi_reset_tmf_bh(s); + virtio_scsi_flush_defer_tmf_to_aio_context(s); qatomic_inc(&s->resetting); bus_cold_reset(BUS(&s->bus)); -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> This is the cleanup function that must be called after apply_iothread_vq_mapping() succeeds. virtio-scsi will need this function too, so extract it. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-9-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/block/virtio-blk.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -XXX,XX +XXX,XX @@ validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list, * Fill in the AioContext for each virtqueue in the @vq_aio_context array given * the iothread-vq-mapping parameter in @iothread_vq_mapping_list. * + * cleanup_iothread_vq_mapping() must be called to free IOThread object + * references after this function returns success. + * * Returns: %true on success, %false on failure. **/ static bool apply_iothread_vq_mapping( @@ -XXX,XX +XXX,XX @@ static bool apply_iothread_vq_mapping( return true; } +/** + * cleanup_iothread_vq_mapping: + * @list: The mapping of virtqueues to IOThreads. + * + * Release IOThread object references that were acquired by + * apply_iothread_vq_mapping(). + */ +static void cleanup_iothread_vq_mapping(IOThreadVirtQueueMappingList *list) +{ + IOThreadVirtQueueMappingList *node; + + for (node = list; node; node = node->next) { + IOThread *iothread = iothread_by_id(node->value->iothread); + object_unref(OBJECT(iothread)); + } +} + /* Context: BQL held */ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp) { @@ -XXX,XX +XXX,XX @@ static void virtio_blk_vq_aio_context_cleanup(VirtIOBlock *s) assert(!s->ioeventfd_started); if (conf->iothread_vq_mapping_list) { - IOThreadVirtQueueMappingList *node; - - for (node = conf->iothread_vq_mapping_list; node; node = node->next) { - IOThread *iothread = iothread_by_id(node->value->iothread); - object_unref(OBJECT(iothread)); - } + cleanup_iothread_vq_mapping(conf->iothread_vq_mapping_list); } if (conf->iothread) { -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> Use noun_verb() function naming instead of verb_noun() because the former is the most common naming style for APIs. The next commit will move these functions into a header file so that virtio-scsi can call them. Shorten iothread_vq_mapping_apply()'s iothread_vq_mapping_list argument to just "list" like in the other functions. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-10-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/block/virtio-blk.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -XXX,XX +XXX,XX @@ static const BlockDevOps virtio_block_ops = { }; static bool -validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list, - uint16_t num_queues, Error **errp) +iothread_vq_mapping_validate(IOThreadVirtQueueMappingList *list, uint16_t + num_queues, Error **errp) { g_autofree unsigned long *vqs = bitmap_new(num_queues); g_autoptr(GHashTable) iothreads = @@ -XXX,XX +XXX,XX @@ validate_iothread_vq_mapping_list(IOThreadVirtQueueMappingList *list, } /** - * apply_iothread_vq_mapping: - * @iothread_vq_mapping_list: The mapping of virtqueues to IOThreads. + * iothread_vq_mapping_apply: + * @list: The mapping of virtqueues to IOThreads. * @vq_aio_context: The array of AioContext pointers to fill in. * @num_queues: The length of @vq_aio_context. * @errp: If an error occurs, a pointer to the area to store the error. * * Fill in the AioContext for each virtqueue in the @vq_aio_context array given - * the iothread-vq-mapping parameter in @iothread_vq_mapping_list. + * the iothread-vq-mapping parameter in @list. * - * cleanup_iothread_vq_mapping() must be called to free IOThread object + * iothread_vq_mapping_cleanup() must be called to free IOThread object * references after this function returns success. * * Returns: %true on success, %false on failure. **/ -static bool apply_iothread_vq_mapping( - IOThreadVirtQueueMappingList *iothread_vq_mapping_list, +static bool iothread_vq_mapping_apply( + IOThreadVirtQueueMappingList *list, AioContext **vq_aio_context, uint16_t num_queues, Error **errp) @@ -XXX,XX +XXX,XX @@ static bool apply_iothread_vq_mapping( size_t num_iothreads = 0; size_t cur_iothread = 0; - if (!validate_iothread_vq_mapping_list(iothread_vq_mapping_list, - num_queues, errp)) { + if (!iothread_vq_mapping_validate(list, num_queues, errp)) { return false; } - for (node = iothread_vq_mapping_list; node; node = node->next) { + for (node = list; node; node = node->next) { num_iothreads++; } - for (node = iothread_vq_mapping_list; node; node = node->next) { + for (node = list; node; node = node->next) { IOThread *iothread = iothread_by_id(node->value->iothread); AioContext *ctx = iothread_get_aio_context(iothread); @@ -XXX,XX +XXX,XX @@ static bool apply_iothread_vq_mapping( } /** - * cleanup_iothread_vq_mapping: + * iothread_vq_mapping_cleanup: * @list: The mapping of virtqueues to IOThreads. * * Release IOThread object references that were acquired by - * apply_iothread_vq_mapping(). + * iothread_vq_mapping_apply(). */ -static void cleanup_iothread_vq_mapping(IOThreadVirtQueueMappingList *list) +static void iothread_vq_mapping_cleanup(IOThreadVirtQueueMappingList *list) { IOThreadVirtQueueMappingList *node; @@ -XXX,XX +XXX,XX @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp) s->vq_aio_context = g_new(AioContext *, conf->num_queues); if (conf->iothread_vq_mapping_list) { - if (!apply_iothread_vq_mapping(conf->iothread_vq_mapping_list, + if (!iothread_vq_mapping_apply(conf->iothread_vq_mapping_list, s->vq_aio_context, conf->num_queues, errp)) { @@ -XXX,XX +XXX,XX @@ static void virtio_blk_vq_aio_context_cleanup(VirtIOBlock *s) assert(!s->ioeventfd_started); if (conf->iothread_vq_mapping_list) { - cleanup_iothread_vq_mapping(conf->iothread_vq_mapping_list); + iothread_vq_mapping_cleanup(conf->iothread_vq_mapping_list); } if (conf->iothread) { -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> The code that builds an array of AioContext pointers indexed by the virtqueue is not specific to virtio-blk. virtio-scsi will need to do the same thing, so extract the functions. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20250311132616.1049687-11-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/virtio/iothread-vq-mapping.h | 45 ++++++++ hw/block/virtio-blk.c | 142 +----------------------- hw/virtio/iothread-vq-mapping.c | 131 ++++++++++++++++++++++ hw/virtio/meson.build | 1 + 4 files changed, 178 insertions(+), 141 deletions(-) create mode 100644 include/hw/virtio/iothread-vq-mapping.h create mode 100644 hw/virtio/iothread-vq-mapping.c diff --git a/include/hw/virtio/iothread-vq-mapping.h b/include/hw/virtio/iothread-vq-mapping.h new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/include/hw/virtio/iothread-vq-mapping.h @@ -XXX,XX +XXX,XX @@ +/* + * IOThread Virtqueue Mapping + * + * Copyright Red Hat, Inc + * + * SPDX-License-Identifier: GPL-2.0-only + */ + +#ifndef HW_VIRTIO_IOTHREAD_VQ_MAPPING_H +#define HW_VIRTIO_IOTHREAD_VQ_MAPPING_H + +#include "qapi/error.h" +#include "qapi/qapi-types-virtio.h" + +/** + * iothread_vq_mapping_apply: + * @list: The mapping of virtqueues to IOThreads. + * @vq_aio_context: The array of AioContext pointers to fill in. + * @num_queues: The length of @vq_aio_context. + * @errp: If an error occurs, a pointer to the area to store the error. + * + * Fill in the AioContext for each virtqueue in the @vq_aio_context array given + * the iothread-vq-mapping parameter in @list. + * + * iothread_vq_mapping_cleanup() must be called to free IOThread object + * references after this function returns success. + * + * Returns: %true on success, %false on failure. + **/ +bool iothread_vq_mapping_apply( + IOThreadVirtQueueMappingList *list, + AioContext **vq_aio_context, + uint16_t num_queues, + Error **errp); + +/** + * iothread_vq_mapping_cleanup: + * @list: The mapping of virtqueues to IOThreads. + * + * Release IOThread object references that were acquired by + * iothread_vq_mapping_apply(). + */ +void iothread_vq_mapping_cleanup(IOThreadVirtQueueMappingList *list); + +#endif /* HW_VIRTIO_IOTHREAD_VQ_MAPPING_H */ diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -XXX,XX +XXX,XX @@ #endif #include "hw/virtio/virtio-bus.h" #include "migration/qemu-file-types.h" +#include "hw/virtio/iothread-vq-mapping.h" #include "hw/virtio/virtio-access.h" #include "hw/virtio/virtio-blk-common.h" #include "qemu/coroutine.h" @@ -XXX,XX +XXX,XX @@ static const BlockDevOps virtio_block_ops = { .drained_end = virtio_blk_drained_end, }; -static bool -iothread_vq_mapping_validate(IOThreadVirtQueueMappingList *list, uint16_t - num_queues, Error **errp) -{ - g_autofree unsigned long *vqs = bitmap_new(num_queues); - g_autoptr(GHashTable) iothreads = - g_hash_table_new(g_str_hash, g_str_equal); - - for (IOThreadVirtQueueMappingList *node = list; node; node = node->next) { - const char *name = node->value->iothread; - uint16List *vq; - - if (!iothread_by_id(name)) { - error_setg(errp, "IOThread \"%s\" object does not exist", name); - return false; - } - - if (!g_hash_table_add(iothreads, (gpointer)name)) { - error_setg(errp, - "duplicate IOThread name \"%s\" in iothread-vq-mapping", - name); - return false; - } - - if (node != list) { - if (!!node->value->vqs != !!list->value->vqs) { - error_setg(errp, "either all items in iothread-vq-mapping " - "must have vqs or none of them must have it"); - return false; - } - } - - for (vq = node->value->vqs; vq; vq = vq->next) { - if (vq->value >= num_queues) { - error_setg(errp, "vq index %u for IOThread \"%s\" must be " - "less than num_queues %u in iothread-vq-mapping", - vq->value, name, num_queues); - return false; - } - - if (test_and_set_bit(vq->value, vqs)) { - error_setg(errp, "cannot assign vq %u to IOThread \"%s\" " - "because it is already assigned", vq->value, name); - return false; - } - } - } - - if (list->value->vqs) { - for (uint16_t i = 0; i < num_queues; i++) { - if (!test_bit(i, vqs)) { - error_setg(errp, - "missing vq %u IOThread assignment in iothread-vq-mapping", - i); - return false; - } - } - } - - return true; -} - -/** - * iothread_vq_mapping_apply: - * @list: The mapping of virtqueues to IOThreads. - * @vq_aio_context: The array of AioContext pointers to fill in. - * @num_queues: The length of @vq_aio_context. - * @errp: If an error occurs, a pointer to the area to store the error. - * - * Fill in the AioContext for each virtqueue in the @vq_aio_context array given - * the iothread-vq-mapping parameter in @list. - * - * iothread_vq_mapping_cleanup() must be called to free IOThread object - * references after this function returns success. - * - * Returns: %true on success, %false on failure. - **/ -static bool iothread_vq_mapping_apply( - IOThreadVirtQueueMappingList *list, - AioContext **vq_aio_context, - uint16_t num_queues, - Error **errp) -{ - IOThreadVirtQueueMappingList *node; - size_t num_iothreads = 0; - size_t cur_iothread = 0; - - if (!iothread_vq_mapping_validate(list, num_queues, errp)) { - return false; - } - - for (node = list; node; node = node->next) { - num_iothreads++; - } - - for (node = list; node; node = node->next) { - IOThread *iothread = iothread_by_id(node->value->iothread); - AioContext *ctx = iothread_get_aio_context(iothread); - - /* Released in virtio_blk_vq_aio_context_cleanup() */ - object_ref(OBJECT(iothread)); - - if (node->value->vqs) { - uint16List *vq; - - /* Explicit vq:IOThread assignment */ - for (vq = node->value->vqs; vq; vq = vq->next) { - assert(vq->value < num_queues); - vq_aio_context[vq->value] = ctx; - } - } else { - /* Round-robin vq:IOThread assignment */ - for (unsigned i = cur_iothread; i < num_queues; - i += num_iothreads) { - vq_aio_context[i] = ctx; - } - } - - cur_iothread++; - } - - return true; -} - -/** - * iothread_vq_mapping_cleanup: - * @list: The mapping of virtqueues to IOThreads. - * - * Release IOThread object references that were acquired by - * iothread_vq_mapping_apply(). - */ -static void iothread_vq_mapping_cleanup(IOThreadVirtQueueMappingList *list) -{ - IOThreadVirtQueueMappingList *node; - - for (node = list; node; node = node->next) { - IOThread *iothread = iothread_by_id(node->value->iothread); - object_unref(OBJECT(iothread)); - } -} - /* Context: BQL held */ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp) { diff --git a/hw/virtio/iothread-vq-mapping.c b/hw/virtio/iothread-vq-mapping.c new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/hw/virtio/iothread-vq-mapping.c @@ -XXX,XX +XXX,XX @@ +/* + * IOThread Virtqueue Mapping + * + * Copyright Red Hat, Inc + * + * SPDX-License-Identifier: GPL-2.0-only + */ + +#include "qemu/osdep.h" +#include "system/iothread.h" +#include "hw/virtio/iothread-vq-mapping.h" + +static bool +iothread_vq_mapping_validate(IOThreadVirtQueueMappingList *list, uint16_t + num_queues, Error **errp) +{ + g_autofree unsigned long *vqs = bitmap_new(num_queues); + g_autoptr(GHashTable) iothreads = + g_hash_table_new(g_str_hash, g_str_equal); + + for (IOThreadVirtQueueMappingList *node = list; node; node = node->next) { + const char *name = node->value->iothread; + uint16List *vq; + + if (!iothread_by_id(name)) { + error_setg(errp, "IOThread \"%s\" object does not exist", name); + return false; + } + + if (!g_hash_table_add(iothreads, (gpointer)name)) { + error_setg(errp, + "duplicate IOThread name \"%s\" in iothread-vq-mapping", + name); + return false; + } + + if (node != list) { + if (!!node->value->vqs != !!list->value->vqs) { + error_setg(errp, "either all items in iothread-vq-mapping " + "must have vqs or none of them must have it"); + return false; + } + } + + for (vq = node->value->vqs; vq; vq = vq->next) { + if (vq->value >= num_queues) { + error_setg(errp, "vq index %u for IOThread \"%s\" must be " + "less than num_queues %u in iothread-vq-mapping", + vq->value, name, num_queues); + return false; + } + + if (test_and_set_bit(vq->value, vqs)) { + error_setg(errp, "cannot assign vq %u to IOThread \"%s\" " + "because it is already assigned", vq->value, name); + return false; + } + } + } + + if (list->value->vqs) { + for (uint16_t i = 0; i < num_queues; i++) { + if (!test_bit(i, vqs)) { + error_setg(errp, + "missing vq %u IOThread assignment in iothread-vq-mapping", + i); + return false; + } + } + } + + return true; +} + +bool iothread_vq_mapping_apply( + IOThreadVirtQueueMappingList *list, + AioContext **vq_aio_context, + uint16_t num_queues, + Error **errp) +{ + IOThreadVirtQueueMappingList *node; + size_t num_iothreads = 0; + size_t cur_iothread = 0; + + if (!iothread_vq_mapping_validate(list, num_queues, errp)) { + return false; + } + + for (node = list; node; node = node->next) { + num_iothreads++; + } + + for (node = list; node; node = node->next) { + IOThread *iothread = iothread_by_id(node->value->iothread); + AioContext *ctx = iothread_get_aio_context(iothread); + + /* Released in virtio_blk_vq_aio_context_cleanup() */ + object_ref(OBJECT(iothread)); + + if (node->value->vqs) { + uint16List *vq; + + /* Explicit vq:IOThread assignment */ + for (vq = node->value->vqs; vq; vq = vq->next) { + assert(vq->value < num_queues); + vq_aio_context[vq->value] = ctx; + } + } else { + /* Round-robin vq:IOThread assignment */ + for (unsigned i = cur_iothread; i < num_queues; + i += num_iothreads) { + vq_aio_context[i] = ctx; + } + } + + cur_iothread++; + } + + return true; +} + +void iothread_vq_mapping_cleanup(IOThreadVirtQueueMappingList *list) +{ + IOThreadVirtQueueMappingList *node; + + for (node = list; node; node = node->next) { + IOThread *iothread = iothread_by_id(node->value->iothread); + object_unref(OBJECT(iothread)); + } +} + diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build index XXXXXXX..XXXXXXX 100644 --- a/hw/virtio/meson.build +++ b/hw/virtio/meson.build @@ -XXX,XX +XXX,XX @@ system_virtio_ss = ss.source_set() system_virtio_ss.add(files('virtio-bus.c')) +system_virtio_ss.add(files('iothread-vq-mapping.c')) system_virtio_ss.add(when: 'CONFIG_VIRTIO_PCI', if_true: files('virtio-pci.c')) system_virtio_ss.add(when: 'CONFIG_VIRTIO_MMIO', if_true: files('virtio-mmio.c')) system_virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-crypto.c')) -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> Allow virtio-scsi virtqueues to be assigned to different IOThreads. This makes it possible to take advantage of host multi-queue block layer scalability by assigning virtqueues that have affinity with vCPUs to different IOThreads that have affinity with host CPUs. The same feature was introduced for virtio-blk in the past: https://developers.redhat.com/articles/2024/09/05/scaling-virtio-blk-disk-io-iothread-virtqueue-mapping Here are fio randread 4k iodepth=64 results from a 4 vCPU guest with an Intel P4800X SSD: iothreads IOPS ------------------------------ 1 189576 2 312698 4 346744 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-ID: <20250311132616.1049687-12-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/virtio/virtio-scsi.h | 5 +- hw/scsi/virtio-scsi-dataplane.c | 90 ++++++++++++++++++++++++--------- hw/scsi/virtio-scsi.c | 63 ++++++++++++++--------- 3 files changed, 107 insertions(+), 51 deletions(-) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -XXX,XX +XXX,XX @@ #include "hw/virtio/virtio.h" #include "hw/scsi/scsi.h" #include "chardev/char-fe.h" +#include "qapi/qapi-types-virtio.h" #include "system/iothread.h" #define TYPE_VIRTIO_SCSI_COMMON "virtio-scsi-common" @@ -XXX,XX +XXX,XX @@ struct VirtIOSCSIConf { CharBackend chardev; uint32_t boot_tpgt; IOThread *iothread; + IOThreadVirtQueueMappingList *iothread_vq_mapping_list; }; struct VirtIOSCSI; @@ -XXX,XX +XXX,XX @@ struct VirtIOSCSI { QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list; /* Fields for dataplane below */ - AioContext *ctx; /* one iothread per virtio-scsi-pci for now */ + AioContext **vq_aio_context; /* per-virtqueue AioContext pointer */ bool dataplane_started; bool dataplane_starting; @@ -XXX,XX +XXX,XX @@ void virtio_scsi_common_realize(DeviceState *dev, void virtio_scsi_common_unrealize(DeviceState *dev); void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp); +void virtio_scsi_dataplane_cleanup(VirtIOSCSI *s); int virtio_scsi_dataplane_start(VirtIODevice *s); void virtio_scsi_dataplane_stop(VirtIODevice *s); diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -XXX,XX +XXX,XX @@ #include "system/block-backend.h" #include "hw/scsi/scsi.h" #include "scsi/constants.h" +#include "hw/virtio/iothread-vq-mapping.h" #include "hw/virtio/virtio-bus.h" /* Context: BQL held */ @@ -XXX,XX +XXX,XX @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp) VirtIODevice *vdev = VIRTIO_DEVICE(s); BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); + uint16_t num_vqs = vs->conf.num_queues + VIRTIO_SCSI_VQ_NUM_FIXED; - if (vs->conf.iothread) { + if (vs->conf.iothread && vs->conf.iothread_vq_mapping_list) { + error_setg(errp, + "iothread and iothread-vq-mapping properties cannot be set " + "at the same time"); + return; + } + + if (vs->conf.iothread || vs->conf.iothread_vq_mapping_list) { if (!k->set_guest_notifiers || !k->ioeventfd_assign) { error_setg(errp, "device is incompatible with iothread " @@ -XXX,XX +XXX,XX @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp) error_setg(errp, "ioeventfd is required for iothread"); return; } - s->ctx = iothread_get_aio_context(vs->conf.iothread); - } else { - if (!virtio_device_ioeventfd_enabled(vdev)) { + } + + s->vq_aio_context = g_new(AioContext *, num_vqs); + + if (vs->conf.iothread_vq_mapping_list) { + if (!iothread_vq_mapping_apply(vs->conf.iothread_vq_mapping_list, + s->vq_aio_context, num_vqs, errp)) { + g_free(s->vq_aio_context); + s->vq_aio_context = NULL; return; } - s->ctx = qemu_get_aio_context(); + } else if (vs->conf.iothread) { + AioContext *ctx = iothread_get_aio_context(vs->conf.iothread); + for (uint16_t i = 0; i < num_vqs; i++) { + s->vq_aio_context[i] = ctx; + } + + /* Released in virtio_scsi_dataplane_cleanup() */ + object_ref(OBJECT(vs->conf.iothread)); + } else { + AioContext *ctx = qemu_get_aio_context(); + for (unsigned i = 0; i < num_vqs; i++) { + s->vq_aio_context[i] = ctx; + } + } +} + +/* Context: BQL held */ +void virtio_scsi_dataplane_cleanup(VirtIOSCSI *s) +{ + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); + + if (vs->conf.iothread_vq_mapping_list) { + iothread_vq_mapping_cleanup(vs->conf.iothread_vq_mapping_list); } + + if (vs->conf.iothread) { + object_unref(OBJECT(vs->conf.iothread)); + } + + g_free(s->vq_aio_context); + s->vq_aio_context = NULL; } static int virtio_scsi_set_host_notifier(VirtIOSCSI *s, VirtQueue *vq, int n) @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_set_host_notifier(VirtIOSCSI *s, VirtQueue *vq, int n) } /* Context: BH in IOThread */ -static void virtio_scsi_dataplane_stop_bh(void *opaque) +static void virtio_scsi_dataplane_stop_vq_bh(void *opaque) { - VirtIOSCSI *s = opaque; - VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); + AioContext *ctx = qemu_get_current_aio_context(); + VirtQueue *vq = opaque; EventNotifier *host_notifier; - int i; - virtio_queue_aio_detach_host_notifier(vs->ctrl_vq, s->ctx); - host_notifier = virtio_queue_get_host_notifier(vs->ctrl_vq); + virtio_queue_aio_detach_host_notifier(vq, ctx); + host_notifier = virtio_queue_get_host_notifier(vq); /* * Test and clear notifier after disabling event, in case poll callback * didn't have time to run. */ virtio_queue_host_notifier_read(host_notifier); - - virtio_queue_aio_detach_host_notifier(vs->event_vq, s->ctx); - host_notifier = virtio_queue_get_host_notifier(vs->event_vq); - virtio_queue_host_notifier_read(host_notifier); - - for (i = 0; i < vs->conf.num_queues; i++) { - virtio_queue_aio_detach_host_notifier(vs->cmd_vqs[i], s->ctx); - host_notifier = virtio_queue_get_host_notifier(vs->cmd_vqs[i]); - virtio_queue_host_notifier_read(host_notifier); - } } /* Context: BQL held */ @@ -XXX,XX +XXX,XX @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev) smp_wmb(); /* paired with aio_notify_accept() */ if (s->bus.drain_count == 0) { - virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx); - virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx); + virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, + s->vq_aio_context[0]); + virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, + s->vq_aio_context[1]); for (i = 0; i < vs->conf.num_queues; i++) { - virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx); + AioContext *ctx = s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED + i]; + virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], ctx); } } return 0; @@ -XXX,XX +XXX,XX @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev) s->dataplane_stopping = true; if (s->bus.drain_count == 0) { - aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s); + for (i = 0; i < vs->conf.num_queues + VIRTIO_SCSI_VQ_NUM_FIXED; i++) { + VirtQueue *vq = virtio_get_queue(&vs->parent_obj, i); + AioContext *ctx = s->vq_aio_context[i]; + aio_wait_bh_oneshot(ctx, virtio_scsi_dataplane_stop_vq_bh, vq); + } } blk_drain_all(); /* ensure there are no in-flight requests */ diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -XXX,XX +XXX,XX @@ #include "hw/qdev-properties.h" #include "hw/scsi/scsi.h" #include "scsi/constants.h" +#include "hw/virtio/iothread-vq-mapping.h" #include "hw/virtio/virtio-bus.h" #include "hw/virtio/virtio-access.h" #include "trace.h" @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_cancel_notify(Notifier *notifier, void *data) g_free(n); } -static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d) -{ - if (s->dataplane_started && d && blk_is_available(d->conf.blk)) { - assert(blk_get_aio_context(d->conf.blk) == s->ctx); - } -} - static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req) { VirtIOSCSI *s = req->dev; @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_flush_defer_tmf_to_aio_context(VirtIOSCSI *s) assert(!s->dataplane_started); - if (s->ctx) { + for (uint32_t i = 0; i < s->parent_obj.conf.num_queues; i++) { + AioContext *ctx = s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED + i]; + /* Our BH only runs after previously scheduled BHs */ - aio_wait_bh_oneshot(s->ctx, dummy_bh, NULL); + aio_wait_bh_oneshot(ctx, dummy_bh, NULL); } } @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) AioContext *ctx; int ret = 0; - virtio_scsi_ctx_check(s, d); /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE". */ req->resp.tmf.response = VIRTIO_SCSI_S_OK; @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET: case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET: { + g_autoptr(GHashTable) aio_contexts = g_hash_table_new(NULL, NULL); + if (!d) { goto fail; } @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) qatomic_inc(&req->remaining); - ctx = s->ctx ?: qemu_get_aio_context(); - virtio_scsi_defer_tmf_to_aio_context(req, ctx); + for (uint32_t i = 0; i < s->parent_obj.conf.num_queues; i++) { + ctx = s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED + i]; + + if (!g_hash_table_add(aio_contexts, ctx)) { + continue; /* skip previously added AioContext */ + } + + virtio_scsi_defer_tmf_to_aio_context(req, ctx); + } virtio_scsi_tmf_dec_remaining(req); ret = -EINPROGRESS; @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq) */ static bool virtio_scsi_defer_to_dataplane(VirtIOSCSI *s) { - if (!s->ctx || s->dataplane_started) { + if (s->dataplane_started) { return false; } + if (s->vq_aio_context[0] == qemu_get_aio_context()) { + return false; /* not using IOThreads */ + } virtio_device_start_ioeventfd(&s->parent_obj.parent_obj); return !s->dataplane_fenced; @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req) virtio_scsi_complete_cmd_req(req); return -ENOENT; } - virtio_scsi_ctx_check(s, d); req->sreq = scsi_req_new(d, req->req.cmd.tag, virtio_scsi_get_lun(req->req.cmd.lun), req->req.cmd.cdb, vs->cdb_size, req); @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, { VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev); VirtIOSCSI *s = VIRTIO_SCSI(vdev); + AioContext *ctx = s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED]; SCSIDevice *sd = SCSI_DEVICE(dev); - int ret; - if (s->ctx && !s->dataplane_fenced) { - ret = blk_set_aio_context(sd->conf.blk, s->ctx, errp); - if (ret < 0) { - return; - } + if (ctx != qemu_get_aio_context() && !s->dataplane_fenced) { + /* + * Try to make the BlockBackend's AioContext match ours. Ignore failure + * because I/O will still work although block jobs and other users + * might be slower when multiple AioContexts use a BlockBackend. + */ + blk_set_aio_context(sd->conf.blk, ctx, errp); } if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev, qdev_simple_device_unplug_cb(hotplug_dev, dev, errp); - if (s->ctx) { + if (s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED] != qemu_get_aio_context()) { /* If other users keep the BlockBackend in the iothread, that's ok */ blk_set_aio_context(sd->conf.blk, qemu_get_aio_context(), NULL); } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_drained_begin(SCSIBus *bus) for (uint32_t i = 0; i < total_queues; i++) { VirtQueue *vq = virtio_get_queue(vdev, i); - virtio_queue_aio_detach_host_notifier(vq, s->ctx); + virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]); } } @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_drained_end(SCSIBus *bus) for (uint32_t i = 0; i < total_queues; i++) { VirtQueue *vq = virtio_get_queue(vdev, i); + AioContext *ctx = s->vq_aio_context[i]; + if (vq == vs->event_vq) { - virtio_queue_aio_attach_host_notifier_no_poll(vq, s->ctx); + virtio_queue_aio_attach_host_notifier_no_poll(vq, ctx); } else { - virtio_queue_aio_attach_host_notifier(vq, s->ctx); + virtio_queue_aio_attach_host_notifier(vq, ctx); } } } @@ -XXX,XX +XXX,XX @@ void virtio_scsi_common_unrealize(DeviceState *dev) virtio_cleanup(vdev); } +/* main loop */ static void virtio_scsi_device_unrealize(DeviceState *dev) { VirtIOSCSI *s = VIRTIO_SCSI(dev); virtio_scsi_reset_tmf_bh(s); - + virtio_scsi_dataplane_cleanup(s); qbus_set_hotplug_handler(BUS(&s->bus), NULL); virtio_scsi_common_unrealize(dev); qemu_mutex_destroy(&s->tmf_bh_lock); @@ -XXX,XX +XXX,XX @@ static const Property virtio_scsi_properties[] = { VIRTIO_SCSI_F_CHANGE, true), DEFINE_PROP_LINK("iothread", VirtIOSCSI, parent_obj.conf.iothread, TYPE_IOTHREAD, IOThread *), + DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST("iothread-vq-mapping", VirtIOSCSI, + parent_obj.conf.iothread_vq_mapping_list), }; static const VMStateDescription vmstate_virtio_scsi = { -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> Previously the ctrl virtqueue was handled in the AioContext where SCSI requests are processed. When IOThread Virtqueue Mapping was added things become more complicated because SCSI requests could run in other AioContexts. Simplify by handling the ctrl virtqueue in the main loop where reset operations can be performed. Note that BHs are still used canceling SCSI requests in their AioContexts but at least the mean loop activity doesn't need BHs anymore. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-ID: <20250311132616.1049687-13-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/virtio/virtio-scsi.h | 8 -- hw/scsi/virtio-scsi-dataplane.c | 6 ++ hw/scsi/virtio-scsi.c | 144 ++++++-------------------------- 3 files changed, 33 insertions(+), 125 deletions(-) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -XXX,XX +XXX,XX @@ struct VirtIOSCSI { QemuMutex ctrl_lock; /* protects ctrl_vq */ - /* - * TMFs deferred to main loop BH. These fields are protected by - * tmf_bh_lock. - */ - QemuMutex tmf_bh_lock; - QEMUBH *tmf_bh; - QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list; - /* Fields for dataplane below */ AioContext **vq_aio_context; /* per-virtqueue AioContext pointer */ diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -XXX,XX +XXX,XX @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp) s->vq_aio_context[i] = ctx; } } + + /* + * Always handle the ctrl virtqueue in the main loop thread where device + * resets can be performed. + */ + s->vq_aio_context[0] = qemu_get_aio_context(); } /* Context: BQL held */ diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_cancel_notify(Notifier *notifier, void *data) g_free(n); } -static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req) -{ - VirtIOSCSI *s = req->dev; - SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun); - BusChild *kid; - int target; - - switch (req->req.tmf.subtype) { - case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET: - if (!d) { - req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET; - goto out; - } - if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) { - req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN; - goto out; - } - qatomic_inc(&s->resetting); - device_cold_reset(&d->qdev); - qatomic_dec(&s->resetting); - break; - - case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: - target = req->req.tmf.lun[1]; - qatomic_inc(&s->resetting); - - rcu_read_lock(); - QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) { - SCSIDevice *d1 = SCSI_DEVICE(kid->child); - if (d1->channel == 0 && d1->id == target) { - device_cold_reset(&d1->qdev); - } - } - rcu_read_unlock(); - - qatomic_dec(&s->resetting); - break; - - default: - g_assert_not_reached(); - } - -out: - object_unref(OBJECT(d)); - virtio_scsi_complete_req(req, &s->ctrl_lock); -} - -/* Some TMFs must be processed from the main loop thread */ -static void virtio_scsi_do_tmf_bh(void *opaque) -{ - VirtIOSCSI *s = opaque; - QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs); - VirtIOSCSIReq *req; - VirtIOSCSIReq *tmp; - - GLOBAL_STATE_CODE(); - - WITH_QEMU_LOCK_GUARD(&s->tmf_bh_lock) { - QTAILQ_FOREACH_SAFE(req, &s->tmf_bh_list, next, tmp) { - QTAILQ_REMOVE(&s->tmf_bh_list, req, next); - QTAILQ_INSERT_TAIL(&reqs, req, next); - } - - qemu_bh_delete(s->tmf_bh); - s->tmf_bh = NULL; - } - - QTAILQ_FOREACH_SAFE(req, &reqs, next, tmp) { - QTAILQ_REMOVE(&reqs, req, next); - virtio_scsi_do_one_tmf_bh(req); - } -} - -static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s) -{ - VirtIOSCSIReq *req; - VirtIOSCSIReq *tmp; - - GLOBAL_STATE_CODE(); - - /* Called after ioeventfd has been stopped, so tmf_bh_lock is not needed */ - if (s->tmf_bh) { - qemu_bh_delete(s->tmf_bh); - s->tmf_bh = NULL; - } - - QTAILQ_FOREACH_SAFE(req, &s->tmf_bh_list, next, tmp) { - QTAILQ_REMOVE(&s->tmf_bh_list, req, next); - - /* SAM-6 6.3.2 Hard reset */ - req->resp.tmf.response = VIRTIO_SCSI_S_TARGET_FAILURE; - virtio_scsi_complete_req(req, &req->dev->ctrl_lock); - } -} - -static void virtio_scsi_defer_tmf_to_main_loop(VirtIOSCSIReq *req) -{ - VirtIOSCSI *s = req->dev; - - WITH_QEMU_LOCK_GUARD(&s->tmf_bh_lock) { - QTAILQ_INSERT_TAIL(&s->tmf_bh_list, req, next); - - if (!s->tmf_bh) { - s->tmf_bh = qemu_bh_new(virtio_scsi_do_tmf_bh, s); - qemu_bh_schedule(s->tmf_bh); - } - } -} - static void virtio_scsi_tmf_cancel_req(VirtIOSCSIReq *tmf, SCSIRequest *r) { VirtIOSCSICancelNotifier *notifier; @@ -XXX,XX +XXX,XX @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) break; case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET: - case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: - virtio_scsi_defer_tmf_to_main_loop(req); - ret = -EINPROGRESS; + if (!d) { + goto fail; + } + if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) { + goto incorrect_lun; + } + qatomic_inc(&s->resetting); + device_cold_reset(&d->qdev); + qatomic_dec(&s->resetting); break; + case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: { + BusChild *kid; + int target = req->req.tmf.lun[1]; + qatomic_inc(&s->resetting); + + rcu_read_lock(); + QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) { + SCSIDevice *d1 = SCSI_DEVICE(kid->child); + if (d1->channel == 0 && d1->id == target) { + device_cold_reset(&d1->qdev); + } + } + rcu_read_unlock(); + + qatomic_dec(&s->resetting); + break; + } + case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET: case VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET: { g_autoptr(GHashTable) aio_contexts = g_hash_table_new(NULL, NULL); @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_reset(VirtIODevice *vdev) assert(!s->dataplane_started); - virtio_scsi_reset_tmf_bh(s); virtio_scsi_flush_defer_tmf_to_aio_context(s); qatomic_inc(&s->resetting); @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp) VirtIOSCSI *s = VIRTIO_SCSI(dev); Error *err = NULL; - QTAILQ_INIT(&s->tmf_bh_list); qemu_mutex_init(&s->ctrl_lock); qemu_mutex_init(&s->event_lock); - qemu_mutex_init(&s->tmf_bh_lock); virtio_scsi_common_realize(dev, virtio_scsi_handle_ctrl, @@ -XXX,XX +XXX,XX @@ static void virtio_scsi_device_unrealize(DeviceState *dev) { VirtIOSCSI *s = VIRTIO_SCSI(dev); - virtio_scsi_reset_tmf_bh(s); virtio_scsi_dataplane_cleanup(s); qbus_set_hotplug_handler(BUS(&s->bus), NULL); virtio_scsi_common_unrealize(dev); - qemu_mutex_destroy(&s->tmf_bh_lock); qemu_mutex_destroy(&s->event_lock); qemu_mutex_destroy(&s->ctrl_lock); } -- 2.48.1 From: Stefan Hajnoczi <stefanha@redhat.com> Peter Krempa and Kevin Wolf observed that iothread-vq-mapping is confusing to use because the control and event virtqueues have a fixed location before the command virtqueues but need to be treated differently. Only expose the command virtqueues via iothread-vq-mapping so that the command-line parameter is intuitive: it controls where SCSI requests are processed. The control virtqueue needs to be hardcoded to the main loop thread for technical reasons anyway. Kevin also pointed out that it's better to place the event virtqueue in the main loop thread since its no poll behavior would prevent polling if assigned to an IOThread. This change is its own commit to avoid squashing the previous commit. Suggested-by: Kevin Wolf <kwolf@redhat.com> Suggested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-ID: <20250311132616.1049687-14-stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/scsi/virtio-scsi-dataplane.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -XXX,XX +XXX,XX @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp) VirtIODevice *vdev = VIRTIO_DEVICE(s); BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); - uint16_t num_vqs = vs->conf.num_queues + VIRTIO_SCSI_VQ_NUM_FIXED; if (vs->conf.iothread && vs->conf.iothread_vq_mapping_list) { error_setg(errp, @@ -XXX,XX +XXX,XX @@ void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp) } } - s->vq_aio_context = g_new(AioContext *, num_vqs); + s->vq_aio_context = g_new(AioContext *, vs->conf.num_queues + + VIRTIO_SCSI_VQ_NUM_FIXED); + + /* + * Handle the ctrl virtqueue in the main loop thread where device resets + * can be performed. + */ + s->vq_aio_context[0] = qemu_get_aio_context(); + + /* + * Handle the event virtqueue in the main loop thread where its no_poll + * behavior won't stop IOThread polling. + */ + s->vq_aio_context[1] = qemu_get_aio_context(); if (vs->conf.iothread_vq_mapping_list) { if (!iothread_vq_mapping_apply(vs->conf.iothread_vq_mapping_list, - s->vq_aio_context, num_vqs, errp)) { + &s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED], + vs->conf.num_queues, errp)) { g_free(s->vq_aio_context); s->vq_aio_context = NULL; return; } } else if (vs->conf.iothread) { AioContext *ctx = iothread_get_aio_context(vs->conf.iothread); - for (uint16_t i = 0; i < num_vqs; i++) { - s->vq_aio_context[i] = ctx; + for (uint16_t i = 0; i < vs->conf.num_queues; i++) { + s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED + i] = ctx; } /* Released in virtio_scsi_dataplane_cleanup() */ object_ref(OBJECT(vs->conf.iothread)); } else { AioContext *ctx = qemu_get_aio_context(); - for (unsigned i = 0; i < num_vqs; i++) { - s->vq_aio_context[i] = ctx; + for (unsigned i = 0; i < vs->conf.num_queues; i++) { + s->vq_aio_context[VIRTIO_SCSI_VQ_NUM_FIXED + i] = ctx; } } - - /* - * Always handle the ctrl virtqueue in the main loop thread where device - * resets can be performed. - */ - s->vq_aio_context[0] = qemu_get_aio_context(); } /* Context: BQL held */ -- 2.48.1