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

[Fiona Ebner <f.ebner@proxmox.com>]

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