ping
27.11.2020 17:44, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal of updating graph changing procedures.
>
> The thing brought me here is a question about "activating" filters after
> insertion, which is done in mirror_top and backup_top. The problem is
> that we can't simply avoid permission conflict when inserting the
> filter: during insertion old permissions of relations to be removed
> conflicting with new permissions of new created relations. And current
> solution is supporting additional "inactive" mode for the filter when it
> doesn't require any permissions.
>
> I suggest to change the order of operations: let's first do all graph
> relations modifications and then refresh permissions. Of course we'll
> need a way to restore old graph if refresh fails.
>
> Another problem with permission update is that we update permissions in
> order of DFS which is not always correct. Better is update node when all
> its parents already updated and require correct permissions. This needs
> a topological sort of nodes prior to permission update, see more in
> patches later.
>
> Patches plan:
>
> 01,02 - add failing tests to illustrate conceptual problems of current
> permission update system.
> [Here is side suggestion: we usually add tests after fix, so careful
> reviewer has to change order of patches to check that test fails before
> fix. I add tests in the way the may be simply executed but not yet take
> part in make check. It seems more native: first show the problem, then
> fix it. And when fixed, make tests available for make check]
>
> 03-09 - some perparations, refactorings which may go in separate
>
> 10 - new transaction API
>
> 15 - toplogical sort implemented for permission update, one of new tests
> now pass
>
> 19 - improve bdrv_replace_node. second new test now pass
>
> 26 - drop .active field and activation procedure for backup-top!
>
> 30 - update bdrv_reopen_multiple. At this point everything is using new
> paradigm of permission update
>
> 31-36 - post refactoring, drop old interfaces, we are done.
>
> Note, that this series does nothing with another graph-update problem
> discussed under "[PATCH RFC 0/5] Fix accidental crash in iotest 30".
>
> The series based on block-next Max's branch and can be found here:
>
> git: https://src.openvz.org/scm/~vsementsov/qemu.git
> tag: up-block-topologic-perm-v2
>
> Vladimir Sementsov-Ogievskiy (36):
> tests/test-bdrv-graph-mod: add test_parallel_exclusive_write
> tests/test-bdrv-graph-mod: add test_parallel_perm_update
> block: bdrv_append(): don't consume reference
> block: bdrv_append(): return status
> block: add bdrv_parent_try_set_aio_context
> block: BdrvChildClass: add .get_parent_aio_context handler
> block: drop ctx argument from bdrv_root_attach_child
> block: make bdrv_reopen_{prepare,commit,abort} private
> block: return value from bdrv_replace_node()
> util: add transactions.c
> block: bdrv_refresh_perms: check parents compliance
> block: refactor bdrv_child* permission functions
> block: rewrite bdrv_child_try_set_perm() using bdrv_refresh_perms()
> block: inline bdrv_child_*() permission functions calls
> block: use topological sort for permission update
> block: add bdrv_drv_set_perm transaction action
> block: add bdrv_list_* permission update functions
> block: add bdrv_replace_child_safe() transaction action
> block: fix bdrv_replace_node_common
> block: add bdrv_attach_child_common() transaction action
> block: add bdrv_attach_child_noperm() transaction action
> block: split out bdrv_replace_node_noperm()
> block: adapt bdrv_append() for inserting filters
> block: add bdrv_remove_backing transaction action
> block: introduce bdrv_drop_filter()
> block/backup-top: drop .active
> block: drop ignore_children for permission update functions
> block: add bdrv_set_backing_noperm() transaction action
> blockdev: qmp_x_blockdev_reopen: acquire all contexts
> block: bdrv_reopen_multiple: refresh permissions on updated graph
> block: drop unused permission update functions
> block: inline bdrv_check_perm_common()
> block: inline bdrv_replace_child()
> block: refactor bdrv_child_set_perm_safe() transaction action
> block: rename bdrv_replace_child_safe() to bdrv_replace_child()
> block: refactor bdrv_node_check_perm()
>
> include/block/block.h | 20 +-
> include/block/block_int.h | 8 +-
> include/qemu/transactions.h | 46 ++
> block.c | 1319 ++++++++++++++++++++---------------
> block/backup-top.c | 39 +-
> block/block-backend.c | 13 +-
> block/commit.c | 7 +-
> block/file-posix.c | 84 +--
> block/mirror.c | 9 +-
> blockdev.c | 33 +-
> blockjob.c | 11 +-
> tests/test-bdrv-drain.c | 2 +-
> tests/test-bdrv-graph-mod.c | 122 +++-
> util/transactions.c | 81 +++
> tests/qemu-iotests/283.out | 2 +-
> util/meson.build | 1 +
> 16 files changed, 1100 insertions(+), 697 deletions(-)
> create mode 100644 include/qemu/transactions.h
> create mode 100644 util/transactions.c
>
--
Best regards,
Vladimir