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 >
signature.asc
Description: OpenPGP digital signature
