On 18.09.19 17:38, Vladimir Sementsov-Ogievskiy wrote: > 12.09.2019 16:56, Max Reitz wrote: >> Hi, >> >> The fix (patch 1) is pretty straightforward; patch 2 (which I need for >> the test) may not be. >> >> The biggest problem with patch 2 is that you can use it to uncover where >> our permission handling is broken. For example, devising the test case >> (patch 4) was very difficult because I kept running into the >> &error_abort that mirror_exit_common() passes when dropping the >> mirror_top_bs. >> >> The problem is that mirror_top_bs does not take the same permissions >> that its parent takes. Ergo using &error_abort when dropping it is >> wrong: The parent may require more permissions that mirror_top_bs did, >> and so dropping mirror_top_bs may fail. >> >> Now what’s really bad is that this cannot be fixed with our current >> permission system. mirror_top_bs was introduced precisely so it does >> not take CONSISTENT_READ, but can still allow parents to take it (for >> active commits). But what if there is actually something besides the >> mirror job that unshares CONSISTENT_READ? >> >> >> Imagine this: >> >> mirror target BB mirror source BB >> | | >> v v >> mirror_top_bs -> top -> mid -> base >> ^ >> | >> other_parent >> >> The source BB unshares CONSISTENT_READ on the base. mirror_top_bs >> ensures that its parents can read from top even though top itself cannot >> allow CONSISTENT_READ to be taken. So far so good. >> >> But what if other_parent also unshares CONSISTENT_READ? Then, >> mirror_top_bs has no business allowing its parents to take it. >> >> No idea how to fix that. (I suppose mirror_top_bs would need some way >> to verify that there is no other party that has unshared CONSISTENT_READ >> but its associated source BB. > > May be we need grouped permissions? > > Some way to define group of children, which may unshare read permission > for other children (out of the group), but still children in group may > have read permission?
Hm, is that different from my idea below where one of mirror_top's children unshares the read permission, and another is allowed to take it still? (The problem is always that if some BDS has a parent that unshares this permission, this condition propagates upwards through its other parents, and we need to keep track of who unshared it in the first place.) > But it don't work here as we are saying about children on different > nodes.. And propagated through backing chain permissions.. Yep. >> In the future, we want the source BB to >> go away and instead have the source be an immediate BdrvChild of >> mirror_top_bs. Maybe we can then build something into the block layer >> so that a node can only restore CONSISTENT_READ when it was that node >> that broke it?) >> >> >> Anyway. You can see something arising from this problem simply by >> unsharing CONSISTENT_READ on the target node. (Just drop the src-perm >> node from the test I add in patch 4.) Replacing the source with the >> target will then work fine (because mirror_top_bs doesn’t care about >> CONSISTENT_READ being removed), but then you cannot drop mirror_top_bs – >> because its parent does want CONSISTENT_READ. Thus, the &error_abort >> aborts. >> >> >> While this is a more special case, I have no idea how to fix this one >> either. >> >> >> Soo... This series just fixes one thing, and leaves another unfixed >> because I have no idea how to fix it. Worse, it adds parameters to >> blkdebug to actually see the problem. Do we want to let blkdebug be >> able to crash qemu (because of a bug in qemu)? >> > > blkdebug is for debugging and not used by end users like libvirt, yes? Correct. Max >> >> Max Reitz (4): >> mirror: Do not dereference invalid pointers >> blkdebug: Allow taking/unsharing permissions >> iotests: Add @error to wait_until_completed >> iotests: Add test for failing mirror complete >> >> qapi/block-core.json | 29 +++++++++- >> block/blkdebug.c | 106 +++++++++++++++++++++++++++++++++- >> block/mirror.c | 13 +++-- >> tests/qemu-iotests/041 | 44 ++++++++++++++ >> tests/qemu-iotests/041.out | 4 +- >> tests/qemu-iotests/iotests.py | 18 ++++-- >> 6 files changed, 200 insertions(+), 14 deletions(-) >> > >
signature.asc
Description: OpenPGP digital signature