The following changes since commit 9e06029aea3b2eca1d5261352e695edc1e7d7b8b:

Update version for v4.1.0 release (2019-08-15 13:03:37 +0100)

for you to fetch changes up to a6b257a08e3d72219f03e461a52152672fec0612:

file-posix: Handle undetectable alignment (2019-08-16 11:29:11 +0200)

- 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

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.

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

--- 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.

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.

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

--- 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.

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

--- 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

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

+++ b/tests/qemu-iotests/262

+#!/usr/bin/env python

+# Copyright (C) 2019 Red Hat, Inc.

+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

+++ b/tests/qemu-iotests/262.out

+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"}}

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.

Reviewed-by: Max Reitz <mreitz@redhat.com>

block.c | 12 ++----------

1 file changed, 2 insertions(+), 10 deletions(-)

@@ -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;

2.20.1

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).

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

block.c | 2 ++

1 file changed, 2 insertions(+)

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

--- 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>

block.c | 49 +++++++++++++++++++++++++++++++++----------------

1 file changed, 33 insertions(+), 16 deletions(-)

@@ -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