Re: [PATCH v2 00/10] mirror: allow switching from background to active mode

[Vladimir Sementsov-Ogievskiy]

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<kwolf@redhat.com>  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 functionality.
>
> For output, your implementation is probably not optimal because you're
> going to look up things the calling code already knows, but it's
> probably not the end of the world.

Yes I thought about it, we'll call find_block_job_locked() twice. That's 
probably solvable by including some opaque pointer to QAPI structure, which 
could be set during json parsing and then reused by qmp_* command itself.. But 
I don't think it worth doing for simple search for a job. Simpler approach 
would be to cache globally last result of find_block_job().. But again that 
would be extra optimization

--
Best regards,
Vladimir