On 29.03.19 18:40, Kevin Wolf wrote: > Am 29.03.2019 um 18:30 hat Max Reitz geschrieben: >> On 29.03.19 18:24, Kevin Wolf wrote: >>> Am 29.03.2019 um 18:15 hat Max Reitz geschrieben: >>>> 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 <[email protected]> >>>>> --- >>>>> 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? >>> >>> I suppose raw_check_lock_bytes() is what is failing, not >>> raw_apply_lock_bytes(). >> >> Hm, maybe in Vladimir's case, but not in e.g. >> https://bugzilla.redhat.com/show_bug.cgi?id=1652572 . > > This is reported against 3.0, which didn't avoid re-locking permissions > that we already hold, so there raw_apply_lock_bytes() can still fail.
That makes sense. Which leaves the question why Vladimir still seems to see the error there...? Max
signature.asc
Description: OpenPGP digital signature
