Am 18.10.23 um 18:59 schrieb Kevin Wolf:
> Am 13.10.2023 um 11:21 hat Fiona Ebner geschrieben:
>> which allows switching the @copy-mode from 'background' to
>> 'write-blocking'.
>>
>> This is useful for management applications, so they can start out in
>> background mode to avoid limiting guest write speed and switch to
>> active mode when certain criteria are fulfilled.
>>
>> In presence of an iothread, the copy_mode member is now shared between
>> the iothread and the main thread, so turn accesses to it atomic.
>>
>> Signed-off-by: Fiona Ebner <f.eb...@proxmox.com>
>> ---
>>
>> Changes in v3:
>> * turn accesses to copy_mode atomic and...
>> * ...slightly adapt error handling in mirror_change as a
>> consequence
>
> It would be good to have a comment at the field declaration that it's
> meant to be accessed with atomics.
>
> As we don't have further synchonisation, is the idea that during the
> switchover it basically doesn't matter if we read the old or the new
> value?
>
> After reading the whole patch, it seems that the field is only ever
> written under the BQL, while iothreads only read it, and only once per
> request (after the previous patch). This is why no further
> synchonisation is needed. If other threads could write it, too,
> mirror_change() would probably have to be more careful. As the code
> depends on this, adding that to the comment would be useful, too.
>
Will do in v4.
>> block/mirror.c | 33 ++++++++++++++++++++++++++++++---
>> qapi/block-core.json | 13 ++++++++++++-
>> 2 files changed, 42 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 8992c09172..889cce5414 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -1075,7 +1075,7 @@ static int coroutine_fn mirror_run(Job *job, Error
>> **errp)
>> */
>> job_transition_to_ready(&s->common.job);
>> }
>> - if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
>> + if (qatomic_read(&s->copy_mode) != MIRROR_COPY_MODE_BACKGROUND)
>> {
>> s->actively_synced = true;
>> }
>
> What resets s->actively_synced when we switch away from active mode?
>
>>
>> @@ -1246,6 +1246,32 @@ static bool commit_active_cancel(Job *job, bool force)
>> return force || !job_is_ready(job);
>> }
>>
>> +static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
>> + Error **errp)
>> +{
>> + MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
>> + BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror;
>> + MirrorCopyMode current;
>
> This is GLOBAL_STATE_CODE(), right? Let's be explicit about it.
>
Maybe it wouldn't need to be if we also set actively_synced to false in
bdrv_mirror_top_do_write() if/when setting the bitmap. Thinking about
it, that change shouldn't hurt in any case. But sure, I'll add the
GLOBAL_STATE_CODE annotation here. If ever required not to be
GLOBAL_STATE_CODE code, it can still be adapted later.
>> +
>> + if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
>> + return;
>> + }
>> +
>> + if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
>> + error_setg(errp, "Change to copy mode '%s' is not implemented",
>> + MirrorCopyMode_str(change_opts->copy_mode));
>> + return;
>> + }
>
> Ah, ok, we don't even allow the switch I was wondering about above. What
> would be needed, apart from removing this check, to make it work?
>
Of course, setting actively_synced to false, as you pointed out above.
But I think it would also require more synchronization, because I think
otherwise the iothread could still read the old value of copy_mode (as
MIRROR_COPY_MODE_WRITE_BLOCKING) right afterwards and might set
actively_synced to true again. Do you want me to think it through in
detail and allow the change in the other direction too? I guess that
would also require using the job mutex instead of atomics. Or should we
wait until somebody actually requires that?
>> + 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.
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.
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
Best Regards,
Fiona
