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 last 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.


Max Reitz (7):
  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 *loosen_restrictions to *check*_perm()
  block: Ignore loosening perm restrictions failures

 include/block/block_int.h |  15 ++++
 block.c                   | 143 ++++++++++++++++++++++++++++++++------
 block/commit.c            |   2 -
 block/file-posix.c        |   3 +
 block/mirror.c            |  32 ++++++---
 5 files changed, 161 insertions(+), 34 deletions(-)

-- 
2.20.1

Am 06.05.2019 um 21:47 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 last 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.

There are some places in this series that were a bit confusing (just
because the whole permission mechanism is rather confusing). I suggested
improvements for two patches, and deleted the half-written mails for the
rest. After all, it does look correct to me, so whether you want to
address my comments or not:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>