Re: [Qemu-block] [PATCH v2] block/file-posix: ignore fail on unlock bytes

[Vladimir Sementsov-Ogievskiy]

29.03.2019 14:08, Kevin Wolf wrote:
> Am 29.03.2019 um 11:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.03.2019 13:12, Kevin Wolf wrote:
>>> Am 29.03.2019 um 10:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 28.03.2019 21:40, Kevin Wolf wrote:
>>>>> Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> 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 ignore such errors, as we do already on permission update commit
>>>>>> and abort.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>>>
>>>>> I think this would better be fixed in block.c code so that unlock never
>>>>> fails for any block driver.
>>>>
>>>> Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And
>>>> in this particular case, yes, the only thing we can do is ignoring error
>>>> and do not fail on loosening permissions..
>>>>
>>>> If we have more drivers with this callback, what should be the common
>>>> behavior?
>>>>
>>>> Do you propose to ignore .bdrv_check_perm errors in common case?
>>>>
>>>> Isn't it better to require, that .bdrv_check_perm handler do not fail on
>>>> loosening permissions, and abort if it fail in this case, like it actually
>>>> works after this patch?
>>>
>>> Maybe an assertion in the common code is actually better, yes.
>>>
>>> I do think that the common behaviour we want is to ignore
>>> .bdrv_check_perm errors for unlock, but it might be surprising for
>>> drivers that .bdrv_set_perm is called when .bdrv_check_perm failed. So
>>> we need the drivers to be aware of the problem anyway.
>>>
>>> Back to your patch: Why do you need to call raw_check_lock_bytes() in
>>> the unlock case? We don't acquire any new permissions and hold the locks
>>> for everything, so nobody else should have taken a conflicting lock.
>>>
>>
>> Hmm.. it check not same arguments, so I didn't drop it as
>> raw_apply_lock_bytes.
> 
> Not exactly the same, but a subset of the old ones.
> 
>> On the other hand, the only meaning of this raw_check_lock_bytes, is
>> that we'll print error if it come (when it should not).
>>
>> Seems OK for me to drop it too and just return 0 immediately.
> 
> Okay, so the plan for v3 is to drop the raw_check_lock_bytes() call
> here, and add an assertion in block.c, right?

but where to assert? No I think - nowhere. We'll abort on error_abort anyway.

And what to assert? We can't do such check in common bdrv_check_perm,
as we don't keep previous cumulative permissions..

So I think, my v3 is OK.

-- 
Best regards,
Vladimir