[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v8 05/36] raw-posix: Add image locking support
From: |
Fam Zheng |
Subject: |
Re: [Qemu-block] [PATCH v8 05/36] raw-posix: Add image locking support |
Date: |
Tue, 25 Oct 2016 21:43:03 +0800 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
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