[prev in list] [next in list] [prev in thread] [next in thread] 

List:       libvir-list
Subject:    Re: [RFC 04/15] qapi: block-job-change: make copy-mode parameter optional
From:       Vladimir Sementsov-Ogievskiy <vsementsov () yandex-team ! ru>
Date:       2024-03-28 10:56:35
Message-ID: 842e4bd6-a7e4-463c-b319-e959e1204943 () yandex-team ! ru
[Download RAW message or body]

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
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org


[prev in list] [next in list] [prev in thread] [next in thread]