The following changes since commit 9e06029aea3b2eca1d5261352e695edc1e7d7b8b: Update version for v4.1.0 release (2019-08-15 13:03:37 +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 a6b257a08e3d72219f03e461a52152672fec0612: file-posix: Handle undetectable alignment (2019-08-16 11:29:11 +0200) ---------------------------------------------------------------- Block layer patches: - file-posix: Fix O_DIRECT alignment detection - Fixes for concurrent block jobs - block-backend: Queue requests while drained (fix IDE vs. job crashes) - qemu-img convert: Deprecate using -n and -o together - iotests: Migration tests with filter nodes - iotests: More media change tests ---------------------------------------------------------------- Kevin Wolf (10): iotests/118: Test media change for scsi-cd iotests/118: Create test classes dynamically iotests/118: Add -blockdev based tests iotests: Move migration helpers to iotests.py iotests: Test migration with all kinds of filter nodes block: Simplify bdrv_filter_default_perms() block: Remove blk_pread_unthrottled() mirror: Keep mirror_top_bs drained after dropping permissions block-backend: Queue requests while drained qemu-img convert: Deprecate using -n and -o together Max Reitz (5): block: Keep subtree drained in drop_intermediate block: Reduce (un)drains when replacing a child tests: Test polling in bdrv_drop_intermediate() tests: Test mid-drain bdrv_replace_child_noperm() iotests: Add test for concurrent stream/commit Nir Soffer (1): file-posix: Handle undetectable alignment include/sysemu/block-backend.h | 3 +- block.c | 63 +++--- block/backup.c | 1 + block/block-backend.c | 69 ++++-- block/commit.c | 2 + block/file-posix.c | 36 +++- block/mirror.c | 7 +- blockjob.c | 3 + hw/block/hd-geometry.c | 7 +- qemu-img.c | 5 + tests/test-bdrv-drain.c | 476 +++++++++++++++++++++++++++++++++++++++++ qemu-deprecated.texi | 7 + tests/qemu-iotests/118 | 84 ++++---- tests/qemu-iotests/118.out | 4 +- tests/qemu-iotests/234 | 30 +-- tests/qemu-iotests/258 | 163 ++++++++++++++ tests/qemu-iotests/258.out | 33 +++ tests/qemu-iotests/262 | 82 +++++++ tests/qemu-iotests/262.out | 17 ++ tests/qemu-iotests/group | 2 + tests/qemu-iotests/iotests.py | 16 ++ 21 files changed, 983 insertions(+), 127 deletions(-) create mode 100755 tests/qemu-iotests/258 create mode 100644 tests/qemu-iotests/258.out create mode 100755 tests/qemu-iotests/262 create mode 100644 tests/qemu-iotests/262.out The test covered only floppy and ide-cd. Add scsi-cd as well. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/118 | 20 ++++++++++++++++++++ tests/qemu-iotests/118.out | 4 ++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/118 +++ b/tests/qemu-iotests/118 @@ -XXX,XX +XXX,XX @@ def interface_to_device_name(interface): return 'ide-cd' elif interface == 'floppy': return 'floppy' + elif interface == 'scsi': + return 'scsi-cd' else: return None @@ -XXX,XX +XXX,XX @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass): qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k') self.vm = iotests.VM() self.vm.add_drive(old_img, 'media=%s' % media, 'none') + if interface == 'scsi': + self.vm.add_device('virtio-scsi-pci') self.vm.add_device('%s,drive=drive0,id=%s' % (interface_to_device_name(interface), self.device_name)) @@ -XXX,XX +XXX,XX @@ class TestInitiallyEmpty(GeneralChangeTestsBaseClass): def setUp(self, media, interface): qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k') self.vm = iotests.VM().add_drive(None, 'media=%s' % media, 'none') + if interface == 'scsi': + self.vm.add_device('virtio-scsi-pci') self.vm.add_device('%s,drive=drive0,id=%s' % (interface_to_device_name(interface), self.device_name)) @@ -XXX,XX +XXX,XX @@ class TestCDInitiallyEmpty(TestInitiallyEmpty): def setUp(self): self.TestInitiallyEmpty.setUp(self, 'cdrom', 'ide') +class TestSCSICDInitiallyFilled(TestInitiallyFilled): + TestInitiallyFilled = TestInitiallyFilled + has_real_tray = True + + def setUp(self): + self.TestInitiallyFilled.setUp(self, 'cdrom', 'scsi') + +class TestSCSICDInitiallyEmpty(TestInitiallyEmpty): + TestInitiallyEmpty = TestInitiallyEmpty + has_real_tray = True + + def setUp(self): + self.TestInitiallyEmpty.setUp(self, 'cdrom', 'scsi') + class TestFloppyInitiallyFilled(TestInitiallyFilled): TestInitiallyFilled = TestInitiallyFilled has_real_tray = False diff --git a/tests/qemu-iotests/118.out b/tests/qemu-iotests/118.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/118.out +++ b/tests/qemu-iotests/118.out @@ -XXX,XX +XXX,XX @@ -............................................................... +......................................................................................... ---------------------------------------------------------------------- -Ran 63 tests +Ran 89 tests OK -- 2.20.1 We're getting a ridiculous number of child classes of TestInitiallyFilled and TestInitiallyEmpty that differ only in a few attributes that we want to test in all combinations. Instead of explicitly writing down every combination, let's use a loop and create those classes dynamically. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/118 | 69 +++++++++++++----------------------------- 1 file changed, 21 insertions(+), 48 deletions(-) diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/118 +++ b/tests/qemu-iotests/118 @@ -XXX,XX +XXX,XX @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): class TestInitiallyFilled(GeneralChangeTestsBaseClass): was_empty = False - def setUp(self, media, interface): + def setUp(self): qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k') qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k') self.vm = iotests.VM() - self.vm.add_drive(old_img, 'media=%s' % media, 'none') - if interface == 'scsi': + self.vm.add_drive(old_img, 'media=%s' % self.media, 'none') + if self.interface == 'scsi': self.vm.add_device('virtio-scsi-pci') self.vm.add_device('%s,drive=drive0,id=%s' % - (interface_to_device_name(interface), + (interface_to_device_name(self.interface), self.device_name)) self.vm.launch() @@ -XXX,XX +XXX,XX @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass): class TestInitiallyEmpty(GeneralChangeTestsBaseClass): was_empty = True - def setUp(self, media, interface): + def setUp(self): qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k') - self.vm = iotests.VM().add_drive(None, 'media=%s' % media, 'none') - if interface == 'scsi': + self.vm = iotests.VM().add_drive(None, 'media=%s' % self.media, 'none') + if self.interface == 'scsi': self.vm.add_device('virtio-scsi-pci') self.vm.add_device('%s,drive=drive0,id=%s' % - (interface_to_device_name(interface), + (interface_to_device_name(self.interface), self.device_name)) self.vm.launch() @@ -XXX,XX +XXX,XX @@ class TestInitiallyEmpty(GeneralChangeTestsBaseClass): # Should be a no-op self.assert_qmp(result, 'return', {}) -class TestCDInitiallyFilled(TestInitiallyFilled): - TestInitiallyFilled = TestInitiallyFilled - has_real_tray = True - - def setUp(self): - self.TestInitiallyFilled.setUp(self, 'cdrom', 'ide') - -class TestCDInitiallyEmpty(TestInitiallyEmpty): - TestInitiallyEmpty = TestInitiallyEmpty - has_real_tray = True - - def setUp(self): - self.TestInitiallyEmpty.setUp(self, 'cdrom', 'ide') +# Do this in a function to avoid leaking variables like case into the global +# name space (otherwise tests would be run for the abstract base classes) +def create_basic_test_classes(): + for (media, interface, has_real_tray) in [ ('cdrom', 'ide', True), + ('cdrom', 'scsi', True), + ('disk', 'floppy', False) ]: -class TestSCSICDInitiallyFilled(TestInitiallyFilled): - TestInitiallyFilled = TestInitiallyFilled - has_real_tray = True + for case in [ TestInitiallyFilled, TestInitiallyEmpty ]: - def setUp(self): - self.TestInitiallyFilled.setUp(self, 'cdrom', 'scsi') + attr = { 'media': media, + 'interface': interface, + 'has_real_tray': has_real_tray } -class TestSCSICDInitiallyEmpty(TestInitiallyEmpty): - TestInitiallyEmpty = TestInitiallyEmpty - has_real_tray = True + name = '%s_%s_%s' % (case.__name__, media, interface) + globals()[name] = type(name, (case, ), attr) - def setUp(self): - self.TestInitiallyEmpty.setUp(self, 'cdrom', 'scsi') - -class TestFloppyInitiallyFilled(TestInitiallyFilled): - TestInitiallyFilled = TestInitiallyFilled - has_real_tray = False - - def setUp(self): - self.TestInitiallyFilled.setUp(self, 'disk', 'floppy') - -class TestFloppyInitiallyEmpty(TestInitiallyEmpty): - TestInitiallyEmpty = TestInitiallyEmpty - has_real_tray = False - - def setUp(self): - self.TestInitiallyEmpty.setUp(self, 'disk', 'floppy') - # FDDs not having a real tray and there not being a medium inside the - # tray at startup means the tray will be considered open - self.has_opened = True +create_basic_test_classes() class TestChangeReadOnly(ChangeBaseClass): device_name = 'qdev0' -- 2.20.1 The code path for -device drive=<node-name> or without a drive=... option for empty drives, which is supposed to be used with -blockdev differs enough from the -drive based path with a user-owned BlockBackend, so we want to test both paths at least for the basic tests implemented by TestInitiallyFilled and TestInitiallyEmpty. This would have caught the bug recently fixed for inserting read-only nodes into a scsi-cd created without a drive=... option. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/118 | 43 ++++++++++++++++++++++++++------------ tests/qemu-iotests/118.out | 4 ++-- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/118 +++ b/tests/qemu-iotests/118 @@ -XXX,XX +XXX,XX @@ class ChangeBaseClass(iotests.QMPTestCase): has_opened = False has_closed = False + device_name = 'qdev0' + use_drive = False + def process_events(self): for event in self.vm.get_qmp_events(wait=False): if (event['event'] == 'DEVICE_TRAY_MOVED' and - event['data']['device'] == 'drive0'): + (event['data']['device'] == 'drive0' or + event['data']['id'] == self.device_name)): if event['data']['tray-open'] == False: self.has_closed = True else: @@ -XXX,XX +XXX,XX @@ class ChangeBaseClass(iotests.QMPTestCase): class GeneralChangeTestsBaseClass(ChangeBaseClass): - device_name = 'qdev0' - def test_change(self): + # 'change' requires a drive name, so skip the test for blockdev + if not self.use_drive: + return + result = self.vm.qmp('change', device='drive0', target=new_img, arg=iotests.imgfmt) self.assert_qmp(result, 'return', {}) @@ -XXX,XX +XXX,XX @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass): qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k') qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k') self.vm = iotests.VM() - self.vm.add_drive(old_img, 'media=%s' % self.media, 'none') + if self.use_drive: + self.vm.add_drive(old_img, 'media=%s' % self.media, 'none') + else: + self.vm.add_blockdev([ 'node-name=drive0', + 'driver=%s' % iotests.imgfmt, + 'file.driver=file', + 'file.filename=%s' % old_img ]) if self.interface == 'scsi': self.vm.add_device('virtio-scsi-pci') self.vm.add_device('%s,drive=drive0,id=%s' % @@ -XXX,XX +XXX,XX @@ class TestInitiallyEmpty(GeneralChangeTestsBaseClass): def setUp(self): qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k') - self.vm = iotests.VM().add_drive(None, 'media=%s' % self.media, 'none') + self.vm = iotests.VM() + if self.use_drive: + self.vm.add_drive(None, 'media=%s' % self.media, 'none') if self.interface == 'scsi': self.vm.add_device('virtio-scsi-pci') - self.vm.add_device('%s,drive=drive0,id=%s' % + self.vm.add_device('%s,%sid=%s' % (interface_to_device_name(self.interface), + 'drive=drive0,' if self.use_drive else '', self.device_name)) self.vm.launch() @@ -XXX,XX +XXX,XX @@ def create_basic_test_classes(): ('disk', 'floppy', False) ]: for case in [ TestInitiallyFilled, TestInitiallyEmpty ]: - - attr = { 'media': media, - 'interface': interface, - 'has_real_tray': has_real_tray } - - name = '%s_%s_%s' % (case.__name__, media, interface) - globals()[name] = type(name, (case, ), attr) + for use_drive in [ True, False ]: + attr = { 'media': media, + 'interface': interface, + 'has_real_tray': has_real_tray, + 'use_drive': use_drive } + + name = '%s_%s_%s_%s' % (case.__name__, media, interface, + 'drive' if use_drive else 'blockdev') + globals()[name] = type(name, (case, ), attr) create_basic_test_classes() diff --git a/tests/qemu-iotests/118.out b/tests/qemu-iotests/118.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/118.out +++ b/tests/qemu-iotests/118.out @@ -XXX,XX +XXX,XX @@ -......................................................................................... +....................................................................................................................................................................... ---------------------------------------------------------------------- -Ran 89 tests +Ran 167 tests OK -- 2.20.1 234 implements functions that are useful for doing migration between two VMs. Move them to iotests.py so that other test cases can use them, too. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/234 | 30 +++++++----------------------- tests/qemu-iotests/iotests.py | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/234 +++ b/tests/qemu-iotests/234 @@ -XXX,XX +XXX,XX @@ import os iotests.verify_image_format(supported_fmts=['qcow2']) iotests.verify_platform(['linux']) -def enable_migration_events(vm, name): - iotests.log('Enabling migration QMP events on %s...' % name) - iotests.log(vm.qmp('migrate-set-capabilities', capabilities=[ - { - 'capability': 'events', - 'state': True - } - ])) - -def wait_migration(vm): - while True: - event = vm.event_wait('MIGRATION') - iotests.log(event, filters=[iotests.filter_qmp_event]) - if event['data']['status'] == 'completed': - break - with iotests.FilePath('img') as img_path, \ iotests.FilePath('backing') as backing_path, \ iotests.FilePath('mig_fifo_a') as fifo_a, \ @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \ .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' % (iotests.imgfmt)) .launch()) - enable_migration_events(vm_a, 'A') + vm_a.enable_migration_events('A') iotests.log('Launching destination VM...') (vm_b.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path)) @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \ .add_incoming("exec: cat '%s'" % (fifo_a)) .launch()) - enable_migration_events(vm_b, 'B') + vm_b.enable_migration_events('B') # Add a child node that was created after the parent node. The reverse case # is covered by the -blockdev options above. @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \ iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo_a))) with iotests.Timeout(3, 'Migration does not complete'): # Wait for the source first (which includes setup=setup) - wait_migration(vm_a) + vm_a.wait_migration() # Wait for the destination second (which does not) - wait_migration(vm_b) + vm_b.wait_migration() iotests.log(vm_a.qmp('query-migrate')['return']['status']) iotests.log(vm_b.qmp('query-migrate')['return']['status']) @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \ .add_incoming("exec: cat '%s'" % (fifo_b)) .launch()) - enable_migration_events(vm_a, 'A') + vm_a.enable_migration_events('A') iotests.log(vm_a.qmp('blockdev-snapshot', node='drive0-backing', overlay='drive0')) @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('img') as img_path, \ iotests.log(vm_b.qmp('migrate', uri='exec:cat >%s' % (fifo_b))) with iotests.Timeout(3, 'Migration does not complete'): # Wait for the source first (which includes setup=setup) - wait_migration(vm_b) + vm_b.wait_migration() # Wait for the destination second (which does not) - wait_migration(vm_a) + vm_a.wait_migration() iotests.log(vm_a.qmp('query-migrate')['return']['status']) iotests.log(vm_b.qmp('query-migrate')['return']['status']) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -XXX,XX +XXX,XX @@ class VM(qtest.QEMUQtestMachine): elif status == 'null': return error + def enable_migration_events(self, name): + log('Enabling migration QMP events on %s...' % name) + log(self.qmp('migrate-set-capabilities', capabilities=[ + { + 'capability': 'events', + 'state': True + } + ])) + + def wait_migration(self): + while True: + event = self.event_wait('MIGRATION') + log(event, filters=[filter_qmp_event]) + if event['data']['status'] == 'completed': + break + def node_info(self, node_name): nodes = self.qmp('query-named-block-nodes') for x in nodes['return']: -- 2.20.1 This test case is motivated by commit 2b23f28639 ('block/copy-on-read: Fix permissions for inactive node'). Instead of just testing copy-on-read on migration, let's stack all sorts of filter nodes on top of each other and try if the resulting VM can still migrate successfully. For good measure, put everything into an iothread, because why not? Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/262 | 82 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/262.out | 17 ++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 100 insertions(+) create mode 100755 tests/qemu-iotests/262 create mode 100644 tests/qemu-iotests/262.out diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/262 @@ -XXX,XX +XXX,XX @@ +#!/usr/bin/env python +# +# Copyright (C) 2019 Red Hat, Inc. +# +# 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: Kevin Wolf <kwolf@redhat.com> +# +# Test migration with filter drivers present. Keep everything in an +# iothread just for fun. + +import iotests +import os + +iotests.verify_image_format(supported_fmts=['qcow2']) +iotests.verify_platform(['linux']) + +with iotests.FilePath('img') as img_path, \ + iotests.FilePath('mig_fifo') as fifo, \ + iotests.VM(path_suffix='a') as vm_a, \ + iotests.VM(path_suffix='b') as vm_b: + + def add_opts(vm): + vm.add_object('iothread,id=iothread0') + vm.add_object('throttle-group,id=tg0,x-bps-total=65536') + vm.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path)) + vm.add_blockdev('%s,file=drive0-file,node-name=drive0-fmt' % (iotests.imgfmt)) + vm.add_blockdev('copy-on-read,file=drive0-fmt,node-name=drive0-cor') + vm.add_blockdev('throttle,file=drive0-cor,node-name=drive0-throttle,throttle-group=tg0') + vm.add_blockdev('blkdebug,image=drive0-throttle,node-name=drive0-dbg') + vm.add_blockdev('null-co,node-name=null,read-zeroes=on') + vm.add_blockdev('blkverify,test=drive0-dbg,raw=null,node-name=drive0-verify') + + if iotests.supports_quorum(): + vm.add_blockdev('quorum,children.0=drive0-verify,vote-threshold=1,node-name=drive0-quorum') + root = "drive0-quorum" + else: + root = "drive0-verify" + + vm.add_device('virtio-blk,drive=%s,iothread=iothread0' % root) + + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M') + + os.mkfifo(fifo) + + iotests.log('Launching source VM...') + add_opts(vm_a) + vm_a.launch() + + vm_a.enable_migration_events('A') + + iotests.log('Launching destination VM...') + add_opts(vm_b) + vm_b.add_incoming("exec: cat '%s'" % (fifo)) + vm_b.launch() + + vm_b.enable_migration_events('B') + + iotests.log('Starting migration to B...') + iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo))) + with iotests.Timeout(3, 'Migration does not complete'): + # Wait for the source first (which includes setup=setup) + vm_a.wait_migration() + # Wait for the destination second (which does not) + vm_b.wait_migration() + + iotests.log(vm_a.qmp('query-migrate')['return']['status']) + iotests.log(vm_b.qmp('query-migrate')['return']['status']) + + iotests.log(vm_a.qmp('query-status')) + iotests.log(vm_b.qmp('query-status')) diff --git a/tests/qemu-iotests/262.out b/tests/qemu-iotests/262.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/262.out @@ -XXX,XX +XXX,XX @@ +Launching source VM... +Enabling migration QMP events on A... +{"return": {}} +Launching destination VM... +Enabling migration QMP events on B... +{"return": {}} +Starting migration to B... +{"return": {}} +{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +completed +completed +{"return": {"running": false, "singlestep": false, "status": "postmigrate"}} +{"return": {"running": true, "singlestep": false, "status": "running"}} 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 @@ 254 rw backing quick 255 rw quick 256 rw quick +262 rw quick migration -- 2.20.1 The same change as commit 2b23f28639 ('block/copy-on-read: Fix permissions for inactive node') made for the copy-on-read driver can be made for bdrv_filter_default_perms(): Retaining the old permissions from the BdrvChild if it is given complicates things unnecessarily when in the end this only means that the options set in the c == NULL case (i.e. during child creation) are retained. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- block.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c, uint64_t perm, uint64_t shared, uint64_t *nperm, uint64_t *nshared) { - if (c == NULL) { - *nperm = perm & DEFAULT_PERM_PASSTHROUGH; - *nshared = (shared & DEFAULT_PERM_PASSTHROUGH) | DEFAULT_PERM_UNCHANGED; - return; - } - - *nperm = (perm & DEFAULT_PERM_PASSTHROUGH) | - (c->perm & DEFAULT_PERM_UNCHANGED); - *nshared = (shared & DEFAULT_PERM_PASSTHROUGH) | - (c->shared_perm & DEFAULT_PERM_UNCHANGED); + *nperm = perm & DEFAULT_PERM_PASSTHROUGH; + *nshared = (shared & DEFAULT_PERM_PASSTHROUGH) | DEFAULT_PERM_UNCHANGED; } void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, -- 2.20.1 From: Max Reitz <mreitz@redhat.com> bdrv_drop_intermediate() calls BdrvChildRole.update_filename(). That may poll, thus changing the graph, which potentially breaks the QLIST_FOREACH_SAFE() loop. Just keep the whole subtree drained. This is probably the right thing to do anyway (dropping nodes while the subtree is not drained seems wrong). Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, int ret = -EIO; bdrv_ref(top); + bdrv_subtree_drained_begin(top); if (!top->drv || !base->drv) { goto exit; @@ -XXX,XX +XXX,XX @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, ret = 0; exit: + bdrv_subtree_drained_end(top); bdrv_unref(top); return ret; } -- 2.20.1 From: Max Reitz <mreitz@redhat.com> Currently, bdrv_replace_child_noperm() undrains the parent until it is completely undrained, then re-drains it after attaching the new child node. This is a problem with bdrv_drop_intermediate(): We want to keep the whole subtree drained, including parents, while the operation is under way. bdrv_replace_child_noperm() breaks this by allowing every parent to become unquiesced briefly, and then redraining it. In fact, there is no reason why the parent should become unquiesced and be allowed to submit requests to the new child node if that new node is supposed to be kept drained. So if anything, we have to drain the parent before detaching the old child node. Conversely, we have to undrain it only after attaching the new child node. Thus, change the whole drain algorithm here: Calculate the number of times we have to drain/undrain the parent before replacing the child node then drain it (if necessary), replace the child node, and then undrain it. Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 49 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *new_bs) { BlockDriverState *old_bs = child->bs; - int i; + int new_bs_quiesce_counter; + int drain_saldo; assert(!child->frozen); if (old_bs && new_bs) { assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)); } + + new_bs_quiesce_counter = (new_bs ? new_bs->quiesce_counter : 0); + drain_saldo = new_bs_quiesce_counter - child->parent_quiesce_counter; + + /* + * If the new child node is drained but the old one was not, flush + * all outstanding requests to the old child node. + */ + while (drain_saldo > 0 && child->role->drained_begin) { + bdrv_parent_drained_begin_single(child, true); + drain_saldo--; + } + if (old_bs) { /* Detach first so that the recursive drain sections coming from @child * are already gone and we only end the drain sections that came from @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, if (child->role->detach) { child->role->detach(child); } - while (child->parent_quiesce_counter) { - bdrv_parent_drained_end_single(child); - } QLIST_REMOVE(child, next_parent); - } else { - assert(child->parent_quiesce_counter == 0); } child->bs = new_bs; if (new_bs) { QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); - if (new_bs->quiesce_counter) { - int num = new_bs->quiesce_counter; - if (child->role->parent_is_bds) { - num -= bdrv_drain_all_count; - } - assert(num >= 0); - for (i = 0; i < num; i++) { - bdrv_parent_drained_begin_single(child, true); - } - } + + /* + * Detaching the old node may have led to the new node's + * quiesce_counter having been decreased. Not a problem, we + * just need to recognize this here and then invoke + * drained_end appropriately more often. + */ + assert(new_bs->quiesce_counter <= new_bs_quiesce_counter); + drain_saldo += new_bs->quiesce_counter - new_bs_quiesce_counter; /* Attach only after starting new drained sections, so that recursive * drain sections coming from @child don't get an extra .drained_begin @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, child->role->attach(child); } } + + /* + * If the old child node was drained but the new one is not, allow + * requests to come in only after the new node has been attached. + */ + while (drain_saldo < 0 && child->role->drained_end) { + bdrv_parent_drained_end_single(child); + drain_saldo++; + } } /* -- 2.20.1 From: Max Reitz <mreitz@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 167 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void bdrv_test_child_perm(BlockDriverState *bs, BdrvChild *c, nperm, nshared); } +static int bdrv_test_change_backing_file(BlockDriverState *bs, + const char *backing_file, + const char *backing_fmt) +{ + return 0; +} + static BlockDriver bdrv_test = { .format_name = "test", .instance_size = sizeof(BDRVTestState), @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_test = { .bdrv_co_drain_end = bdrv_test_co_drain_end, .bdrv_child_perm = bdrv_test_child_perm, + + .bdrv_change_backing_file = bdrv_test_change_backing_file, }; static void aio_ret_cb(void *opaque, int ret) @@ -XXX,XX +XXX,XX @@ static void test_blockjob_commit_by_drained_end(void) bdrv_unref(bs_child); } + +typedef struct TestSimpleBlockJob { + BlockJob common; + bool should_complete; + bool *did_complete; +} TestSimpleBlockJob; + +static int coroutine_fn test_simple_job_run(Job *job, Error **errp) +{ + TestSimpleBlockJob *s = container_of(job, TestSimpleBlockJob, common.job); + + while (!s->should_complete) { + job_sleep_ns(job, 0); + } + + return 0; +} + +static void test_simple_job_clean(Job *job) +{ + TestSimpleBlockJob *s = container_of(job, TestSimpleBlockJob, common.job); + *s->did_complete = true; +} + +static const BlockJobDriver test_simple_job_driver = { + .job_driver = { + .instance_size = sizeof(TestSimpleBlockJob), + .free = block_job_free, + .user_resume = block_job_user_resume, + .drain = block_job_drain, + .run = test_simple_job_run, + .clean = test_simple_job_clean, + }, +}; + +static int drop_intermediate_poll_update_filename(BdrvChild *child, + BlockDriverState *new_base, + const char *filename, + Error **errp) +{ + /* + * We are free to poll here, which may change the block graph, if + * it is not drained. + */ + + /* If the job is not drained: Complete it, schedule job_exit() */ + aio_poll(qemu_get_current_aio_context(), false); + /* If the job is not drained: Run job_exit(), finish the job */ + aio_poll(qemu_get_current_aio_context(), false); + + return 0; +} + +/** + * Test a poll in the midst of bdrv_drop_intermediate(). + * + * bdrv_drop_intermediate() calls BdrvChildRole.update_filename(), + * which can yield or poll. This may lead to graph changes, unless + * the whole subtree in question is drained. + * + * We test this on the following graph: + * + * Job + * + * | + * job-node + * | + * v + * + * job-node + * + * | + * backing + * | + * v + * + * node-2 --chain--> node-1 --chain--> node-0 + * + * We drop node-1 with bdrv_drop_intermediate(top=node-1, base=node-0). + * + * This first updates node-2's backing filename by invoking + * drop_intermediate_poll_update_filename(), which polls twice. This + * causes the job to finish, which in turns causes the job-node to be + * deleted. + * + * bdrv_drop_intermediate() uses a QLIST_FOREACH_SAFE() loop, so it + * already has a pointer to the BdrvChild edge between job-node and + * node-1. When it tries to handle that edge, we probably get a + * segmentation fault because the object no longer exists. + * + * + * The solution is for bdrv_drop_intermediate() to drain top's + * subtree. This prevents graph changes from happening just because + * BdrvChildRole.update_filename() yields or polls. Thus, the block + * job is paused during that drained section and must finish before or + * after. + * + * (In addition, bdrv_replace_child() must keep the job paused.) + */ +static void test_drop_intermediate_poll(void) +{ + static BdrvChildRole chain_child_role; + BlockDriverState *chain[3]; + TestSimpleBlockJob *job; + BlockDriverState *job_node; + bool job_has_completed = false; + int i; + int ret; + + chain_child_role = child_backing; + chain_child_role.update_filename = drop_intermediate_poll_update_filename; + + for (i = 0; i < 3; i++) { + char name[32]; + snprintf(name, 32, "node-%i", i); + + chain[i] = bdrv_new_open_driver(&bdrv_test, name, 0, &error_abort); + } + + job_node = bdrv_new_open_driver(&bdrv_test, "job-node", BDRV_O_RDWR, + &error_abort); + bdrv_set_backing_hd(job_node, chain[1], &error_abort); + + /* + * Establish the chain last, so the chain links are the first + * elements in the BDS.parents lists + */ + for (i = 0; i < 3; i++) { + if (i) { + /* Takes the reference to chain[i - 1] */ + chain[i]->backing = bdrv_attach_child(chain[i], chain[i - 1], + "chain", &chain_child_role, + &error_abort); + } + } + + job = block_job_create("job", &test_simple_job_driver, NULL, job_node, + 0, BLK_PERM_ALL, 0, 0, NULL, NULL, &error_abort); + + /* The job has a reference now */ + bdrv_unref(job_node); + + job->did_complete = &job_has_completed; + + job_start(&job->common.job); + job->should_complete = true; + + g_assert(!job_has_completed); + ret = bdrv_drop_intermediate(chain[1], chain[0], NULL); + g_assert(ret == 0); + g_assert(job_has_completed); + + bdrv_unref(chain[2]); +} + int main(int argc, char **argv) { int ret; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/bdrv-drain/blockjob/commit_by_drained_end", test_blockjob_commit_by_drained_end); + g_test_add_func("/bdrv-drain/bdrv_drop_intermediate/poll", + test_drop_intermediate_poll); + ret = g_test_run(); qemu_event_destroy(&done_event); return ret; -- 2.20.1 From: Max Reitz <mreitz@redhat.com> Add a test for what happens when you call bdrv_replace_child_noperm() for various drain situations ({old,new} child {drained,not drained}). Most importantly, if both the old and the new child are drained, the parent must not be undrained at any point. Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 308 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 308 insertions(+) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_drop_intermediate_poll(void) bdrv_unref(chain[2]); } + +typedef struct BDRVReplaceTestState { + bool was_drained; + bool was_undrained; + bool has_read; + + int drain_count; + + bool yield_before_read; + Coroutine *io_co; + Coroutine *drain_co; +} BDRVReplaceTestState; + +static void bdrv_replace_test_close(BlockDriverState *bs) +{ +} + +/** + * If @bs has a backing file: + * Yield if .yield_before_read is true (and wait for drain_begin to + * wake us up). + * Forward the read to bs->backing. Set .has_read to true. + * If drain_begin has woken us, wake it in turn. + * + * Otherwise: + * Set .has_read to true and return success. + */ +static int coroutine_fn bdrv_replace_test_co_preadv(BlockDriverState *bs, + uint64_t offset, + uint64_t bytes, + QEMUIOVector *qiov, + int flags) +{ + BDRVReplaceTestState *s = bs->opaque; + + if (bs->backing) { + int ret; + + g_assert(!s->drain_count); + + s->io_co = qemu_coroutine_self(); + if (s->yield_before_read) { + s->yield_before_read = false; + qemu_coroutine_yield(); + } + s->io_co = NULL; + + ret = bdrv_preadv(bs->backing, offset, qiov); + s->has_read = true; + + /* Wake up drain_co if it runs */ + if (s->drain_co) { + aio_co_wake(s->drain_co); + } + + return ret; + } + + s->has_read = true; + return 0; +} + +/** + * If .drain_count is 0, wake up .io_co if there is one; and set + * .was_drained. + * Increment .drain_count. + */ +static void coroutine_fn bdrv_replace_test_co_drain_begin(BlockDriverState *bs) +{ + BDRVReplaceTestState *s = bs->opaque; + + if (!s->drain_count) { + /* Keep waking io_co up until it is done */ + s->drain_co = qemu_coroutine_self(); + while (s->io_co) { + aio_co_wake(s->io_co); + s->io_co = NULL; + qemu_coroutine_yield(); + } + s->drain_co = NULL; + + s->was_drained = true; + } + s->drain_count++; +} + +/** + * Reduce .drain_count, set .was_undrained once it reaches 0. + * If .drain_count reaches 0 and the node has a backing file, issue a + * read request. + */ +static void coroutine_fn bdrv_replace_test_co_drain_end(BlockDriverState *bs) +{ + BDRVReplaceTestState *s = bs->opaque; + + g_assert(s->drain_count > 0); + if (!--s->drain_count) { + int ret; + + s->was_undrained = true; + + if (bs->backing) { + char data; + QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, &data, 1); + + /* Queue a read request post-drain */ + ret = bdrv_replace_test_co_preadv(bs, 0, 1, &qiov, 0); + g_assert(ret >= 0); + } + } +} + +static BlockDriver bdrv_replace_test = { + .format_name = "replace_test", + .instance_size = sizeof(BDRVReplaceTestState), + + .bdrv_close = bdrv_replace_test_close, + .bdrv_co_preadv = bdrv_replace_test_co_preadv, + + .bdrv_co_drain_begin = bdrv_replace_test_co_drain_begin, + .bdrv_co_drain_end = bdrv_replace_test_co_drain_end, + + .bdrv_child_perm = bdrv_format_default_perms, +}; + +static void coroutine_fn test_replace_child_mid_drain_read_co(void *opaque) +{ + int ret; + char data; + + ret = blk_co_pread(opaque, 0, 1, &data, 0); + g_assert(ret >= 0); +} + +/** + * We test two things: + * (1) bdrv_replace_child_noperm() must not undrain the parent if both + * children are drained. + * (2) bdrv_replace_child_noperm() must never flush I/O requests to a + * drained child. If the old child is drained, it must flush I/O + * requests after the new one has been attached. If the new child + * is drained, it must flush I/O requests before the old one is + * detached. + * + * To do so, we create one parent node and two child nodes; then + * attach one of the children (old_child_bs) to the parent, then + * drain both old_child_bs and new_child_bs according to + * old_drain_count and new_drain_count, respectively, and finally + * we invoke bdrv_replace_node() to replace old_child_bs by + * new_child_bs. + * + * The test block driver we use here (bdrv_replace_test) has a read + * function that: + * - For the parent node, can optionally yield, and then forwards the + * read to bdrv_preadv(), + * - For the child node, just returns immediately. + * + * If the read yields, the drain_begin function will wake it up. + * + * The drain_end function issues a read on the parent once it is fully + * undrained (which simulates requests starting to come in again). + */ +static void do_test_replace_child_mid_drain(int old_drain_count, + int new_drain_count) +{ + BlockBackend *parent_blk; + BlockDriverState *parent_bs; + BlockDriverState *old_child_bs, *new_child_bs; + BDRVReplaceTestState *parent_s; + BDRVReplaceTestState *old_child_s, *new_child_s; + Coroutine *io_co; + int i; + + parent_bs = bdrv_new_open_driver(&bdrv_replace_test, "parent", 0, + &error_abort); + parent_s = parent_bs->opaque; + + parent_blk = blk_new(qemu_get_aio_context(), + BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL); + blk_insert_bs(parent_blk, parent_bs, &error_abort); + + old_child_bs = bdrv_new_open_driver(&bdrv_replace_test, "old-child", 0, + &error_abort); + new_child_bs = bdrv_new_open_driver(&bdrv_replace_test, "new-child", 0, + &error_abort); + old_child_s = old_child_bs->opaque; + new_child_s = new_child_bs->opaque; + + /* So that we can read something */ + parent_bs->total_sectors = 1; + old_child_bs->total_sectors = 1; + new_child_bs->total_sectors = 1; + + bdrv_ref(old_child_bs); + parent_bs->backing = bdrv_attach_child(parent_bs, old_child_bs, "child", + &child_backing, &error_abort); + + for (i = 0; i < old_drain_count; i++) { + bdrv_drained_begin(old_child_bs); + } + for (i = 0; i < new_drain_count; i++) { + bdrv_drained_begin(new_child_bs); + } + + if (!old_drain_count) { + /* + * Start a read operation that will yield, so it will not + * complete before the node is drained. + */ + parent_s->yield_before_read = true; + io_co = qemu_coroutine_create(test_replace_child_mid_drain_read_co, + parent_blk); + qemu_coroutine_enter(io_co); + } + + /* If we have started a read operation, it should have yielded */ + g_assert(!parent_s->has_read); + + /* Reset drained status so we can see what bdrv_replace_node() does */ + parent_s->was_drained = false; + parent_s->was_undrained = false; + + g_assert(parent_bs->quiesce_counter == old_drain_count); + bdrv_replace_node(old_child_bs, new_child_bs, &error_abort); + g_assert(parent_bs->quiesce_counter == new_drain_count); + + if (!old_drain_count && !new_drain_count) { + /* + * From undrained to undrained drains and undrains the parent, + * because bdrv_replace_node() contains a drained section for + * @old_child_bs. + */ + g_assert(parent_s->was_drained && parent_s->was_undrained); + } else if (!old_drain_count && new_drain_count) { + /* + * From undrained to drained should drain the parent and keep + * it that way. + */ + g_assert(parent_s->was_drained && !parent_s->was_undrained); + } else if (old_drain_count && !new_drain_count) { + /* + * From drained to undrained should undrain the parent and + * keep it that way. + */ + g_assert(!parent_s->was_drained && parent_s->was_undrained); + } else /* if (old_drain_count && new_drain_count) */ { + /* + * From drained to drained must not undrain the parent at any + * point + */ + g_assert(!parent_s->was_drained && !parent_s->was_undrained); + } + + if (!old_drain_count || !new_drain_count) { + /* + * If !old_drain_count, we have started a read request before + * bdrv_replace_node(). If !new_drain_count, the parent must + * have been undrained at some point, and + * bdrv_replace_test_co_drain_end() starts a read request + * then. + */ + g_assert(parent_s->has_read); + } else { + /* + * If the parent was never undrained, there is no way to start + * a read request. + */ + g_assert(!parent_s->has_read); + } + + /* A drained child must have not received any request */ + g_assert(!(old_drain_count && old_child_s->has_read)); + g_assert(!(new_drain_count && new_child_s->has_read)); + + for (i = 0; i < new_drain_count; i++) { + bdrv_drained_end(new_child_bs); + } + for (i = 0; i < old_drain_count; i++) { + bdrv_drained_end(old_child_bs); + } + + /* + * By now, bdrv_replace_test_co_drain_end() must have been called + * at some point while the new child was attached to the parent. + */ + g_assert(parent_s->has_read); + g_assert(new_child_s->has_read); + + blk_unref(parent_blk); + bdrv_unref(parent_bs); + bdrv_unref(old_child_bs); + bdrv_unref(new_child_bs); +} + +static void test_replace_child_mid_drain(void) +{ + int old_drain_count, new_drain_count; + + for (old_drain_count = 0; old_drain_count < 2; old_drain_count++) { + for (new_drain_count = 0; new_drain_count < 2; new_drain_count++) { + do_test_replace_child_mid_drain(old_drain_count, new_drain_count); + } + } +} + int main(int argc, char **argv) { int ret; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/bdrv-drain/bdrv_drop_intermediate/poll", test_drop_intermediate_poll); + g_test_add_func("/bdrv-drain/replace_child/mid-drain", + test_replace_child_mid_drain); + ret = g_test_run(); qemu_event_destroy(&done_event); return ret; -- 2.20.1 From: Max Reitz <mreitz@redhat.com> We already have 030 for that in general, but this tests very specific cases of both jobs finishing concurrently. Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/258 | 163 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/258.out | 33 ++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 197 insertions(+) create mode 100755 tests/qemu-iotests/258 create mode 100644 tests/qemu-iotests/258.out diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/258 @@ -XXX,XX +XXX,XX @@ +#!/usr/bin/env python +# +# Very specific tests for adjacent commit/stream block jobs +# +# Copyright (C) 2019 Red Hat, Inc. +# +# 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: Max Reitz <mreitz@redhat.com> + +import iotests +from iotests import log, qemu_img, qemu_io_silent, \ + filter_qmp_testfiles, filter_qmp_imgfmt + +# Need backing file and change-backing-file support +iotests.verify_image_format(supported_fmts=['qcow2', 'qed']) +iotests.verify_platform(['linux']) + + +# Returns a node for blockdev-add +def node(node_name, path, backing=None, fmt=None, throttle=None): + if fmt is None: + fmt = iotests.imgfmt + + res = { + 'node-name': node_name, + 'driver': fmt, + 'file': { + 'driver': 'file', + 'filename': path + } + } + + if backing is not None: + res['backing'] = backing + + if throttle: + res['file'] = { + 'driver': 'throttle', + 'throttle-group': throttle, + 'file': res['file'] + } + + return res + +# Finds a node in the debug block graph +def find_graph_node(graph, node_id): + return next(node for node in graph['nodes'] if node['id'] == node_id) + + +def test_concurrent_finish(write_to_stream_node): + log('') + log('=== Commit and stream finish concurrently (letting %s write) ===' % \ + ('stream' if write_to_stream_node else 'commit')) + log('') + + # All chosen in such a way that when the commit job wants to + # finish, it polls and thus makes stream finish concurrently -- + # and the other way around, depending on whether the commit job + # is finalized before stream completes or not. + + with iotests.FilePath('node4.img') as node4_path, \ + iotests.FilePath('node3.img') as node3_path, \ + iotests.FilePath('node2.img') as node2_path, \ + iotests.FilePath('node1.img') as node1_path, \ + iotests.FilePath('node0.img') as node0_path, \ + iotests.VM() as vm: + + # It is important to use raw for the base layer (so that + # permissions are just handed through to the protocol layer) + assert qemu_img('create', '-f', 'raw', node0_path, '64M') == 0 + + stream_throttle=None + commit_throttle=None + + for path in [node1_path, node2_path, node3_path, node4_path]: + assert qemu_img('create', '-f', iotests.imgfmt, path, '64M') == 0 + + if write_to_stream_node: + # This is what (most of the time) makes commit finish + # earlier and then pull in stream + assert qemu_io_silent(node2_path, + '-c', 'write %iK 64K' % (65536 - 192), + '-c', 'write %iK 64K' % (65536 - 64)) == 0 + + stream_throttle='tg' + else: + # And this makes stream finish earlier + assert qemu_io_silent(node1_path, + '-c', 'write %iK 64K' % (65536 - 64)) == 0 + + commit_throttle='tg' + + vm.launch() + + vm.qmp_log('object-add', + qom_type='throttle-group', + id='tg', + props={ + 'x-iops-write': 1, + 'x-iops-write-max': 1 + }) + + vm.qmp_log('blockdev-add', + filters=[filter_qmp_testfiles, filter_qmp_imgfmt], + **node('node4', node4_path, throttle=stream_throttle, + backing=node('node3', node3_path, + backing=node('node2', node2_path, + backing=node('node1', node1_path, + backing=node('node0', node0_path, throttle=commit_throttle, + fmt='raw')))))) + + vm.qmp_log('block-commit', + job_id='commit', + device='node4', + filter_node_name='commit-filter', + top_node='node1', + base_node='node0', + auto_finalize=False) + + vm.qmp_log('block-stream', + job_id='stream', + device='node3', + base_node='commit-filter') + + if write_to_stream_node: + vm.run_job('commit', auto_finalize=False, auto_dismiss=True) + vm.run_job('stream', auto_finalize=True, auto_dismiss=True) + else: + # No, the jobs do not really finish concurrently here, + # the stream job does complete strictly before commit. + # But still, this is close enough for what we want to + # test. + vm.run_job('stream', auto_finalize=True, auto_dismiss=True) + vm.run_job('commit', auto_finalize=False, auto_dismiss=True) + + # Assert that the backing node of node3 is node 0 now + graph = vm.qmp('x-debug-query-block-graph')['return'] + for edge in graph['edges']: + if edge['name'] == 'backing' and \ + find_graph_node(graph, edge['parent'])['name'] == 'node3': + assert find_graph_node(graph, edge['child'])['name'] == 'node0' + break + + +def main(): + log('Running tests:') + test_concurrent_finish(True) + test_concurrent_finish(False) + +if __name__ == '__main__': + main() diff --git a/tests/qemu-iotests/258.out b/tests/qemu-iotests/258.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/258.out @@ -XXX,XX +XXX,XX @@ +Running tests: + +=== Commit and stream finish concurrently (letting stream write) === + +{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}} +{"return": {}} +{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "throttle-group": "tg"}, "node-name": "node4"}} +{"return": {}} +{"execute": "block-commit", "arguments": {"auto-finalize": false, "base-node": "node0", "device": "node4", "filter-node-name": "commit-filter", "job-id": "commit", "top-node": "node1"}} +{"return": {}} +{"execute": "block-stream", "arguments": {"base-node": "commit-filter", "device": "node3", "job-id": "stream"}} +{"return": {}} +{"execute": "job-finalize", "arguments": {"id": "commit"}} +{"return": {}} +{"data": {"id": "commit", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"device": "commit", "len": 67108864, "offset": 67108864, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"device": "stream", "len": 67108864, "offset": 67108864, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} + +=== Commit and stream finish concurrently (letting commit write) === + +{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}} +{"return": {}} +{"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "throttle-group": "tg"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "node-name": "node4"}} +{"return": {}} +{"execute": "block-commit", "arguments": {"auto-finalize": false, "base-node": "node0", "device": "node4", "filter-node-name": "commit-filter", "job-id": "commit", "top-node": "node1"}} +{"return": {}} +{"execute": "block-stream", "arguments": {"base-node": "commit-filter", "device": "node3", "job-id": "stream"}} +{"return": {}} +{"data": {"device": "stream", "len": 67108864, "offset": 67108864, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"execute": "job-finalize", "arguments": {"id": "commit"}} +{"return": {}} +{"data": {"id": "commit", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"device": "commit", "len": 67108864, "offset": 67108864, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} 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 @@ 254 rw backing quick 255 rw quick 256 rw quick +258 rw quick 262 rw quick migration -- 2.20.1 The functionality offered by blk_pread_unthrottled() goes back to commit 498e386c584. Then, we couldn't perform I/O throttling with synchronous requests because timers wouldn't be executed in polling loops. So the commit automatically disabled I/O throttling as soon as a synchronous request was issued. However, for geometry detection during disk initialisation, we always used (and still use) synchronous requests even if guest requests use AIO later. Geometry detection was not wanted to disable I/O throttling, so bdrv_pread_unthrottled() was introduced which disabled throttling only temporarily. All of this isn't necessary any more because we do run timers in polling loop and even synchronous requests are now using coroutine infrastructure internally. For this reason, commit 90c78624f already removed the automatic disabling of I/O throttling. It's time to get rid of the workaround for the removed code, and its abuse of blk_root_drained_begin()/end(), as well. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- include/sysemu/block-backend.h | 2 -- block/block-backend.c | 16 ---------------- hw/block/hd-geometry.c | 7 +------ 3 files changed, 1 insertion(+), 24 deletions(-) diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -XXX,XX +XXX,XX @@ char *blk_get_attached_dev_id(BlockBackend *blk); BlockBackend *blk_by_dev(void *dev); BlockBackend *blk_by_qdev_id(const char *id, Error **errp); void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque); -int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf, - int bytes); int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset, unsigned int bytes, QEMUIOVector *qiov, BdrvRequestFlags flags); 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 @@ static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf, return rwco.ret; } -int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf, - int count) -{ - int ret; - - ret = blk_check_byte_request(blk, offset, count); - if (ret < 0) { - return ret; - } - - blk_root_drained_begin(blk->root); - ret = blk_pread(blk, offset, buf, count); - blk_root_drained_end(blk->root, NULL); - return ret; -} - int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset, int bytes, BdrvRequestFlags flags) { diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/hd-geometry.c +++ b/hw/block/hd-geometry.c @@ -XXX,XX +XXX,XX @@ static int guess_disk_lchs(BlockBackend *blk, blk_get_geometry(blk, &nb_sectors); - /** - * The function will be invoked during startup not only in sync I/O mode, - * but also in async I/O mode. So the I/O throttling function has to - * be disabled temporarily here, not permanently. - */ - if (blk_pread_unthrottled(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) { + if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) { return -1; } /* test msdos magic */ -- 2.20.1 mirror_top_bs is currently implicitly drained through its connection to the source or the target node. However, the drain section for target_bs ends early after moving mirror_top_bs from src to target_bs, so that requests can already be restarted while mirror_top_bs is still present in the chain, but has dropped all permissions and therefore runs into an assertion failure like this: qemu-system-x86_64: block/io.c:1634: bdrv_co_write_req_prepare: Assertion `child->perm & BLK_PERM_WRITE' failed. Keep mirror_top_bs drained until all graph changes have completed. Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- block/mirror.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/block/mirror.c b/block/mirror.c index XXXXXXX..XXXXXXX 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -XXX,XX +XXX,XX @@ static int mirror_exit_common(Job *job) s->target = NULL; /* We don't access the source any more. Dropping any WRITE/RESIZE is - * required before it could become a backing file of target_bs. */ + * required before it could become a backing file of target_bs. Not having + * these permissions any more means that we can't allow any new requests on + * mirror_top_bs from now on, so keep it drained. */ + bdrv_drained_begin(mirror_top_bs); bs_opaque->stop = true; bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing, &error_abort); @@ -XXX,XX +XXX,XX @@ static int mirror_exit_common(Job *job) bs_opaque->job = NULL; bdrv_drained_end(src); + bdrv_drained_end(mirror_top_bs); s->in_drain = false; bdrv_unref(mirror_top_bs); bdrv_unref(src); -- 2.20.1 This fixes devices like IDE that can still start new requests from I/O handlers in the CPU thread while the block backend is drained. The basic assumption is that in a drain section, no new requests should be allowed through a BlockBackend (blk_drained_begin/end don't exist, we get drain sections only on the node level). However, there are two special cases where requests should not be queued: 1. Block jobs: We already make sure that block jobs are paused in a drain section, so they won't start new requests. However, if the drain_begin is called on the job's BlockBackend first, it can happen that we deadlock because the job stays busy until it reaches a pause point - which it can't if its requests aren't processed any more. The proper solution here would be to make all requests through the job's filter node instead of using a BlockBackend. For now, just disabling request queuing on the job BlockBackend is simpler. 2. In test cases where making requests through bdrv_* would be cumbersome because we'd need a BdrvChild. As we already got the functionality to disable request queuing from 1., use it in tests, too, for convenience. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- include/sysemu/block-backend.h | 1 + block/backup.c | 1 + block/block-backend.c | 53 ++++++++++++++++++++++++++++++++-- block/commit.c | 2 ++ block/mirror.c | 1 + blockjob.c | 3 ++ tests/test-bdrv-drain.c | 1 + 7 files changed, 59 insertions(+), 3 deletions(-) diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -XXX,XX +XXX,XX @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm); void blk_set_allow_write_beyond_eof(BlockBackend *blk, bool allow); void blk_set_allow_aio_context_change(BlockBackend *blk, bool allow); +void blk_set_disable_request_queuing(BlockBackend *blk, bool disable); void blk_iostatus_enable(BlockBackend *blk); bool blk_iostatus_is_enabled(const BlockBackend *blk); BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk); 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, if (ret < 0) { goto error; } + blk_set_disable_request_queuing(job->target, true); job->on_source_error = on_source_error; job->on_target_error = on_target_error; 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 @@ struct BlockBackend { QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers; int quiesce_counter; + CoQueue queued_requests; + bool disable_request_queuing; + VMChangeStateEntry *vmsh; bool force_allow_inactivate; @@ -XXX,XX +XXX,XX @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm) block_acct_init(&blk->stats); + qemu_co_queue_init(&blk->queued_requests); notifier_list_init(&blk->remove_bs_notifiers); notifier_list_init(&blk->insert_bs_notifiers); QLIST_INIT(&blk->aio_notifiers); @@ -XXX,XX +XXX,XX @@ void blk_set_allow_aio_context_change(BlockBackend *blk, bool allow) blk->allow_aio_context_change = allow; } +void blk_set_disable_request_queuing(BlockBackend *blk, bool disable) +{ + blk->disable_request_queuing = disable; +} + static int blk_check_byte_request(BlockBackend *blk, int64_t offset, size_t size) { @@ -XXX,XX +XXX,XX @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset, return 0; } +static void coroutine_fn blk_wait_while_drained(BlockBackend *blk) +{ + if (blk->quiesce_counter && !blk->disable_request_queuing) { + qemu_co_queue_wait(&blk->queued_requests, NULL); + } +} + int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset, unsigned int bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) { int ret; - BlockDriverState *bs = blk_bs(blk); + BlockDriverState *bs; + blk_wait_while_drained(blk); + + /* Call blk_bs() only after waiting, the graph may have changed */ + bs = blk_bs(blk); trace_blk_co_preadv(blk, bs, offset, bytes, flags); ret = blk_check_byte_request(blk, offset, bytes); @@ -XXX,XX +XXX,XX @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, BdrvRequestFlags flags) { int ret; - BlockDriverState *bs = blk_bs(blk); + BlockDriverState *bs; + blk_wait_while_drained(blk); + + /* Call blk_bs() only after waiting, the graph may have changed */ + bs = blk_bs(blk); trace_blk_co_pwritev(blk, bs, offset, bytes, flags); ret = blk_check_byte_request(blk, offset, bytes); @@ -XXX,XX +XXX,XX @@ static void blk_aio_read_entry(void *opaque) BlkRwCo *rwco = &acb->rwco; QEMUIOVector *qiov = rwco->iobuf; + if (rwco->blk->quiesce_counter) { + blk_dec_in_flight(rwco->blk); + blk_wait_while_drained(rwco->blk); + blk_inc_in_flight(rwco->blk); + } + assert(qiov->size == acb->bytes); rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, acb->bytes, qiov, rwco->flags); @@ -XXX,XX +XXX,XX @@ static void blk_aio_write_entry(void *opaque) BlkRwCo *rwco = &acb->rwco; QEMUIOVector *qiov = rwco->iobuf; + if (rwco->blk->quiesce_counter) { + blk_dec_in_flight(rwco->blk); + blk_wait_while_drained(rwco->blk); + blk_inc_in_flight(rwco->blk); + } + assert(!qiov || qiov->size == acb->bytes); rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, acb->bytes, qiov, rwco->flags); @@ -XXX,XX +XXX,XX @@ void blk_aio_cancel_async(BlockAIOCB *acb) int blk_co_ioctl(BlockBackend *blk, unsigned long int req, void *buf) { + blk_wait_while_drained(blk); + if (!blk_is_available(blk)) { return -ENOMEDIUM; } @@ -XXX,XX +XXX,XX @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf, int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes) { - int ret = blk_check_byte_request(blk, offset, bytes); + int ret; + + blk_wait_while_drained(blk); + + ret = blk_check_byte_request(blk, offset, bytes); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes) int blk_co_flush(BlockBackend *blk) { + blk_wait_while_drained(blk); + if (!blk_is_available(blk)) { return -ENOMEDIUM; } @@ -XXX,XX +XXX,XX @@ static void blk_root_drained_end(BdrvChild *child, int *drained_end_counter) if (blk->dev_ops && blk->dev_ops->drained_end) { blk->dev_ops->drained_end(blk->dev_opaque); } + while (qemu_co_enter_next(&blk->queued_requests, NULL)) { + /* Resume all queued requests */ + } } } diff --git a/block/commit.c b/block/commit.c index XXXXXXX..XXXXXXX 100644 --- a/block/commit.c +++ b/block/commit.c @@ -XXX,XX +XXX,XX @@ void commit_start(const char *job_id, BlockDriverState *bs, if (ret < 0) { goto fail; } + blk_set_disable_request_queuing(s->base, true); s->base_bs = base; /* Required permissions are already taken with block_job_add_bdrv() */ @@ -XXX,XX +XXX,XX @@ void commit_start(const char *job_id, BlockDriverState *bs, if (ret < 0) { goto fail; } + blk_set_disable_request_queuing(s->top, true); s->backing_file_str = g_strdup(backing_file_str); s->on_error = on_error; diff --git a/block/mirror.c b/block/mirror.c index XXXXXXX..XXXXXXX 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -XXX,XX +XXX,XX @@ static BlockJob *mirror_start_job( blk_set_force_allow_inactivate(s->target); } blk_set_allow_aio_context_change(s->target, true); + blk_set_disable_request_queuing(s->target, true); s->replaces = g_strdup(replaces); s->on_source_error = on_source_error; diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker); + /* Disable request queuing in the BlockBackend to avoid deadlocks on drain: + * The job reports that it's busy until it reaches a pause point. */ + blk_set_disable_request_queuing(blk, true); blk_set_allow_aio_context_change(blk, true); /* Only set speed when necessary to avoid NotSupported error */ diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_iothread_common(enum drain_type drain_type, int drain_thread) &error_abort); s = bs->opaque; blk_insert_bs(blk, bs, &error_abort); + blk_set_disable_request_queuing(blk, true); blk_set_aio_context(blk, ctx_a, &error_abort); aio_context_acquire(ctx_a); -- 2.20.1 bdrv_create options specified with -o have no effect when skipping image creation with -n, so this doesn't make sense. Warn against the misuse and deprecate the combination so we can make it a hard error later. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- qemu-img.c | 5 +++++ qemu-deprecated.texi | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/qemu-img.c b/qemu-img.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -XXX,XX +XXX,XX @@ static int img_convert(int argc, char **argv) goto fail_getopt; } + if (skip_create && options) { + warn_report("-o has no effect when skipping image creation"); + warn_report("This will become an error in future QEMU versions."); + } + s.src_num = argc - optind - 1; out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL; diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi index XXXXXXX..XXXXXXX 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -XXX,XX +XXX,XX @@ to just export the entire image and then mount only /dev/nbd0p1 than it is to reinvoke @command{qemu-nbd -c /dev/nbd0} limited to just a subset of the image. +@subsection qemu-img convert -n -o (since 4.2.0) + +All options specified in @option{-o} are image creation options, so +they have no effect when used with @option{-n} to skip image creation. +Silently ignored options can be confusing, so this combination of +options will be made an error in future versions. + @section Build system @subsection Python 2 support (since 4.1.0) -- 2.20.1 From: Nir Soffer <nirsof@gmail.com> In some cases buf_align or request_alignment cannot be detected: 1. With Gluster, buf_align cannot be detected since the actual I/O is done on Gluster server, and qemu buffer alignment does not matter. Since we don't have alignment requirement, buf_align=1 is the best value. 2. With local XFS filesystem, buf_align cannot be detected if reading from unallocated area. In this we must align the buffer, but we don't know what is the correct size. Using the wrong alignment results in I/O error. 3. With Gluster backed by XFS, request_alignment cannot be detected if reading from unallocated area. In this case we need to use the correct alignment, and failing to do so results in I/O errors. 4. With NFS, the server does not use direct I/O, so both buf_align cannot be detected. In this case we don't need any alignment so we can use buf_align=1 and request_alignment=1. These cases seems to work when storage sector size is 512 bytes, because the current code starts checking align=512. If the check succeeds because alignment cannot be detected we use 512. But this does not work for storage with 4k sector size. To determine if we can detect the alignment, we probe first with align=1. If probing succeeds, maybe there are no alignment requirement (cases 1, 4) or we are probing unallocated area (cases 2, 3). Since we don't have any way to tell, we treat this as undetectable alignment. If probing with align=1 fails with EINVAL, but probing with one of the expected alignments succeeds, we know that we found a working alignment. Practically the alignment requirements are the same for buffer alignment, buffer length, and offset in file. So in case we cannot detect buf_align, we can use request alignment. If we cannot detect request alignment, we can fallback to a safe value. To use this logic, we probe first request alignment instead of buf_align. Here is a table showing the behaviour with current code (the value in parenthesis is the optimal value). Case Sector buf_align (opt) request_alignment (opt) result ====================================================================== 1 512 512 (1) 512 (512) OK 1 4096 512 (1) 4096 (4096) FAIL ---------------------------------------------------------------------- 2 512 512 (512) 512 (512) OK 2 4096 512 (4096) 4096 (4096) FAIL ---------------------------------------------------------------------- 3 512 512 (1) 512 (512) OK 3 4096 512 (1) 512 (4096) FAIL ---------------------------------------------------------------------- 4 512 512 (1) 512 (1) OK 4 4096 512 (1) 512 (1) OK Same cases with this change: Case Sector buf_align (opt) request_alignment (opt) result ====================================================================== 1 512 512 (1) 512 (512) OK 1 4096 4096 (1) 4096 (4096) OK ---------------------------------------------------------------------- 2 512 512 (512) 512 (512) OK 2 4096 4096 (4096) 4096 (4096) OK ---------------------------------------------------------------------- 3 512 4096 (1) 4096 (512) OK 3 4096 4096 (1) 4096 (4096) OK ---------------------------------------------------------------------- 4 512 4096 (1) 4096 (1) OK 4 4096 4096 (1) 4096 (1) OK I tested that provisioning VMs and copying disks on local XFS and Gluster with 4k bytes sector size work now, resolving bugs [1],[2]. I tested also on XFS, NFS, Gluster with 512 bytes sector size. [1] https://bugzilla.redhat.com/1737256 [2] https://bugzilla.redhat.com/1738657 Signed-off-by: Nir Soffer <nsoffer@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/file-posix.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) 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 void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) BDRVRawState *s = bs->opaque; char *buf; size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize()); + size_t alignments[] = {1, 512, 1024, 2048, 4096}; /* For SCSI generic devices the alignment is not really used. With buffered I/O, we don't have any restrictions. */ @@ -XXX,XX +XXX,XX @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) } #endif - /* If we could not get the sizes so far, we can only guess them */ - if (!s->buf_align) { + /* + * If we could not get the sizes so far, we can only guess them. First try + * to detect request alignment, since it is more likely to succeed. Then + * try to detect buf_align, which cannot be detected in some cases (e.g. + * Gluster). If buf_align cannot be detected, we fallback to the value of + * request_alignment. + */ + + if (!bs->bl.request_alignment) { + int i; size_t align; - buf = qemu_memalign(max_align, 2 * max_align); - for (align = 512; align <= max_align; align <<= 1) { - if (raw_is_io_aligned(fd, buf + align, max_align)) { - s->buf_align = align; + buf = qemu_memalign(max_align, max_align); + for (i = 0; i < ARRAY_SIZE(alignments); i++) { + align = alignments[i]; + if (raw_is_io_aligned(fd, buf, align)) { + /* Fallback to safe value. */ + bs->bl.request_alignment = (align != 1) ? align : max_align; break; } } qemu_vfree(buf); } - if (!bs->bl.request_alignment) { + if (!s->buf_align) { + int i; size_t align; - buf = qemu_memalign(s->buf_align, max_align); - for (align = 512; align <= max_align; align <<= 1) { - if (raw_is_io_aligned(fd, buf, align)) { - bs->bl.request_alignment = align; + buf = qemu_memalign(max_align, 2 * max_align); + for (i = 0; i < ARRAY_SIZE(alignments); i++) { + align = alignments[i]; + if (raw_is_io_aligned(fd, buf + align, max_align)) { + /* Fallback to request_aligment. */ + s->buf_align = (align != 1) ? align : bs->bl.request_alignment; break; } } -- 2.20.1 The following changes since commit 281f327487c9c9b1599f93c589a408bbf4a651b8: Merge remote-tracking branch 'remotes/vivier/tags/m68k-for-2.12-pull-request' into staging (2017-12-22 00:11:36 +0000) are available in the git repository at: git://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 1a63a907507fbbcfaee3f622907ec244b7eabda8: block: Keep nodes drained between reopen_queue/multiple (2017-12-22 15:05:32 +0100) ---------------------------------------------------------------- Block layer patches ---------------------------------------------------------------- Doug Gale (1): nvme: Add tracing Edgar Kaziakhmedov (1): qcow2: get rid of qcow2_backing_read1 routine Fam Zheng (2): block: Open backing image in force share mode for size probe block: Remove unused bdrv_requests_pending John Snow (1): iotests: fix 197 for vpc Kevin Wolf (27): block: Formats don't need CONSISTENT_READ with NO_IO block: Make bdrv_drain_invoke() recursive block: Call .drain_begin only once in bdrv_drain_all_begin() test-bdrv-drain: Test BlockDriver callbacks for drain block: bdrv_drain_recurse(): Remove unused begin parameter block: Don't wait for requests in bdrv_drain*_end() block: Unify order in drain functions block: Don't acquire AioContext in hmp_qemu_io() block: Document that x-blockdev-change breaks quorum children list block: Assert drain_all is only called from main AioContext block: Make bdrv_drain() driver callbacks non-recursive test-bdrv-drain: Test callback for bdrv_drain test-bdrv-drain: Test bs->quiesce_counter blockjob: Pause job on draining any job BDS test-bdrv-drain: Test drain vs. block jobs block: Don't block_job_pause_all() in bdrv_drain_all() block: Nested drain_end must still call callbacks test-bdrv-drain: Test nested drain sections block: Don't notify parents in drain call chain block: Add bdrv_subtree_drained_begin/end() test-bdrv-drain: Tests for bdrv_subtree_drain test-bdrv-drain: Test behaviour in coroutine context test-bdrv-drain: Recursive draining with multiple parents block: Allow graph changes in subtree drained section test-bdrv-drain: Test graph changes in drained section commit: Simplify reopen of base block: Keep nodes drained between reopen_queue/multiple Thomas Huth (3): block: Remove the obsolete -drive boot=on|off parameter block: Remove the deprecated -hdachs option block: Mention -drive cyls/heads/secs/trans/serial/addr in deprecation chapter qapi/block-core.json | 4 + block/qcow2.h | 3 - include/block/block.h | 15 +- include/block/block_int.h | 6 +- block.c | 75 ++++- block/commit.c | 8 +- block/io.c | 164 +++++++--- block/qcow2.c | 51 +-- block/replication.c | 6 + blockdev.c | 11 - blockjob.c | 22 +- hmp.c | 6 - hw/block/nvme.c | 349 +++++++++++++++++---- qemu-io-cmds.c | 3 + tests/test-bdrv-drain.c | 651 +++++++++++++++++++++++++++++++++++++++ vl.c | 86 +----- hw/block/trace-events | 93 ++++++ qemu-doc.texi | 29 +- qemu-options.hx | 19 +- tests/Makefile.include | 2 + tests/qemu-iotests/197 | 4 + tests/qemu-iotests/common.filter | 3 +- 22 files changed, 1294 insertions(+), 316 deletions(-) create mode 100644 tests/test-bdrv-drain.c Commit 1f4ad7d fixed 'qemu-img info' for raw images that are currently in use as a mirror target. It is not enough for image formats, though, as these still unconditionally request BLK_PERM_CONSISTENT_READ. As this permission is geared towards whether the guest-visible data is consistent, and has no impact on whether the metadata is sane, and 'qemu-img info' does not read guest-visible data (except for the raw format), it makes sense to not require BLK_PERM_CONSISTENT_READ if there is not going to be any guest I/O performed, regardless of image format. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, assert(role == &child_backing || role == &child_file); if (!backing) { + int flags = bdrv_reopen_get_flags(reopen_queue, bs); + /* Apart from the modifications below, the same permissions are * forwarded and left alone as for filters */ bdrv_filter_default_perms(bs, c, role, reopen_queue, perm, shared, @@ -XXX,XX +XXX,XX @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, /* bs->file always needs to be consistent because of the metadata. We * can never allow other users to resize or write to it. */ - perm |= BLK_PERM_CONSISTENT_READ; + if (!(flags & BDRV_O_NO_IO)) { + perm |= BLK_PERM_CONSISTENT_READ; + } shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE); } else { /* We want consistent read from backing files if the parent needs it. -- 2.13.6 From: John Snow <jsnow@redhat.com> VPC has some difficulty creating geometries of particular size. However, we can indeed force it to use a literal one, so let's do that for the sake of test 197, which is testing some specific offsets. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Lukáš Doktor <ldoktor@redhat.com> --- tests/qemu-iotests/197 | 4 ++++ tests/qemu-iotests/common.filter | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/197 +++ b/tests/qemu-iotests/197 @@ -XXX,XX +XXX,XX @@ echo '=== Copy-on-read ===' echo # Prep the images +# VPC rounds image sizes to a specific geometry, force a specific size. +if [ "$IMGFMT" = "vpc" ]; then + IMGOPTS=$(_optstr_add "$IMGOPTS" "force_size") +fi _make_test_img 4G $QEMU_IO -c "write -P 55 3G 1k" "$TEST_IMG" | _filter_qemu_io IMGPROTO=file IMGFMT=qcow2 IMGOPTS= TEST_IMG_FILE="$TEST_WRAP" \ diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/common.filter +++ b/tests/qemu-iotests/common.filter @@ -XXX,XX +XXX,XX @@ _filter_img_create() -e "s# log_size=[0-9]\\+##g" \ -e "s# refcount_bits=[0-9]\\+##g" \ -e "s# key-secret=[a-zA-Z0-9]\\+##g" \ - -e "s# iter-time=[0-9]\\+##g" + -e "s# iter-time=[0-9]\\+##g" \ + -e "s# force_size=\\(on\\|off\\)##g" } _filter_img_info() -- 2.13.6 This change separates bdrv_drain_invoke(), which calls the BlockDriver drain callbacks, from bdrv_drain_recurse(). Instead, the function performs its own recursion now. One reason for this is that bdrv_drain_recurse() can be called multiple times by bdrv_drain_all_begin(), but the callbacks may only be called once. The separation is necessary to fix this bug. The other reason is that we intend to go to a model where we call all driver callbacks first, and only then start polling. This is not fully achieved yet with this patch, as bdrv_drain_invoke() contains a BDRV_POLL_WHILE() loop for the block driver callbacks, which can still call callbacks for any unrelated event. It's a step in this direction anyway. Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/io.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) 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 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque) bdrv_wakeup(bs); } +/* Recursively call BlockDriver.bdrv_co_drain_begin/end callbacks */ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin) { + BdrvChild *child, *tmp; BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin}; if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) || @@ -XXX,XX +XXX,XX @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin) data.co = qemu_coroutine_create(bdrv_drain_invoke_entry, &data); bdrv_coroutine_enter(bs, data.co); BDRV_POLL_WHILE(bs, !data.done); + + QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { + bdrv_drain_invoke(child->bs, begin); + } } static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin) @@ -XXX,XX +XXX,XX @@ static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin) BdrvChild *child, *tmp; bool waited; - /* Ensure any pending metadata writes are submitted to bs->file. */ - bdrv_drain_invoke(bs, begin); - /* Wait for drained requests to finish */ waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0); @@ -XXX,XX +XXX,XX @@ void bdrv_drained_begin(BlockDriverState *bs) bdrv_parent_drained_begin(bs); } + bdrv_drain_invoke(bs, true); bdrv_drain_recurse(bs, true); } @@ -XXX,XX +XXX,XX @@ void bdrv_drained_end(BlockDriverState *bs) } bdrv_parent_drained_end(bs); + bdrv_drain_invoke(bs, false); bdrv_drain_recurse(bs, false); aio_enable_external(bdrv_get_aio_context(bs)); } @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_begin(void) aio_context_acquire(aio_context); for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { if (aio_context == bdrv_get_aio_context(bs)) { + /* FIXME Calling this multiple times is wrong */ + bdrv_drain_invoke(bs, true); waited |= bdrv_drain_recurse(bs, true); } } @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_end(void) aio_context_acquire(aio_context); aio_enable_external(aio_context); bdrv_parent_drained_end(bs); + bdrv_drain_invoke(bs, false); bdrv_drain_recurse(bs, false); aio_context_release(aio_context); } -- 2.13.6 bdrv_drain_all_begin() used to call the .bdrv_co_drain_begin() driver callback inside its polling loop. This means that how many times it got called for each node depended on long it had to poll the event loop. This is obviously not right and results in nodes that stay drained even after bdrv_drain_all_end(), which calls .bdrv_co_drain_begin() once per node. Fix bdrv_drain_all_begin() to call the callback only once, too. Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/io.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 @@ void bdrv_drain_all_begin(void) aio_context_acquire(aio_context); bdrv_parent_drained_begin(bs); aio_disable_external(aio_context); + bdrv_drain_invoke(bs, true); aio_context_release(aio_context); if (!g_slist_find(aio_ctxs, aio_context)) { @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_begin(void) aio_context_acquire(aio_context); for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { if (aio_context == bdrv_get_aio_context(bs)) { - /* FIXME Calling this multiple times is wrong */ - bdrv_drain_invoke(bs, true); waited |= bdrv_drain_recurse(bs, true); } } -- 2.13.6 This adds a test case that the BlockDriver callbacks for drain are called in bdrv_drained_all_begin/end(), and that both of them are called exactly once. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- tests/test-bdrv-drain.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/Makefile.include | 2 + 2 files changed, 139 insertions(+) create mode 100644 tests/test-bdrv-drain.c diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ +/* + * Block node draining tests + * + * Copyright (c) 2017 Kevin Wolf <kwolf@redhat.com> + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "qemu/osdep.h" +#include "block/block.h" +#include "sysemu/block-backend.h" +#include "qapi/error.h" + +typedef struct BDRVTestState { + int drain_count; +} BDRVTestState; + +static void coroutine_fn bdrv_test_co_drain_begin(BlockDriverState *bs) +{ + BDRVTestState *s = bs->opaque; + s->drain_count++; +} + +static void coroutine_fn bdrv_test_co_drain_end(BlockDriverState *bs) +{ + BDRVTestState *s = bs->opaque; + s->drain_count--; +} + +static void bdrv_test_close(BlockDriverState *bs) +{ + BDRVTestState *s = bs->opaque; + g_assert_cmpint(s->drain_count, >, 0); +} + +static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs, + uint64_t offset, uint64_t bytes, + QEMUIOVector *qiov, int flags) +{ + /* We want this request to stay until the polling loop in drain waits for + * it to complete. We need to sleep a while as bdrv_drain_invoke() comes + * first and polls its result, too, but it shouldn't accidentally complete + * this request yet. */ + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000); + + return 0; +} + +static BlockDriver bdrv_test = { + .format_name = "test", + .instance_size = sizeof(BDRVTestState), + + .bdrv_close = bdrv_test_close, + .bdrv_co_preadv = bdrv_test_co_preadv, + + .bdrv_co_drain_begin = bdrv_test_co_drain_begin, + .bdrv_co_drain_end = bdrv_test_co_drain_end, +}; + +static void aio_ret_cb(void *opaque, int ret) +{ + int *aio_ret = opaque; + *aio_ret = ret; +} + +static void test_drv_cb_drain_all(void) +{ + BlockBackend *blk; + BlockDriverState *bs; + BDRVTestState *s; + BlockAIOCB *acb; + int aio_ret; + + QEMUIOVector qiov; + struct iovec iov = { + .iov_base = NULL, + .iov_len = 0, + }; + qemu_iovec_init_external(&qiov, &iov, 1); + + blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR, + &error_abort); + s = bs->opaque; + blk_insert_bs(blk, bs, &error_abort); + + /* Simple bdrv_drain_all_begin/end pair, check that CBs are called */ + g_assert_cmpint(s->drain_count, ==, 0); + bdrv_drain_all_begin(); + g_assert_cmpint(s->drain_count, ==, 1); + bdrv_drain_all_end(); + g_assert_cmpint(s->drain_count, ==, 0); + + /* Now do the same while a request is pending */ + aio_ret = -EINPROGRESS; + acb = blk_aio_preadv(blk, 0, &qiov, 0, aio_ret_cb, &aio_ret); + g_assert(acb != NULL); + g_assert_cmpint(aio_ret, ==, -EINPROGRESS); + + g_assert_cmpint(s->drain_count, ==, 0); + bdrv_drain_all_begin(); + g_assert_cmpint(aio_ret, ==, 0); + g_assert_cmpint(s->drain_count, ==, 1); + bdrv_drain_all_end(); + g_assert_cmpint(s->drain_count, ==, 0); + + bdrv_unref(bs); + blk_unref(blk); +} + +int main(int argc, char **argv) +{ + bdrv_init(); + qemu_init_main_loop(&error_abort); + + g_test_init(&argc, &argv, NULL); + + g_test_add_func("/bdrv-drain/driver-cb/drain_all", test_drv_cb_drain_all); + + return g_test_run(); +} diff --git a/tests/Makefile.include b/tests/Makefile.include index XXXXXXX..XXXXXXX 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -XXX,XX +XXX,XX @@ gcov-files-test-thread-pool-y = thread-pool.c gcov-files-test-hbitmap-y = util/hbitmap.c check-unit-y += tests/test-hbitmap$(EXESUF) gcov-files-test-hbitmap-y = blockjob.c +check-unit-y += tests/test-bdrv-drain$(EXESUF) check-unit-y += tests/test-blockjob$(EXESUF) check-unit-y += tests/test-blockjob-txn$(EXESUF) check-unit-y += tests/test-x86-cpuid$(EXESUF) @@ -XXX,XX +XXX,XX @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(test-block-obj-y) tests/test-aio$(EXESUF): tests/test-aio.o $(test-block-obj-y) tests/test-aio-multithread$(EXESUF): tests/test-aio-multithread.o $(test-block-obj-y) tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y) +tests/test-bdrv-drain$(EXESUF): tests/test-bdrv-drain.o $(test-block-obj-y) $(test-util-obj-y) tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-util-obj-y) tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y) tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y) -- 2.13.6 Now that the bdrv_drain_invoke() calls are pulled up to the callers of bdrv_drain_recurse(), the 'begin' parameter isn't needed any more. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/io.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin) } } -static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin) +static bool bdrv_drain_recurse(BlockDriverState *bs) { BdrvChild *child, *tmp; bool waited; @@ -XXX,XX +XXX,XX @@ static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin) */ bdrv_ref(bs); } - waited |= bdrv_drain_recurse(bs, begin); + waited |= bdrv_drain_recurse(bs); if (in_main_loop) { bdrv_unref(bs); } @@ -XXX,XX +XXX,XX @@ void bdrv_drained_begin(BlockDriverState *bs) } bdrv_drain_invoke(bs, true); - bdrv_drain_recurse(bs, true); + bdrv_drain_recurse(bs); } void bdrv_drained_end(BlockDriverState *bs) @@ -XXX,XX +XXX,XX @@ void bdrv_drained_end(BlockDriverState *bs) bdrv_parent_drained_end(bs); bdrv_drain_invoke(bs, false); - bdrv_drain_recurse(bs, false); + bdrv_drain_recurse(bs); aio_enable_external(bdrv_get_aio_context(bs)); } @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_begin(void) aio_context_acquire(aio_context); for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { if (aio_context == bdrv_get_aio_context(bs)) { - waited |= bdrv_drain_recurse(bs, true); + waited |= bdrv_drain_recurse(bs); } } aio_context_release(aio_context); @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_end(void) aio_enable_external(aio_context); bdrv_parent_drained_end(bs); bdrv_drain_invoke(bs, false); - bdrv_drain_recurse(bs, false); + bdrv_drain_recurse(bs); aio_context_release(aio_context); } -- 2.13.6 The device is drained, so there is no point in waiting for requests at the end of the drained section. Remove the bdrv_drain_recurse() calls there. The bdrv_drain_recurse() calls were introduced in commit 481cad48e5e in order to call the .bdrv_co_drain_end() driver callback. This is now done by a separate bdrv_drain_invoke() call. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/io.c | 2 -- 1 file changed, 2 deletions(-) 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 @@ void bdrv_drained_end(BlockDriverState *bs) bdrv_parent_drained_end(bs); bdrv_drain_invoke(bs, false); - bdrv_drain_recurse(bs); aio_enable_external(bdrv_get_aio_context(bs)); } @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_end(void) aio_enable_external(aio_context); bdrv_parent_drained_end(bs); bdrv_drain_invoke(bs, false); - bdrv_drain_recurse(bs); aio_context_release(aio_context); } -- 2.13.6 Drain requests are propagated to child nodes, parent nodes and directly to the AioContext. The order in which this happened was different between all combinations of drain/drain_all and begin/end. The correct order is to keep children only drained when their parents are also drained. This means that at the start of a drained section, the AioContext needs to be drained first, the parents second and only then the children. The correct order for the end of a drained section is the opposite. This patch changes the three other functions to follow the example of bdrv_drained_begin(), which is the only one that got it right. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/io.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) 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 @@ void bdrv_drained_begin(BlockDriverState *bs) return; } + /* Stop things in parent-to-child order */ if (atomic_fetch_inc(&bs->quiesce_counter) == 0) { aio_disable_external(bdrv_get_aio_context(bs)); bdrv_parent_drained_begin(bs); @@ -XXX,XX +XXX,XX @@ void bdrv_drained_end(BlockDriverState *bs) return; } - bdrv_parent_drained_end(bs); + /* Re-enable things in child-to-parent order */ bdrv_drain_invoke(bs, false); + bdrv_parent_drained_end(bs); aio_enable_external(bdrv_get_aio_context(bs)); } @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_begin(void) for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { AioContext *aio_context = bdrv_get_aio_context(bs); + /* Stop things in parent-to-child order */ aio_context_acquire(aio_context); - bdrv_parent_drained_begin(bs); aio_disable_external(aio_context); + bdrv_parent_drained_begin(bs); bdrv_drain_invoke(bs, true); aio_context_release(aio_context); @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_end(void) for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { AioContext *aio_context = bdrv_get_aio_context(bs); + /* Re-enable things in child-to-parent order */ aio_context_acquire(aio_context); - aio_enable_external(aio_context); - bdrv_parent_drained_end(bs); bdrv_drain_invoke(bs, false); + bdrv_parent_drained_end(bs); + aio_enable_external(aio_context); aio_context_release(aio_context); } -- 2.13.6 Commit 15afd94a047 added code to acquire and release the AioContext in qemuio_command(). This means that the lock is taken twice now in the call path from hmp_qemu_io(). This causes BDRV_POLL_WHILE() to hang for any requests issued to nodes in a non-mainloop AioContext. Dropping the first locking from hmp_qemu_io() fixes the problem. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- hmp.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hmp.c b/hmp.c index XXXXXXX..XXXXXXX 100644 --- a/hmp.c +++ b/hmp.c @@ -XXX,XX +XXX,XX @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) { BlockBackend *blk; BlockBackend *local_blk = NULL; - AioContext *aio_context; const char* device = qdict_get_str(qdict, "device"); const char* command = qdict_get_str(qdict, "command"); Error *err = NULL; @@ -XXX,XX +XXX,XX @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) } } - aio_context = blk_get_aio_context(blk); - aio_context_acquire(aio_context); - /* * Notably absent: Proper permission management. This is sad, but it seems * almost impossible to achieve without changing the semantics and thereby @@ -XXX,XX +XXX,XX @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) */ qemuio_command(blk, command); - aio_context_release(aio_context); - fail: blk_unref(local_blk); hmp_handle_error(mon, &err); -- 2.13.6 From: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com> Since bdrv_co_preadv does all neccessary checks including reading after the end of the backing file, avoid duplication of verification before bdrv_co_preadv call. Signed-off-by: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.h | 3 --- block/qcow2.c | 51 ++++++++------------------------------------------- 2 files changed, 8 insertions(+), 46 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -XXX,XX +XXX,XX @@ uint32_t offset_to_reftable_index(BDRVQcow2State *s, uint64_t offset) } /* qcow2.c functions */ -int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov, - int64_t sector_num, int nb_sectors); - int64_t qcow2_refcount_metadata_size(int64_t clusters, size_t cluster_size, int refcount_order, bool generous_increase, uint64_t *refblock_count); 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 int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs, return status; } -/* handle reading after the end of the backing file */ -int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov, - int64_t offset, int bytes) -{ - uint64_t bs_size = bs->total_sectors * BDRV_SECTOR_SIZE; - int n1; - - if ((offset + bytes) <= bs_size) { - return bytes; - } - - if (offset >= bs_size) { - n1 = 0; - } else { - n1 = bs_size - offset; - } - - qemu_iovec_memset(qiov, n1, 0, bytes - n1); - - return n1; -} - static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) { BDRVQcow2State *s = bs->opaque; - int offset_in_cluster, n1; + int offset_in_cluster; int ret; unsigned int cur_bytes; /* number of bytes in current iteration */ uint64_t cluster_offset = 0; @@ -XXX,XX +XXX,XX @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset, case QCOW2_CLUSTER_UNALLOCATED: if (bs->backing) { - /* read from the base image */ - n1 = qcow2_backing_read1(bs->backing->bs, &hd_qiov, - offset, cur_bytes); - if (n1 > 0) { - QEMUIOVector local_qiov; - - qemu_iovec_init(&local_qiov, hd_qiov.niov); - qemu_iovec_concat(&local_qiov, &hd_qiov, 0, n1); - - BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO); - qemu_co_mutex_unlock(&s->lock); - ret = bdrv_co_preadv(bs->backing, offset, n1, - &local_qiov, 0); - qemu_co_mutex_lock(&s->lock); - - qemu_iovec_destroy(&local_qiov); - - if (ret < 0) { - goto fail; - } + BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO); + qemu_co_mutex_unlock(&s->lock); + ret = bdrv_co_preadv(bs->backing, offset, cur_bytes, + &hd_qiov, 0); + qemu_co_mutex_lock(&s->lock); + if (ret < 0) { + goto fail; } } else { /* Note: in this case, no need to wait */ -- 2.13.6 Removing a quorum child node with x-blockdev-change results in a quorum driver state that cannot be recreated with create options because it would require a list with gaps. This causes trouble in at least .bdrv_refresh_filename(). Document this problem so that we won't accidentally mark the command stable without having addressed it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> --- qapi/block-core.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/qapi/block-core.json b/qapi/block-core.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -XXX,XX +XXX,XX @@ # does not support all kinds of operations, all kinds of children, nor # all block drivers. # +# FIXME Removing children from a quorum node means introducing gaps in the +# child indices. This cannot be represented in the 'children' list of +# BlockdevOptionsQuorum, as returned by .bdrv_refresh_filename(). +# # Warning: The data in a new quorum child MUST be consistent with that of # the rest of the array. # -- 2.13.6 From: Doug Gale <doug16k@gmail.com> Add trace output for commands, errors, and undefined behavior. Add guest error log output for undefined behavior. Report invalid undefined accesses to MMIO. Annotate unlikely error checks with unlikely. Signed-off-by: Doug Gale <doug16k@gmail.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/block/nvme.c | 349 ++++++++++++++++++++++++++++++++++++++++++-------- hw/block/trace-events | 93 ++++++++++++++ 2 files changed, 390 insertions(+), 52 deletions(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -XXX,XX +XXX,XX @@ #include "qapi/visitor.h" #include "sysemu/block-backend.h" +#include "qemu/log.h" +#include "trace.h" #include "nvme.h" +#define NVME_GUEST_ERR(trace, fmt, ...) \ + do { \ + (trace_##trace)(__VA_ARGS__); \ + qemu_log_mask(LOG_GUEST_ERROR, #trace \ + " in %s: " fmt "\n", __func__, ## __VA_ARGS__); \ + } while (0) + static void nvme_process_sq(void *opaque); static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size) @@ -XXX,XX +XXX,XX @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq) { if (cq->irq_enabled) { if (msix_enabled(&(n->parent_obj))) { + trace_nvme_irq_msix(cq->vector); msix_notify(&(n->parent_obj), cq->vector); } else { + trace_nvme_irq_pin(); pci_irq_pulse(&n->parent_obj); } + } else { + trace_nvme_irq_masked(); } } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1, trans_len = MIN(len, trans_len); int num_prps = (len >> n->page_bits) + 1; - if (!prp1) { + if (unlikely(!prp1)) { + trace_nvme_err_invalid_prp(); return NVME_INVALID_FIELD | NVME_DNR; } else if (n->cmbsz && prp1 >= n->ctrl_mem.addr && prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) { @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1, } len -= trans_len; if (len) { - if (!prp2) { + if (unlikely(!prp2)) { + trace_nvme_err_invalid_prp2_missing(); goto unmap; } if (len > n->page_size) { @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1, uint64_t prp_ent = le64_to_cpu(prp_list[i]); if (i == n->max_prp_ents - 1 && len > n->page_size) { - if (!prp_ent || prp_ent & (n->page_size - 1)) { + if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) { + trace_nvme_err_invalid_prplist_ent(prp_ent); goto unmap; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1, prp_ent = le64_to_cpu(prp_list[i]); } - if (!prp_ent || prp_ent & (n->page_size - 1)) { + if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) { + trace_nvme_err_invalid_prplist_ent(prp_ent); goto unmap; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1, i++; } } else { - if (prp2 & (n->page_size - 1)) { + if (unlikely(prp2 & (n->page_size - 1))) { + trace_nvme_err_invalid_prp2_align(prp2); goto unmap; } if (qsg->nsg) { @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len, QEMUIOVector iov; uint16_t status = NVME_SUCCESS; + trace_nvme_dma_read(prp1, prp2); + if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) { return NVME_INVALID_FIELD | NVME_DNR; } if (qsg.nsg > 0) { - if (dma_buf_read(ptr, len, &qsg)) { + if (unlikely(dma_buf_read(ptr, len, &qsg))) { + trace_nvme_err_invalid_dma(); status = NVME_INVALID_FIELD | NVME_DNR; } qemu_sglist_destroy(&qsg); } else { - if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) { + if (unlikely(qemu_iovec_to_buf(&iov, 0, ptr, len) != len)) { + trace_nvme_err_invalid_dma(); status = NVME_INVALID_FIELD | NVME_DNR; } qemu_iovec_destroy(&iov); @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd, uint64_t aio_slba = slba << (data_shift - BDRV_SECTOR_BITS); uint32_t aio_nlb = nlb << (data_shift - BDRV_SECTOR_BITS); - if (slba + nlb > ns->id_ns.nsze) { + if (unlikely(slba + nlb > ns->id_ns.nsze)) { + trace_nvme_err_invalid_lba_range(slba, nlb, ns->id_ns.nsze); return NVME_LBA_RANGE | NVME_DNR; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd, int is_write = rw->opcode == NVME_CMD_WRITE ? 1 : 0; enum BlockAcctType acct = is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ; - if ((slba + nlb) > ns->id_ns.nsze) { + trace_nvme_rw(is_write ? "write" : "read", nlb, data_size, slba); + + if (unlikely((slba + nlb) > ns->id_ns.nsze)) { block_acct_invalid(blk_get_stats(n->conf.blk), acct); + trace_nvme_err_invalid_lba_range(slba, nlb, ns->id_ns.nsze); return NVME_LBA_RANGE | NVME_DNR; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) NvmeNamespace *ns; uint32_t nsid = le32_to_cpu(cmd->nsid); - if (nsid == 0 || nsid > n->num_namespaces) { + if (unlikely(nsid == 0 || nsid > n->num_namespaces)) { + trace_nvme_err_invalid_ns(nsid, n->num_namespaces); return NVME_INVALID_NSID | NVME_DNR; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) case NVME_CMD_READ: return nvme_rw(n, ns, cmd, req); default: + trace_nvme_err_invalid_opc(cmd->opcode); return NVME_INVALID_OPCODE | NVME_DNR; } } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeCmd *cmd) NvmeCQueue *cq; uint16_t qid = le16_to_cpu(c->qid); - if (!qid || nvme_check_sqid(n, qid)) { + if (unlikely(!qid || nvme_check_sqid(n, qid))) { + trace_nvme_err_invalid_del_sq(qid); return NVME_INVALID_QID | NVME_DNR; } + trace_nvme_del_sq(qid); + sq = n->sq[qid]; while (!QTAILQ_EMPTY(&sq->out_req_list)) { req = QTAILQ_FIRST(&sq->out_req_list); @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd) uint16_t qflags = le16_to_cpu(c->sq_flags); uint64_t prp1 = le64_to_cpu(c->prp1); - if (!cqid || nvme_check_cqid(n, cqid)) { + trace_nvme_create_sq(prp1, sqid, cqid, qsize, qflags); + + if (unlikely(!cqid || nvme_check_cqid(n, cqid))) { + trace_nvme_err_invalid_create_sq_cqid(cqid); return NVME_INVALID_CQID | NVME_DNR; } - if (!sqid || !nvme_check_sqid(n, sqid)) { + if (unlikely(!sqid || !nvme_check_sqid(n, sqid))) { + trace_nvme_err_invalid_create_sq_sqid(sqid); return NVME_INVALID_QID | NVME_DNR; } - if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) { + if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) { + trace_nvme_err_invalid_create_sq_size(qsize); return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR; } - if (!prp1 || prp1 & (n->page_size - 1)) { + if (unlikely(!prp1 || prp1 & (n->page_size - 1))) { + trace_nvme_err_invalid_create_sq_addr(prp1); return NVME_INVALID_FIELD | NVME_DNR; } - if (!(NVME_SQ_FLAGS_PC(qflags))) { + if (unlikely(!(NVME_SQ_FLAGS_PC(qflags)))) { + trace_nvme_err_invalid_create_sq_qflags(NVME_SQ_FLAGS_PC(qflags)); return NVME_INVALID_FIELD | NVME_DNR; } sq = g_malloc0(sizeof(*sq)); @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_del_cq(NvmeCtrl *n, NvmeCmd *cmd) NvmeCQueue *cq; uint16_t qid = le16_to_cpu(c->qid); - if (!qid || nvme_check_cqid(n, qid)) { + if (unlikely(!qid || nvme_check_cqid(n, qid))) { + trace_nvme_err_invalid_del_cq_cqid(qid); return NVME_INVALID_CQID | NVME_DNR; } cq = n->cq[qid]; - if (!QTAILQ_EMPTY(&cq->sq_list)) { + if (unlikely(!QTAILQ_EMPTY(&cq->sq_list))) { + trace_nvme_err_invalid_del_cq_notempty(qid); return NVME_INVALID_QUEUE_DEL; } + trace_nvme_del_cq(qid); nvme_free_cq(cq, n); return NVME_SUCCESS; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd) uint16_t qflags = le16_to_cpu(c->cq_flags); uint64_t prp1 = le64_to_cpu(c->prp1); - if (!cqid || !nvme_check_cqid(n, cqid)) { + trace_nvme_create_cq(prp1, cqid, vector, qsize, qflags, + NVME_CQ_FLAGS_IEN(qflags) != 0); + + if (unlikely(!cqid || !nvme_check_cqid(n, cqid))) { + trace_nvme_err_invalid_create_cq_cqid(cqid); return NVME_INVALID_CQID | NVME_DNR; } - if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) { + if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) { + trace_nvme_err_invalid_create_cq_size(qsize); return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR; } - if (!prp1) { + if (unlikely(!prp1)) { + trace_nvme_err_invalid_create_cq_addr(prp1); return NVME_INVALID_FIELD | NVME_DNR; } - if (vector > n->num_queues) { + if (unlikely(vector > n->num_queues)) { + trace_nvme_err_invalid_create_cq_vector(vector); return NVME_INVALID_IRQ_VECTOR | NVME_DNR; } - if (!(NVME_CQ_FLAGS_PC(qflags))) { + if (unlikely(!(NVME_CQ_FLAGS_PC(qflags)))) { + trace_nvme_err_invalid_create_cq_qflags(NVME_CQ_FLAGS_PC(qflags)); return NVME_INVALID_FIELD | NVME_DNR; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeIdentify *c) uint64_t prp1 = le64_to_cpu(c->prp1); uint64_t prp2 = le64_to_cpu(c->prp2); + trace_nvme_identify_ctrl(); + return nvme_dma_read_prp(n, (uint8_t *)&n->id_ctrl, sizeof(n->id_ctrl), prp1, prp2); } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeIdentify *c) uint64_t prp1 = le64_to_cpu(c->prp1); uint64_t prp2 = le64_to_cpu(c->prp2); - if (nsid == 0 || nsid > n->num_namespaces) { + trace_nvme_identify_ns(nsid); + + if (unlikely(nsid == 0 || nsid > n->num_namespaces)) { + trace_nvme_err_invalid_ns(nsid, n->num_namespaces); return NVME_INVALID_NSID | NVME_DNR; } ns = &n->namespaces[nsid - 1]; + return nvme_dma_read_prp(n, (uint8_t *)&ns->id_ns, sizeof(ns->id_ns), prp1, prp2); } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c) uint16_t ret; int i, j = 0; + trace_nvme_identify_nslist(min_nsid); + list = g_malloc0(data_len); for (i = 0; i < n->num_namespaces; i++) { if (i < min_nsid) { @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd) case 0x02: return nvme_identify_nslist(n, c); default: + trace_nvme_err_invalid_identify_cns(le32_to_cpu(c->cns)); return NVME_INVALID_FIELD | NVME_DNR; } } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) switch (dw10) { case NVME_VOLATILE_WRITE_CACHE: result = blk_enable_write_cache(n->conf.blk); + trace_nvme_getfeat_vwcache(result ? "enabled" : "disabled"); break; case NVME_NUMBER_OF_QUEUES: result = cpu_to_le32((n->num_queues - 2) | ((n->num_queues - 2) << 16)); + trace_nvme_getfeat_numq(result); break; default: + trace_nvme_err_invalid_getfeat(dw10); return NVME_INVALID_FIELD | NVME_DNR; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) blk_set_enable_write_cache(n->conf.blk, dw11 & 1); break; case NVME_NUMBER_OF_QUEUES: + trace_nvme_setfeat_numq((dw11 & 0xFFFF) + 1, + ((dw11 >> 16) & 0xFFFF) + 1, + n->num_queues - 1, n->num_queues - 1); req->cqe.result = cpu_to_le32((n->num_queues - 2) | ((n->num_queues - 2) << 16)); break; default: + trace_nvme_err_invalid_setfeat(dw10); return NVME_INVALID_FIELD | NVME_DNR; } return NVME_SUCCESS; @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) case NVME_ADM_CMD_GET_FEATURES: return nvme_get_feature(n, cmd, req); default: + trace_nvme_err_invalid_admin_opc(cmd->opcode); return NVME_INVALID_OPCODE | NVME_DNR; } } @@ -XXX,XX +XXX,XX @@ static int nvme_start_ctrl(NvmeCtrl *n) uint32_t page_bits = NVME_CC_MPS(n->bar.cc) + 12; uint32_t page_size = 1 << page_bits; - if (n->cq[0] || n->sq[0] || !n->bar.asq || !n->bar.acq || - n->bar.asq & (page_size - 1) || n->bar.acq & (page_size - 1) || - NVME_CC_MPS(n->bar.cc) < NVME_CAP_MPSMIN(n->bar.cap) || - NVME_CC_MPS(n->bar.cc) > NVME_CAP_MPSMAX(n->bar.cap) || - NVME_CC_IOCQES(n->bar.cc) < NVME_CTRL_CQES_MIN(n->id_ctrl.cqes) || - NVME_CC_IOCQES(n->bar.cc) > NVME_CTRL_CQES_MAX(n->id_ctrl.cqes) || - NVME_CC_IOSQES(n->bar.cc) < NVME_CTRL_SQES_MIN(n->id_ctrl.sqes) || - NVME_CC_IOSQES(n->bar.cc) > NVME_CTRL_SQES_MAX(n->id_ctrl.sqes) || - !NVME_AQA_ASQS(n->bar.aqa) || !NVME_AQA_ACQS(n->bar.aqa)) { + if (unlikely(n->cq[0])) { + trace_nvme_err_startfail_cq(); + return -1; + } + if (unlikely(n->sq[0])) { + trace_nvme_err_startfail_sq(); + return -1; + } + if (unlikely(!n->bar.asq)) { + trace_nvme_err_startfail_nbarasq(); + return -1; + } + if (unlikely(!n->bar.acq)) { + trace_nvme_err_startfail_nbaracq(); + return -1; + } + if (unlikely(n->bar.asq & (page_size - 1))) { + trace_nvme_err_startfail_asq_misaligned(n->bar.asq); + return -1; + } + if (unlikely(n->bar.acq & (page_size - 1))) { + trace_nvme_err_startfail_acq_misaligned(n->bar.acq); + return -1; + } + if (unlikely(NVME_CC_MPS(n->bar.cc) < + NVME_CAP_MPSMIN(n->bar.cap))) { + trace_nvme_err_startfail_page_too_small( + NVME_CC_MPS(n->bar.cc), + NVME_CAP_MPSMIN(n->bar.cap)); + return -1; + } + if (unlikely(NVME_CC_MPS(n->bar.cc) > + NVME_CAP_MPSMAX(n->bar.cap))) { + trace_nvme_err_startfail_page_too_large( + NVME_CC_MPS(n->bar.cc), + NVME_CAP_MPSMAX(n->bar.cap)); + return -1; + } + if (unlikely(NVME_CC_IOCQES(n->bar.cc) < + NVME_CTRL_CQES_MIN(n->id_ctrl.cqes))) { + trace_nvme_err_startfail_cqent_too_small( + NVME_CC_IOCQES(n->bar.cc), + NVME_CTRL_CQES_MIN(n->bar.cap)); + return -1; + } + if (unlikely(NVME_CC_IOCQES(n->bar.cc) > + NVME_CTRL_CQES_MAX(n->id_ctrl.cqes))) { + trace_nvme_err_startfail_cqent_too_large( + NVME_CC_IOCQES(n->bar.cc), + NVME_CTRL_CQES_MAX(n->bar.cap)); + return -1; + } + if (unlikely(NVME_CC_IOSQES(n->bar.cc) < + NVME_CTRL_SQES_MIN(n->id_ctrl.sqes))) { + trace_nvme_err_startfail_sqent_too_small( + NVME_CC_IOSQES(n->bar.cc), + NVME_CTRL_SQES_MIN(n->bar.cap)); + return -1; + } + if (unlikely(NVME_CC_IOSQES(n->bar.cc) > + NVME_CTRL_SQES_MAX(n->id_ctrl.sqes))) { + trace_nvme_err_startfail_sqent_too_large( + NVME_CC_IOSQES(n->bar.cc), + NVME_CTRL_SQES_MAX(n->bar.cap)); + return -1; + } + if (unlikely(!NVME_AQA_ASQS(n->bar.aqa))) { + trace_nvme_err_startfail_asqent_sz_zero(); + return -1; + } + if (unlikely(!NVME_AQA_ACQS(n->bar.aqa))) { + trace_nvme_err_startfail_acqent_sz_zero(); return -1; } @@ -XXX,XX +XXX,XX @@ static int nvme_start_ctrl(NvmeCtrl *n) static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, unsigned size) { + if (unlikely(offset & (sizeof(uint32_t) - 1))) { + NVME_GUEST_ERR(nvme_ub_mmiowr_misaligned32, + "MMIO write not 32-bit aligned," + " offset=0x%"PRIx64"", offset); + /* should be ignored, fall through for now */ + } + + if (unlikely(size < sizeof(uint32_t))) { + NVME_GUEST_ERR(nvme_ub_mmiowr_toosmall, + "MMIO write smaller than 32-bits," + " offset=0x%"PRIx64", size=%u", + offset, size); + /* should be ignored, fall through for now */ + } + switch (offset) { - case 0xc: + case 0xc: /* INTMS */ + if (unlikely(msix_enabled(&(n->parent_obj)))) { + NVME_GUEST_ERR(nvme_ub_mmiowr_intmask_with_msix, + "undefined access to interrupt mask set" + " when MSI-X is enabled"); + /* should be ignored, fall through for now */ + } n->bar.intms |= data & 0xffffffff; n->bar.intmc = n->bar.intms; + trace_nvme_mmio_intm_set(data & 0xffffffff, + n->bar.intmc); break; - case 0x10: + case 0x10: /* INTMC */ + if (unlikely(msix_enabled(&(n->parent_obj)))) { + NVME_GUEST_ERR(nvme_ub_mmiowr_intmask_with_msix, + "undefined access to interrupt mask clr" + " when MSI-X is enabled"); + /* should be ignored, fall through for now */ + } n->bar.intms &= ~(data & 0xffffffff); n->bar.intmc = n->bar.intms; + trace_nvme_mmio_intm_clr(data & 0xffffffff, + n->bar.intmc); break; - case 0x14: + case 0x14: /* CC */ + trace_nvme_mmio_cfg(data & 0xffffffff); /* Windows first sends data, then sends enable bit */ if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) && !NVME_CC_SHN(data) && !NVME_CC_SHN(n->bar.cc)) @@ -XXX,XX +XXX,XX @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, if (NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc)) { n->bar.cc = data; - if (nvme_start_ctrl(n)) { + if (unlikely(nvme_start_ctrl(n))) { + trace_nvme_err_startfail(); n->bar.csts = NVME_CSTS_FAILED; } else { + trace_nvme_mmio_start_success(); n->bar.csts = NVME_CSTS_READY; } } else if (!NVME_CC_EN(data) && NVME_CC_EN(n->bar.cc)) { + trace_nvme_mmio_stopped(); nvme_clear_ctrl(n); n->bar.csts &= ~NVME_CSTS_READY; } if (NVME_CC_SHN(data) && !(NVME_CC_SHN(n->bar.cc))) { - nvme_clear_ctrl(n); - n->bar.cc = data; - n->bar.csts |= NVME_CSTS_SHST_COMPLETE; + trace_nvme_mmio_shutdown_set(); + nvme_clear_ctrl(n); + n->bar.cc = data; + n->bar.csts |= NVME_CSTS_SHST_COMPLETE; } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(n->bar.cc)) { - n->bar.csts &= ~NVME_CSTS_SHST_COMPLETE; - n->bar.cc = data; + trace_nvme_mmio_shutdown_cleared(); + n->bar.csts &= ~NVME_CSTS_SHST_COMPLETE; + n->bar.cc = data; + } + break; + case 0x1C: /* CSTS */ + if (data & (1 << 4)) { + NVME_GUEST_ERR(nvme_ub_mmiowr_ssreset_w1c_unsupported, + "attempted to W1C CSTS.NSSRO" + " but CAP.NSSRS is zero (not supported)"); + } else if (data != 0) { + NVME_GUEST_ERR(nvme_ub_mmiowr_ro_csts, + "attempted to set a read only bit" + " of controller status"); + } + break; + case 0x20: /* NSSR */ + if (data == 0x4E564D65) { + trace_nvme_ub_mmiowr_ssreset_unsupported(); + } else { + /* The spec says that writes of other values have no effect */ + return; } break; - case 0x24: + case 0x24: /* AQA */ n->bar.aqa = data & 0xffffffff; + trace_nvme_mmio_aqattr(data & 0xffffffff); break; - case 0x28: + case 0x28: /* ASQ */ n->bar.asq = data; + trace_nvme_mmio_asqaddr(data); break; - case 0x2c: + case 0x2c: /* ASQ hi */ n->bar.asq |= data << 32; + trace_nvme_mmio_asqaddr_hi(data, n->bar.asq); break; - case 0x30: + case 0x30: /* ACQ */ + trace_nvme_mmio_acqaddr(data); n->bar.acq = data; break; - case 0x34: + case 0x34: /* ACQ hi */ n->bar.acq |= data << 32; + trace_nvme_mmio_acqaddr_hi(data, n->bar.acq); break; + case 0x38: /* CMBLOC */ + NVME_GUEST_ERR(nvme_ub_mmiowr_cmbloc_reserved, + "invalid write to reserved CMBLOC" + " when CMBSZ is zero, ignored"); + return; + case 0x3C: /* CMBSZ */ + NVME_GUEST_ERR(nvme_ub_mmiowr_cmbsz_readonly, + "invalid write to read only CMBSZ, ignored"); + return; default: + NVME_GUEST_ERR(nvme_ub_mmiowr_invalid, + "invalid MMIO write," + " offset=0x%"PRIx64", data=%"PRIx64"", + offset, data); break; } } @@ -XXX,XX +XXX,XX @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size) uint8_t *ptr = (uint8_t *)&n->bar; uint64_t val = 0; + if (unlikely(addr & (sizeof(uint32_t) - 1))) { + NVME_GUEST_ERR(nvme_ub_mmiord_misaligned32, + "MMIO read not 32-bit aligned," + " offset=0x%"PRIx64"", addr); + /* should RAZ, fall through for now */ + } else if (unlikely(size < sizeof(uint32_t))) { + NVME_GUEST_ERR(nvme_ub_mmiord_toosmall, + "MMIO read smaller than 32-bits," + " offset=0x%"PRIx64"", addr); + /* should RAZ, fall through for now */ + } + if (addr < sizeof(n->bar)) { memcpy(&val, ptr + addr, size); + } else { + NVME_GUEST_ERR(nvme_ub_mmiord_invalid_ofs, + "MMIO read beyond last register," + " offset=0x%"PRIx64", returning 0", addr); } + return val; } @@ -XXX,XX +XXX,XX @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) { uint32_t qid; - if (addr & ((1 << 2) - 1)) { + if (unlikely(addr & ((1 << 2) - 1))) { + NVME_GUEST_ERR(nvme_ub_db_wr_misaligned, + "doorbell write not 32-bit aligned," + " offset=0x%"PRIx64", ignoring", addr); return; } if (((addr - 0x1000) >> 2) & 1) { + /* Completion queue doorbell write */ + uint16_t new_head = val & 0xffff; int start_sqs; NvmeCQueue *cq; qid = (addr - (0x1000 + (1 << 2))) >> 3; - if (nvme_check_cqid(n, qid)) { + if (unlikely(nvme_check_cqid(n, qid))) { + NVME_GUEST_ERR(nvme_ub_db_wr_invalid_cq, + "completion queue doorbell write" + " for nonexistent queue," + " sqid=%"PRIu32", ignoring", qid); return; } cq = n->cq[qid]; - if (new_head >= cq->size) { + if (unlikely(new_head >= cq->size)) { + NVME_GUEST_ERR(nvme_ub_db_wr_invalid_cqhead, + "completion queue doorbell write value" + " beyond queue size, sqid=%"PRIu32"," + " new_head=%"PRIu16", ignoring", + qid, new_head); return; } @@ -XXX,XX +XXX,XX @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) nvme_isr_notify(n, cq); } } else { + /* Submission queue doorbell write */ + uint16_t new_tail = val & 0xffff; NvmeSQueue *sq; qid = (addr - 0x1000) >> 3; - if (nvme_check_sqid(n, qid)) { + if (unlikely(nvme_check_sqid(n, qid))) { + NVME_GUEST_ERR(nvme_ub_db_wr_invalid_sq, + "submission queue doorbell write" + " for nonexistent queue," + " sqid=%"PRIu32", ignoring", qid); return; } sq = n->sq[qid]; - if (new_tail >= sq->size) { + if (unlikely(new_tail >= sq->size)) { + NVME_GUEST_ERR(nvme_ub_db_wr_invalid_sqtail, + "submission queue doorbell write value" + " beyond queue size, sqid=%"PRIu32"," + " new_tail=%"PRIu16", ignoring", + qid, new_tail); return; } diff --git a/hw/block/trace-events b/hw/block/trace-events index XXXXXXX..XXXXXXX 100644 --- a/hw/block/trace-events +++ b/hw/block/trace-events @@ -XXX,XX +XXX,XX @@ virtio_blk_submit_multireq(void *vdev, void *mrb, int start, int num_reqs, uint6 hd_geometry_lchs_guess(void *blk, int cyls, int heads, int secs) "blk %p LCHS %d %d %d" hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads, uint32_t secs, int trans) "blk %p CHS %u %u %u trans %d" +# hw/block/nvme.c +# nvme traces for successful events +nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u" +nvme_irq_pin(void) "pulsing IRQ pin" +nvme_irq_masked(void) "IRQ is masked" +nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64" prp2=0x%"PRIx64"" +nvme_rw(char const *verb, uint32_t blk_count, uint64_t byte_count, uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64"" +nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64", sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16"" +nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d" +nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16"" +nvme_del_cq(uint16_t cqid) "deleted completion queue, sqid=%"PRIu16"" +nvme_identify_ctrl(void) "identify controller" +nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16"" +nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16"" +nvme_getfeat_vwcache(char const* result) "get feature volatile write cache, result=%s" +nvme_getfeat_numq(int result) "get feature number of queues, result=%d" +nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d" +nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64"" +nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64"" +nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller config=0x%"PRIx64"" +nvme_mmio_aqattr(uint64_t data) "wrote MMIO, admin queue attributes=0x%"PRIx64"" +nvme_mmio_asqaddr(uint64_t data) "wrote MMIO, admin submission queue address=0x%"PRIx64"" +nvme_mmio_acqaddr(uint64_t data) "wrote MMIO, admin completion queue address=0x%"PRIx64"" +nvme_mmio_asqaddr_hi(uint64_t data, uint64_t new_addr) "wrote MMIO, admin submission queue high half=0x%"PRIx64", new_address=0x%"PRIx64"" +nvme_mmio_acqaddr_hi(uint64_t data, uint64_t new_addr) "wrote MMIO, admin completion queue high half=0x%"PRIx64", new_address=0x%"PRIx64"" +nvme_mmio_start_success(void) "setting controller enable bit succeeded" +nvme_mmio_stopped(void) "cleared controller enable bit" +nvme_mmio_shutdown_set(void) "shutdown bit set" +nvme_mmio_shutdown_cleared(void) "shutdown bit cleared" + +# nvme traces for error conditions +nvme_err_invalid_dma(void) "PRP/SGL is too small for transfer size" +nvme_err_invalid_prplist_ent(uint64_t prplist) "PRP list entry is null or not page aligned: 0x%"PRIx64"" +nvme_err_invalid_prp2_align(uint64_t prp2) "PRP2 is not page aligned: 0x%"PRIx64"" +nvme_err_invalid_prp2_missing(void) "PRP2 is null and more data to be transferred" +nvme_err_invalid_field(void) "invalid field" +nvme_err_invalid_prp(void) "invalid PRP" +nvme_err_invalid_sgl(void) "invalid SGL" +nvme_err_invalid_ns(uint32_t ns, uint32_t limit) "invalid namespace %u not within 1-%u" +nvme_err_invalid_opc(uint8_t opc) "invalid opcode 0x%"PRIx8"" +nvme_err_invalid_admin_opc(uint8_t opc) "invalid admin opcode 0x%"PRIx8"" +nvme_err_invalid_lba_range(uint64_t start, uint64_t len, uint64_t limit) "Invalid LBA start=%"PRIu64" len=%"PRIu64" limit=%"PRIu64"" +nvme_err_invalid_del_sq(uint16_t qid) "invalid submission queue deletion, sid=%"PRIu16"" +nvme_err_invalid_create_sq_cqid(uint16_t cqid) "failed creating submission queue, invalid cqid=%"PRIu16"" +nvme_err_invalid_create_sq_sqid(uint16_t sqid) "failed creating submission queue, invalid sqid=%"PRIu16"" +nvme_err_invalid_create_sq_size(uint16_t qsize) "failed creating submission queue, invalid qsize=%"PRIu16"" +nvme_err_invalid_create_sq_addr(uint64_t addr) "failed creating submission queue, addr=0x%"PRIx64"" +nvme_err_invalid_create_sq_qflags(uint16_t qflags) "failed creating submission queue, qflags=%"PRIu16"" +nvme_err_invalid_del_cq_cqid(uint16_t cqid) "failed deleting completion queue, cqid=%"PRIu16"" +nvme_err_invalid_del_cq_notempty(uint16_t cqid) "failed deleting completion queue, it is not empty, cqid=%"PRIu16"" +nvme_err_invalid_create_cq_cqid(uint16_t cqid) "failed creating completion queue, cqid=%"PRIu16"" +nvme_err_invalid_create_cq_size(uint16_t size) "failed creating completion queue, size=%"PRIu16"" +nvme_err_invalid_create_cq_addr(uint64_t addr) "failed creating completion queue, addr=0x%"PRIx64"" +nvme_err_invalid_create_cq_vector(uint16_t vector) "failed creating completion queue, vector=%"PRIu16"" +nvme_err_invalid_create_cq_qflags(uint16_t qflags) "failed creating completion queue, qflags=%"PRIu16"" +nvme_err_invalid_identify_cns(uint16_t cns) "identify, invalid cns=0x%"PRIx16"" +nvme_err_invalid_getfeat(int dw10) "invalid get features, dw10=0x%"PRIx32"" +nvme_err_invalid_setfeat(uint32_t dw10) "invalid set features, dw10=0x%"PRIx32"" +nvme_err_startfail_cq(void) "nvme_start_ctrl failed because there are non-admin completion queues" +nvme_err_startfail_sq(void) "nvme_start_ctrl failed because there are non-admin submission queues" +nvme_err_startfail_nbarasq(void) "nvme_start_ctrl failed because the admin submission queue address is null" +nvme_err_startfail_nbaracq(void) "nvme_start_ctrl failed because the admin completion queue address is null" +nvme_err_startfail_asq_misaligned(uint64_t addr) "nvme_start_ctrl failed because the admin submission queue address is misaligned: 0x%"PRIx64"" +nvme_err_startfail_acq_misaligned(uint64_t addr) "nvme_start_ctrl failed because the admin completion queue address is misaligned: 0x%"PRIx64"" +nvme_err_startfail_page_too_small(uint8_t log2ps, uint8_t maxlog2ps) "nvme_start_ctrl failed because the page size is too small: log2size=%u, min=%u" +nvme_err_startfail_page_too_large(uint8_t log2ps, uint8_t maxlog2ps) "nvme_start_ctrl failed because the page size is too large: log2size=%u, max=%u" +nvme_err_startfail_cqent_too_small(uint8_t log2ps, uint8_t maxlog2ps) "nvme_start_ctrl failed because the completion queue entry size is too small: log2size=%u, min=%u" +nvme_err_startfail_cqent_too_large(uint8_t log2ps, uint8_t maxlog2ps) "nvme_start_ctrl failed because the completion queue entry size is too large: log2size=%u, max=%u" +nvme_err_startfail_sqent_too_small(uint8_t log2ps, uint8_t maxlog2ps) "nvme_start_ctrl failed because the submission queue entry size is too small: log2size=%u, min=%u" +nvme_err_startfail_sqent_too_large(uint8_t log2ps, uint8_t maxlog2ps) "nvme_start_ctrl failed because the submission queue entry size is too large: log2size=%u, max=%u" +nvme_err_startfail_asqent_sz_zero(void) "nvme_start_ctrl failed because the admin submission queue size is zero" +nvme_err_startfail_acqent_sz_zero(void) "nvme_start_ctrl failed because the admin completion queue size is zero" +nvme_err_startfail(void) "setting controller enable bit failed" + +# Traces for undefined behavior +nvme_ub_mmiowr_misaligned32(uint64_t offset) "MMIO write not 32-bit aligned, offset=0x%"PRIx64"" +nvme_ub_mmiowr_toosmall(uint64_t offset, unsigned size) "MMIO write smaller than 32 bits, offset=0x%"PRIx64", size=%u" +nvme_ub_mmiowr_intmask_with_msix(void) "undefined access to interrupt mask set when MSI-X is enabled" +nvme_ub_mmiowr_ro_csts(void) "attempted to set a read only bit of controller status" +nvme_ub_mmiowr_ssreset_w1c_unsupported(void) "attempted to W1C CSTS.NSSRO but CAP.NSSRS is zero (not supported)" +nvme_ub_mmiowr_ssreset_unsupported(void) "attempted NVM subsystem reset but CAP.NSSRS is zero (not supported)" +nvme_ub_mmiowr_cmbloc_reserved(void) "invalid write to reserved CMBLOC when CMBSZ is zero, ignored" +nvme_ub_mmiowr_cmbsz_readonly(void) "invalid write to read only CMBSZ, ignored" +nvme_ub_mmiowr_invalid(uint64_t offset, uint64_t data) "invalid MMIO write, offset=0x%"PRIx64", data=0x%"PRIx64"" +nvme_ub_mmiord_misaligned32(uint64_t offset) "MMIO read not 32-bit aligned, offset=0x%"PRIx64"" +nvme_ub_mmiord_toosmall(uint64_t offset) "MMIO read smaller than 32-bits, offset=0x%"PRIx64"" +nvme_ub_mmiord_invalid_ofs(uint64_t offset) "MMIO read beyond last register, offset=0x%"PRIx64", returning 0" +nvme_ub_db_wr_misaligned(uint64_t offset) "doorbell write not 32-bit aligned, offset=0x%"PRIx64", ignoring" +nvme_ub_db_wr_invalid_cq(uint32_t qid) "completion queue doorbell write for nonexistent queue, cqid=%"PRIu32", ignoring" +nvme_ub_db_wr_invalid_cqhead(uint32_t qid, uint16_t new_head) "completion queue doorbell write value beyond queue size, cqid=%"PRIu32", new_head=%"PRIu16", ignoring" +nvme_ub_db_wr_invalid_sq(uint32_t qid) "submission queue doorbell write for nonexistent queue, sqid=%"PRIu32", ignoring" +nvme_ub_db_wr_invalid_sqtail(uint32_t qid, uint16_t new_tail) "submission queue doorbell write value beyond queue size, sqid=%"PRIu32", new_head=%"PRIu16", ignoring" + # hw/block/xen_disk.c xen_disk_alloc(char *name) "%s" xen_disk_init(char *name) "%s" -- 2.13.6 From: Fam Zheng <famz@redhat.com> Management tools create overlays of running guests with qemu-img: $ qemu-img create -b /image/in/use.qcow2 -f qcow2 /overlay/image.qcow2 but this doesn't work anymore due to image locking: qemu-img: /overlay/image.qcow2: Failed to get shared "write" lock Is another process using the image? Could not open backing image to determine size. Use the force share option to allow this use case again. Cc: qemu-stable@nongnu.org Signed-off-by: Fam Zheng <famz@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ void bdrv_img_create(const char *filename, const char *fmt, back_flags = flags; back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING); + backing_options = qdict_new(); if (backing_fmt) { - backing_options = qdict_new(); qdict_put_str(backing_options, "driver", backing_fmt); } + qdict_put_bool(backing_options, BDRV_OPT_FORCE_SHARE, true); bs = bdrv_open(full_backing, NULL, backing_options, back_flags, &local_err); -- 2.13.6 From: Thomas Huth <thuth@redhat.com> It's not working anymore since QEMU v1.3.0 - time to remove it now. Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 11 ----------- qemu-doc.texi | 6 ------ 2 files changed, 17 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ QemuOptsList qemu_legacy_drive_opts = { .type = QEMU_OPT_STRING, .help = "chs translation (auto, lba, none)", },{ - .name = "boot", - .type = QEMU_OPT_BOOL, - .help = "(deprecated, ignored)", - },{ .name = "addr", .type = QEMU_OPT_STRING, .help = "pci address (virtio only)", @@ -XXX,XX +XXX,XX @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) goto fail; } - /* Deprecated option boot=[on|off] */ - if (qemu_opt_get(legacy_opts, "boot") != NULL) { - fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be " - "ignored. Future versions will reject this parameter. Please " - "update your scripts.\n"); - } - /* Other deprecated options */ if (!qtest_enabled()) { for (i = 0; i < ARRAY_SIZE(deprecated); i++) { diff --git a/qemu-doc.texi b/qemu-doc.texi index XXXXXXX..XXXXXXX 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -XXX,XX +XXX,XX @@ deprecated. @section System emulator command line arguments -@subsection -drive boot=on|off (since 1.3.0) - -The ``boot=on|off'' option to the ``-drive'' argument is -ignored. Applications should use the ``bootindex=N'' parameter -to set an absolute ordering between devices instead. - @subsection -tdf (since 1.3.0) The ``-tdf'' argument is ignored. The behaviour implemented -- 2.13.6 From: Thomas Huth <thuth@redhat.com> It's been marked as deprecated since QEMU v2.10.0, and so far nobody complained that we should keep it, so let's remove this legacy option now to simplify the code quite a bit. Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- vl.c | 86 ++------------------------------------------------------- qemu-doc.texi | 8 ------ qemu-options.hx | 19 ++----------- 3 files changed, 4 insertions(+), 109 deletions(-) diff --git a/vl.c b/vl.c index XXXXXXX..XXXXXXX 100644 --- a/vl.c +++ b/vl.c @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv, char **envp) const char *boot_order = NULL; const char *boot_once = NULL; DisplayState *ds; - int cyls, heads, secs, translation; QemuOpts *opts, *machine_opts; - QemuOpts *hda_opts = NULL, *icount_opts = NULL, *accel_opts = NULL; + QemuOpts *icount_opts = NULL, *accel_opts = NULL; QemuOptsList *olist; int optind; const char *optarg; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv, char **envp) cpu_model = NULL; snapshot = 0; - cyls = heads = secs = 0; - translation = BIOS_ATA_TRANSLATION_AUTO; nb_nics = 0; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv, char **envp) if (optind >= argc) break; if (argv[optind][0] != '-') { - hda_opts = drive_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS); + drive_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS); } else { const QEMUOption *popt; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv, char **envp) cpu_model = optarg; break; case QEMU_OPTION_hda: - { - char buf[256]; - if (cyls == 0) - snprintf(buf, sizeof(buf), "%s", HD_OPTS); - else - snprintf(buf, sizeof(buf), - "%s,cyls=%d,heads=%d,secs=%d%s", - HD_OPTS , cyls, heads, secs, - translation == BIOS_ATA_TRANSLATION_LBA ? - ",trans=lba" : - translation == BIOS_ATA_TRANSLATION_NONE ? - ",trans=none" : ""); - drive_add(IF_DEFAULT, 0, optarg, buf); - break; - } case QEMU_OPTION_hdb: case QEMU_OPTION_hdc: case QEMU_OPTION_hdd: @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_snapshot: snapshot = 1; break; - case QEMU_OPTION_hdachs: - { - const char *p; - p = optarg; - cyls = strtol(p, (char **)&p, 0); - if (cyls < 1 || cyls > 16383) - goto chs_fail; - if (*p != ',') - goto chs_fail; - p++; - heads = strtol(p, (char **)&p, 0); - if (heads < 1 || heads > 16) - goto chs_fail; - if (*p != ',') - goto chs_fail; - p++; - secs = strtol(p, (char **)&p, 0); - if (secs < 1 || secs > 63) - goto chs_fail; - if (*p == ',') { - p++; - if (!strcmp(p, "large")) { - translation = BIOS_ATA_TRANSLATION_LARGE; - } else if (!strcmp(p, "rechs")) { - translation = BIOS_ATA_TRANSLATION_RECHS; - } else if (!strcmp(p, "none")) { - translation = BIOS_ATA_TRANSLATION_NONE; - } else if (!strcmp(p, "lba")) { - translation = BIOS_ATA_TRANSLATION_LBA; - } else if (!strcmp(p, "auto")) { - translation = BIOS_ATA_TRANSLATION_AUTO; - } else { - goto chs_fail; - } - } else if (*p != '\0') { - chs_fail: - error_report("invalid physical CHS format"); - exit(1); - } - if (hda_opts != NULL) { - qemu_opt_set_number(hda_opts, "cyls", cyls, - &error_abort); - qemu_opt_set_number(hda_opts, "heads", heads, - &error_abort); - qemu_opt_set_number(hda_opts, "secs", secs, - &error_abort); - if (translation == BIOS_ATA_TRANSLATION_LARGE) { - qemu_opt_set(hda_opts, "trans", "large", - &error_abort); - } else if (translation == BIOS_ATA_TRANSLATION_RECHS) { - qemu_opt_set(hda_opts, "trans", "rechs", - &error_abort); - } else if (translation == BIOS_ATA_TRANSLATION_LBA) { - qemu_opt_set(hda_opts, "trans", "lba", - &error_abort); - } else if (translation == BIOS_ATA_TRANSLATION_NONE) { - qemu_opt_set(hda_opts, "trans", "none", - &error_abort); - } - } - } - error_report("'-hdachs' is deprecated, please use '-device" - " ide-hd,cyls=c,heads=h,secs=s,...' instead"); - break; case QEMU_OPTION_numa: opts = qemu_opts_parse_noisily(qemu_find_opts("numa"), optarg, true); diff --git a/qemu-doc.texi b/qemu-doc.texi index XXXXXXX..XXXXXXX 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -XXX,XX +XXX,XX @@ The ``--net dump'' argument is now replaced with the ``-object filter-dump'' argument which works in combination with the modern ``-netdev`` backends instead. -@subsection -hdachs (since 2.10.0) - -The ``-hdachs'' argument is now a synonym for setting -the ``cyls'', ``heads'', ``secs'', and ``trans'' properties -on the ``ide-hd'' device using the ``-device'' argument. -The new syntax allows different settings to be provided -per disk. - @subsection -usbdevice (since 2.10.0) The ``-usbdevice DEV'' argument is now a synonym for setting diff --git a/qemu-options.hx b/qemu-options.hx index XXXXXXX..XXXXXXX 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -XXX,XX +XXX,XX @@ of available connectors of a given interface type. @item media=@var{media} This option defines the type of the media: disk or cdrom. @item cyls=@var{c},heads=@var{h},secs=@var{s}[,trans=@var{t}] -These options have the same definition as they have in @option{-hdachs}. -These parameters are deprecated, use the corresponding parameters +Force disk physical geometry and the optional BIOS translation (trans=none or +lba). These parameters are deprecated, use the corresponding parameters of @code{-device} instead. @item snapshot=@var{snapshot} @var{snapshot} is "on" or "off" and controls snapshot mode for the given drive @@ -XXX,XX +XXX,XX @@ the raw disk image you use is not written back. You can however force the write back by pressing @key{C-a s} (@pxref{disk_images}). ETEXI -DEF("hdachs", HAS_ARG, QEMU_OPTION_hdachs, \ - "-hdachs c,h,s[,t]\n" \ - " force hard disk 0 physical geometry and the optional BIOS\n" \ - " translation (t=none or lba) (usually QEMU can guess them)\n", - QEMU_ARCH_ALL) -STEXI -@item -hdachs @var{c},@var{h},@var{s},[,@var{t}] -@findex -hdachs -Force hard disk 0 physical geometry (1 <= @var{c} <= 16383, 1 <= -@var{h} <= 16, 1 <= @var{s} <= 63) and optionally force the BIOS -translation mode (@var{t}=none, lba or auto). Usually QEMU can guess -all those parameters. This option is deprecated, please use -@code{-device ide-hd,cyls=c,heads=h,secs=s,...} instead. -ETEXI - DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev, "-fsdev fsdriver,id=id[,path=path,][security_model={mapped-xattr|mapped-file|passthrough|none}]\n" " [,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n" -- 2.13.6 From: Thomas Huth <thuth@redhat.com> Looks like we forgot to announce the deprecation of these options in the corresponding chapter of the qemu-doc text, so let's do that now. Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qemu-doc.texi | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/qemu-doc.texi b/qemu-doc.texi index XXXXXXX..XXXXXXX 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -XXX,XX +XXX,XX @@ longer be directly supported in QEMU. The ``-drive if=scsi'' argument is replaced by the the ``-device BUS-TYPE'' argument combined with ``-drive if=none''. +@subsection -drive cyls=...,heads=...,secs=...,trans=... (since 2.10.0) + +The drive geometry arguments are replaced by the the geometry arguments +that can be specified with the ``-device'' parameter. + +@subsection -drive serial=... (since 2.10.0) + +The drive serial argument is replaced by the the serial argument +that can be specified with the ``-device'' parameter. + +@subsection -drive addr=... (since 2.10.0) + +The drive addr argument is replaced by the the addr argument +that can be specified with the ``-device'' parameter. + @subsection -net dump (since 2.10.0) The ``--net dump'' argument is now replaced with the -- 2.13.6 From: Fam Zheng <famz@redhat.com> Signed-off-by: Fam Zheng <famz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/block_int.h | 1 - block/io.c | 18 ------------------ 2 files changed, 19 deletions(-) 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 @@ bool blk_dev_is_tray_open(BlockBackend *blk); bool blk_dev_is_medium_locked(BlockBackend *blk); void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes); -bool bdrv_requests_pending(BlockDriverState *bs); void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out); void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in); 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 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs) assert(old >= 1); } -/* Check if any requests are in-flight (including throttled requests) */ -bool bdrv_requests_pending(BlockDriverState *bs) -{ - BdrvChild *child; - - if (atomic_read(&bs->in_flight)) { - return true; - } - - QLIST_FOREACH(child, &bs->children, next) { - if (bdrv_requests_pending(child->bs)) { - return true; - } - } - - return false; -} - typedef struct { Coroutine *co; BlockDriverState *bs; -- 2.13.6 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Fam Zheng <famz@redhat.com> --- block/io.c | 6 ++++++ 1 file changed, 6 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 @@ void bdrv_drain_all_begin(void) BdrvNextIterator it; GSList *aio_ctxs = NULL, *ctx; + /* BDRV_POLL_WHILE() for a node can only be called from its own I/O thread + * or the main loop AioContext. We potentially use BDRV_POLL_WHILE() on + * nodes in several different AioContexts, so make sure we're in the main + * context. */ + assert(qemu_get_current_aio_context() == qemu_get_aio_context()); + block_job_pause_all(); for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { -- 2.13.6 bdrv_drained_begin() doesn't increase bs->quiesce_counter recursively and also doesn't notify other parent nodes of children, which both means that the child nodes are not actually drained, and bdrv_drained_begin() is providing useful functionality only on a single node. To keep things consistent, we also shouldn't call the block driver callbacks recursively. A proper recursive drain version that provides an actually working drained section for child nodes will be introduced later. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Fam Zheng <famz@redhat.com> --- block/io.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) 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 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque) } /* Recursively call BlockDriver.bdrv_co_drain_begin/end callbacks */ -static void bdrv_drain_invoke(BlockDriverState *bs, bool begin) +static void bdrv_drain_invoke(BlockDriverState *bs, bool begin, bool recursive) { BdrvChild *child, *tmp; BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin}; @@ -XXX,XX +XXX,XX @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin) bdrv_coroutine_enter(bs, data.co); BDRV_POLL_WHILE(bs, !data.done); - QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { - bdrv_drain_invoke(child->bs, begin); + if (recursive) { + QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { + bdrv_drain_invoke(child->bs, begin, true); + } } } @@ -XXX,XX +XXX,XX @@ void bdrv_drained_begin(BlockDriverState *bs) bdrv_parent_drained_begin(bs); } - bdrv_drain_invoke(bs, true); + bdrv_drain_invoke(bs, true, false); bdrv_drain_recurse(bs); } @@ -XXX,XX +XXX,XX @@ void bdrv_drained_end(BlockDriverState *bs) } /* Re-enable things in child-to-parent order */ - bdrv_drain_invoke(bs, false); + bdrv_drain_invoke(bs, false, false); bdrv_parent_drained_end(bs); aio_enable_external(bdrv_get_aio_context(bs)); } @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_begin(void) aio_context_acquire(aio_context); aio_disable_external(aio_context); bdrv_parent_drained_begin(bs); - bdrv_drain_invoke(bs, true); + bdrv_drain_invoke(bs, true, true); aio_context_release(aio_context); if (!g_slist_find(aio_ctxs, aio_context)) { @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_end(void) /* Re-enable things in child-to-parent order */ aio_context_acquire(aio_context); - bdrv_drain_invoke(bs, false); + bdrv_drain_invoke(bs, false, true); bdrv_parent_drained_end(bs); aio_enable_external(aio_context); aio_context_release(aio_context); -- 2.13.6 The existing test is for bdrv_drain_all_begin/end() only. Generalise the test case so that it can be run for the other variants as well. At the moment this is only bdrv_drain_begin/end(), but in a while, we'll add another one. Also, add a backing file to the test node to test whether the operations work recursively. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 69 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 62 insertions(+), 7 deletions(-) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_test = { .bdrv_co_drain_begin = bdrv_test_co_drain_begin, .bdrv_co_drain_end = bdrv_test_co_drain_end, + + .bdrv_child_perm = bdrv_format_default_perms, }; static void aio_ret_cb(void *opaque, int ret) @@ -XXX,XX +XXX,XX @@ static void aio_ret_cb(void *opaque, int ret) *aio_ret = ret; } -static void test_drv_cb_drain_all(void) +enum drain_type { + BDRV_DRAIN_ALL, + BDRV_DRAIN, +}; + +static void do_drain_begin(enum drain_type drain_type, BlockDriverState *bs) +{ + switch (drain_type) { + case BDRV_DRAIN_ALL: bdrv_drain_all_begin(); break; + case BDRV_DRAIN: bdrv_drained_begin(bs); break; + default: g_assert_not_reached(); + } +} + +static void do_drain_end(enum drain_type drain_type, BlockDriverState *bs) +{ + switch (drain_type) { + case BDRV_DRAIN_ALL: bdrv_drain_all_end(); break; + case BDRV_DRAIN: bdrv_drained_end(bs); break; + default: g_assert_not_reached(); + } +} + +static void test_drv_cb_common(enum drain_type drain_type, bool recursive) { BlockBackend *blk; - BlockDriverState *bs; - BDRVTestState *s; + BlockDriverState *bs, *backing; + BDRVTestState *s, *backing_s; BlockAIOCB *acb; int aio_ret; @@ -XXX,XX +XXX,XX @@ static void test_drv_cb_drain_all(void) s = bs->opaque; blk_insert_bs(blk, bs, &error_abort); + backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort); + backing_s = backing->opaque; + bdrv_set_backing_hd(bs, backing, &error_abort); + /* Simple bdrv_drain_all_begin/end pair, check that CBs are called */ g_assert_cmpint(s->drain_count, ==, 0); - bdrv_drain_all_begin(); + g_assert_cmpint(backing_s->drain_count, ==, 0); + + do_drain_begin(drain_type, bs); + g_assert_cmpint(s->drain_count, ==, 1); - bdrv_drain_all_end(); + g_assert_cmpint(backing_s->drain_count, ==, !!recursive); + + do_drain_end(drain_type, bs); + g_assert_cmpint(s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); /* Now do the same while a request is pending */ aio_ret = -EINPROGRESS; @@ -XXX,XX +XXX,XX @@ static void test_drv_cb_drain_all(void) g_assert_cmpint(aio_ret, ==, -EINPROGRESS); g_assert_cmpint(s->drain_count, ==, 0); - bdrv_drain_all_begin(); + g_assert_cmpint(backing_s->drain_count, ==, 0); + + do_drain_begin(drain_type, bs); + g_assert_cmpint(aio_ret, ==, 0); g_assert_cmpint(s->drain_count, ==, 1); - bdrv_drain_all_end(); + g_assert_cmpint(backing_s->drain_count, ==, !!recursive); + + do_drain_end(drain_type, bs); + g_assert_cmpint(s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + bdrv_unref(backing); bdrv_unref(bs); blk_unref(blk); } +static void test_drv_cb_drain_all(void) +{ + test_drv_cb_common(BDRV_DRAIN_ALL, true); +} + +static void test_drv_cb_drain(void) +{ + test_drv_cb_common(BDRV_DRAIN, false); +} + int main(int argc, char **argv) { bdrv_init(); @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_init(&argc, &argv, NULL); g_test_add_func("/bdrv-drain/driver-cb/drain_all", test_drv_cb_drain_all); + g_test_add_func("/bdrv-drain/driver-cb/drain", test_drv_cb_drain); return g_test_run(); } -- 2.13.6 This is currently only working correctly for bdrv_drain(), not for bdrv_drain_all(). Leave a comment for the drain_all case, we'll address it later. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_drv_cb_drain(void) test_drv_cb_common(BDRV_DRAIN, false); } +static void test_quiesce_common(enum drain_type drain_type, bool recursive) +{ + BlockBackend *blk; + BlockDriverState *bs, *backing; + + blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR, + &error_abort); + blk_insert_bs(blk, bs, &error_abort); + + backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort); + bdrv_set_backing_hd(bs, backing, &error_abort); + + g_assert_cmpint(bs->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + + do_drain_begin(drain_type, bs); + + g_assert_cmpint(bs->quiesce_counter, ==, 1); + g_assert_cmpint(backing->quiesce_counter, ==, !!recursive); + + do_drain_end(drain_type, bs); + + g_assert_cmpint(bs->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + + bdrv_unref(backing); + bdrv_unref(bs); + blk_unref(blk); +} + +static void test_quiesce_drain_all(void) +{ + // XXX drain_all doesn't quiesce + //test_quiesce_common(BDRV_DRAIN_ALL, true); +} + +static void test_quiesce_drain(void) +{ + test_quiesce_common(BDRV_DRAIN, false); +} + int main(int argc, char **argv) { bdrv_init(); @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/bdrv-drain/driver-cb/drain_all", test_drv_cb_drain_all); g_test_add_func("/bdrv-drain/driver-cb/drain", test_drv_cb_drain); + g_test_add_func("/bdrv-drain/quiesce/drain_all", test_quiesce_drain_all); + g_test_add_func("/bdrv-drain/quiesce/drain", test_quiesce_drain); + return g_test_run(); } -- 2.13.6 Block jobs already paused themselves when their main BlockBackend entered a drained section. This is not good enough: We also want to pause a block job and may not submit new requests if, for example, the mirror target node should be drained. This implements .drained_begin/end callbacks in child_job in order to consider all block nodes related to the job, and removes the BlockBackend callbacks which are unnecessary now because the root of the job main BlockBackend is always referenced with a child_job, too. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockjob.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static char *child_job_get_parent_desc(BdrvChild *c) job->id); } -static const BdrvChildRole child_job = { - .get_parent_desc = child_job_get_parent_desc, - .stay_at_node = true, -}; - -static void block_job_drained_begin(void *opaque) +static void child_job_drained_begin(BdrvChild *c) { - BlockJob *job = opaque; + BlockJob *job = c->opaque; block_job_pause(job); } -static void block_job_drained_end(void *opaque) +static void child_job_drained_end(BdrvChild *c) { - BlockJob *job = opaque; + BlockJob *job = c->opaque; block_job_resume(job); } -static const BlockDevOps block_job_dev_ops = { - .drained_begin = block_job_drained_begin, - .drained_end = block_job_drained_end, +static const BdrvChildRole child_job = { + .get_parent_desc = child_job_get_parent_desc, + .drained_begin = child_job_drained_begin, + .drained_end = child_job_drained_end, + .stay_at_node = true, }; void block_job_remove_all_bdrv(BlockJob *job) @@ -XXX,XX +XXX,XX @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort); bs->job = job; - blk_set_dev_ops(blk, &block_job_dev_ops, job); bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker); QLIST_INSERT_HEAD(&block_jobs, job, job_list); -- 2.13.6 Block jobs must be paused if any of the involved nodes are drained. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ #include "qemu/osdep.h" #include "block/block.h" +#include "block/blockjob_int.h" #include "sysemu/block-backend.h" #include "qapi/error.h" @@ -XXX,XX +XXX,XX @@ static void test_quiesce_drain(void) test_quiesce_common(BDRV_DRAIN, false); } + +typedef struct TestBlockJob { + BlockJob common; + bool should_complete; +} TestBlockJob; + +static void test_job_completed(BlockJob *job, void *opaque) +{ + block_job_completed(job, 0); +} + +static void coroutine_fn test_job_start(void *opaque) +{ + TestBlockJob *s = opaque; + + while (!s->should_complete) { + block_job_sleep_ns(&s->common, 100000); + } + + block_job_defer_to_main_loop(&s->common, test_job_completed, NULL); +} + +static void test_job_complete(BlockJob *job, Error **errp) +{ + TestBlockJob *s = container_of(job, TestBlockJob, common); + s->should_complete = true; +} + +BlockJobDriver test_job_driver = { + .instance_size = sizeof(TestBlockJob), + .start = test_job_start, + .complete = test_job_complete, +}; + +static void test_blockjob_common(enum drain_type drain_type) +{ + BlockBackend *blk_src, *blk_target; + BlockDriverState *src, *target; + BlockJob *job; + int ret; + + src = bdrv_new_open_driver(&bdrv_test, "source", BDRV_O_RDWR, + &error_abort); + blk_src = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + blk_insert_bs(blk_src, src, &error_abort); + + target = bdrv_new_open_driver(&bdrv_test, "target", BDRV_O_RDWR, + &error_abort); + blk_target = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + blk_insert_bs(blk_target, target, &error_abort); + + job = block_job_create("job0", &test_job_driver, src, 0, BLK_PERM_ALL, 0, + 0, NULL, NULL, &error_abort); + block_job_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort); + block_job_start(job); + + g_assert_cmpint(job->pause_count, ==, 0); + g_assert_false(job->paused); + g_assert_false(job->busy); /* We're in block_job_sleep_ns() */ + + do_drain_begin(drain_type, src); + + if (drain_type == BDRV_DRAIN_ALL) { + /* bdrv_drain_all() drains both src and target, and involves an + * additional block_job_pause_all() */ + g_assert_cmpint(job->pause_count, ==, 3); + } else { + g_assert_cmpint(job->pause_count, ==, 1); + } + /* XXX We don't wait until the job is actually paused. Is this okay? */ + /* g_assert_true(job->paused); */ + g_assert_false(job->busy); /* The job is paused */ + + do_drain_end(drain_type, src); + + g_assert_cmpint(job->pause_count, ==, 0); + g_assert_false(job->paused); + g_assert_false(job->busy); /* We're in block_job_sleep_ns() */ + + do_drain_begin(drain_type, target); + + if (drain_type == BDRV_DRAIN_ALL) { + /* bdrv_drain_all() drains both src and target, and involves an + * additional block_job_pause_all() */ + g_assert_cmpint(job->pause_count, ==, 3); + } else { + g_assert_cmpint(job->pause_count, ==, 1); + } + /* XXX We don't wait until the job is actually paused. Is this okay? */ + /* g_assert_true(job->paused); */ + g_assert_false(job->busy); /* The job is paused */ + + do_drain_end(drain_type, target); + + g_assert_cmpint(job->pause_count, ==, 0); + g_assert_false(job->paused); + g_assert_false(job->busy); /* We're in block_job_sleep_ns() */ + + ret = block_job_complete_sync(job, &error_abort); + g_assert_cmpint(ret, ==, 0); + + blk_unref(blk_src); + blk_unref(blk_target); + bdrv_unref(src); + bdrv_unref(target); +} + +static void test_blockjob_drain_all(void) +{ + test_blockjob_common(BDRV_DRAIN_ALL); +} + +static void test_blockjob_drain(void) +{ + test_blockjob_common(BDRV_DRAIN); +} + int main(int argc, char **argv) { bdrv_init(); @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/bdrv-drain/quiesce/drain_all", test_quiesce_drain_all); g_test_add_func("/bdrv-drain/quiesce/drain", test_quiesce_drain); + g_test_add_func("/bdrv-drain/blockjob/drain_all", test_blockjob_drain_all); + g_test_add_func("/bdrv-drain/blockjob/drain", test_blockjob_drain); + return g_test_run(); } -- 2.13.6 Block jobs are already paused using the BdrvChildRole drain callbacks, so we don't need an additional block_job_pause_all() call. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/io.c | 4 ---- tests/test-bdrv-drain.c | 10 ++++------ 2 files changed, 4 insertions(+), 10 deletions(-) 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 @@ void bdrv_drain_all_begin(void) * context. */ assert(qemu_get_current_aio_context() == qemu_get_aio_context()); - block_job_pause_all(); - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { AioContext *aio_context = bdrv_get_aio_context(bs); @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_end(void) aio_enable_external(aio_context); aio_context_release(aio_context); } - - block_job_resume_all(); } void bdrv_drain_all(void) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_blockjob_common(enum drain_type drain_type) do_drain_begin(drain_type, src); if (drain_type == BDRV_DRAIN_ALL) { - /* bdrv_drain_all() drains both src and target, and involves an - * additional block_job_pause_all() */ - g_assert_cmpint(job->pause_count, ==, 3); + /* bdrv_drain_all() drains both src and target */ + g_assert_cmpint(job->pause_count, ==, 2); } else { g_assert_cmpint(job->pause_count, ==, 1); } @@ -XXX,XX +XXX,XX @@ static void test_blockjob_common(enum drain_type drain_type) do_drain_begin(drain_type, target); if (drain_type == BDRV_DRAIN_ALL) { - /* bdrv_drain_all() drains both src and target, and involves an - * additional block_job_pause_all() */ - g_assert_cmpint(job->pause_count, ==, 3); + /* bdrv_drain_all() drains both src and target */ + g_assert_cmpint(job->pause_count, ==, 2); } else { g_assert_cmpint(job->pause_count, ==, 1); } -- 2.13.6 bdrv_do_drained_begin() restricts the call of parent callbacks and aio_disable_external() to the outermost drain section, but the block driver callbacks are always called. bdrv_do_drained_end() must match this behaviour, otherwise nodes stay drained even if begin/end calls were balanced. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/io.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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 @@ void bdrv_drained_begin(BlockDriverState *bs) void bdrv_drained_end(BlockDriverState *bs) { + int old_quiesce_counter; + if (qemu_in_coroutine()) { bdrv_co_yield_to_drain(bs, false); return; } assert(bs->quiesce_counter > 0); - if (atomic_fetch_dec(&bs->quiesce_counter) > 1) { - return; - } + old_quiesce_counter = atomic_fetch_dec(&bs->quiesce_counter); /* Re-enable things in child-to-parent order */ bdrv_drain_invoke(bs, false, false); - bdrv_parent_drained_end(bs); - aio_enable_external(bdrv_get_aio_context(bs)); + if (old_quiesce_counter == 1) { + bdrv_parent_drained_end(bs); + aio_enable_external(bdrv_get_aio_context(bs)); + } } /* -- 2.13.6 Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void aio_ret_cb(void *opaque, int ret) enum drain_type { BDRV_DRAIN_ALL, BDRV_DRAIN, + DRAIN_TYPE_MAX, }; static void do_drain_begin(enum drain_type drain_type, BlockDriverState *bs) @@ -XXX,XX +XXX,XX @@ static void test_quiesce_drain(void) test_quiesce_common(BDRV_DRAIN, false); } +static void test_nested(void) +{ + BlockBackend *blk; + BlockDriverState *bs, *backing; + BDRVTestState *s, *backing_s; + enum drain_type outer, inner; + + blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR, + &error_abort); + s = bs->opaque; + blk_insert_bs(blk, bs, &error_abort); + + backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort); + backing_s = backing->opaque; + bdrv_set_backing_hd(bs, backing, &error_abort); + + for (outer = 0; outer < DRAIN_TYPE_MAX; outer++) { + for (inner = 0; inner < DRAIN_TYPE_MAX; inner++) { + /* XXX bdrv_drain_all() doesn't increase the quiesce_counter */ + int bs_quiesce = (outer != BDRV_DRAIN_ALL) + + (inner != BDRV_DRAIN_ALL); + int backing_quiesce = 0; + int backing_cb_cnt = (outer != BDRV_DRAIN) + + (inner != BDRV_DRAIN); + + g_assert_cmpint(bs->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + + do_drain_begin(outer, bs); + do_drain_begin(inner, bs); + + g_assert_cmpint(bs->quiesce_counter, ==, bs_quiesce); + g_assert_cmpint(backing->quiesce_counter, ==, backing_quiesce); + g_assert_cmpint(s->drain_count, ==, 2); + g_assert_cmpint(backing_s->drain_count, ==, backing_cb_cnt); + + do_drain_end(inner, bs); + do_drain_end(outer, bs); + + g_assert_cmpint(bs->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + } + } + + bdrv_unref(backing); + bdrv_unref(bs); + blk_unref(blk); +} + typedef struct TestBlockJob { BlockJob common; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/bdrv-drain/quiesce/drain_all", test_quiesce_drain_all); g_test_add_func("/bdrv-drain/quiesce/drain", test_quiesce_drain); + g_test_add_func("/bdrv-drain/nested", test_nested); + g_test_add_func("/bdrv-drain/blockjob/drain_all", test_blockjob_drain_all); g_test_add_func("/bdrv-drain/blockjob/drain", test_blockjob_drain); -- 2.13.6 This is in preparation for subtree drains, i.e. drained sections that affect not only a single node, but recursively all child nodes, too. Calling the parent callbacks for drain is pointless when we just came from that parent node recursively and leads to multiple increases of bs->quiesce_counter in a single drain call. Don't do it. In order for this to work correctly, the parent callback must be called for every bdrv_drain_begin/end() call, not only for the outermost one: If we have a node N with two parents A and B, recursive draining of A should cause the quiesce_counter of B to increase because its child N is drained independently of B. If now B is recursively drained, too, A must increase its quiesce_counter because N is drained independently of A only now, even if N is going from quiesce_counter 1 to 2. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/block.h | 4 ++-- block.c | 13 +++++++++---- block/io.c | 47 ++++++++++++++++++++++++++++++++++------------- 3 files changed, 45 insertions(+), 19 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 @@ void bdrv_io_unplug(BlockDriverState *bs); * Begin a quiesced section of all users of @bs. This is part of * bdrv_drained_begin. */ -void bdrv_parent_drained_begin(BlockDriverState *bs); +void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore); /** * bdrv_parent_drained_end: @@ -XXX,XX +XXX,XX @@ void bdrv_parent_drained_begin(BlockDriverState *bs); * End a quiesced section of all users of @bs. This is part of * bdrv_drained_end. */ -void bdrv_parent_drained_end(BlockDriverState *bs); +void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore); /** * bdrv_drained_begin: diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *new_bs) { BlockDriverState *old_bs = child->bs; + int i; if (old_bs && new_bs) { assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)); } if (old_bs) { if (old_bs->quiesce_counter && child->role->drained_end) { - child->role->drained_end(child); + for (i = 0; i < old_bs->quiesce_counter; i++) { + child->role->drained_end(child); + } } if (child->role->detach) { child->role->detach(child); @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, if (new_bs) { QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); if (new_bs->quiesce_counter && child->role->drained_begin) { - child->role->drained_begin(child); + for (i = 0; i < new_bs->quiesce_counter; i++) { + child->role->drained_begin(child); + } } if (child->role->attach) { @@ -XXX,XX +XXX,XX @@ void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context) AioContext *ctx = bdrv_get_aio_context(bs); aio_disable_external(ctx); - bdrv_parent_drained_begin(bs); + bdrv_parent_drained_begin(bs, NULL); bdrv_drain(bs); /* ensure there are no in-flight requests */ while (aio_poll(ctx, false)) { @@ -XXX,XX +XXX,XX @@ void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context) */ aio_context_acquire(new_context); bdrv_attach_aio_context(bs, new_context); - bdrv_parent_drained_end(bs); + bdrv_parent_drained_end(bs, NULL); aio_enable_external(ctx); aio_context_release(new_context); } 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 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes, BdrvRequestFlags flags); -void bdrv_parent_drained_begin(BlockDriverState *bs) +void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore) { BdrvChild *c, *next; QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) { + if (c == ignore) { + continue; + } if (c->role->drained_begin) { c->role->drained_begin(c); } } } -void bdrv_parent_drained_end(BlockDriverState *bs) +void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore) { BdrvChild *c, *next; QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) { + if (c == ignore) { + continue; + } if (c->role->drained_end) { c->role->drained_end(c); } @@ -XXX,XX +XXX,XX @@ typedef struct { BlockDriverState *bs; bool done; bool begin; + BdrvChild *parent; } BdrvCoDrainData; static void coroutine_fn bdrv_drain_invoke_entry(void *opaque) @@ -XXX,XX +XXX,XX @@ static bool bdrv_drain_recurse(BlockDriverState *bs) return waited; } +static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent); +static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent); + static void bdrv_co_drain_bh_cb(void *opaque) { BdrvCoDrainData *data = opaque; @@ -XXX,XX +XXX,XX @@ static void bdrv_co_drain_bh_cb(void *opaque) bdrv_dec_in_flight(bs); if (data->begin) { - bdrv_drained_begin(bs); + bdrv_do_drained_begin(bs, data->parent); } else { - bdrv_drained_end(bs); + bdrv_do_drained_end(bs, data->parent); } data->done = true; @@ -XXX,XX +XXX,XX @@ static void bdrv_co_drain_bh_cb(void *opaque) } static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, - bool begin) + bool begin, BdrvChild *parent) { BdrvCoDrainData data; @@ -XXX,XX +XXX,XX @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, .bs = bs, .done = false, .begin = begin, + .parent = parent, }; bdrv_inc_in_flight(bs); aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), @@ -XXX,XX +XXX,XX @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, assert(data.done); } -void bdrv_drained_begin(BlockDriverState *bs) +static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent) { if (qemu_in_coroutine()) { - bdrv_co_yield_to_drain(bs, true); + bdrv_co_yield_to_drain(bs, true, parent); return; } /* Stop things in parent-to-child order */ if (atomic_fetch_inc(&bs->quiesce_counter) == 0) { aio_disable_external(bdrv_get_aio_context(bs)); - bdrv_parent_drained_begin(bs); } + bdrv_parent_drained_begin(bs, parent); bdrv_drain_invoke(bs, true, false); bdrv_drain_recurse(bs); } -void bdrv_drained_end(BlockDriverState *bs) +void bdrv_drained_begin(BlockDriverState *bs) +{ + bdrv_do_drained_begin(bs, NULL); +} + +static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent) { int old_quiesce_counter; if (qemu_in_coroutine()) { - bdrv_co_yield_to_drain(bs, false); + bdrv_co_yield_to_drain(bs, false, parent); return; } assert(bs->quiesce_counter > 0); @@ -XXX,XX +XXX,XX @@ void bdrv_drained_end(BlockDriverState *bs) /* Re-enable things in child-to-parent order */ bdrv_drain_invoke(bs, false, false); + bdrv_parent_drained_end(bs, parent); if (old_quiesce_counter == 1) { - bdrv_parent_drained_end(bs); aio_enable_external(bdrv_get_aio_context(bs)); } } +void bdrv_drained_end(BlockDriverState *bs) +{ + bdrv_do_drained_end(bs, NULL); +} + /* * Wait for pending requests to complete on a single BlockDriverState subtree, * and suspend block driver's internal I/O until next request arrives. @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_begin(void) /* Stop things in parent-to-child order */ aio_context_acquire(aio_context); aio_disable_external(aio_context); - bdrv_parent_drained_begin(bs); + bdrv_parent_drained_begin(bs, NULL); bdrv_drain_invoke(bs, true, true); aio_context_release(aio_context); @@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_end(void) /* Re-enable things in child-to-parent order */ aio_context_acquire(aio_context); bdrv_drain_invoke(bs, false, true); - bdrv_parent_drained_end(bs); + bdrv_parent_drained_end(bs, NULL); aio_enable_external(aio_context); aio_context_release(aio_context); } -- 2.13.6 bdrv_drained_begin() waits for the completion of requests in the whole subtree, but it only actually keeps its immediate bs parameter quiesced until bdrv_drained_end(). Add a version that keeps the whole subtree drained. As of this commit, graph changes cannot be allowed during a subtree drained section, but this will be fixed soon. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/block.h | 13 +++++++++++++ block/io.c | 54 ++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 56 insertions(+), 11 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 @@ void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore); void bdrv_drained_begin(BlockDriverState *bs); /** + * Like bdrv_drained_begin, but recursively begins a quiesced section for + * exclusive access to all child nodes as well. + * + * Graph changes are not allowed during a subtree drain section. + */ +void bdrv_subtree_drained_begin(BlockDriverState *bs); + +/** * bdrv_drained_end: * * End a quiescent section started by bdrv_drained_begin(). */ void bdrv_drained_end(BlockDriverState *bs); +/** + * End a quiescent section started by bdrv_subtree_drained_begin(). + */ +void bdrv_subtree_drained_end(BlockDriverState *bs); + void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, Error **errp); void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); 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 @@ typedef struct { BlockDriverState *bs; bool done; bool begin; + bool recursive; BdrvChild *parent; } BdrvCoDrainData; @@ -XXX,XX +XXX,XX @@ static bool bdrv_drain_recurse(BlockDriverState *bs) return waited; } -static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent); -static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent); +static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, + BdrvChild *parent); +static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, + BdrvChild *parent); static void bdrv_co_drain_bh_cb(void *opaque) { @@ -XXX,XX +XXX,XX @@ static void bdrv_co_drain_bh_cb(void *opaque) bdrv_dec_in_flight(bs); if (data->begin) { - bdrv_do_drained_begin(bs, data->parent); + bdrv_do_drained_begin(bs, data->recursive, data->parent); } else { - bdrv_do_drained_end(bs, data->parent); + bdrv_do_drained_end(bs, data->recursive, data->parent); } data->done = true; @@ -XXX,XX +XXX,XX @@ static void bdrv_co_drain_bh_cb(void *opaque) } static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, - bool begin, BdrvChild *parent) + bool begin, bool recursive, + BdrvChild *parent) { BdrvCoDrainData data; @@ -XXX,XX +XXX,XX @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, .bs = bs, .done = false, .begin = begin, + .recursive = recursive, .parent = parent, }; bdrv_inc_in_flight(bs); @@ -XXX,XX +XXX,XX @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, assert(data.done); } -static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent) +static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, + BdrvChild *parent) { + BdrvChild *child, *next; + if (qemu_in_coroutine()) { - bdrv_co_yield_to_drain(bs, true, parent); + bdrv_co_yield_to_drain(bs, true, recursive, parent); return; } @@ -XXX,XX +XXX,XX @@ static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent) bdrv_parent_drained_begin(bs, parent); bdrv_drain_invoke(bs, true, false); bdrv_drain_recurse(bs); + + if (recursive) { + QLIST_FOREACH_SAFE(child, &bs->children, next, next) { + bdrv_do_drained_begin(child->bs, true, child); + } + } } void bdrv_drained_begin(BlockDriverState *bs) { - bdrv_do_drained_begin(bs, NULL); + bdrv_do_drained_begin(bs, false, NULL); +} + +void bdrv_subtree_drained_begin(BlockDriverState *bs) +{ + bdrv_do_drained_begin(bs, true, NULL); } -static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent) +static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, + BdrvChild *parent) { + BdrvChild *child, *next; int old_quiesce_counter; if (qemu_in_coroutine()) { - bdrv_co_yield_to_drain(bs, false, parent); + bdrv_co_yield_to_drain(bs, false, recursive, parent); return; } assert(bs->quiesce_counter > 0); @@ -XXX,XX +XXX,XX @@ static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent) if (old_quiesce_counter == 1) { aio_enable_external(bdrv_get_aio_context(bs)); } + + if (recursive) { + QLIST_FOREACH_SAFE(child, &bs->children, next, next) { + bdrv_do_drained_end(child->bs, true, child); + } + } } void bdrv_drained_end(BlockDriverState *bs) { - bdrv_do_drained_end(bs, NULL); + bdrv_do_drained_end(bs, false, NULL); +} + +void bdrv_subtree_drained_end(BlockDriverState *bs) +{ + bdrv_do_drained_end(bs, true, NULL); } /* -- 2.13.6 Add a subtree drain version to the existing test cases. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void aio_ret_cb(void *opaque, int ret) enum drain_type { BDRV_DRAIN_ALL, BDRV_DRAIN, + BDRV_SUBTREE_DRAIN, DRAIN_TYPE_MAX, }; @@ -XXX,XX +XXX,XX @@ static void do_drain_begin(enum drain_type drain_type, BlockDriverState *bs) switch (drain_type) { case BDRV_DRAIN_ALL: bdrv_drain_all_begin(); break; case BDRV_DRAIN: bdrv_drained_begin(bs); break; + case BDRV_SUBTREE_DRAIN: bdrv_subtree_drained_begin(bs); break; default: g_assert_not_reached(); } } @@ -XXX,XX +XXX,XX @@ static void do_drain_end(enum drain_type drain_type, BlockDriverState *bs) switch (drain_type) { case BDRV_DRAIN_ALL: bdrv_drain_all_end(); break; case BDRV_DRAIN: bdrv_drained_end(bs); break; + case BDRV_SUBTREE_DRAIN: bdrv_subtree_drained_end(bs); break; default: g_assert_not_reached(); } } @@ -XXX,XX +XXX,XX @@ static void test_drv_cb_drain(void) test_drv_cb_common(BDRV_DRAIN, false); } +static void test_drv_cb_drain_subtree(void) +{ + test_drv_cb_common(BDRV_SUBTREE_DRAIN, true); +} + static void test_quiesce_common(enum drain_type drain_type, bool recursive) { BlockBackend *blk; @@ -XXX,XX +XXX,XX @@ static void test_quiesce_drain(void) test_quiesce_common(BDRV_DRAIN, false); } +static void test_quiesce_drain_subtree(void) +{ + test_quiesce_common(BDRV_SUBTREE_DRAIN, true); +} + static void test_nested(void) { BlockBackend *blk; @@ -XXX,XX +XXX,XX @@ static void test_nested(void) /* XXX bdrv_drain_all() doesn't increase the quiesce_counter */ int bs_quiesce = (outer != BDRV_DRAIN_ALL) + (inner != BDRV_DRAIN_ALL); - int backing_quiesce = 0; + int backing_quiesce = (outer == BDRV_SUBTREE_DRAIN) + + (inner == BDRV_SUBTREE_DRAIN); int backing_cb_cnt = (outer != BDRV_DRAIN) + (inner != BDRV_DRAIN); @@ -XXX,XX +XXX,XX @@ static void test_blockjob_drain(void) test_blockjob_common(BDRV_DRAIN); } +static void test_blockjob_drain_subtree(void) +{ + test_blockjob_common(BDRV_SUBTREE_DRAIN); +} + int main(int argc, char **argv) { bdrv_init(); @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/bdrv-drain/driver-cb/drain_all", test_drv_cb_drain_all); g_test_add_func("/bdrv-drain/driver-cb/drain", test_drv_cb_drain); + g_test_add_func("/bdrv-drain/driver-cb/drain_subtree", + test_drv_cb_drain_subtree); g_test_add_func("/bdrv-drain/quiesce/drain_all", test_quiesce_drain_all); g_test_add_func("/bdrv-drain/quiesce/drain", test_quiesce_drain); + g_test_add_func("/bdrv-drain/quiesce/drain_subtree", + test_quiesce_drain_subtree); g_test_add_func("/bdrv-drain/nested", test_nested); g_test_add_func("/bdrv-drain/blockjob/drain_all", test_blockjob_drain_all); g_test_add_func("/bdrv-drain/blockjob/drain", test_blockjob_drain); + g_test_add_func("/bdrv-drain/blockjob/drain_subtree", + test_blockjob_drain_subtree); return g_test_run(); } -- 2.13.6 If bdrv_do_drained_begin/end() are called in coroutine context, they first use a BH to get out of the coroutine context. Call some existing tests again from a coroutine to cover this code path. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void aio_ret_cb(void *opaque, int ret) *aio_ret = ret; } +typedef struct CallInCoroutineData { + void (*entry)(void); + bool done; +} CallInCoroutineData; + +static coroutine_fn void call_in_coroutine_entry(void *opaque) +{ + CallInCoroutineData *data = opaque; + + data->entry(); + data->done = true; +} + +static void call_in_coroutine(void (*entry)(void)) +{ + Coroutine *co; + CallInCoroutineData data = { + .entry = entry, + .done = false, + }; + + co = qemu_coroutine_create(call_in_coroutine_entry, &data); + qemu_coroutine_enter(co); + while (!data.done) { + aio_poll(qemu_get_aio_context(), true); + } +} + enum drain_type { BDRV_DRAIN_ALL, BDRV_DRAIN, @@ -XXX,XX +XXX,XX @@ static void test_drv_cb_drain_subtree(void) test_drv_cb_common(BDRV_SUBTREE_DRAIN, true); } +static void test_drv_cb_co_drain(void) +{ + call_in_coroutine(test_drv_cb_drain); +} + +static void test_drv_cb_co_drain_subtree(void) +{ + call_in_coroutine(test_drv_cb_drain_subtree); +} + static void test_quiesce_common(enum drain_type drain_type, bool recursive) { BlockBackend *blk; @@ -XXX,XX +XXX,XX @@ static void test_quiesce_drain_subtree(void) test_quiesce_common(BDRV_SUBTREE_DRAIN, true); } +static void test_quiesce_co_drain(void) +{ + call_in_coroutine(test_quiesce_drain); +} + +static void test_quiesce_co_drain_subtree(void) +{ + call_in_coroutine(test_quiesce_drain_subtree); +} + static void test_nested(void) { BlockBackend *blk; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/bdrv-drain/driver-cb/drain_subtree", test_drv_cb_drain_subtree); + // XXX bdrv_drain_all() doesn't work in coroutine context + g_test_add_func("/bdrv-drain/driver-cb/co/drain", test_drv_cb_co_drain); + g_test_add_func("/bdrv-drain/driver-cb/co/drain_subtree", + test_drv_cb_co_drain_subtree); + + g_test_add_func("/bdrv-drain/quiesce/drain_all", test_quiesce_drain_all); g_test_add_func("/bdrv-drain/quiesce/drain", test_quiesce_drain); g_test_add_func("/bdrv-drain/quiesce/drain_subtree", test_quiesce_drain_subtree); + // XXX bdrv_drain_all() doesn't work in coroutine context + g_test_add_func("/bdrv-drain/quiesce/co/drain", test_quiesce_co_drain); + g_test_add_func("/bdrv-drain/quiesce/co/drain_subtree", + test_quiesce_co_drain_subtree); + g_test_add_func("/bdrv-drain/nested", test_nested); g_test_add_func("/bdrv-drain/blockjob/drain_all", test_blockjob_drain_all); -- 2.13.6 Test that drain sections are correctly propagated through the graph. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_nested(void) blk_unref(blk); } +static void test_multiparent(void) +{ + BlockBackend *blk_a, *blk_b; + BlockDriverState *bs_a, *bs_b, *backing; + BDRVTestState *a_s, *b_s, *backing_s; + + blk_a = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + bs_a = bdrv_new_open_driver(&bdrv_test, "test-node-a", BDRV_O_RDWR, + &error_abort); + a_s = bs_a->opaque; + blk_insert_bs(blk_a, bs_a, &error_abort); + + blk_b = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + bs_b = bdrv_new_open_driver(&bdrv_test, "test-node-b", BDRV_O_RDWR, + &error_abort); + b_s = bs_b->opaque; + blk_insert_bs(blk_b, bs_b, &error_abort); + + backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort); + backing_s = backing->opaque; + bdrv_set_backing_hd(bs_a, backing, &error_abort); + bdrv_set_backing_hd(bs_b, backing, &error_abort); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 0); + g_assert_cmpint(bs_b->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(a_s->drain_count, ==, 0); + g_assert_cmpint(b_s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + + do_drain_begin(BDRV_SUBTREE_DRAIN, bs_a); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 1); + g_assert_cmpint(bs_b->quiesce_counter, ==, 1); + g_assert_cmpint(backing->quiesce_counter, ==, 1); + g_assert_cmpint(a_s->drain_count, ==, 1); + g_assert_cmpint(b_s->drain_count, ==, 1); + g_assert_cmpint(backing_s->drain_count, ==, 1); + + do_drain_begin(BDRV_SUBTREE_DRAIN, bs_b); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 2); + g_assert_cmpint(bs_b->quiesce_counter, ==, 2); + g_assert_cmpint(backing->quiesce_counter, ==, 2); + g_assert_cmpint(a_s->drain_count, ==, 2); + g_assert_cmpint(b_s->drain_count, ==, 2); + g_assert_cmpint(backing_s->drain_count, ==, 2); + + do_drain_end(BDRV_SUBTREE_DRAIN, bs_b); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 1); + g_assert_cmpint(bs_b->quiesce_counter, ==, 1); + g_assert_cmpint(backing->quiesce_counter, ==, 1); + g_assert_cmpint(a_s->drain_count, ==, 1); + g_assert_cmpint(b_s->drain_count, ==, 1); + g_assert_cmpint(backing_s->drain_count, ==, 1); + + do_drain_end(BDRV_SUBTREE_DRAIN, bs_a); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 0); + g_assert_cmpint(bs_b->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(a_s->drain_count, ==, 0); + g_assert_cmpint(b_s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + + bdrv_unref(backing); + bdrv_unref(bs_a); + bdrv_unref(bs_b); + blk_unref(blk_a); + blk_unref(blk_b); +} + typedef struct TestBlockJob { BlockJob common; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) test_quiesce_co_drain_subtree); g_test_add_func("/bdrv-drain/nested", test_nested); + g_test_add_func("/bdrv-drain/multiparent", test_multiparent); g_test_add_func("/bdrv-drain/blockjob/drain_all", test_blockjob_drain_all); g_test_add_func("/bdrv-drain/blockjob/drain", test_blockjob_drain); -- 2.13.6 We need to remember how many of the drain sections in which a node is were recursive (i.e. subtree drain rather than node drain), so that they can be correctly applied when children are added or removed during the drained section. With this change, it is safe to modify the graph even inside a bdrv_subtree_drained_begin/end() section. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/block.h | 2 -- include/block/block_int.h | 5 +++++ block.c | 32 +++++++++++++++++++++++++++++--- block/io.c | 28 ++++++++++++++++++++++++---- 4 files changed, 58 insertions(+), 9 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 @@ void bdrv_drained_begin(BlockDriverState *bs); /** * Like bdrv_drained_begin, but recursively begins a quiesced section for * exclusive access to all child nodes as well. - * - * Graph changes are not allowed during a subtree drain section. */ void bdrv_subtree_drained_begin(BlockDriverState *bs); 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 BlockDriverState { /* Accessed with atomic ops. */ int quiesce_counter; + int recursive_quiesce_counter; + unsigned int write_gen; /* Current data generation */ /* Protected by reqs_lock. */ @@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child, int64_t offset, unsigned int bytes, QEMUIOVector *qiov, BdrvRequestFlags flags); +void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent); +void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent); + int get_tmp_filename(char *filename, int size); BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, const char *filename); diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static void bdrv_child_cb_drained_end(BdrvChild *child) bdrv_drained_end(bs); } +static void bdrv_child_cb_attach(BdrvChild *child) +{ + BlockDriverState *bs = child->opaque; + bdrv_apply_subtree_drain(child, bs); +} + +static void bdrv_child_cb_detach(BdrvChild *child) +{ + BlockDriverState *bs = child->opaque; + bdrv_unapply_subtree_drain(child, bs); +} + static int bdrv_child_cb_inactivate(BdrvChild *child) { BlockDriverState *bs = child->opaque; @@ -XXX,XX +XXX,XX @@ const BdrvChildRole child_file = { .inherit_options = bdrv_inherited_options, .drained_begin = bdrv_child_cb_drained_begin, .drained_end = bdrv_child_cb_drained_end, + .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate, }; @@ -XXX,XX +XXX,XX @@ const BdrvChildRole child_format = { .inherit_options = bdrv_inherited_fmt_options, .drained_begin = bdrv_child_cb_drained_begin, .drained_end = bdrv_child_cb_drained_end, + .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate, }; @@ -XXX,XX +XXX,XX @@ static void bdrv_backing_attach(BdrvChild *c) parent->backing_blocker); bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_TARGET, parent->backing_blocker); + + bdrv_child_cb_attach(c); } static void bdrv_backing_detach(BdrvChild *c) @@ -XXX,XX +XXX,XX @@ static void bdrv_backing_detach(BdrvChild *c) bdrv_op_unblock_all(c->bs, parent->backing_blocker); error_free(parent->backing_blocker); parent->backing_blocker = NULL; + + bdrv_child_cb_detach(c); } /* @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)); } if (old_bs) { + /* Detach first so that the recursive drain sections coming from @child + * are already gone and we only end the drain sections that came from + * elsewhere. */ + if (child->role->detach) { + child->role->detach(child); + } if (old_bs->quiesce_counter && child->role->drained_end) { for (i = 0; i < old_bs->quiesce_counter; i++) { child->role->drained_end(child); } } - if (child->role->detach) { - child->role->detach(child); - } QLIST_REMOVE(child, next_parent); } @@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child, } } + /* Attach only after starting new drained sections, so that recursive + * drain sections coming from @child don't get an extra .drained_begin + * callback. */ if (child->role->attach) { child->role->attach(child); } 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 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, assert(data.done); } -static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, - BdrvChild *parent) +void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, + BdrvChild *parent) { BdrvChild *child, *next; @@ -XXX,XX +XXX,XX @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, bdrv_drain_recurse(bs); if (recursive) { + bs->recursive_quiesce_counter++; QLIST_FOREACH_SAFE(child, &bs->children, next, next) { bdrv_do_drained_begin(child->bs, true, child); } @@ -XXX,XX +XXX,XX @@ void bdrv_subtree_drained_begin(BlockDriverState *bs) bdrv_do_drained_begin(bs, true, NULL); } -static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, - BdrvChild *parent) +void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, + BdrvChild *parent) { BdrvChild *child, *next; int old_quiesce_counter; @@ -XXX,XX +XXX,XX @@ static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, } if (recursive) { + bs->recursive_quiesce_counter--; QLIST_FOREACH_SAFE(child, &bs->children, next, next) { bdrv_do_drained_end(child->bs, true, child); } @@ -XXX,XX +XXX,XX @@ void bdrv_subtree_drained_end(BlockDriverState *bs) bdrv_do_drained_end(bs, true, NULL); } +void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent) +{ + int i; + + for (i = 0; i < new_parent->recursive_quiesce_counter; i++) { + bdrv_do_drained_begin(child->bs, true, child); + } +} + +void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent) +{ + int i; + + for (i = 0; i < old_parent->recursive_quiesce_counter; i++) { + bdrv_do_drained_end(child->bs, true, child); + } +} + /* * Wait for pending requests to complete on a single BlockDriverState subtree, * and suspend block driver's internal I/O until next request arrives. -- 2.13.6 Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/test-bdrv-drain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -XXX,XX +XXX,XX @@ static void test_multiparent(void) blk_unref(blk_b); } +static void test_graph_change(void) +{ + BlockBackend *blk_a, *blk_b; + BlockDriverState *bs_a, *bs_b, *backing; + BDRVTestState *a_s, *b_s, *backing_s; + + blk_a = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + bs_a = bdrv_new_open_driver(&bdrv_test, "test-node-a", BDRV_O_RDWR, + &error_abort); + a_s = bs_a->opaque; + blk_insert_bs(blk_a, bs_a, &error_abort); + + blk_b = blk_new(BLK_PERM_ALL, BLK_PERM_ALL); + bs_b = bdrv_new_open_driver(&bdrv_test, "test-node-b", BDRV_O_RDWR, + &error_abort); + b_s = bs_b->opaque; + blk_insert_bs(blk_b, bs_b, &error_abort); + + backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort); + backing_s = backing->opaque; + bdrv_set_backing_hd(bs_a, backing, &error_abort); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 0); + g_assert_cmpint(bs_b->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(a_s->drain_count, ==, 0); + g_assert_cmpint(b_s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + + do_drain_begin(BDRV_SUBTREE_DRAIN, bs_a); + do_drain_begin(BDRV_SUBTREE_DRAIN, bs_a); + do_drain_begin(BDRV_SUBTREE_DRAIN, bs_a); + do_drain_begin(BDRV_SUBTREE_DRAIN, bs_b); + do_drain_begin(BDRV_SUBTREE_DRAIN, bs_b); + + bdrv_set_backing_hd(bs_b, backing, &error_abort); + g_assert_cmpint(bs_a->quiesce_counter, ==, 5); + g_assert_cmpint(bs_b->quiesce_counter, ==, 5); + g_assert_cmpint(backing->quiesce_counter, ==, 5); + g_assert_cmpint(a_s->drain_count, ==, 5); + g_assert_cmpint(b_s->drain_count, ==, 5); + g_assert_cmpint(backing_s->drain_count, ==, 5); + + bdrv_set_backing_hd(bs_b, NULL, &error_abort); + g_assert_cmpint(bs_a->quiesce_counter, ==, 3); + g_assert_cmpint(bs_b->quiesce_counter, ==, 2); + g_assert_cmpint(backing->quiesce_counter, ==, 3); + g_assert_cmpint(a_s->drain_count, ==, 3); + g_assert_cmpint(b_s->drain_count, ==, 2); + g_assert_cmpint(backing_s->drain_count, ==, 3); + + bdrv_set_backing_hd(bs_b, backing, &error_abort); + g_assert_cmpint(bs_a->quiesce_counter, ==, 5); + g_assert_cmpint(bs_b->quiesce_counter, ==, 5); + g_assert_cmpint(backing->quiesce_counter, ==, 5); + g_assert_cmpint(a_s->drain_count, ==, 5); + g_assert_cmpint(b_s->drain_count, ==, 5); + g_assert_cmpint(backing_s->drain_count, ==, 5); + + do_drain_end(BDRV_SUBTREE_DRAIN, bs_b); + do_drain_end(BDRV_SUBTREE_DRAIN, bs_b); + do_drain_end(BDRV_SUBTREE_DRAIN, bs_a); + do_drain_end(BDRV_SUBTREE_DRAIN, bs_a); + do_drain_end(BDRV_SUBTREE_DRAIN, bs_a); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 0); + g_assert_cmpint(bs_b->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(a_s->drain_count, ==, 0); + g_assert_cmpint(b_s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + + bdrv_unref(backing); + bdrv_unref(bs_a); + bdrv_unref(bs_b); + blk_unref(blk_a); + blk_unref(blk_b); +} + typedef struct TestBlockJob { BlockJob common; @@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv) g_test_add_func("/bdrv-drain/nested", test_nested); g_test_add_func("/bdrv-drain/multiparent", test_multiparent); + g_test_add_func("/bdrv-drain/graph-change", test_graph_change); g_test_add_func("/bdrv-drain/blockjob/drain_all", test_blockjob_drain_all); g_test_add_func("/bdrv-drain/blockjob/drain", test_blockjob_drain); -- 2.13.6 Since commit bde70715, base is the only node that is reopened in commit_start(). This means that the code, which still involves an explicit BlockReopenQueue, can now be simplified by using bdrv_reopen(). Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Fam Zheng <famz@redhat.com> --- block/commit.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/block/commit.c b/block/commit.c index XXXXXXX..XXXXXXX 100644 --- a/block/commit.c +++ b/block/commit.c @@ -XXX,XX +XXX,XX @@ void commit_start(const char *job_id, BlockDriverState *bs, const char *filter_node_name, Error **errp) { CommitBlockJob *s; - BlockReopenQueue *reopen_queue = NULL; int orig_base_flags; BlockDriverState *iter; BlockDriverState *commit_top_bs = NULL; @@ -XXX,XX +XXX,XX @@ void commit_start(const char *job_id, BlockDriverState *bs, /* convert base to r/w, if necessary */ orig_base_flags = bdrv_get_flags(base); if (!(orig_base_flags & BDRV_O_RDWR)) { - reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL, - orig_base_flags | BDRV_O_RDWR); - } - - if (reopen_queue) { - bdrv_reopen_multiple(bdrv_get_aio_context(bs), reopen_queue, &local_err); + bdrv_reopen(base, orig_base_flags | BDRV_O_RDWR, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); goto fail; -- 2.13.6 The bdrv_reopen*() implementation doesn't like it if the graph is changed between queuing nodes for reopen and actually reopening them (one of the reasons is that queuing can be recursive). So instead of draining the device only in bdrv_reopen_multiple(), require that callers already drained all affected nodes, and assert this in bdrv_reopen_queue(). Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Fam Zheng <famz@redhat.com> --- block.c | 23 ++++++++++++++++------- block/replication.c | 6 ++++++ qemu-io-cmds.c | 3 +++ 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_open(const char *filename, const char *reference, * returns a pointer to bs_queue, which is either the newly allocated * bs_queue, or the existing bs_queue being used. * + * bs must be drained between bdrv_reopen_queue() and bdrv_reopen_multiple(). */ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs, @@ -XXX,XX +XXX,XX @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BdrvChild *child; QDict *old_options, *explicit_options; + /* Make sure that the caller remembered to use a drained section. This is + * important to avoid graph changes between the recursive queuing here and + * bdrv_reopen_multiple(). */ + assert(bs->quiesce_counter > 0); + if (bs_queue == NULL) { bs_queue = g_new0(BlockReopenQueue, 1); QSIMPLEQ_INIT(bs_queue); @@ -XXX,XX +XXX,XX @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, * If all devices prepare successfully, then the changes are committed * to all devices. * + * All affected nodes must be drained between bdrv_reopen_queue() and + * bdrv_reopen_multiple(). */ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **errp) { @@ -XXX,XX +XXX,XX @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er assert(bs_queue != NULL); - aio_context_release(ctx); - bdrv_drain_all_begin(); - aio_context_acquire(ctx); - QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) { + assert(bs_entry->state.bs->quiesce_counter > 0); if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, &local_err)) { error_propagate(errp, local_err); goto cleanup; @@ -XXX,XX +XXX,XX @@ cleanup: } g_free(bs_queue); - bdrv_drain_all_end(); - return ret; } @@ -XXX,XX +XXX,XX @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp) { int ret = -1; Error *local_err = NULL; - BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags); + BlockReopenQueue *queue; + bdrv_subtree_drained_begin(bs); + + queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags); ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); } + + bdrv_subtree_drained_end(bs); + return ret; } diff --git a/block/replication.c b/block/replication.c index XXXXXXX..XXXXXXX 100644 --- a/block/replication.c +++ b/block/replication.c @@ -XXX,XX +XXX,XX @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, new_secondary_flags = s->orig_secondary_flags; } + bdrv_subtree_drained_begin(s->hidden_disk->bs); + bdrv_subtree_drained_begin(s->secondary_disk->bs); + if (orig_hidden_flags != new_hidden_flags) { reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs, NULL, new_hidden_flags); @@ -XXX,XX +XXX,XX @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, reopen_queue, &local_err); error_propagate(errp, local_err); } + + bdrv_subtree_drained_end(s->hidden_disk->bs); + bdrv_subtree_drained_end(s->secondary_disk->bs); } static void backup_job_cleanup(BlockDriverState *bs) diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -XXX,XX +XXX,XX @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL; qemu_opts_reset(&reopen_opts); + bdrv_subtree_drained_begin(bs); brq = bdrv_reopen_queue(NULL, bs, opts, flags); bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err); + bdrv_subtree_drained_end(bs); + if (local_err) { error_report_err(local_err); } else { -- 2.13.6