| Message ID | 20231009094619.469668-6-f.ebner@proxmox.com |
|---|---|
| State | New |
| Headers | show |
| Series | mirror: allow switching from background to active mode | expand |
On 09.10.23 12:46, Fiona Ebner wrote: > which allows switching the @copy-mode from 'background' to > 'write-blocking'. > > This is useful for management applications, so they can start out in > background mode to avoid limiting guest write speed and switch to > active mode when certain criteria are fulfilled. > > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > --- > > Changes in v2: > * update QEMU version in QAPI > * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat > doc comments to conform to current conventions")) > * drop drained section and disable dirty bitmap call. It's already > disabled, because the bitmap is now attached to the filter and > set in bdrv_mirror_top_do_write(). See the earlier patch > "block/mirror: move dirty bitmap to filter" > > block/mirror.c | 22 ++++++++++++++++++++++ > qapi/block-core.json | 13 ++++++++++++- > 2 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/block/mirror.c b/block/mirror.c > index b84de56734..83aa4176c2 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1246,6 +1246,27 @@ static bool commit_active_cancel(Job *job, bool force) > return force || !job_is_ready(job); > } > > +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. > + > static const BlockJobDriver mirror_job_driver = { > .job_driver = { > .instance_size = sizeof(MirrorBlockJob), > @@ -1260,6 +1281,7 @@ static const BlockJobDriver mirror_job_driver = { > .cancel = mirror_cancel, > }, > .drained_poll = mirror_drained_poll, > + .change = mirror_change, > }; > > static const BlockJobDriver commit_active_job_driver = { > diff --git a/qapi/block-core.json b/qapi/block-core.json > index c6f31a9399..01427c259a 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3044,6 +3044,17 @@ > { 'command': 'block-job-finalize', 'data': { 'id': 'str' }, > 'allow-preconfig': true } > > +## > +# @BlockJobChangeOptionsMirror: > +# > +# @copy-mode: Switch to this copy mode. Currenlty, only the switch > +# from 'background' to 'write-blocking' is implemented. > +# > +# Since: 8.2 > +## > +{ 'struct': 'BlockJobChangeOptionsMirror', > + 'data': { 'copy-mode' : 'MirrorCopyMode' } } > + > ## > # @BlockJobChangeOptions: > # > @@ -3058,7 +3069,7 @@ > { 'union': 'BlockJobChangeOptions', > 'base': { 'id': 'str', 'type': 'JobType' }, > 'discriminator': 'type', > - 'data': {} } > + 'data': { 'mirror': 'BlockJobChangeOptionsMirror' } } > > ## > # @block-job-change:
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
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 >
diff --git a/block/mirror.c b/block/mirror.c index b84de56734..83aa4176c2 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1246,6 +1246,27 @@ static bool commit_active_cancel(Job *job, bool force) return force || !job_is_ready(job); } +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; +} + static const BlockJobDriver mirror_job_driver = { .job_driver = { .instance_size = sizeof(MirrorBlockJob), @@ -1260,6 +1281,7 @@ static const BlockJobDriver mirror_job_driver = { .cancel = mirror_cancel, }, .drained_poll = mirror_drained_poll, + .change = mirror_change, }; static const BlockJobDriver commit_active_job_driver = { diff --git a/qapi/block-core.json b/qapi/block-core.json index c6f31a9399..01427c259a 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3044,6 +3044,17 @@ { 'command': 'block-job-finalize', 'data': { 'id': 'str' }, 'allow-preconfig': true } +## +# @BlockJobChangeOptionsMirror: +# +# @copy-mode: Switch to this copy mode. Currenlty, only the switch +# from 'background' to 'write-blocking' is implemented. +# +# Since: 8.2 +## +{ 'struct': 'BlockJobChangeOptionsMirror', + 'data': { 'copy-mode' : 'MirrorCopyMode' } } + ## # @BlockJobChangeOptions: # @@ -3058,7 +3069,7 @@ { 'union': 'BlockJobChangeOptions', 'base': { 'id': 'str', 'type': 'JobType' }, 'discriminator': 'type', - 'data': {} } + 'data': { 'mirror': 'BlockJobChangeOptionsMirror' } } ## # @block-job-change:
which allows switching the @copy-mode from 'background' to 'write-blocking'. This is useful for management applications, so they can start out in background mode to avoid limiting guest write speed and switch to active mode when certain criteria are fulfilled. Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> --- Changes in v2: * update QEMU version in QAPI * update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat doc comments to conform to current conventions")) * drop drained section and disable dirty bitmap call. It's already disabled, because the bitmap is now attached to the filter and set in bdrv_mirror_top_do_write(). See the earlier patch "block/mirror: move dirty bitmap to filter" block/mirror.c | 22 ++++++++++++++++++++++ qapi/block-core.json | 13 ++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-)