Re: [PATCH v2 00/36] block: update graph permissions update

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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