Re: [Qemu-devel] [PATCH 0/3] blockjobs: add explicit job culling

[John Snow]

On 10/03/2017 05:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.10.2017 06:15, John Snow wrote:
>> For jobs that complete when a monitor isn't looking, there's no way to
>> tell what the job's final return code was. We need to allow jobs to
>> remain in the list until queried for reliable management.
>>
>> This is an RFC; tests are on the way.
>> (Tested only manually via qmp-shell for now.)
> 
> That's a cool feature!
> What about transactions support?
> 

Didn't test that yet (!) but the intent is that it will be compatible.
The jobs in the transaction, whether using grouped-completion mode or
not, will simply hang around in the list after completion.

For grouped-completion=false:

The jobs will complete individually and then remain in the list.

For grouped-completion=true:

The jobs will remain in their ready-to-commit-or-abort state until all
jobs in the transaction are ready to commit-or-abort, then all jobs will
either commit or abort. After commit-or-abort, all jobs (that were
created with manual-cull=true !) will remain in the query list.

The intended effect here is that this property changes NOTHING except
that the job will remain in the query list until it is dismissed, and
should not change anything about how it behaves during its lifetime.

One downside here is that since we have no universal "job creation
argument list" that I can't add it to all jobs universally. In the case
of transactions, though, I could at least add a property that *forces*
all jobs below to become manual-cull style jobs, and that way you'd only
have to specify it once instead of for each action.

--js

>>
>> John Snow (3):
>>    blockjob: add manual-cull property
>>    qmp: add block-job-cull command
>>    blockjob: expose manual-cull property
>>
>>   block/backup.c               | 20 +++++++++---------
>>   block/commit.c               |  2 +-
>>   block/mirror.c               |  2 +-
>>   block/replication.c          |  5 +++--
>>   block/stream.c               |  2 +-
>>   block/trace-events           |  1 +
>>   blockdev.c                   | 28 +++++++++++++++++++++----
>>   blockjob.c                   | 46
>> +++++++++++++++++++++++++++++++++++++++--
>>   include/block/block_int.h    |  8 +++++---
>>   include/block/blockjob.h     | 21 +++++++++++++++++++
>>   include/block/blockjob_int.h |  2 +-
>>   qapi/block-core.json         | 49
>> ++++++++++++++++++++++++++++++++++++--------
>>   tests/test-blockjob-txn.c    |  2 +-
>>   tests/test-blockjob.c        |  2 +-
>>   14 files changed, 155 insertions(+), 35 deletions(-)
>>
> 
> 

Oh, while I'm here, I should point out that another downside of this
patch is that it doesn't prevent "cancel" from attempting to re-enter
the job. Or rather, I had to patch that out specifically. The job
remains in a list of jobs that some portions of the code consider to be
"active" jobs. (look for any code that checks to see if the job has
started.)

A (perhaps) more provably cleaner approach would be to simply move any
completed job onto a different list upon completion, and patch
query-jobs to query both lists, and allow the cull command to remove any
jobs on that list. A downside of that approach, however, is that without
multiple job support, you may launch a second job that perhaps
overwrites the first job unless you're careful about how you manage that
data.

There are pros and cons to either way, but I'd rather not get in the
business of overhauling the blockjobs API unless it's for universal jobs.

--js