Re: [PATCH v3 5/9] mirror: implement mirror_change method

[Kevin Wolf <kwolf@redhat.com>]

Am 23.10.2023 um 16:14 hat Fiona Ebner geschrieben:
> Am 23.10.23 um 14:59 schrieb Kevin Wolf:
> > Am 23.10.2023 um 13:37 hat Fiona Ebner geschrieben: 
> >>>> +    current = qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
> >>>> +                              change_opts->copy_mode);
> >>>> +    if (current != MIRROR_COPY_MODE_BACKGROUND) {
> >>>> +        error_setg(errp, "Expected current copy mode '%s', got '%s'",
> >>>> +                   MirrorCopyMode_str(MIRROR_COPY_MODE_BACKGROUND),
> >>>> +                   MirrorCopyMode_str(current));
> >>>> +    }
> >>>
> >>> The error path is strange. We return an error, but the new mode is still
> >>> set. On the other hand, this is probably also the old mode unless
> >>> someone added a new value to the enum, so it didn't actually change. And
> >>> because this function is the only place that changes copy_mode and we're
> >>> holding the BQL, the case can't even happen and this could be an
> >>> assertion.
> >>>
> >>
> >> AFAIU and testing seem to confirm this, the new mode is only set when
> >> the current mode is MIRROR_COPY_MODE_BACKGROUND. The error is only set
> >> when the current mode is not MIRROR_COPY_MODE_BACKGROUND and thus when
> >> the mode wasn't changed.
> > 
> > Yes, the new mode is only set when it was MIRROR_COPY_MODE_BACKGROUND,
> > that's the meaning of cmpxchg.
> > 
> > And now that I checked the return value of qatomic_cmpxchg(), it's not
> > the actual value, but it returns the second parameter (the expected old
> > value). As this is a constant in our call, that's what we'll always get
> > back. So the whole check is pointless, even as an assertion. It's
> > trivially true, and I expect it's even obvious enough for the compiler
> > that it might just optimise it away.
> > 
> 
> From testing, I can see that it returns the current value, not the
> second parameter. I.e. if I am in MIRROR_COPY_MODE_WRITE_BLOCKING, it
> will return MIRROR_COPY_MODE_WRITE_BLOCKING. (Of course, I have to
> comment out the other check to reach the cmpxchg call while in that mode).

You're right, I misread. Sorry for the noise.

> > Just qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND,
> > change_opts->copy_mode); without using the (constant) result should be
> > enough.
> > 
> >> Adding a new copy mode shouldn't cause issues either? It's just not
> >> going to be supported to change away from that mode (or to that mode,
> >> because of the change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING
> >> check above) without adapting the code first.
> > 
> > The checks above won't prevent NEW_MODE -> WRITE_BLOCKING. Of course,
> > the cmpxchg() won't actually do anything as long as we still have
> > BACKGROUND there as the expected old value. So in this case, QMP would
> > probably return success, but we would stay in NEW_MODE.
> > 
> 
> No, that's the whole point of the check. It would fail with the error,
> saying that it expected the current mode to be background and not the
> new mode.

Yes, this makes sense now.

> > That's different from what I thought (I didn't really realise that we
> > have a cmpxchg here and not just a xchg), but also not entirely right.
> > 
> > Of course, all of this is hypothetical. I'm not aware of any desire to
> > add a new copy mode.
> > 
> >> Of course, if we want to allow switching from active to background mode,
> >> the function needs to be adapted too.
> >>
> >> I wanted to make it more future-proof for the case where it might not be
> >> the only place changing the value and based it on what Vladimir
> >> suggested in the review of v2:
> >> https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg03552.html
> > 
> > As long as all of these places are GLOBAL_STATE_CODE(), we should be
> > fine. If we get iothread code that changes it, too, I think your code
> > becomes racy because the value could be changed by the iothread between
> > the first check if we already have the new value and the actual change.
> > 
> 
> Right, but I think the only issue would be if the mode changes from
> MIRROR_COPY_MODE_BACKGROUND to MIRROR_COPY_MODE_WRITE_BLOCKING between
> the checks, because then the QMP call would fail with the error that the
> mode was not the expected MIRROR_COPY_MODE_BACKGROUND. But arguably,
> that is still correct. If we are already in the requested mode at the
> time of the first check, we're fine.
> 
> Still, I'll add the GLOBAL_STATE_CODE() and a comment for the future :)

Thanks, sounds good.

Kevin