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

[Max Reitz]

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?

[...]

>>> +static int raw_reopen_handle_lock(BDRVReopenState *state,
>>> +                                  RawReopenOperation op,
>>> +                                  Error **errp)
>>> +{
>>> +    BDRVRawReopenState *raw_s = state->opaque;
>>
>> Please choose another name, it's hard not to confuse this with the
>> BDRVRawState all the time. (e.g. raw_rs or just rs would be enough.)
> 
> Sorry I can't change the name in this patch, it will cause more inconsistency
> because raw_reopen_* already use the name broadly. If you really insist it's a
> bad name, I can append or prepend a patch in the next version.

Well, I don't insist, but it really confused me a couple of times, even
after I had realized my mistake once.

Max

signature.asc
Description: OpenPGP digital signature