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

[Vladimir Sementsov-Ogievskiy]

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' } }


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 example)

BlockJobChangeOptions_mapper() should be defined by hand, like this:

bool BlockJobChangeOptions_mapper(BlockJobChangeOptions *opts, JobType *out,
                                  Error **errp)
{
    BlockJob *job;

    JOB_LOCK_GUARD();

    job = find_block_job_locked(opts->id, errp);
    if (!job) {
        return false;
    }

    return job_type(&job->job);
}



--
Best regards,
Vladimir