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

[Fam Zheng <famz@redhat.com>]

On Tue, 10/25 15:28, Max Reitz wrote:
> 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?

Oh I missed to reason about that branch. Here we go:

If our preparation succeeded, raw_s->lock_fd is exclusively locked, s->lock_fd
is unlocked. In abort we should try to return to the old state (raw_s->lock_fd
is _not_ exclusively locked and s->lock_fd is shared locked), hence the two
raw_lock_fd() calls at [4]. In this case, if the second raw_lock_fd() at [4]
doesn't work, s->lock_fd is unlocked, and raw_s->lock_fd will be closed anyway,
which again matches what you suggested.

Fam