[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 05/10] mirror: implement mirror_change method
From: |
Fiona Ebner |
Subject: |
Re: [PATCH v2 05/10] mirror: implement mirror_change method |
Date: |
Wed, 11 Oct 2023 13:22:06 +0200 |
User-agent: |
Mozilla Thunderbird |
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
- Re: [PATCH v2 03/10] block/mirror: move dirty bitmap to filter, (continued)
- [PATCH v2 04/10] block/mirror: determine copy_to_target only once, Fiona Ebner, 2023/10/09
- [PATCH v2 09/10] mirror: return mirror-specific information upon query, Fiona Ebner, 2023/10/09
- [PATCH v2 08/10] blockjob: query driver-specific info via a new 'query' driver method, Fiona Ebner, 2023/10/09
- [PATCH v2 07/10] qapi/block-core: turn BlockJobInfo into a union, Fiona Ebner, 2023/10/09
- [PATCH v2 05/10] mirror: implement mirror_change method, Fiona Ebner, 2023/10/09
- [PATCH v2 10/10] iotests: adapt test output for new mirror query property, Fiona Ebner, 2023/10/09
- [PATCH v2 06/10] qapi/block-core: use JobType for BlockJobInfo's type, Fiona Ebner, 2023/10/09
- Re: [PATCH v2 00/10] mirror: allow switching from background to active mode, Vladimir Sementsov-Ogievskiy, 2023/10/10