[RFC,00/21] block: update graph permissions update

Message

Vladimir Sementsov-Ogievskiy Nov. 23, 2020, 8:12 p.m. UTC

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.

Key patches here:

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]

08 - toplogical sort implemented for permission update, one of new tests
now pass

13 - improve bdrv_replace_node. second new test now pass

21 - drop .active field and activation procedure for backup-top!

Series marked as RFC as they are non-complete. Some things like
bdrv_replace_node() and bdrv_append() are moved into new paradigm
(update graph first) some not (like bdrv_reopen_multiple()). Because of
it we have to still support old interfaces (like ignore_children).

Still, I'd be very grateful for some feedback before investigating more
time to this thing.

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

Vladimir Sementsov-Ogievskiy (21):
  tests/test-bdrv-graph-mod: add test_parallel_exclusive_write
  tests/test-bdrv-graph-mod: add test_parallel_perm_update
  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: return value from bdrv_replace_node()
  block: fix bdrv_replace_node_common
  block: add bdrv_attach_child_noperm() transaction action
  block: split out bdrv_replace_node_noperm()
  block: bdrv_append(): don't consume reference
  block: bdrv_append(): return status
  block: adapt bdrv_append() for inserting filters
  block: add bdrv_remove_backing transaction action
  block: introduce bdrv_drop_filter()
  block/backup-top: drop .active

 include/block/block.h       |   9 +-
 include/qemu/transactions.h |  46 +++
 block.c                     | 789 ++++++++++++++++++++++++++++--------
 block/backup-top.c          |  39 +-
 block/commit.c              |   7 +-
 block/mirror.c              |   9 +-
 blockdev.c                  |  10 +-
 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 +
 12 files changed, 872 insertions(+), 245 deletions(-)
 create mode 100644 include/qemu/transactions.h
 create mode 100644 util/transactions.c