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