include/block/block_int.h | 15 ++++ block.c | 153 +++++++++++++++++++++++++++++++------ block/commit.c | 2 - block/file-posix.c | 4 + block/mirror.c | 32 +++++--- tests/qemu-iotests/182 | 21 +++++ tests/qemu-iotests/182.out | 6 ++ 7 files changed, 198 insertions(+), 35 deletions(-)
Hi,
This series is mainly a fix for
https://bugzilla.redhat.com/show_bug.cgi?id=1703793. The problem
described there is that mirroring to a gluster volume, then switching
off the volume makes qemu crash. There are two problems here:
(1) file-posix reopens the FD all the time because it thinks the FD it
has is RDONLY. It actually isn’t after the first reopen, we just
forgot to change the internal flags. That’s what patch 1 is for.
(2) Even then, when mirror completes, it drops its write permission on
the FD. This requires a reopen, which will fail if the volume is
down. Mirror doesn’t expect that. Nobody ever expects that
dropping permissions can fail, and rightfully so because that’s what
I think we have generally agreed on.
Therefore, the block layer should hide this error. This is what the
last two patches are for.
The penultimate patch adds two assertions: bdrv_replace_child() (for the
old BDS) and bdrv_inactivate_recurse() assume they only ever drop
assertions. This is now substantiated by these new assertions.
It turns out that this assumption was just plain wrong. Patches 3 to 5
make it right.
v3:
- Received no reply to my “Hm, warnings break 'make check', so maybe we
should just keep quiet if loosening restrictions fails?” question, so
I assume silence means agreement. Changed patch 7 accordingly.
- Added a test: The fact how make check kind-of-but-not-really broke
showed a nice reproducer: Launching qemu with some file, then deleting
that file, then quitting qemu.
- Rebase “conflict” in patch 6: The forward declaration of
bdrv_get_cumulative_perm() is already in qemu thanks to commit
481e0eeef4f.
git-backport-diff against v2:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/8:[----] [--] 'file-posix: Update open_flags in raw_set_perm()'
002/8:[----] [--] 'block: Add bdrv_child_refresh_perms()'
003/8:[----] [--] 'block/mirror: Fix child permissions'
004/8:[----] [--] 'block/commit: Drop bdrv_child_try_set_perm()'
005/8:[0018] [FC] 'block: Fix order in bdrv_replace_child()'
^^^^ Again confuses my v2 patch with 8aecf1d1bd250a, should be:
[----] : patches are identical
006/8:[0002] [FC] 'block: Add *tighten_restrictions to *check*_perm()'
007/8:[0018] [FC] 'block: Ignore loosening perm restrictions failures'
008/8:[down] 'iotests: Test failure to loosen restrictions'
Max Reitz (8):
file-posix: Update open_flags in raw_set_perm()
block: Add bdrv_child_refresh_perms()
block/mirror: Fix child permissions
block/commit: Drop bdrv_child_try_set_perm()
block: Fix order in bdrv_replace_child()
block: Add *tighten_restrictions to *check*_perm()
block: Ignore loosening perm restrictions failures
iotests: Test failure to loosen restrictions
include/block/block_int.h | 15 ++++
block.c | 153 +++++++++++++++++++++++++++++++------
block/commit.c | 2 -
block/file-posix.c | 4 +
block/mirror.c | 32 +++++---
tests/qemu-iotests/182 | 21 +++++
tests/qemu-iotests/182.out | 6 ++
7 files changed, 198 insertions(+), 35 deletions(-)
--
2.21.0
Am 22.05.2019 um 19:03 hat Max Reitz geschrieben: > Hi, > > This series is mainly a fix for > https://bugzilla.redhat.com/show_bug.cgi?id=1703793. The problem > described there is that mirroring to a gluster volume, then switching > off the volume makes qemu crash. There are two problems here: > > (1) file-posix reopens the FD all the time because it thinks the FD it > has is RDONLY. It actually isn’t after the first reopen, we just > forgot to change the internal flags. That’s what patch 1 is for. > > (2) Even then, when mirror completes, it drops its write permission on > the FD. This requires a reopen, which will fail if the volume is > down. Mirror doesn’t expect that. Nobody ever expects that > dropping permissions can fail, and rightfully so because that’s what > I think we have generally agreed on. > Therefore, the block layer should hide this error. This is what the > last two patches are for. > > The penultimate patch adds two assertions: bdrv_replace_child() (for the > old BDS) and bdrv_inactivate_recurse() assume they only ever drop > assertions. This is now substantiated by these new assertions. > It turns out that this assumption was just plain wrong. Patches 3 to 5 > make it right. Thanks, applied to the block branch. Kevin
On 5/22/19 12:03 PM, Max Reitz wrote: > Hi, > > This series is mainly a fix for > https://bugzilla.redhat.com/show_bug.cgi?id=1703793. The problem > described there is that mirroring to a gluster volume, then switching > off the volume makes qemu crash. There are two problems here: > > (1) file-posix reopens the FD all the time because it thinks the FD it > has is RDONLY. It actually isn’t after the first reopen, we just > forgot to change the internal flags. That’s what patch 1 is for. > > (2) Even then, when mirror completes, it drops its write permission on > the FD. This requires a reopen, which will fail if the volume is > down. Mirror doesn’t expect that. Nobody ever expects that > dropping permissions can fail, and rightfully so because that’s what > I think we have generally agreed on. > Therefore, the block layer should hide this error. This is what the > last two patches are for. > > The penultimate patch adds two assertions: bdrv_replace_child() (for the > old BDS) and bdrv_inactivate_recurse() assume they only ever drop > assertions. This is now substantiated by these new assertions. > It turns out that this assumption was just plain wrong. Patches 3 to 5 > make it right. > > > v3: > - Received no reply to my “Hm, warnings break 'make check', so maybe we > should just keep quiet if loosening restrictions fails?” question, so > I assume silence means agreement. Changed patch 7 accordingly. > I don't know if there is an easy way to warn for normal users, but silence the warnings if run under test setups to keep 'make check' output unchanged (I know we've silenced warnings in the past when we detect we are running qtest, but this isn't necessarily the same setup). So not a show-stopper for me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 22.05.19 20:24, Eric Blake wrote: > On 5/22/19 12:03 PM, Max Reitz wrote: >> Hi, >> >> This series is mainly a fix for >> https://bugzilla.redhat.com/show_bug.cgi?id=1703793. The problem >> described there is that mirroring to a gluster volume, then switching >> off the volume makes qemu crash. There are two problems here: >> >> (1) file-posix reopens the FD all the time because it thinks the FD it >> has is RDONLY. It actually isn’t after the first reopen, we just >> forgot to change the internal flags. That’s what patch 1 is for. >> >> (2) Even then, when mirror completes, it drops its write permission on >> the FD. This requires a reopen, which will fail if the volume is >> down. Mirror doesn’t expect that. Nobody ever expects that >> dropping permissions can fail, and rightfully so because that’s what >> I think we have generally agreed on. >> Therefore, the block layer should hide this error. This is what the >> last two patches are for. >> >> The penultimate patch adds two assertions: bdrv_replace_child() (for the >> old BDS) and bdrv_inactivate_recurse() assume they only ever drop >> assertions. This is now substantiated by these new assertions. >> It turns out that this assumption was just plain wrong. Patches 3 to 5 >> make it right. >> >> >> v3: >> - Received no reply to my “Hm, warnings break 'make check', so maybe we >> should just keep quiet if loosening restrictions fails?” question, so >> I assume silence means agreement. Changed patch 7 accordingly. >> > > I don't know if there is an easy way to warn for normal users, but > silence the warnings if run under test setups to keep 'make check' > output unchanged (I know we've silenced warnings in the past when we > detect we are running qtest, but this isn't necessarily the same setup). > So not a show-stopper for me. Hm. That doesn’t sound too bad. I don’t think there is an easy way to silence the warning in qemu, but we might be able to just modify the test. But I don’t even know whether the warnings are even useful or whether they would just confuse users more than anything. So far, every case I know where loosening restrictions failed was because the file is just gone completely. The only purpose of a warning is to show the user that qemu might have locks on the file that it doesn’t need, so they will know what’s up if they try to open the file in another qemu instance in a way that should normally work but suddenly doesn’t. But if the file’s just gone, you can’t open it in another qemu, so I don’t even know whether there’s actually any point in warning. Max
On 5/22/19 1:37 PM, Max Reitz wrote: >> I don't know if there is an easy way to warn for normal users, but >> silence the warnings if run under test setups to keep 'make check' >> output unchanged (I know we've silenced warnings in the past when we >> detect we are running qtest, but this isn't necessarily the same setup). >> So not a show-stopper for me. > > Hm. That doesn’t sound too bad. I don’t think there is an easy way to > silence the warning in qemu, but we might be able to just modify the test. > > But I don’t even know whether the warnings are even useful or whether > they would just confuse users more than anything. So far, every case I > know where loosening restrictions failed was because the file is just > gone completely. The only purpose of a warning is to show the user that > qemu might have locks on the file that it doesn’t need, so they will > know what’s up if they try to open the file in another qemu instance in > a way that should normally work but suddenly doesn’t. But if the file’s > just gone, you can’t open it in another qemu, so I don’t even know > whether there’s actually any point in warning. Good point - if we unlink()ed the file, we can't loosen permissions, but neither can anyone else open() it to collide with us :) A network connection going down is a bit harder to justify (it might come back up), but I think it still fits the bill (if we can't loosen permissions, who else can interfere with us?) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Ping <bait>just the final three patches left to review</bait> On 22.05.19 19:03, Max Reitz wrote: > Hi, > > This series is mainly a fix for > https://bugzilla.redhat.com/show_bug.cgi?id=1703793. The problem > described there is that mirroring to a gluster volume, then switching > off the volume makes qemu crash. There are two problems here: > > (1) file-posix reopens the FD all the time because it thinks the FD it > has is RDONLY. It actually isn’t after the first reopen, we just > forgot to change the internal flags. That’s what patch 1 is for. > > (2) Even then, when mirror completes, it drops its write permission on > the FD. This requires a reopen, which will fail if the volume is > down. Mirror doesn’t expect that. Nobody ever expects that > dropping permissions can fail, and rightfully so because that’s what > I think we have generally agreed on. > Therefore, the block layer should hide this error. This is what the > last two patches are for. > > The penultimate patch adds two assertions: bdrv_replace_child() (for the > old BDS) and bdrv_inactivate_recurse() assume they only ever drop > assertions. This is now substantiated by these new assertions. > It turns out that this assumption was just plain wrong. Patches 3 to 5 > make it right. > > > v3: > - Received no reply to my “Hm, warnings break 'make check', so maybe we > should just keep quiet if loosening restrictions fails?” question, so > I assume silence means agreement. Changed patch 7 accordingly. > > - Added a test: The fact how make check kind-of-but-not-really broke > showed a nice reproducer: Launching qemu with some file, then deleting > that file, then quitting qemu. > > - Rebase “conflict” in patch 6: The forward declaration of > bdrv_get_cumulative_perm() is already in qemu thanks to commit > 481e0eeef4f. > > > git-backport-diff against v2: > > Key: > [----] : patches are identical > [####] : number of functional differences between upstream/downstream patch > [down] : patch is downstream-only > The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively > > 001/8:[----] [--] 'file-posix: Update open_flags in raw_set_perm()' > 002/8:[----] [--] 'block: Add bdrv_child_refresh_perms()' > 003/8:[----] [--] 'block/mirror: Fix child permissions' > 004/8:[----] [--] 'block/commit: Drop bdrv_child_try_set_perm()' > 005/8:[0018] [FC] 'block: Fix order in bdrv_replace_child()' > ^^^^ Again confuses my v2 patch with 8aecf1d1bd250a, should be: > [----] : patches are identical > 006/8:[0002] [FC] 'block: Add *tighten_restrictions to *check*_perm()' > 007/8:[0018] [FC] 'block: Ignore loosening perm restrictions failures' > 008/8:[down] 'iotests: Test failure to loosen restrictions' > > > Max Reitz (8): > file-posix: Update open_flags in raw_set_perm() > block: Add bdrv_child_refresh_perms() > block/mirror: Fix child permissions > block/commit: Drop bdrv_child_try_set_perm() > block: Fix order in bdrv_replace_child() > block: Add *tighten_restrictions to *check*_perm() > block: Ignore loosening perm restrictions failures > iotests: Test failure to loosen restrictions > > include/block/block_int.h | 15 ++++ > block.c | 153 +++++++++++++++++++++++++++++++------ > block/commit.c | 2 - > block/file-posix.c | 4 + > block/mirror.c | 32 +++++--- > tests/qemu-iotests/182 | 21 +++++ > tests/qemu-iotests/182.out | 6 ++ > 7 files changed, 198 insertions(+), 35 deletions(-) >
© 2016 - 2024 Red Hat, Inc.