Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 12.03.24 18:49, Kevin Wolf wrote:
Am 12.03.2024 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
On 11.03.24 18:15, Vladimir Sementsov-Ogievskiy wrote:
On 08.03.24 11:52, Kevin Wolf wrote:
Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
On 04.03.24 14:09, Peter Krempa wrote:
On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
On 03.11.23 18:56, Markus Armbruster wrote:
Kevin Wolf writes:
[...]
Is the job abstraction a failure?
We have
block-job- command since job- command since
-
block-job-set-speed 1.1
block-job-cancel 1.1 job-cancel 3.0
block-job-pause 1.3 job-pause 3.0
block-job-resume 1.3 job-resume 3.0
block-job-complete 1.3 job-complete 3.0
block-job-dismiss 2.12 job-dismiss 3.0
block-job-finalize 2.12 job-finalize 3.0
block-job-change 8.2
query-block-jobs 1.1 query-jobs
[...]
I consider these strictly optional. We don't really have strong reasons
to deprecate these commands (they are just thin wrappers), and I think
libvirt still uses block-job-* in some places.
Libvirt uses 'block-job-cancel' because it has different semantics from
'job-cancel' which libvirt documented as the behaviour of the API that
uses it. (Semantics regarding the expectation of what is written to the
destination node at the point when the job is cancelled).
That's the following semantics:
# Note that if you issue 'block-job-cancel' after 'drive-mirror' has
# indicated (via the event BLOCK_JOB_READY) that the source and
# destination are synchronized, then the event triggered by this
# command changes to BLOCK_JOB_COMPLETED, to indicate that the
# mirroring has ended and the destination now has a point-in-time copy
# tied to the time of the cancellation.
Hmm. Looking at this, it looks for me, that should probably a
'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).
Yes, it's just a different completion mode.
Actually, what is the difference between block-job-complete and
block-job-cancel(force=false) for mirror in ready state?
I only see the following differencies:
1. block-job-complete documents that it completes the job
synchronously.. But looking at mirror code I see it just set
s->should_complete = true, which will be then handled
asynchronously.. So I doubt that documentation is correct.
2. block-job-complete will trigger final graph changes.
block-job-cancel will not.
Is [2] really useful? Seems yes: in case of some failure before
starting migration target, we'd like to continue executing source. So,
no reason to break block-graph in source, better keep it unchanged.
But I think, such behavior better be setup by mirror-job start
parameter, rather then by special option for cancel (or even
compelete) command, useful only for mirror.
I'm not sure, having the option on the complete command makes more sense
to me than having it in blockdev-mirror.
I do see the challenge of representing this meaningfully in QAPI,
though. Semantically it should be a union with job-specific options and
only mirror adds the graph-changes option. But the union variant
can't be directly selected from another option - instead we have a job
ID, and the variant is the job type of the job with this ID.
We already have such command: block-job-change. Which has id and type
parameters, so user have to pass both, to identify the job itself and pick
corresponding variant of the union type.
That would be good to somehow teach QAPI to get the type automatically from the
job itself...
Seems, that's easy enough to implement such a possibility, I'll try. At least
now I have a prototype, which compiles
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0ae8ae62dc..332de67e52 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3116,13 +3116,11 @@
#
# @id: The job identifier
#
-# @type: The job type
-#
# Since: 8.2
##
{ 'union': 'BlockJobChangeOptions',
- 'base': { 'id': 'str', 'type': 'JobType' },
- 'discriminator': 'type',
+ 'base': { 'id': 'str' },
+ 'discriminator': 'JobType',
'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
We probably need some different syntax because I think in theory we
could get conflicts between option names and type names.
I think we shouldn't, as enum types in QAPI are CamelCase and members are lowercase.
Still, it's probably a bad way to rely on text case (I just imagine documenting this:))
It could be "'discriminator-type': 'JobType'" instead.
But I'll leave
this discussion to Markus. I hope you can figure out something that he
is willing to accept, getting the variant from a callback looks like
useful function
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Am 12.03.2024 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > On 11.03.24 18:15, Vladimir Sementsov-Ogievskiy wrote: > > On 08.03.24 11:52, Kevin Wolf wrote: > > > Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > On 04.03.24 14:09, Peter Krempa wrote: > > > > > On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote: > > > > > > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > > > > On 03.11.23 18:56, Markus Armbruster wrote: > > > > > > > > Kevin Wolf writes: > > > > > > > > > > [...] > > > > > > > > > > > > > Is the job abstraction a failure? > > > > > > > > > > > > > > > > We have > > > > > > > > > > > > > > > > block-job- command since job- command since > > > > > > > > - > > > > > > > > block-job-set-speed 1.1 > > > > > > > > block-job-cancel 1.1 job-cancel 3.0 > > > > > > > > block-job-pause 1.3 job-pause 3.0 > > > > > > > > block-job-resume 1.3 job-resume 3.0 > > > > > > > > block-job-complete 1.3 job-complete 3.0 > > > > > > > > block-job-dismiss 2.12 job-dismiss 3.0 > > > > > > > > block-job-finalize 2.12 job-finalize 3.0 > > > > > > > > block-job-change 8.2 > > > > > > > > query-block-jobs 1.1 query-jobs > > > > > > > > > > [...] > > > > > > > > > > > I consider these strictly optional. We don't really have strong > > > > > > reasons > > > > > > to deprecate these commands (they are just thin wrappers), and I > > > > > > think > > > > > > libvirt still uses block-job-* in some places. > > > > > > > > > > Libvirt uses 'block-job-cancel' because it has different semantics > > > > > from > > > > > 'job-cancel' which libvirt documented as the behaviour of the API that > > > > > uses it. (Semantics regarding the expectation of what is written to > > > > > the > > > > > destination node at the point when the job is cancelled). > > > > > > > > > > > > > That's the following semantics: > > > > > > > > # Note that if you issue 'block-job-cancel' after 'drive-mirror' has > > > > # indicated (via the event BLOCK_JOB_READY) that the source and > > > > # destination are synchronized, then the event triggered by this > > > > # command changes to BLOCK_JOB_COMPLETED, to indicate that the > > > > # mirroring has ended and the destination now has a point-in-time > > > > copy > > > > # tied to the time of the cancellation. > > > > > > > > Hmm. Looking at this, it looks for me, that should probably a > > > > 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED). > > > > > > Yes, it's just a different completion mode. > > > > > > > Actually, what is the difference between block-job-complete and > > > > block-job-cancel(force=false) for mirror in ready state? > > > > > > > > I only see the following differencies: > > > > > > > > 1. block-job-complete documents that it completes the job > > > > synchronously.. But looking at mirror code I see it just set > > > > s->should_complete = true, which will be then handled > > > > asynchronously.. So I doubt that documentation is correct. > > > > > > > > 2. block-job-complete will trigger final graph changes. > > > > block-job-cancel will not. > > > > > > > > Is [2] really useful? Seems yes: in case of some failure before > > > > starting migration target, we'd like to continue executing source. So, > > > > no reason to break block-graph in source, better keep it unchanged. > > > > > > > > But I think, such behavior better be setup by mirror-job start > > > > parameter, rather then by special option for cancel (or even > > > > compelete) command, useful only for mirror. > > > > > > I'm not sure, having the option on the complete command makes more sense > > > to me than having it in blockdev-mirror. > > > > > > I do see the challenge of representing this meaningfully in QAPI, > > > though. Semantically it should be a union with job-specific options and > > > only mirror adds the graph-changes option. But the union variant > > > can't be directly selected from another option - instead we have a job > > > ID, and the variant is the job type of the job with this ID. > > > > We already have such command: block-job-change. Which has id and type > > parameters, so user have to pass both, to identify the job itself and pick > > corresponding variant of the union type. > > > > That would be good to somehow teach QAPI to get the type automatically from > > the job itself... > > > Seems, that's easy enough to implement such a possibility, I'll try. At least > now I have a prototype, which compiles > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 0ae8ae62dc..332de67e52 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3116,13 +3116,11 @@ > # > # @id: The job
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 11.03.24 18:15, Vladimir Sementsov-Ogievskiy wrote:
On 08.03.24 11:52, Kevin Wolf wrote:
Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
On 04.03.24 14:09, Peter Krempa wrote:
On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote:
Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
On 03.11.23 18:56, Markus Armbruster wrote:
Kevin Wolf writes:
[...]
Is the job abstraction a failure?
We have
block-job- command since job- command since
-
block-job-set-speed 1.1
block-job-cancel 1.1 job-cancel 3.0
block-job-pause 1.3 job-pause 3.0
block-job-resume 1.3 job-resume 3.0
block-job-complete 1.3 job-complete 3.0
block-job-dismiss 2.12 job-dismiss 3.0
block-job-finalize 2.12 job-finalize 3.0
block-job-change 8.2
query-block-jobs 1.1 query-jobs
[...]
I consider these strictly optional. We don't really have strong reasons
to deprecate these commands (they are just thin wrappers), and I think
libvirt still uses block-job-* in some places.
Libvirt uses 'block-job-cancel' because it has different semantics from
'job-cancel' which libvirt documented as the behaviour of the API that
uses it. (Semantics regarding the expectation of what is written to the
destination node at the point when the job is cancelled).
That's the following semantics:
# Note that if you issue 'block-job-cancel' after 'drive-mirror' has
# indicated (via the event BLOCK_JOB_READY) that the source and
# destination are synchronized, then the event triggered by this
# command changes to BLOCK_JOB_COMPLETED, to indicate that the
# mirroring has ended and the destination now has a point-in-time copy
# tied to the time of the cancellation.
Hmm. Looking at this, it looks for me, that should probably a
'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED).
Yes, it's just a different completion mode.
Actually, what is the difference between block-job-complete and
block-job-cancel(force=false) for mirror in ready state?
I only see the following differencies:
1. block-job-complete documents that it completes the job
synchronously.. But looking at mirror code I see it just set
s->should_complete = true, which will be then handled
asynchronously.. So I doubt that documentation is correct.
2. block-job-complete will trigger final graph changes.
block-job-cancel will not.
Is [2] really useful? Seems yes: in case of some failure before
starting migration target, we'd like to continue executing source. So,
no reason to break block-graph in source, better keep it unchanged.
But I think, such behavior better be setup by mirror-job start
parameter, rather then by special option for cancel (or even
compelete) command, useful only for mirror.
I'm not sure, having the option on the complete command makes more sense
to me than having it in blockdev-mirror.
I do see the challenge of representing this meaningfully in QAPI,
though. Semantically it should be a union with job-specific options and
only mirror adds the graph-changes option. But the union variant
can't be directly selected from another option - instead we have a job
ID, and the variant is the job type of the job with this ID.
We already have such command: block-job-change. Which has id and type
parameters, so user have to pass both, to identify the job itself and pick
corresponding variant of the union type.
That would be good to somehow teach QAPI to get the type automatically from the
job itself...
Seems, that's easy enough to implement such a possibility, I'll try. At least
now I have a prototype, which compiles
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0ae8ae62dc..332de67e52 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3116,13 +3116,11 @@
#
# @id: The job identifier
#
-# @type: The job type
-#
# Since: 8.2
##
{ 'union': 'BlockJobChangeOptions',
- 'base': { 'id': 'str', 'type': 'JobType' },
- 'discriminator': 'type',
+ 'base': { 'id': 'str' },
+ 'discriminator': 'JobType',
'data': { 'mirror': 'BlockJobChangeOptionsMirror' } }
to
bool visit_type_BlockJobChangeOptions_members(Visitor *v, BlockJobChangeOptions
*obj, Error **errp)
{
JobType tag;
if (!visit_type_q_obj_BlockJobChangeOptions_base_members(v,
(q_obj_BlockJobChangeOptions_base *)obj, errp)) {
return false;
}
if (!BlockJobChangeOptions_mapper(obj, &tag, errp)) {
return false;
}
switch (tag) {
case JOB_TYPE_MIRROR:
return visit_type_BlockJobChangeOptionsMirror_members(v,
&obj->u.mirror, errp);
case JOB_TYPE_COMMIT:
break;
...
(specifying member as descriminator works too, and I'm not going to change the
interface of block-job-change, it's just and e
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On Mon, Mar 11, 2024 at 18:51:18 +0300, Vladimir Sementsov-Ogievskiy wrote: > On 11.03.24 00:07, Peter Krempa wrote: > > On Thu, Mar 07, 2024 at 22:42:56 +0300, Vladimir Sementsov-Ogievskiy wrote: [...] > > Libvirt can adapt to any option that will give us the above semantics > > (extra parameter at completion time, different completion command or > > extra command to switch job properties right before completion), but to > > be honest all of these feel like they would be more hassle than keeping > > 'block-job-cancel' around from qemu's side. > > > > I understand. But still, it would be good to finally resolve the duplication > between job-* and block-job-* APIs. We can keep old quirk working for a long > time even after making a new consistent API. Sure, if you decide to go that way it's okay as long as we can avoid the graph change at 'completion' time. However you decide to implement it just let me know in advance so that I can prepare the libvirt patches for it.
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 11.03.24 00:07, Peter Krempa wrote: On Thu, Mar 07, 2024 at 22:42:56 +0300, Vladimir Sementsov-Ogievskiy wrote: On 04.03.24 14:09, Peter Krempa wrote: On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote: Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: On 03.11.23 18:56, Markus Armbruster wrote: Kevin Wolf writes: [...] I only see the following differencies: 1. block-job-complete documents that it completes the job synchronously.. But looking at mirror code I see it just set s->should_complete = true, which will be then handled asynchronously.. So I doubt that documentation is correct. 2. block-job-complete will trigger final graph changes. block-job-cancel will not. Is [2] really useful? Seems yes: in case of some failure before starting migration target, we'd like to continue executing source. So, no reason to break block-graph in source, better keep it unchanged. But I think, such behavior better be setup by mirror-job start parameter, rather then by special option for cancel (or even compelete) command, useful only for mirror. Libvirt's API design was based on the qemu quirk and thus we allow users to do the decision after starting the job, thus anything you pick needs to allow us to do this at "completion" time. OK, this means that simply switch to an option at job-start is not enough. Libvirt can adapt to any option that will give us the above semantics (extra parameter at completion time, different completion command or extra command to switch job properties right before completion), but to be honest all of these feel like they would be more hassle than keeping 'block-job-cancel' around from qemu's side. I understand. But still, it would be good to finally resolve the duplication between job-* and block-job-* APIs. We can keep old quirk working for a long time even after making a new consistent API. -- Best regards, Vladimir
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 08.03.24 11:52, Kevin Wolf wrote: Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben: On 04.03.24 14:09, Peter Krempa wrote: On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote: Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: On 03.11.23 18:56, Markus Armbruster wrote: Kevin Wolf writes: [...] Is the job abstraction a failure? We have block-job- command since job- commandsince - block-job-set-speed 1.1 block-job-cancel1.1 job-cancel 3.0 block-job-pause 1.3 job-pause 3.0 block-job-resume1.3 job-resume 3.0 block-job-complete 1.3 job-complete3.0 block-job-dismiss 2.12job-dismiss 3.0 block-job-finalize 2.12job-finalize3.0 block-job-change8.2 query-block-jobs1.1 query-jobs [...] I consider these strictly optional. We don't really have strong reasons to deprecate these commands (they are just thin wrappers), and I think libvirt still uses block-job-* in some places. Libvirt uses 'block-job-cancel' because it has different semantics from 'job-cancel' which libvirt documented as the behaviour of the API that uses it. (Semantics regarding the expectation of what is written to the destination node at the point when the job is cancelled). That's the following semantics: # Note that if you issue 'block-job-cancel' after 'drive-mirror' has # indicated (via the event BLOCK_JOB_READY) that the source and # destination are synchronized, then the event triggered by this # command changes to BLOCK_JOB_COMPLETED, to indicate that the # mirroring has ended and the destination now has a point-in-time copy # tied to the time of the cancellation. Hmm. Looking at this, it looks for me, that should probably a 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED). Yes, it's just a different completion mode. Actually, what is the difference between block-job-complete and block-job-cancel(force=false) for mirror in ready state? I only see the following differencies: 1. block-job-complete documents that it completes the job synchronously.. But looking at mirror code I see it just set s->should_complete = true, which will be then handled asynchronously.. So I doubt that documentation is correct. 2. block-job-complete will trigger final graph changes. block-job-cancel will not. Is [2] really useful? Seems yes: in case of some failure before starting migration target, we'd like to continue executing source. So, no reason to break block-graph in source, better keep it unchanged. But I think, such behavior better be setup by mirror-job start parameter, rather then by special option for cancel (or even compelete) command, useful only for mirror. I'm not sure, having the option on the complete command makes more sense to me than having it in blockdev-mirror. I do see the challenge of representing this meaningfully in QAPI, though. Semantically it should be a union with job-specific options and only mirror adds the graph-changes option. But the union variant can't be directly selected from another option - instead we have a job ID, and the variant is the job type of the job with this ID. We already have such command: block-job-change. Which has id and type parameters, so user have to pass both, to identify the job itself and pick corresponding variant of the union type. That would be good to somehow teach QAPI to get the type automatically from the job itself... Probably, best way is to utilize the new "change" command? So, to avoid final graph change, user should either set graph-change=false in blockdev-mirror, or just call job-change(graph-change=false) before job-complete? Still having the option for job-complete looks nicer. But right, we either have to add type parameter like in block-job-change, or add a common option, which would be irrelevant to some jobs. Practically speaking, we would probably indeed end up with an optional field in the existing completion command. So, what about the following substitution for block-job-cancel: block-job-cancel(force=true) --> use job-cancel block-job-cancel(force=false) for backup, stream, commit --> use job-cancel block-job-cancel(force=false) for mirror in ready mode --> instead, use block-job-complete. If you don't need final graph modification which mirror job normally does, use graph-change=false parameter for blockdev-mirror command. Apart from the open question where to put the option, agreed. (I can hardly remember, that we've already discussed something like this long time ago, but I don't remember the results) I think everyone agreed that this is how things should be, and nobody did anything to achieve it. I also a bit unsure about active commit soft-cancelling sem
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On Thu, Mar 07, 2024 at 22:42:56 +0300, Vladimir Sementsov-Ogievskiy wrote: > On 04.03.24 14:09, Peter Krempa wrote: > > On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote: > > > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > On 03.11.23 18:56, Markus Armbruster wrote: > > > > > Kevin Wolf writes: [...] > > I only see the following differencies: > > 1. block-job-complete documents that it completes the job synchronously.. But > looking at mirror code I see it just set s->should_complete = true, which > will be then handled asynchronously.. So I doubt that documentation is > correct. > > 2. block-job-complete will trigger final graph changes. block-job-cancel will > not. > > Is [2] really useful? Seems yes: in case of some failure before starting > migration target, we'd like to continue executing source. So, no reason to > break block-graph in source, better keep it unchanged. > > But I think, such behavior better be setup by mirror-job start parameter, > rather then by special option for cancel (or even compelete) command, useful > only for mirror. Libvirt's API design was based on the qemu quirk and thus we allow users to do the decision after starting the job, thus anything you pick needs to allow us to do this at "completion" time. Libvirt can adapt to any option that will give us the above semantics (extra parameter at completion time, different completion command or extra command to switch job properties right before completion), but to be honest all of these feel like they would be more hassle than keeping 'block-job-cancel' around from qemu's side.
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Am 07.03.2024 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben: > On 04.03.24 14:09, Peter Krempa wrote: > > On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote: > > > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > On 03.11.23 18:56, Markus Armbruster wrote: > > > > > Kevin Wolf writes: > > > > [...] > > > > > > > Is the job abstraction a failure? > > > > > > > > > > We have > > > > > > > > > > block-job- command since job- commandsince > > > > > - > > > > > block-job-set-speed 1.1 > > > > > block-job-cancel1.1 job-cancel 3.0 > > > > > block-job-pause 1.3 job-pause 3.0 > > > > > block-job-resume1.3 job-resume 3.0 > > > > > block-job-complete 1.3 job-complete3.0 > > > > > block-job-dismiss 2.12job-dismiss 3.0 > > > > > block-job-finalize 2.12job-finalize3.0 > > > > > block-job-change8.2 > > > > > query-block-jobs1.1 query-jobs > > > > [...] > > > > > I consider these strictly optional. We don't really have strong reasons > > > to deprecate these commands (they are just thin wrappers), and I think > > > libvirt still uses block-job-* in some places. > > > > Libvirt uses 'block-job-cancel' because it has different semantics from > > 'job-cancel' which libvirt documented as the behaviour of the API that > > uses it. (Semantics regarding the expectation of what is written to the > > destination node at the point when the job is cancelled). > > > > That's the following semantics: > > # Note that if you issue 'block-job-cancel' after 'drive-mirror' has > # indicated (via the event BLOCK_JOB_READY) that the source and > # destination are synchronized, then the event triggered by this > # command changes to BLOCK_JOB_COMPLETED, to indicate that the > # mirroring has ended and the destination now has a point-in-time copy > # tied to the time of the cancellation. > > Hmm. Looking at this, it looks for me, that should probably a > 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED). Yes, it's just a different completion mode. > Actually, what is the difference between block-job-complete and > block-job-cancel(force=false) for mirror in ready state? > > I only see the following differencies: > > 1. block-job-complete documents that it completes the job >synchronously.. But looking at mirror code I see it just set >s->should_complete = true, which will be then handled >asynchronously.. So I doubt that documentation is correct. > > 2. block-job-complete will trigger final graph changes. >block-job-cancel will not. > > Is [2] really useful? Seems yes: in case of some failure before > starting migration target, we'd like to continue executing source. So, > no reason to break block-graph in source, better keep it unchanged. > > But I think, such behavior better be setup by mirror-job start > parameter, rather then by special option for cancel (or even > compelete) command, useful only for mirror. I'm not sure, having the option on the complete command makes more sense to me than having it in blockdev-mirror. I do see the challenge of representing this meaningfully in QAPI, though. Semantically it should be a union with job-specific options and only mirror adds the graph-changes option. But the union variant can't be directly selected from another option - instead we have a job ID, and the variant is the job type of the job with this ID. Practically speaking, we would probably indeed end up with an optional field in the existing completion command. > So, what about the following substitution for block-job-cancel: > > block-job-cancel(force=true) --> use job-cancel > > block-job-cancel(force=false) for backup, stream, commit --> use job-cancel > > block-job-cancel(force=false) for mirror in ready mode --> > > instead, use block-job-complete. If you don't need final graph > modification which mirror job normally does, use graph-change=false > parameter for blockdev-mirror command. Apart from the open question where to put the option, agreed. > (I can hardly remember, that we've already discussed something like > this long time ago, but I don't remember the results) I think everyone agreed that this is how things should be, and nobody did anything to achieve it. > I also a bit unsure about active commit soft-cancelling semantics. Is > it actually useful? If yes, block-commit command will need similar > option. Hm... That would commit everything down to the lower layer and then keep the old overlays still around? I could see a limited use case for committing into the immediate backing file and then keep using the overlay to accumulate new changes (would be more useful if we discarded the old content). Once you have intermediate backing files, I don't think it makes
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Am 07.03.24 um 20:42 schrieb Vladimir Sementsov-Ogievskiy: > On 04.03.24 14:09, Peter Krempa wrote: >> On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote: >>> Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: On 03.11.23 18:56, Markus Armbruster wrote: > Kevin Wolf writes: >> >> [...] >> > Is the job abstraction a failure? > > We have > > block-job- command since job- command since > - > block-job-set-speed 1.1 > block-job-cancel 1.1 job-cancel 3.0 > block-job-pause 1.3 job-pause 3.0 > block-job-resume 1.3 job-resume 3.0 > block-job-complete 1.3 job-complete 3.0 > block-job-dismiss 2.12 job-dismiss 3.0 > block-job-finalize 2.12 job-finalize 3.0 > block-job-change 8.2 > query-block-jobs 1.1 query-jobs >> >> [...] >> >>> I consider these strictly optional. We don't really have strong reasons >>> to deprecate these commands (they are just thin wrappers), and I think >>> libvirt still uses block-job-* in some places. >> >> Libvirt uses 'block-job-cancel' because it has different semantics from >> 'job-cancel' which libvirt documented as the behaviour of the API that >> uses it. (Semantics regarding the expectation of what is written to the >> destination node at the point when the job is cancelled). >> > > That's the following semantics: > > # Note that if you issue 'block-job-cancel' after 'drive-mirror' has > # indicated (via the event BLOCK_JOB_READY) that the source and > # destination are synchronized, then the event triggered by this > # command changes to BLOCK_JOB_COMPLETED, to indicate that the > # mirroring has ended and the destination now has a point-in-time copy > # tied to the time of the cancellation. > > Hmm. Looking at this, it looks for me, that should probably a > 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED). > > Actually, what is the difference between block-job-complete and > block-job-cancel(force=false) for mirror in ready state? > > I only see the following differencies: > > 1. block-job-complete documents that it completes the job > synchronously.. But looking at mirror code I see it just set > s->should_complete = true, which will be then handled asynchronously.. > So I doubt that documentation is correct. > > 2. block-job-complete will trigger final graph changes. block-job-cancel > will not. > > Is [2] really useful? Seems yes: in case of some failure before starting > migration target, we'd like to continue executing source. So, no reason > to break block-graph in source, better keep it unchanged. > FWIW, we also rely on these special semantics. We allow cloning the disk state of a running guest using drive-mirror (and before finishing, fsfreeze in the guest for consistency). We cannot use block-job-complete there, because we do not want to switch the source's drive. > But I think, such behavior better be setup by mirror-job start > parameter, rather then by special option for cancel (or even compelete) > command, useful only for mirror. > > So, what about the following substitution for block-job-cancel: > > block-job-cancel(force=true) --> use job-cancel > > block-job-cancel(force=false) for backup, stream, commit --> use > job-cancel > > block-job-cancel(force=false) for mirror in ready mode --> > > instead, use block-job-complete. If you don't need final graph > modification which mirror job normally does, use graph-change=false > parameter for blockdev-mirror command. > But yes, having a graph-change parameter would work for us too :) Best Regards, Fiona
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 04.03.24 14:09, Peter Krempa wrote: On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote: Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: On 03.11.23 18:56, Markus Armbruster wrote: Kevin Wolf writes: [...] Is the job abstraction a failure? We have block-job- command since job- commandsince - block-job-set-speed 1.1 block-job-cancel1.1 job-cancel 3.0 block-job-pause 1.3 job-pause 3.0 block-job-resume1.3 job-resume 3.0 block-job-complete 1.3 job-complete3.0 block-job-dismiss 2.12job-dismiss 3.0 block-job-finalize 2.12job-finalize3.0 block-job-change8.2 query-block-jobs1.1 query-jobs [...] I consider these strictly optional. We don't really have strong reasons to deprecate these commands (they are just thin wrappers), and I think libvirt still uses block-job-* in some places. Libvirt uses 'block-job-cancel' because it has different semantics from 'job-cancel' which libvirt documented as the behaviour of the API that uses it. (Semantics regarding the expectation of what is written to the destination node at the point when the job is cancelled). That's the following semantics: # Note that if you issue 'block-job-cancel' after 'drive-mirror' has # indicated (via the event BLOCK_JOB_READY) that the source and # destination are synchronized, then the event triggered by this # command changes to BLOCK_JOB_COMPLETED, to indicate that the # mirroring has ended and the destination now has a point-in-time copy # tied to the time of the cancellation. Hmm. Looking at this, it looks for me, that should probably a 'block-job-complete" command (as leading to BLOCK_JOB_COMPLETED). Actually, what is the difference between block-job-complete and block-job-cancel(force=false) for mirror in ready state? I only see the following differencies: 1. block-job-complete documents that it completes the job synchronously.. But looking at mirror code I see it just set s->should_complete = true, which will be then handled asynchronously.. So I doubt that documentation is correct. 2. block-job-complete will trigger final graph changes. block-job-cancel will not. Is [2] really useful? Seems yes: in case of some failure before starting migration target, we'd like to continue executing source. So, no reason to break block-graph in source, better keep it unchanged. But I think, such behavior better be setup by mirror-job start parameter, rather then by special option for cancel (or even compelete) command, useful only for mirror. So, what about the following substitution for block-job-cancel: block-job-cancel(force=true) --> use job-cancel block-job-cancel(force=false) for backup, stream, commit --> use job-cancel block-job-cancel(force=false) for mirror in ready mode --> instead, use block-job-complete. If you don't need final graph modification which mirror job normally does, use graph-change=false parameter for blockdev-mirror command. (I can hardly remember, that we've already discussed something like this long time ago, but I don't remember the results) I also a bit unsure about active commit soft-cancelling semantics. Is it actually useful? If yes, block-commit command will need similar option. -- Best regards, Vladimir
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Kevin Wolf writes: > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: [...] >> About the APIs, I think, of course we should deprecate block-job-* API, >> because we already have jobs which are not block-jobs, so we can't deprecate >> job-* API. >> >> So I suggest a plan: >> >> 1. Add job-change command simply in block-core.json, as a simple copy >>of block-job-change, to not care with resolving inclusion loops. >>(ha we could simply name our block-job-change to be job-change and >>place it in block-core.json, but now is too late) >> >> 2. Support changing speed in a new job-chage command. (or both in >>block-job-change and job-change, keeping them equal) > > It should be both block-job-change and job-change. > > Having job-change in block-core.json rather than job.json is ugly, but > if Markus doesn't complain, why would I. What we have is ugly and confusing: two interfaces with insufficient guidance on which one to use. Unifying the interfaces will reduce confusion immediately, and may reduce ugliness eventually. I take it. [...]
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On Mon, Mar 04, 2024 at 11:48:54 +0100, Kevin Wolf wrote: > Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: > > On 03.11.23 18:56, Markus Armbruster wrote: > > > Kevin Wolf writes: [...] > > > Is the job abstraction a failure? > > > > > > We have > > > > > > block-job- command since job- commandsince > > > - > > > block-job-set-speed 1.1 > > > block-job-cancel1.1 job-cancel 3.0 > > > block-job-pause 1.3 job-pause 3.0 > > > block-job-resume1.3 job-resume 3.0 > > > block-job-complete 1.3 job-complete3.0 > > > block-job-dismiss 2.12job-dismiss 3.0 > > > block-job-finalize 2.12job-finalize3.0 > > > block-job-change8.2 > > > query-block-jobs1.1 query-jobs [...] > I consider these strictly optional. We don't really have strong reasons > to deprecate these commands (they are just thin wrappers), and I think > libvirt still uses block-job-* in some places. Libvirt uses 'block-job-cancel' because it has different semantics from 'job-cancel' which libvirt documented as the behaviour of the API that uses it. (Semantics regarding the expectation of what is written to the destination node at the point when the job is cancelled). Libvirt also uses 'block-job-set-speed' and 'query-block-jobs' but those can be replaced easily and looking at the above table even without any feature checks. Thus the plan to deprecate at least 'block-job-cancel' will not work unless the semantics are ported into 'job-cancel'.
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Am 28.02.2024 um 19:07 hat Vladimir Sementsov-Ogievskiy geschrieben: > On 03.11.23 18:56, Markus Armbruster wrote: > > Kevin Wolf writes: > > > > > Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben: > > > > Vladimir Sementsov-Ogievskiy writes: > > > > > > > > > On 11.10.23 13:18, Fiona Ebner wrote: > > > > > > Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy: > > > > > > > On 09.10.23 12:46, Fiona Ebner wrote: > > > > > > > > Initially, I tried to go for a more general 'job-change' > > > > > > > > command, but > > > > > > > > I couldn't figure out a way to avoid mutual inclusion between > > > > > > > > block-core.json and job.json. > > > > > > > > > > > > > > > What is the problem with it? I still think that job-change would > > > > > > > be better. > > > > > > > > > > > > > If going for job-change in job.json, the dependencies would be > > > > > > job-change -> JobChangeOptions -> JobChangeOptionsMirror -> > > > > > > MirrorCopyMode > > > > > > query-jobs -> JobInfo -> JobInfoMirror > > > > > > and we can't include block-core.json in job.json, because an > > > > > > inclusion > > > > > > loop gives a build error. > > > > Let me try to understand this. > > > > > > > > Command job-change needs its argument type JobChangeOptions. > > > > > > > > JobChangeOptions is a union, and JobChangeOptionsMirror is one of its > > > > branches. > > > > > > > > JobChangeOptionsMirror needs MirrorCopyMode from block-core.json. > > > > > > > > block-core.json needs job.json for JobType and JobStatus. > > > > > > > > > > Could be made to work by moving MirrorCopyMode (and > > > > > > JobChangeOptionsMirror, JobInfoMirror) to job.json or some place > > > > > > that > > > > > > can be included by both job.json and block-core.json. Moving the > > > > > > type-specific definitions to the general job.json didn't feel right > > > > > > to > > > > > > me. Including another file with type-specific definitions in > > > > > > job.json > > > > > > feels slightly less wrong, but still not quite right and I didn't > > > > > > want > > > > > > to create a new file just for MirrorCopyMode (and > > > > > > JobChangeOptionsMirror, JobInfoMirror). > > > > > > And going further and moving all mirror-related things to a separate > > > > > > file would require moving along things like NewImageMode with it or > > > > > > create yet another file for such general things used by multiple > > > > > > block-jobs. > > > > > > If preferred, I can try and go with some version of the above. > > > > > > > > > > > OK, I see the problem. Seems, that all requires some good > > > > > refactoring. But that's a preexisting big work, and should not hold > > > > > up your series. I'm OK to proceed with block-job-change. > > > > Saving ourselves some internal refactoring is a poor excuse for > > > > undesirable external interfaces. > > > I'm not sure how undesirable it is. We have block-job-* commands for > > > pretty much every other operation, so it's only consistent to have > > > block-job-change, too. > > Is the job abstraction a failure? > > > > We have > > > > block-job- command since job- commandsince > > - > > block-job-set-speed 1.1 > > block-job-cancel1.1 job-cancel 3.0 > > block-job-pause 1.3 job-pause 3.0 > > block-job-resume1.3 job-resume 3.0 > > block-job-complete 1.3 job-complete3.0 > > block-job-dismiss 2.12job-dismiss 3.0 > > block-job-finalize 2.12job-finalize3.0 > > block-job-change8.2 > > query-block-jobs1.1 query-jobs > > > > I was under the impression that we added the (more general) job- > > commands to replace the (less general) block-job commands, and we're > > keeping the latter just for compatibility. Am I mistaken? > > > > Which one should be used? > > > > Why not deprecate the one that shouldn't be used? > > > > The addition of block-job-change without even trying to do job-change > > makes me wonder: have we given up on the job- interface? > > > > I'm okay with giving up on failures. All I want is clarity. Right now, > > I feel thoroughly confused about the status block-jobs and jobs, and how > > they're related. > > Hi! I didn't notice, that the series was finally merged. > > About the APIs, I think, of course we should deprecate block-job-* API, > because we already have jobs which are not block-jobs, so we can't deprecate > job-* API. > > So I suggest a plan: > > 1. Add job-change command simply in block-core.json, as a simple copy >of block-job-change, to not care with resolving inclusion loops. >(ha we could simply name our block-job-change to be job-change and >place it in block-core.json, but now is too late) > > 2. Support changing speed in a new job-chage command. (or both in >block-job-change and job-change, keeping them equa
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Vladimir Sementsov-Ogievskiy writes:
> On 03.11.23 18:56, Markus Armbruster wrote:
>> Is the job abstraction a failure?
>>
>> We have
>>
>> block-job- command since job- commandsince
>> -
>> block-job-set-speed 1.1
>> block-job-cancel1.1 job-cancel 3.0
>> block-job-pause 1.3 job-pause 3.0
>> block-job-resume1.3 job-resume 3.0
>> block-job-complete 1.3 job-complete3.0
>> block-job-dismiss 2.12job-dismiss 3.0
>> block-job-finalize 2.12job-finalize3.0
>> block-job-change8.2
>> query-block-jobs1.1 query-jobs
>>
>> I was under the impression that we added the (more general) job-
>> commands to replace the (less general) block-job commands, and we're
>> keeping the latter just for compatibility. Am I mistaken?
>>
>> Which one should be used?
>>
>> Why not deprecate the one that shouldn't be used?
>>
>> The addition of block-job-change without even trying to do job-change
>> makes me wonder: have we given up on the job- interface?
>>
>> I'm okay with giving up on failures. All I want is clarity. Right now,
>> I feel thoroughly confused about the status block-jobs and jobs, and how
>> they're related.
>
> Hi! I didn't notice, that the series was finally merged.
>
> About the APIs, I think, of course we should deprecate block-job-* API,
> because we already have jobs which are not block-jobs, so we can't deprecate
> job-* API.
>
> So I suggest a plan:
>
> 1. Add job-change command simply in block-core.json, as a simple copy of
> block-job-change, to not care with resolving inclusion loops. (ha we could
> simply name our block-job-change to be job-change and place it in
> block-core.json, but now is too late)
>
> 2. Support changing speed in a new job-chage command. (or both in
> block-job-change and job-change, keeping them equal)
>
> 3. Deprecate block-job-* APIs
>
> 4. Wait 3 releases
>
> 5. Drop block-job-* APIs
>
> 6. Move all job-related stuff to job.json, drop `{ 'include': 'job.json' }`
> from block-core.json, and instead include block-core.json into job.json
Sounds good to me.
> If it's OK, I can go through the steps.
Lovely!
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 03.11.23 18:56, Markus Armbruster wrote:
Kevin Wolf writes:
Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
Vladimir Sementsov-Ogievskiy writes:
On 11.10.23 13:18, Fiona Ebner wrote:
Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
On 09.10.23 12:46, Fiona Ebner wrote:
Initially, I tried to go for a more general 'job-change' command, but
I couldn't figure out a way to avoid mutual inclusion between
block-core.json and job.json.
What is the problem with it? I still think that job-change would be better.
If going for job-change in job.json, the dependencies would be
job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
query-jobs -> JobInfo -> JobInfoMirror
and we can't include block-core.json in job.json, because an inclusion
loop gives a build error.
Let me try to understand this.
Command job-change needs its argument type JobChangeOptions.
JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
branches.
JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
block-core.json needs job.json for JobType and JobStatus.
Could be made to work by moving MirrorCopyMode (and
JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
can be included by both job.json and block-core.json. Moving the
type-specific definitions to the general job.json didn't feel right to
me. Including another file with type-specific definitions in job.json
feels slightly less wrong, but still not quite right and I didn't want
to create a new file just for MirrorCopyMode (and
JobChangeOptionsMirror, JobInfoMirror).
And going further and moving all mirror-related things to a separate
file would require moving along things like NewImageMode with it or
create yet another file for such general things used by multiple block-jobs.
If preferred, I can try and go with some version of the above.
OK, I see the problem. Seems, that all requires some good refactoring. But
that's a preexisting big work, and should not hold up your series. I'm OK to
proceed with block-job-change.
Saving ourselves some internal refactoring is a poor excuse for
undesirable external interfaces.
I'm not sure how undesirable it is. We have block-job-* commands for
pretty much every other operation, so it's only consistent to have
block-job-change, too.
Is the job abstraction a failure?
We have
block-job- command since job- commandsince
-
block-job-set-speed 1.1
block-job-cancel1.1 job-cancel 3.0
block-job-pause 1.3 job-pause 3.0
block-job-resume1.3 job-resume 3.0
block-job-complete 1.3 job-complete3.0
block-job-dismiss 2.12job-dismiss 3.0
block-job-finalize 2.12job-finalize3.0
block-job-change8.2
query-block-jobs1.1 query-jobs
I was under the impression that we added the (more general) job-
commands to replace the (less general) block-job commands, and we're
keeping the latter just for compatibility. Am I mistaken?
Which one should be used?
Why not deprecate the one that shouldn't be used?
The addition of block-job-change without even trying to do job-change
makes me wonder: have we given up on the job- interface?
I'm okay with giving up on failures. All I want is clarity. Right now,
I feel thoroughly confused about the status block-jobs and jobs, and how
they're related.
Hi! I didn't notice, that the series was finally merged.
About the APIs, I think, of course we should deprecate block-job-* API, because
we already have jobs which are not block-jobs, so we can't deprecate job-* API.
So I suggest a plan:
1. Add job-change command simply in block-core.json, as a simple copy of
block-job-change, to not care with resolving inclusion loops. (ha we could
simply name our block-job-change to be job-change and place it in
block-core.json, but now is too late)
2. Support changing speed in a new job-chage command. (or both in
block-job-change and job-change, keeping them equal)
3. Deprecate block-job-* APIs
4. Wait 3 releases
5. Drop block-job-* APIs
6. Move all job-related stuff to job.json, drop `{ 'include': 'job.json' }`
from block-core.json, and instead include block-core.json into job.json
If it's OK, I can go through the steps.
--
Best regards,
Vladimir
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Kevin Wolf writes: > Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben: >> Vladimir Sementsov-Ogievskiy writes: >> >> > On 11.10.23 13:18, Fiona Ebner wrote: >> >> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy: >> >>> On 09.10.23 12:46, Fiona Ebner wrote: >> >> Initially, I tried to go for a more general 'job-change' command, but >> I couldn't figure out a way to avoid mutual inclusion between >> block-core.json and job.json. >> >> >>> >> >>> What is the problem with it? I still think that job-change would be >> >>> better. >> >>> >> >> If going for job-change in job.json, the dependencies would be >> >> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode >> >> query-jobs -> JobInfo -> JobInfoMirror >> >> and we can't include block-core.json in job.json, because an inclusion >> >> loop gives a build error. >> >> Let me try to understand this. >> >> Command job-change needs its argument type JobChangeOptions. >> >> JobChangeOptions is a union, and JobChangeOptionsMirror is one of its >> branches. >> >> JobChangeOptionsMirror needs MirrorCopyMode from block-core.json. >> >> block-core.json needs job.json for JobType and JobStatus. >> >> >> Could be made to work by moving MirrorCopyMode (and >> >> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that >> >> can be included by both job.json and block-core.json. Moving the >> >> type-specific definitions to the general job.json didn't feel right to >> >> me. Including another file with type-specific definitions in job.json >> >> feels slightly less wrong, but still not quite right and I didn't want >> >> to create a new file just for MirrorCopyMode (and >> >> JobChangeOptionsMirror, JobInfoMirror). >> >> And going further and moving all mirror-related things to a separate >> >> file would require moving along things like NewImageMode with it or >> >> create yet another file for such general things used by multiple >> >> block-jobs. >> >> If preferred, I can try and go with some version of the above. >> >> >> > >> > OK, I see the problem. Seems, that all requires some good refactoring. But >> > that's a preexisting big work, and should not hold up your series. I'm OK >> > to proceed with block-job-change. >> >> Saving ourselves some internal refactoring is a poor excuse for >> undesirable external interfaces. > > I'm not sure how undesirable it is. We have block-job-* commands for > pretty much every other operation, so it's only consistent to have > block-job-change, too. Is the job abstraction a failure? We have block-job- command since job- commandsince - block-job-set-speed 1.1 block-job-cancel1.1 job-cancel 3.0 block-job-pause 1.3 job-pause 3.0 block-job-resume1.3 job-resume 3.0 block-job-complete 1.3 job-complete3.0 block-job-dismiss 2.12job-dismiss 3.0 block-job-finalize 2.12job-finalize3.0 block-job-change8.2 query-block-jobs1.1 query-jobs I was under the impression that we added the (more general) job- commands to replace the (less general) block-job commands, and we're keeping the latter just for compatibility. Am I mistaken? Which one should be used? Why not deprecate the one that shouldn't be used? The addition of block-job-change without even trying to do job-change makes me wonder: have we given up on the job- interface? I'm okay with giving up on failures. All I want is clarity. Right now, I feel thoroughly confused about the status block-jobs and jobs, and how they're related. > Having job-change, too, might be nice in theory, but we don't have even > a potential user for it at this point (i.e. a job type that isn't a > block job, but for which changing options at runtime makes sense). > >> We need to answer two questions before we do that: >> >> 1. How much work would the refactoring be? >> >> 2. Is the interface improvement this enables worth the work? >> >> Let's start with 1. >> >> An obvious solution is to split JobType and JobStatus off job.json to >> break the dependency of block-core.json on job.json. >> >> But I'd like us to investigate another one. block-core.json is *huge*. >> It's almost a quarter of the entire QAPI schema. Can we spin out block >> jobs into block-job.json? Moves the dependency on job.json from >> block-core.json to block-job.json. > > It also makes job.json depend on block-job.json instead of > block-core.json, so you only moved the problem without solving it. block-job.json needs block-core.json and job.json. job.json needs block-core.json. No circle so far.
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben: > Vladimir Sementsov-Ogievskiy writes: > > > On 11.10.23 13:18, Fiona Ebner wrote: > >> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy: > >>> On 09.10.23 12:46, Fiona Ebner wrote: > > Initially, I tried to go for a more general 'job-change' command, but > I couldn't figure out a way to avoid mutual inclusion between > block-core.json and job.json. > > >>> > >>> What is the problem with it? I still think that job-change would be > >>> better. > >>> > >> If going for job-change in job.json, the dependencies would be > >> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode > >> query-jobs -> JobInfo -> JobInfoMirror > >> and we can't include block-core.json in job.json, because an inclusion > >> loop gives a build error. > > Let me try to understand this. > > Command job-change needs its argument type JobChangeOptions. > > JobChangeOptions is a union, and JobChangeOptionsMirror is one of its > branches. > > JobChangeOptionsMirror needs MirrorCopyMode from block-core.json. > > block-core.json needs job.json for JobType and JobStatus. > > >> Could be made to work by moving MirrorCopyMode (and > >> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that > >> can be included by both job.json and block-core.json. Moving the > >> type-specific definitions to the general job.json didn't feel right to > >> me. Including another file with type-specific definitions in job.json > >> feels slightly less wrong, but still not quite right and I didn't want > >> to create a new file just for MirrorCopyMode (and > >> JobChangeOptionsMirror, JobInfoMirror). > >> And going further and moving all mirror-related things to a separate > >> file would require moving along things like NewImageMode with it or > >> create yet another file for such general things used by multiple > >> block-jobs. > >> If preferred, I can try and go with some version of the above. > >> > > > > OK, I see the problem. Seems, that all requires some good refactoring. But > > that's a preexisting big work, and should not hold up your series. I'm OK > > to proceed with block-job-change. > > Saving ourselves some internal refactoring is a poor excuse for > undesirable external interfaces. I'm not sure how undesirable it is. We have block-job-* commands for pretty much every other operation, so it's only consistent to have block-job-change, too. Having job-change, too, might be nice in theory, but we don't have even a potential user for it at this point (i.e. a job type that isn't a block job, but for which changing options at runtime makes sense). > We need to answer two questions before we do that: > > 1. How much work would the refactoring be? > > 2. Is the interface improvement this enables worth the work? > > Let's start with 1. > > An obvious solution is to split JobType and JobStatus off job.json to > break the dependency of block-core.json on job.json. > > But I'd like us to investigate another one. block-core.json is *huge*. > It's almost a quarter of the entire QAPI schema. Can we spin out block > jobs into block-job.json? Moves the dependency on job.json from > block-core.json to block-job.json. It also makes job.json depend on block-job.json instead of block-core.json, so you only moved the problem without solving it. Kevin
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Vladimir Sementsov-Ogievskiy writes: > On 11.10.23 13:18, Fiona Ebner wrote: >> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy: >>> On 09.10.23 12:46, Fiona Ebner wrote: Initially, I tried to go for a more general 'job-change' command, but I couldn't figure out a way to avoid mutual inclusion between block-core.json and job.json. >>> >>> What is the problem with it? I still think that job-change would be better. >>> >> If going for job-change in job.json, the dependencies would be >> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode >> query-jobs -> JobInfo -> JobInfoMirror >> and we can't include block-core.json in job.json, because an inclusion >> loop gives a build error. Let me try to understand this. Command job-change needs its argument type JobChangeOptions. JobChangeOptions is a union, and JobChangeOptionsMirror is one of its branches. JobChangeOptionsMirror needs MirrorCopyMode from block-core.json. block-core.json needs job.json for JobType and JobStatus. >> Could be made to work by moving MirrorCopyMode (and >> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that >> can be included by both job.json and block-core.json. Moving the >> type-specific definitions to the general job.json didn't feel right to >> me. Including another file with type-specific definitions in job.json >> feels slightly less wrong, but still not quite right and I didn't want >> to create a new file just for MirrorCopyMode (and >> JobChangeOptionsMirror, JobInfoMirror). >> And going further and moving all mirror-related things to a separate >> file would require moving along things like NewImageMode with it or >> create yet another file for such general things used by multiple block-jobs. >> If preferred, I can try and go with some version of the above. >> > > OK, I see the problem. Seems, that all requires some good refactoring. But > that's a preexisting big work, and should not hold up your series. I'm OK to > proceed with block-job-change. Saving ourselves some internal refactoring is a poor excuse for undesirable external interfaces. We need to answer two questions before we do that: 1. How much work would the refactoring be? 2. Is the interface improvement this enables worth the work? Let's start with 1. An obvious solution is to split JobType and JobStatus off job.json to break the dependency of block-core.json on job.json. But I'd like us to investigate another one. block-core.json is *huge*. It's almost a quarter of the entire QAPI schema. Can we spin out block jobs into block-job.json? Moves the dependency on job.json from block-core.json to block-job.json. Thoughts?
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 11.10.23 13:18, Fiona Ebner wrote: Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy: On 09.10.23 12:46, Fiona Ebner wrote: Initially, I tried to go for a more general 'job-change' command, but I couldn't figure out a way to avoid mutual inclusion between block-core.json and job.json. What is the problem with it? I still think that job-change would be better. If going for job-change in job.json, the dependencies would be job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode query-jobs -> JobInfo -> JobInfoMirror and we can't include block-core.json in job.json, because an inclusion loop gives a build error. Could be made to work by moving MirrorCopyMode (and JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that can be included by both job.json and block-core.json. Moving the type-specific definitions to the general job.json didn't feel right to me. Including another file with type-specific definitions in job.json feels slightly less wrong, but still not quite right and I didn't want to create a new file just for MirrorCopyMode (and JobChangeOptionsMirror, JobInfoMirror). And going further and moving all mirror-related things to a separate file would require moving along things like NewImageMode with it or create yet another file for such general things used by multiple block-jobs. If preferred, I can try and go with some version of the above. OK, I see the problem. Seems, that all requires some good refactoring. But that's a preexisting big work, and should not hold up your series. I'm OK to proceed with block-job-change. -- Best regards, Vladimir
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy: > On 09.10.23 12:46, Fiona Ebner wrote: >> >> Initially, I tried to go for a more general 'job-change' command, but >> I couldn't figure out a way to avoid mutual inclusion between >> block-core.json and job.json. >> > > What is the problem with it? I still think that job-change would be better. > If going for job-change in job.json, the dependencies would be job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode query-jobs -> JobInfo -> JobInfoMirror and we can't include block-core.json in job.json, because an inclusion loop gives a build error. Could be made to work by moving MirrorCopyMode (and JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that can be included by both job.json and block-core.json. Moving the type-specific definitions to the general job.json didn't feel right to me. Including another file with type-specific definitions in job.json feels slightly less wrong, but still not quite right and I didn't want to create a new file just for MirrorCopyMode (and JobChangeOptionsMirror, JobInfoMirror). And going further and moving all mirror-related things to a separate file would require moving along things like NewImageMode with it or create yet another file for such general things used by multiple block-jobs. If preferred, I can try and go with some version of the above. Best Regards, Fiona
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 10.10.23 20:55, Vladimir Sementsov-Ogievskiy wrote:
On 09.10.23 12:46, Fiona Ebner wrote:
Changes in v2:
* move bitmap to filter which allows to avoid draining when
changing the copy mode
* add patch to determine copy_to_target only once
* drop patches returning redundant information upon query
* update QEMU version in QAPI
* update indentation in QAPI
* update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
doc comments to conform to current conventions"))
* add patch to adapt iotest output
Discussion of v1:
https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg07216.html
With active mode, the guest write speed is limited by the synchronous
writes to the mirror target. For this reason, management applications
might want to start out in background mode and only switch to active
mode later, when certain conditions are met. This series adds a
block-job-change QMP command to achieve that, as well as
job-type-specific information when querying block jobs, which
can be used to decide when the switch should happen.
For now, only the direction background -> active is supported.
The information added upon querying is whether the target is actively
synced, the total data sent, and the remaining dirty bytes.
Initially, I tried to go for a more general 'job-change' command, but
I couldn't figure out a way to avoid mutual inclusion between
block-core.json and job.json.
What is the problem with it? I still think that job-change would be better.
However, that's not a show-stopper. We have block-job-* commands, they are not
deprecated, no problem to add one more.
--
Best regards,
Vladimir
Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
On 09.10.23 12:46, Fiona Ebner wrote:
Changes in v2:
* move bitmap to filter which allows to avoid draining when
changing the copy mode
* add patch to determine copy_to_target only once
* drop patches returning redundant information upon query
* update QEMU version in QAPI
* update indentation in QAPI
* update indentation in QAPI (like in a937b6aa73 ("qapi: Reformat
doc comments to conform to current conventions"))
* add patch to adapt iotest output
Discussion of v1:
https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg07216.html
With active mode, the guest write speed is limited by the synchronous
writes to the mirror target. For this reason, management applications
might want to start out in background mode and only switch to active
mode later, when certain conditions are met. This series adds a
block-job-change QMP command to achieve that, as well as
job-type-specific information when querying block jobs, which
can be used to decide when the switch should happen.
For now, only the direction background -> active is supported.
The information added upon querying is whether the target is actively
synced, the total data sent, and the remaining dirty bytes.
Initially, I tried to go for a more general 'job-change' command, but
I couldn't figure out a way to avoid mutual inclusion between
block-core.json and job.json.
What is the problem with it? I still think that job-change would be better.
Fiona Ebner (10):
blockjob: introduce block-job-change QMP command
block/mirror: set actively_synced even after the job is ready
block/mirror: move dirty bitmap to filter
block/mirror: determine copy_to_target only once
mirror: implement mirror_change method
qapi/block-core: use JobType for BlockJobInfo's type
qapi/block-core: turn BlockJobInfo into a union
blockjob: query driver-specific info via a new 'query' driver method
mirror: return mirror-specific information upon query
iotests: adapt test output for new mirror query property
block/mirror.c | 95 +++---
block/monitor/block-hmp-cmds.c | 4 +-
blockdev.c | 14 +
blockjob.c | 26 +-
include/block/blockjob.h | 11
include/block/blockjob_int.h | 10
job.c | 1 +
qapi/block-core.json | 59 +++--
qapi/job.json | 4 +-
tests/qemu-iotests/109.out | 24 -
10 files changed, 199 insertions(+), 49 deletions(-)
--
Best regards,
Vladimir
