[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 5/9] mirror: implement mirror_change method
From: |
Fiona Ebner |
Subject: |
Re: [PATCH v3 5/9] mirror: implement mirror_change method |
Date: |
Mon, 23 Oct 2023 16:14:03 +0200 |
User-agent: |
Mozilla Thunderbird |
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
- [PATCH v3 3/9] block/mirror: move dirty bitmap to filter, (continued)
- [PATCH v3 3/9] block/mirror: move dirty bitmap to filter, Fiona Ebner, 2023/10/13
- [PATCH v3 8/9] blockjob: query driver-specific info via a new 'query' driver method, Fiona Ebner, 2023/10/13
- [PATCH v3 4/9] block/mirror: determine copy_to_target only once, Fiona Ebner, 2023/10/13
- [PATCH v3 6/9] qapi/block-core: use JobType for BlockJobInfo's type, Fiona Ebner, 2023/10/13
- [PATCH v3 5/9] mirror: implement mirror_change method, Fiona Ebner, 2023/10/13
[PATCH v3 9/9] mirror: return mirror-specific information upon query, Fiona Ebner, 2023/10/13
Re: [PATCH v3 0/9] mirror: allow switching from background to active mode, Markus Armbruster, 2023/10/18
Re: [PATCH v3 0/9] mirror: allow switching from background to active mode, Kevin Wolf, 2023/10/19