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

[Fam Zheng]

On Wed, 02/08 14:18, Max Reitz wrote:
> 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

Right, will fix!

Fam