Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/40] Generic background jobs
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180518132114.4070-1-kw...@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/40] Generic background jobs
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
t [tag update]patchew/20180517092513.735-1-kra...@redhat.com ->
patchew/20180517092513.735-1-kra...@redhat.com
* [new tag] patchew/20180518132114.4070-1-kw...@redhat.com ->
patchew/20180518132114.4070-1-kw...@redhat.com
Switched to a new branch 'test'
be01aa198a qemu-iotests: Test job-* with block jobs
62233b758c iotests: Move qmp_to_opts() to VM
3f946bc442 blockjob: Remove BlockJob.driver
cae6373cbd job: Add query-jobs QMP command
9bf0565e0c job: Add lifecycle QMP commands
f7363a69b4 job: Add JOB_STATUS_CHANGE QMP event
5a63efe4e4 job: Introduce qapi/job.json
2a5e7375fa job: Move progress fields to Job
2ae08c1d06 job: Add job_transition_to_ready()
9d76be11bd job: Add job_is_ready()
9b51cc2a7e job: Add job_dismiss()
555a61a69f job: Add job_yield()
798978bcb1 block: Cancel job in bdrv_close_all() callers
4dcb71e4e5 job: Move completion and cancellation to Job
ffad8fad4b job: Move transactions to Job
0edc41ca48 job: Switch transactions to JobTxn
a1073023f2 job: Move job_finish_sync() to Job
6ec521dbdb job: Move .complete callback to Job
9660a731ab job: Add job_drain()
1958d0ac38 job: Convert block_job_cancel_async() to Job
dc1a8d70b8 job: Move single job finalisation to Job
4e7a12008f job: Add job_event_*()
d973eb6546 blockjob: Split block_job_event_pending()
7d447f8ab3 job: Move BlockJobCreateFlags to Job
ffa1c9ad30 job: Replace BlockJob.completed with job_is_completed()
13e426855c job: Move pause/resume functions to Job
0c52ff1b24 job: Add job_sleep_ns()
5ebf3ba66a job: Move coroutine and related code to Job
4e7b469718 job: Move defer_to_main_loop to Job
896fe05128 job: Add Job.aio_context
d73a94fde9 job: Move cancelled to Job
e386ae83f9 job: Add reference counting
3915da0ada job: Move state transitions to Job
e2ea58c7b0 job: Maintain a list of all jobs
717f2e60bf job: Add job_delete()
f31f336bca job: Add JobDriver.job_type
048bb87d0a job: Rename BlockJobType into JobType
b2707abc22 job: Create Job, JobDriver and job_create()
f7c7867229 blockjob: Improve BlockJobInfo.offset/len documentation
7611d0c75f blockjob: Update block-job-pause/resume documentation
=== OUTPUT BEGIN ===
Checking PATCH 1/40: blockjob: Update block-job-pause/resume documentation...
Checking PATCH 2/40: blockjob: Improve BlockJobInfo.offset/len documentation...
Checking PATCH 3/40: job: Create Job, JobDriver and job_create()...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#353:
new file mode 100644
total: 0 errors, 1 warnings, 424 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/40: job: Rename BlockJobType into JobType...
Checking PATCH 5/40: job: Add JobDriver.job_type...
Checking PATCH 6/40: job: Add job_delete()...
Checking PATCH 7/40: job: Maintain a list of all jobs...
Checking PATCH 8/40: job: Move state transitions to Job...
ERROR: space prohibited before open square bracket '['
#342: FILE: job.c:38:
+/* U: */ [JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#343: FILE: job.c:39:
+/* C: */ [JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0, 0, 1, 0, 1},
ERROR: space prohibited before open square bracket '['
#344: FILE: job.c:40:
+/* R: */ [JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 0, 1, 0, 0},
ERROR: space prohibited before open square bracket '['
#345: FILE: job.c:41:
+/* P: */ [JOB_STATUS_PAUSED]= {0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#346: FILE: job.c:42:
+/* Y: */ [JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 0, 1, 0, 0},
ERROR: space prohibited before open square bracket '['
#347: FILE: job.c:43:
+/* S: */ [JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0},
ERROR: space prohibited before open square bracket '['
#348: FILE: job.c:44:
+/* W: */ [JOB_STATUS_WAITING] = {0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0},
ERROR: space prohibited before open square bracket '['
#349: FILE: job.c:45:
+/* D: */ [JOB_STATUS_PENDING] = {
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/40] Generic background jobs
* Kevin Wolf (kw...@redhat.com) wrote: > Before we can make x-blockdev-create a background job, we need to > generalise the job infrastructure so that it can be used without any > associated block node. Is there any relationship between what this does, and what Marc-André's 'monitor: add asynchronous command type' tries to do? (See 20180326150916.9602-1-marcandre.lur...@redhat.com 26th March) Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/40] Generic background jobs
Am 18.05.2018 um 20:41 hat Dr. David Alan Gilbert geschrieben: > * Kevin Wolf (kw...@redhat.com) wrote: > > Before we can make x-blockdev-create a background job, we need to > > generalise the job infrastructure so that it can be used without any > > associated block node. > > Is there any relationship between what this does, and what > Marc-André's 'monitor: add asynchronous command type' tries to do? > (See 20180326150916.9602-1-marcandre.lur...@redhat.com 26th March) Depends on who you ask. :-) In a way, yes. Marc-André's async commands are for long-running operations and jobs are for long-running operations. However, they are for a different kind of "long-running". Basically, Marc-André's work is for commands that are generally quick, but perform some blocking operation. Usually commands that are currently implemented synchronously, but always bothered us because they block the VM for a while. They take maybe a few seconds at most, but you really don't want them to block the guest even for short time. The important part here is you don't want to modify the operations while they're running, they just send their return value when they are done. (In the latest version, Marc-André even made sure not to modify the wire protocol, so IIUC the commands aren't really async any more in the sense that you can have multiple commands running, but they are just non-blocking in the sense that the guest can keep running while they are doing their work.) In comparison, jobs are typically really long running, like several minutes (like copying a whole disk image). Some of them can even run indefinitely (like mirroring) until you explicitly tell them to stop. You want to continue using the monitor while they are running, and to be able to manage them at runtime (pause/resume/set speed/cancel/...). So I think both address different problems. Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/40] Generic background jobs
Hi On Tue, May 22, 2018 at 1:01 PM, Kevin Wolf wrote: > Am 18.05.2018 um 20:41 hat Dr. David Alan Gilbert geschrieben: >> * Kevin Wolf (kw...@redhat.com) wrote: >> > Before we can make x-blockdev-create a background job, we need to >> > generalise the job infrastructure so that it can be used without any >> > associated block node. >> >> Is there any relationship between what this does, and what >> Marc-André's 'monitor: add asynchronous command type' tries to do? >> (See 20180326150916.9602-1-marcandre.lur...@redhat.com 26th March) > > Depends on who you ask. :-) > > In a way, yes. Marc-André's async commands are for long-running > operations and jobs are for long-running operations. However, they are > for a different kind of "long-running". > > Basically, Marc-André's work is for commands that are generally quick, > but perform some blocking operation. Usually commands that are currently > implemented synchronously, but always bothered us because they block the > VM for a while. They take maybe a few seconds at most, but you really > don't want them to block the guest even for short time. > > The important part here is you don't want to modify the operations while > they're running, they just send their return value when they are done. > (In the latest version, Marc-André even made sure not to modify the wire > protocol, so IIUC the commands aren't really async any more in the sense > that you can have multiple commands running, but they are just > non-blocking in the sense that the guest can keep running while they are > doing their work.) > > In comparison, jobs are typically really long running, like several > minutes (like copying a whole disk image). Some of them can even run > indefinitely (like mirroring) until you explicitly tell them to stop. > You want to continue using the monitor while they are running, and to be > able to manage them at runtime (pause/resume/set speed/cancel/...). > > So I think both address different problems. I agree with Kevin that both address different needs and complement each other. Right now, QMP commands are handled synchronous, so they block the main thread (or the "OOB" thread). I needed a simple way to handle them asynchronously, *without* breaking or changing an existing QMP command API. Job, on the other hand, provides a lot of facilities that are unnecessary for most short living commands, while adding complexity, at the protocol level (for instance, by using an extra "job-id" space and not being tied to the qmp session). Depending on the facilities Job provide, and the guarantee that must hold when using it, it may make sense or help to transform an existing command to an asynchronous version, but I doubt many of the existing commands will need it. However, if there is a need to introduce job-like facilities to QMP async (such as running concurrent commands, listing running commands, being able to cancel them, being notified on progression etc), then I think we should be careful to use Job. For now, this is unneeded, as QMP async is internal to qemu, the client can't run anything to manage an ongoing async command, so I think both are improvements and will coexist harmoniously.
Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/40] Generic background jobs
* Marc-André Lureau (marcandre.lur...@redhat.com) wrote: > Hi > > On Tue, May 22, 2018 at 1:01 PM, Kevin Wolf wrote: > > Am 18.05.2018 um 20:41 hat Dr. David Alan Gilbert geschrieben: > >> * Kevin Wolf (kw...@redhat.com) wrote: > >> > Before we can make x-blockdev-create a background job, we need to > >> > generalise the job infrastructure so that it can be used without any > >> > associated block node. > >> > >> Is there any relationship between what this does, and what > >> Marc-André's 'monitor: add asynchronous command type' tries to do? > >> (See 20180326150916.9602-1-marcandre.lur...@redhat.com 26th March) > > > > Depends on who you ask. :-) > > > > In a way, yes. Marc-André's async commands are for long-running > > operations and jobs are for long-running operations. However, they are > > for a different kind of "long-running". > > > > Basically, Marc-André's work is for commands that are generally quick, > > but perform some blocking operation. Usually commands that are currently > > implemented synchronously, but always bothered us because they block the > > VM for a while. They take maybe a few seconds at most, but you really > > don't want them to block the guest even for short time. > > > > The important part here is you don't want to modify the operations while > > they're running, they just send their return value when they are done. > > (In the latest version, Marc-André even made sure not to modify the wire > > protocol, so IIUC the commands aren't really async any more in the sense > > that you can have multiple commands running, but they are just > > non-blocking in the sense that the guest can keep running while they are > > doing their work.) > > > > In comparison, jobs are typically really long running, like several > > minutes (like copying a whole disk image). Some of them can even run > > indefinitely (like mirroring) until you explicitly tell them to stop. > > You want to continue using the monitor while they are running, and to be > > able to manage them at runtime (pause/resume/set speed/cancel/...). > > > > So I think both address different problems. > > I agree with Kevin that both address different needs and complement > each other. Right now, QMP commands are handled synchronous, so they > block the main thread (or the "OOB" thread). I needed a simple way to > handle them asynchronously, *without* breaking or changing an existing > QMP command API. Job, on the other hand, provides a lot of facilities > that are unnecessary for most short living commands, while adding > complexity, at the protocol level (for instance, by using an extra > "job-id" space and not being tied to the qmp session). Depending on > the facilities Job provide, and the guarantee that must hold when > using it, it may make sense or help to transform an existing command > to an asynchronous version, but I doubt many of the existing commands > will need it. However, if there is a need to introduce job-like > facilities to QMP async (such as running concurrent commands, listing > running commands, being able to cancel them, being notified on > progression etc), then I think we should be careful to use Job. For > now, this is unneeded, as QMP async is internal to qemu, the client > can't run anything to manage an ongoing async command, so I think > both are improvements and will coexist harmoniously. OK; it seems we are carving stuff up into a whole bunch of different categories based on their longevity and blockableness. (Normal, OOB, Async, Job) I do worry a bit that pretty much anything that's not 'instant' means it's waiting for something somewhere, and because you're waiting for something that means you've got to be able to fail or cancel it in case that thing never happens. Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
