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
