Re: [RFC 04/15] qapi: block-job-change: make copy-mode parameter optional

[Vladimir Sementsov-Ogievskiy]

On 28.03.24 12:24, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>
> > We are going to add more parameters to change. We want to make possible
> > to change only one or any subset of available options. So all the
> > options should be optional.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > ---
> >   block/mirror.c       | 5 +++++
> >   qapi/block-core.json | 2 +-
> >   2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/mirror.c b/block/mirror.c
> > index a177502e4f..2d0cd22c06 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -1265,6 +1265,11 @@ static void mirror_change(BlockJob *job, 
> > JobChangeOptions *opts,
> >   
> >       GLOBAL_STATE_CODE();
> >   
> > +    if (!change_opts->has_copy_mode) {
> > +        /* Nothing to do */
>
> I doubt the comment is useful.
>
> > +        return;
> > +    }
> > +
> >       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;
>         }
>
>         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));
>         }
>
> Now I'm curious: what could be changing @copy_mode behind our backs
> here?

For now - nothing. But it may be read from another thread, so it's declared as 
atomic access.

So, we can check it with qatomic_read() instead, check the value first, and 
write then with qatomic_set().

But, I think it would be extra optimization (if it is). The operation is not 
often, and cmpxchg looks like a correct way to conditionally change atomic 
variable (even being a bit too safe)    .

> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 67dd0ef038..6041e7bd8f 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3071,7 +3071,7 @@
>     ##
>     # @BlockJobChangeOptionsMirror:
>     #
>     # @copy-mode: Switch to this copy mode.  Currently, only the switch
>     #     from 'background' to 'write-blocking' is implemented.
>     #
> >   # Since: 8.2
> >   ##
> >   { 'struct': 'JobChangeOptionsMirror',
> > -  'data': { 'copy-mode' : 'MirrorCopyMode' } }
> > +  'data': { '*copy-mode' : 'MirrorCopyMode' } }
> >   
> >   ##
> >   # @JobChangeOptions:
>
> A member becoming optional is backward compatible.  Okay.
>
> We may want to document "(optional since 9.1)".  We haven't done so in
> the past.

Will do, I think it's useful.

> The doc comment needs to spell out how absent members are handled.

Will add.

--
Best regards,
Vladimir