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

[Fam Zheng]

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.

> 
> (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