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

[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<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).

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