Re: [Qemu-block] [PATCH v3] block/file-posix: do not fail on unlock bytes

[Max Reitz]

On 29.03.19 12:04, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
> loosening permissions. However file-locking operations may fail even
> in this case, for example on NFS. And this leads to Qemu crash.
> 
> Let's avoid such errors. Note, that we ignore such things anyway on
> permission update commit and abort.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/file-posix.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index db4cccbe51..1cf4ee49eb 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -815,6 +815,18 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
>  
>      switch (op) {
>      case RAW_PL_PREPARE:
> +        if ((s->perm | new_perm) == s->perm &&
> +            (s->shared_perm & new_shared) == s->shared_perm)
> +        {
> +            /*
> +             * We are going to unlock bytes, it should not fail. If it fail 
> due
> +             * to some fs-dependent permission-unrelated reasons (which 
> occurs
> +             * sometimes on NFS and leads to abort in bdrv_replace_child) we
> +             * can't prevent such errors by any check here. And we ignore 
> them
> +             * anyway in ABORT and COMMIT.
> +             */
> +            return 0;
> +        }
>          ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
>                                     ~s->shared_perm | ~new_shared,
>                                     false, errp);

Help me understand the exact issue, please.  I understand that there are
operations like bdrv_replace_child() that pass &error_abort to
bdrv_check_perm() because they just loosen the permissions, so it should
not fail.

However, if the whole effect really would be to loosen permissions,
raw_apply_lock_bytes() wouldn't have failed here in PREPARE anyway:
@unlock is passed as false, so no bytes will be unlocked.  And if
permissions are just loosened (as your condition checks), it should not
lock any bytes.

So why does it attempt lock any bytes in the first place?  There must be
some discrepancy between s->perm and s->locked_perm, or ~s->shared_perm
and s->locked_shared_perm.  How does that occur?

Max

signature.asc
Description: OpenPGP digital signature