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).
> 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.
> 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 :)
Best Regards,
Fiona
