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

[Vladimir Sementsov-Ogievskiy]

On 11.10.23 14:22, Fiona Ebner wrote:
> 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?

Yes, I also think, that actually setting int variable is "atomic enough". But 
I'm not sure about all architectures/OSes/compilers)

> 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.


It could still be written like this

if (change_opts->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING) {
  report error
}


if (qatomic_cmpxchg(&s->copy_mode, MIRROR_COPY_MODE_BACKGROUND, 
MIRROR_COPY_MODE_WRITE_BLOCKING) != MIRROR_COPY_MODE_BACKGROUND) {
  report error
}

report success

===

and we'll have to access it as qatomic_read(&s->copy_mode) in other places

> I suppose, I should re-use the job mutex then?
>
> Best Regards,
> Fiona

--
Best regards,
Vladimir