| Message ID | 20201127144522.29991-1-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | block: update graph permissions update | expand |
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 >
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