3:21 a.m.
On 28.11.2016 11:56, Peter Krempa wrote:
On Mon, Nov 28, 2016 at 11:15:41 +0300, Nikolay Shirokovskiy wrote:
> Instead I suggest to serialize qemu monitor commands.
That won't be possible. There are a few APIs that call multiple commands
and expect the VM not to change in between. Thus they rely on nested
jobs. See migration and snapshot code (btw, the two most complex parts
of libvirt).
But I suggest to drop only nested jobs and use other means to accomplish
the same goal. The jobs in general stays as they are after this series. Sync jobs are
mutually
exclusive, async jobs are mutually exclusive, query (and some other "safe") sync
jobs
can run in parallel with async etc. The only thing is change how the last
are executed in parallels.
Probably I'd better use 'replace' instead of 'drop'.
> My original motivation is to prepare job exclusion code to add agent jobs. This
> job is a special job to operate on guest agent only so first it can be run in
> parallel with sync, async and nested jobs and second it should not block any
That is possible. The async jobs allow for query jobs to be run while
they are executed you need to properly configure them.
This is not enough. I don't want agent jobs to block any current async or
sync jobs from executing. This is not possible with current code.
> qemu monitor job at all. Eventually I want to move from a lot of different
> types of jobs to just a job with some kind of matrix which describes what jobs
> can run in a parallel. As a first step I want to drop nested jobs.
I don't think you will be able to do this in steps. Nested jobs have a
vital functions and unless you replace the mechanism or overhaul all the
code that is doing nested jobs you won't be able to guarantee that the
state of the objects are as the code expects.
Gosh, why i used 'drop', it is 'replace'. I replace serializing thru
nested
jobs and sync jobs with just serializing at monitor level.
Doin a ful N*N matrix of possible parallel operations will be very hard
to maintain. You will need to group them together in a way so that we
don't have to deal with the quadratic complexity. How are you expecting
to do this?
This matrix exists already and it based of job types not every individual
job itsel. It is just not apparent in code. Here it's sample:
Q M A
Q x x o
M x x x
A x x x
Q - query job
M - modify job
A - async job
x - inpossible
o - possible
This matrix shows can jobs run in parallel. It is assymetrical now, you can
run query while async job is executing but can not start async job while
in query. This assymetric is probably current implementation restrictment.
> AFAIU *main* nested job purpuse is to serialize requests to
monitor from async
> jobs and allowed sync jobs as monitor does not do it by himself now. Strictly
No, the main purpose is to allow to call multiple monitor commands while
you are guaranteed that something did not change the VM state and thus
invalidate the complex operation you are trying to execute.
For this reason in matrix above AM, AX are impossible. AQ are possible and
it is decided by matrix not nested jobs.
> speaking this assertion is rather rough because nested jobs and sync jobs
> serialized at job level and thus chunks of code that serialized differ from
> case when only monitor requests are serialized. Nevertheless IFAIU this part of
> libvirt is not strict too. We have a lot of query commands and async commands
> that can execute in parallel and have no definition of what query job or async
> job are. Thus it is not clear is this code correct or not. So the same argument
Certainly it's more correct this way than dropping it without
replacement.
> is applicable to new approach: query jobs are safe to be called during async
> jobs. (Except that new approach opens situation when async job interrupts
Some of qemu monitor commands block on execution. The job mechanism
prevents the second query call to get stuck waiting forever for the job
to finish by doing a timed wait.
This will not change. Monitor serialization has timeout too.
> query job itself but it generally not clear how this can break query job).
> On this path job became high level concept which I think it
should be. Also we
> drop boring passing of current async job which we need only for safety check
> deep down below the stack. We drop driver passing for nested jobs which
> I guess we really don't need to pass at all as we need it only to check queue
> size and nested job probably should skip this check.
I did not get what you wanted to say here.
Code becomes more simple and clear.
> The diff stat looks large but most of code is dumb renaming or dropping unused
> parameter in patches 4 and 5. To reduce risk of introducing an error on this
> tedious task I used sed and perl where I can.
>
> I ran only a simple test with this series of quering a domain during its
> migration.
That is a weak test for doing such a complex change. I'd suggest you run
parallel migrations and snapshot to see if it works. You need to run
operations that change the VM configuration.
I don't need event test it as I don't touch code that make them mutually
exclusive.
I don't think we can do this with a complete overhaul of snapshots,
migration and a few others that actually have an expectation of the VM
object not changing while we are in an async job.
Please, reevalute this series, I don't unrerstand how you see this series that
radical. Thing stay the same, async jobs are still mutually exclusive with everything
except queries basically. I don't change this.
Nikolay