qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 00/10] mirror: allow switching from background to active mode
Date: Mon, 11 Mar 2024 18:15:53 +0300
User-agent: Mozilla Thunderbird 
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...


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 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 any sense any more.

Kevin



--
Best regards,
Vladimir
reply via email to
[Prev in Thread] Current Thread [Next in Thread]