qmp_block_dirty_bitmap_add() marks a new bitmap persistent without checking write access to its node. bdrv_create_dirty_bitmap() always creates bitmaps writable, so a persistent bitmap added to an already read-only node stays writable in memory on a node that can never store it, and the next global inactivation fails:
Lost persistent bitmaps during inactivation of node '<node>': No write access migration_block_inactivate: bdrv_inactivate_all() failed: -22 Forcing it read-only instead does not help: it was never stored, so it stays unpromotable on the next reopen to read-write and can trip bdrv_set_dirty()'s readonly assert on the first write. Reject the add instead, for both read-only and inactive nodes -- an already-inactive node skips qcow2_inactivate() on close, so a bitmap added during that window would never get stored either. Wrapped in a transaction, this denies the whole transaction, since qmp_transaction() is already all-or-none. Signed-off-by: Denis V. Lunev <[email protected]> CC: Eric Blake <[email protected]> CC: Vladimir Sementsov-Ogievskiy <[email protected]> CC: John Snow <[email protected]> CC: Andrey Drobyshev <[email protected]> --- block/monitor/bitmap-qmp-cmds.c | 15 +++++++--- qapi/block-core.json | 4 ++- .../tests/remove-bitmap-from-backing | 29 ++++++++++++++++++- .../tests/remove-bitmap-from-backing.out | 20 +++++++++++++ 4 files changed, 62 insertions(+), 6 deletions(-) diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c index a738e7bbf7..d87ca982aa 100644 --- a/block/monitor/bitmap-qmp-cmds.c +++ b/block/monitor/bitmap-qmp-cmds.c @@ -125,10 +125,17 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, disabled = false; } - if (persistent && - !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) - { - return; + if (persistent) { + if (!bdrv_is_writable(bs)) { + error_setg(errp, "Cannot add a persistent bitmap to " + "read-only or inactive node '%s'", + bdrv_get_node_name(bs)); + return; + } + + if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) { + return; + } } bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); diff --git a/qapi/block-core.json b/qapi/block-core.json index 1f87b07850..199efc1e00 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2353,7 +2353,9 @@ # @persistent: the bitmap is persistent, i.e. it will be saved to the # corresponding block device image file on its close. For now # only Qcow2 disks support persistent bitmaps. Default is false -# for `block-dirty-bitmap-add`. (Since: 2.10) +# for `block-dirty-bitmap-add`. This fails if the node is +# read-only or inactive, since such a bitmap could never be +# stored. (Since: 2.10) # # @disabled: the bitmap is created in the disabled state, which means # that it will not track drive changes. The bitmap may be enabled diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing b/tests/qemu-iotests/tests/remove-bitmap-from-backing index 15be32dcb9..a54984fa58 100755 --- a/tests/qemu-iotests/tests/remove-bitmap-from-backing +++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing @@ -35,7 +35,7 @@ qemu_img('bitmap', '--add', base, 'bitmap0') # Just assert that our method of checking bitmaps in the image works. assert 'bitmaps' in qemu_img_info(base)['format-specific']['data'] -vm = iotests.VM().add_drive(top, 'backing.node-name=base') +vm = iotests.VM().add_drive(top, 'node-name=top,backing.node-name=base') vm.launch() log('Trying to remove persistent bitmap from r-o base node, should fail:') @@ -66,6 +66,33 @@ result = vm.qmp('blockdev-reopen', **new_base_opts) if result != {'return': {}}: log('Failed to reopen: ' + str(result)) +log('Adding a persistent bitmap to the r-o base node, should fail:') +vm.qmp_log('block-dirty-bitmap-add', node='base', name='bitmap1', + persistent=True) + +log('Same add inside a transaction, preceded by an otherwise valid') +log('action: the whole transaction must fail and roll back the') +log('already-succeeded first action too:') +vm.qmp_log('transaction', actions=[ + {'type': 'block-dirty-bitmap-add', + 'data': {'node': 'top', 'name': 'bitmap2', 'persistent': True}}, + {'type': 'block-dirty-bitmap-add', + 'data': {'node': 'base', 'name': 'bitmap1', 'persistent': True}}, +]) + +log('bitmap2 on the rw top node must not have survived the rollback:') +vm.qmp_log('block-dirty-bitmap-remove', node='top', name='bitmap2') + +log('Marking the rw top node inactive:') +vm.qmp_log('blockdev-set-active', **{'node-name': 'top', 'active': False}) + +log('Adding a persistent bitmap to a rw but inactive node, should fail:') +vm.qmp_log('block-dirty-bitmap-add', node='top', name='bitmap3', + persistent=True) + +log('Reactivating the top node:') +vm.qmp_log('blockdev-set-active', **{'node-name': 'top', 'active': True}) + vm.shutdown() if 'bitmaps' in qemu_img_info(base)['format-specific']['data']: diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing.out b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out index c28af82c75..fe105fe0a3 100644 --- a/tests/qemu-iotests/tests/remove-bitmap-from-backing.out +++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out @@ -4,3 +4,23 @@ Trying to remove persistent bitmap from r-o base node, should fail: Remove persistent bitmap from base node reopened to RW: {"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "base"}} {"return": {}} +Adding a persistent bitmap to the r-o base node, should fail: +{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap1", "node": "base", "persistent": true}} +{"error": {"class": "GenericError", "desc": "Cannot add a persistent bitmap to read-only or inactive node 'base'"}} +Same add inside a transaction, preceded by an otherwise valid +action: the whole transaction must fail and roll back the +already-succeeded first action too: +{"execute": "transaction", "arguments": {"actions": [{"data": {"name": "bitmap2", "node": "top", "persistent": true}, "type": "block-dirty-bitmap-add"}, {"data": {"name": "bitmap1", "node": "base", "persistent": true}, "type": "block-dirty-bitmap-add"}]}} +{"error": {"class": "GenericError", "desc": "Cannot add a persistent bitmap to read-only or inactive node 'base'"}} +bitmap2 on the rw top node must not have survived the rollback: +{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap2", "node": "top"}} +{"error": {"class": "GenericError", "desc": "Dirty bitmap 'bitmap2' not found"}} +Marking the rw top node inactive: +{"execute": "blockdev-set-active", "arguments": {"active": false, "node-name": "top"}} +{"return": {}} +Adding a persistent bitmap to a rw but inactive node, should fail: +{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap3", "node": "top", "persistent": true}} +{"error": {"class": "GenericError", "desc": "Cannot add a persistent bitmap to read-only or inactive node 'top'"}} +Reactivating the top node: +{"execute": "blockdev-set-active", "arguments": {"active": true, "node-name": "top"}} +{"return": {}} -- 2.53.0
