Re: [Qemu-block] [PATCH v8 05/36] raw-posix: Add image locking support

[Max Reitz]

On 25.10.2016 15:43, Fam Zheng wrote:
> On Tue, 10/25 15:28, Max Reitz wrote:
>> On 25.10.2016 08:31, Fam Zheng wrote:
>>> On Sat, 10/22 01:40, Max Reitz wrote:
>>>> On 30.09.2016 14:09, Fam Zheng wrote:
>>
>> [...]
>>
>>>>> +static int
>>>>> +raw_reopen_upgrade(BDRVReopenState *state,
>>>>> +                   RawReopenOperation op,
>>>>> +                   ImageLockMode old_lock,
>>>>> +                   ImageLockMode new_lock,
>>>>> +                   Error **errp)
>>>>> +{
>>>>> +    BDRVRawReopenState *raw_s = state->opaque;
>>>>> +    BDRVRawState *s = state->bs->opaque;
>>>>> +    int ret = 0, ret2;
>>>>> +
>>>>> +    assert(old_lock == IMAGE_LOCK_MODE_SHARED);
>>>>> +    assert(new_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
>>>>> +    switch (op) {
>>>>> +    case RAW_REOPEN_PREPARE:
>>>>> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>
>>> [1]
>>>
>>>>> +        if (ret) {
>>>>> +            error_setg_errno(errp, -ret, "Failed to lock new fd 
>>>>> (shared)");
>>>>> +            break;
>>>>> +        }
>>>>> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK);
>>>
>>> [2]
>>>
>>>>> +        if (ret) {
>>>>> +            error_setg_errno(errp, -ret, "Failed to unlock old fd");
>>>>> +            goto restore;
>>>>> +        }
>>>>> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUSIVE);
>>>
>>> [3]
>>>
>>>>> +        if (ret) {
>>>>> +            error_setg_errno(errp, -ret, "Failed to lock new fd 
>>>>> (exclusive)");
>>>>> +            goto restore;
>>>>> +        }
>>>>> +        break;
>>>>> +    case RAW_REOPEN_COMMIT:
>>>>> +        break;
>>>>> +    case RAW_REOPEN_ABORT:
>>>>> +        raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>>> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>
>>> [4]
>>>
>>>>> +        if (ret) {
>>>>> +            error_report("Failed to restore lock on old fd");
>>>>
>>>> If we get here, s->lock_fd is still locked exclusively. The following is
>>>> a very personal opinion, but anyway: I think it would be be better for
>>>> it to be unlocked. If it's locked too strictly, this can really break
>>>> something; but if it's just not locked (while it should be locked in
>>>> shared mode), everything's going to be fine unless the user makes a
>>>> mistake. I think the latter is less bad.
>>>
>>> There are four lock states when we land on this "abort" branch:
>>>
>>>   a) Lock "prepare" was not run, some other error happened earlier, so the 
>>> lock
>>>      aren't changed compared to before the transaction starts: 
>>> raw_s->lock_fd is
>>>      unlocked, s->lock_fd is shared locked. In this case raw_lock_fd [4] 
>>> cannot
>>>      fail, and even if it does, s->lock_fd is in the correct state.
>>>
>>>   b) The raw_lock_fd [1] failed. This is similar to 1), s->lock_fd is 
>>> intact, so
>>>      we are good.
>>>
>>>   c) The raw_lock_fd [2] failed. Again, similar to above.
>>>
>>>   d) The raw_lock_fd [3] failed. Here raw_s->lock_fd is shared locked, and
>>>      s->lock_fd is unlocked. The correct "abort" sequence is downgrade
>>>      raw_s->lock_fd and "shared lock" s->lock_fd again. If the "abort" 
>>> sequence
>>>      failed, s->lock_fd is unlocked.
>>
>> OK, you're right, I somehow forgot about the cases where our prepare
>> sequence was either not run at all or failed. But I was thinking about
>> the case where our preparation was successful, but some later
>> preparation in the reopen transaction failed. Then, s->lock_fd should be
>> locked exclusively, no?
> 
> Oh I missed to reason about that branch. Here we go:
> 
> If our preparation succeeded, raw_s->lock_fd is exclusively locked, s->lock_fd
> is unlocked. In abort we should try to return to the old state (raw_s->lock_fd
> is _not_ exclusively locked and s->lock_fd is shared locked), hence the two
> raw_lock_fd() calls at [4]. In this case, if the second raw_lock_fd() at [4]
> doesn't work, s->lock_fd is unlocked, and raw_s->lock_fd will be closed 
> anyway,
> which again matches what you suggested.

Oh, right, it's raw_s->lock_fd that's exclusively locked, not s->lock_fd...

See? I really can't tell the difference between raw_s and s. :-/

So you're right, but that makes me just all the more skeptical about
raw_s...

Max

signature.asc
Description: OpenPGP digital signature