Re: [RFC 04/15] qapi: block-job-change: make copy-mode parameter optional
On 28.03.24 12:24, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov(a)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(a)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