Re: [Qemu-devel] [PATCH v12 14/16] file-posix: Implement image locking

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

On 08.02.2017 07:00, Fam Zheng wrote:
> On Wed, 02/08 04:05, Max Reitz wrote:
>>> +static int raw_lt_write_to_write_share(RawLockTransOp op,
>>> +                                       int old_lock_fd, int new_lock_fd,
>>> +                                       BDRVRawLockMode old_lock,
>>> +                                       BDRVRawLockMode new_lock,
>>> +                                       Error **errp)
>>> +{
>>> +    int ret = 0;
>>> +
>>> +    assert(old_lock == RAW_L_WRITE);
>>> +    assert(new_lock == RAW_L_WRITE_SHARE_RW);
>>> +    /*
>>> +     *        lock byte "no other writer"      lock byte "write"
>>> +     * old                X                           X
>>> +     * new                0                           S
>>> +     *
>>> +     * (0 = unlocked; S = shared; X = exclusive.)
>>> +     */
>>> +    switch (op) {
>>> +    case RAW_LT_PREPARE:
>>> +        break;
>>> +    case RAW_LT_COMMIT:
>>> +        ret = qemu_lock_fd(old_lock_fd, RAW_LOCK_BYTE_WRITE, 1, false);
>>> +        if (ret) {
>>> +            error_report("Failed to downgrade old fd (share byte)");
>>> +            break;
>>> +        }
>>> +        ret = qemu_lock_fd(new_lock_fd, RAW_LOCK_BYTE_WRITE, 1, false);
>>> +        if (ret) {
>>> +            error_report("Failed to unlock new fd (share byte)");
>>> +            break;
>>> +        }
>>
>> The second one is not an "unlock", but a new shared lock.
> 
> You are right.
> 
>> Which brings
>> me to the point that both of these commands can fail and thus should be
>> in the prepare path.
> 
> We cannot. If we lose the exclusive lock already in prepare, and some other
> things fail later in the transaction, abort() may not be able to restore that
> lock (another process took a shared lock in between).
> 
> The reason for my code is, the lock semantics implies both of these commands can
> succeed, so it doesn't hurt if we ignore ret codes here. I'm just trying to
> catch the very unlikely abnormalities.

Indeed. Well, then raw_lt_write_to_read() should do the same, though.

Max

>> (This function should be a mirror of raw_lt_write_to_read, if I'm not
>> mistaken.)
>>
>>> +        break;
>>> +    case RAW_LT_ABORT:
>>> +        break;
>>> +    }
>>> +    return ret;
>>> +}
> 
> Fam
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]