Re: [PATCH v2 05/10] mirror: implement mirror_change method

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

Am 10.10.23 um 21:37 schrieb Vladimir Sementsov-Ogievskiy:
> On 09.10.23 12:46, Fiona Ebner wrote:
>>   +static void mirror_change(BlockJob *job, BlockJobChangeOptions *opts,
>> +                          Error **errp)
>> +{
>> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
>> +    BlockJobChangeOptionsMirror *change_opts = &opts->u.mirror;
>> +
>> +    if (s->copy_mode == change_opts->copy_mode) {
>> +        return;
>> +    }
>> +
>> +    if (s->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) {
>> +        error_setg(errp, "Cannot switch away from copy mode
>> 'write-blocking'");
>> +        return;
>> +    }
>> +
>> +    assert(s->copy_mode == MIRROR_COPY_MODE_BACKGROUND &&
>> +           change_opts->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING);
>> +
>> +    s->copy_mode = MIRROR_COPY_MODE_WRITE_BLOCKING;
>> +}
> 
> So, s->copy_mode becomes shared between main thread and iothread.
> 
> We should either use mutex or atomic operations.
> 
> Note, that the only realization of .set_speed uses thread-safe API.
> 

Can it be an issue if it's only ever set from the main thread?

But sure, I'm implicitly relying on that, which is not ideal. The
mirror_change() function does multiple checks based on the current
value, and only then changes it, so I suppose it would actually need a
mutex rather than just changing to atomic accesses? Otherwise, the
current value can't be guaranteed to be the same in the different checks
if we ever add something that can change the value from another thread.

I suppose, I should re-use the job mutex then?

Best Regards,
Fiona