[Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management

John Snow posted 14 patches 6 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180127020515.27137-1-jsnow@redhat.com
Test checkpatch failed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test ppc passed
Test s390x passed
There is a newer version of this series
block/backup.c               |  22 ++--
block/commit.c               |   2 +-
block/mirror.c               |   2 +-
block/replication.c          |   5 +-
block/stream.c               |   2 +-
block/trace-events           |   2 +
blockdev.c                   |  42 ++++++-
blockjob.c                   | 279 ++++++++++++++++++++++++++++++++++++++++---
include/block/block_int.h    |   9 +-
include/block/blockjob.h     |  38 ++++++
include/block/blockjob_int.h |  12 +-
qapi/block-core.json         | 135 +++++++++++++++++++--
tests/qemu-iotests/056       | 241 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/056.out   |   4 +-
tests/test-bdrv-drain.c      |   4 +-
tests/test-blockjob-txn.c    |   2 +-
tests/test-blockjob.c        |   4 +-
17 files changed, 750 insertions(+), 55 deletions(-)
[Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management
Posted by John Snow 6 years, 2 months ago
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.

Furthermore, it's not viable to have graph changes when the monitor
isn't looking either. We need at least another event for that.

This series is a rough sketch for the WAITING, PENDING and CONCLUDED
events that accompany an expanded job management scheme that a management
client can opt-in to.

V3:
 - Added WAITING and PENDING events
 - Added block_job_finalize verb
 - Added .pending() callback for jobs
 - Tweaked how .commit/.abort work

V2:
 - Added tests!
 - Changed property name (Jeff, Paolo)

RFC / Known problems:
- I need a lot more tests.
- Jobs need to actually implement their .pending callback for this to be of any
  actual use.
- Mirror needs to be refactored to use the commit/abort/pending/clean callbacks
  to fulfill the promise made by "no graph changes without user authorization"
  that PENDING is supposed to offer
- Jobs beyond backup will be able to opt-in to the new management scheme in the
  next version.
- V4 will include a forced synchronicity for jobs in a transaction; i.e. all
  jobs will be forced to use either the old or new styles, but not a mix.

Please take a look and shout loudly if I'm wandering in the wrong direction.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch block-job-reap
https://github.com/jnsnow/qemu/tree/block-job-reap

This version is tagged block-job-reap-v3:
https://github.com/jnsnow/qemu/releases/tag/block-job-reap-v3

John Snow (14):
  blockjobs: add manual property
  blockjobs: Add status enum
  blockjobs: add state transition table
  blockjobs: RFC add block_job_verb permission table
  blockjobs: add block_job_dismiss
  blockjobs: add CONCLUDED state
  blockjobs: ensure abort is called for cancelled jobs
  blockjobs: add commit, abort, clean helpers
  blockjobs: add prepare callback
  blockjobs: Add waiting event
  blockjobs: add PENDING status and event
  blockjobs: add block-job-finalize
  blockjobs: Expose manual property
  iotests: test manual job dismissal

 block/backup.c               |  22 ++--
 block/commit.c               |   2 +-
 block/mirror.c               |   2 +-
 block/replication.c          |   5 +-
 block/stream.c               |   2 +-
 block/trace-events           |   2 +
 blockdev.c                   |  42 ++++++-
 blockjob.c                   | 279 ++++++++++++++++++++++++++++++++++++++++---
 include/block/block_int.h    |   9 +-
 include/block/blockjob.h     |  38 ++++++
 include/block/blockjob_int.h |  12 +-
 qapi/block-core.json         | 135 +++++++++++++++++++--
 tests/qemu-iotests/056       | 241 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/056.out   |   4 +-
 tests/test-bdrv-drain.c      |   4 +-
 tests/test-blockjob-txn.c    |   2 +-
 tests/test-blockjob.c        |   4 +-
 17 files changed, 750 insertions(+), 55 deletions(-)

-- 
2.14.3


Re: [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management
Posted by no-reply@patchew.org 6 years, 2 months ago
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180127020515.27137-1-jsnow@redhat.com
Subject: [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management

=== 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

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/151670719721.7533.6287116389556641300.stgit@bahia -> patchew/151670719721.7533.6287116389556641300.stgit@bahia
 * [new tag]               patchew/20180127020515.27137-1-jsnow@redhat.com -> patchew/20180127020515.27137-1-jsnow@redhat.com
Switched to a new branch 'test'
10959f8618 iotests: test manual job dismissal
fa3b3419d9 blockjobs: Expose manual property
acf29467dd blockjobs: add block-job-finalize
b8250669f5 blockjobs: add PENDING status and event
67b2f8d3ed blockjobs: Add waiting event
22b6b95b6e blockjobs: add prepare callback
51530e8516 blockjobs: add commit, abort, clean helpers
45761a2219 blockjobs: ensure abort is called for cancelled jobs
d7317d4525 blockjobs: add CONCLUDED state
a4939037ed blockjobs: add block_job_dismiss
83b14366be blockjobs: RFC add block_job_verb permission table
446a6e487b blockjobs: add state transition table
bf5fb419c1 blockjobs: Add status enum
85487f375a blockjobs: add manual property

=== OUTPUT BEGIN ===
Checking PATCH 1/14: blockjobs: add manual property...
Checking PATCH 2/14: blockjobs: Add status enum...
Checking PATCH 3/14: blockjobs: add state transition table...
ERROR: space prohibited before open square bracket '['
#37: FILE: blockjob.c:48:
+    /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0},

ERROR: space prohibited before open square bracket '['
#38: FILE: blockjob.c:49:
+    /* C: */ [BLOCK_JOB_STATUS_CREATED]   = {0, 0, 1, 0, 0},

ERROR: space prohibited before open square bracket '['
#39: FILE: blockjob.c:50:
+    /* R: */ [BLOCK_JOB_STATUS_RUNNING]   = {0, 0, 0, 1, 1},

ERROR: space prohibited before open square bracket '['
#40: FILE: blockjob.c:51:
+    /* P: */ [BLOCK_JOB_STATUS_PAUSED]    = {0, 0, 1, 0, 1},

ERROR: space prohibited before open square bracket '['
#41: FILE: blockjob.c:52:
+    /* Y: */ [BLOCK_JOB_STATUS_READY]     = {0, 0, 0, 1, 0},

total: 5 errors, 0 warnings, 72 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/14: blockjobs: RFC add block_job_verb permission table...
Checking PATCH 5/14: blockjobs: add block_job_dismiss...
Checking PATCH 6/14: blockjobs: add CONCLUDED state...
ERROR: space prohibited before open square bracket '['
#40: FILE: blockjob.c:48:
+    /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0},

ERROR: space prohibited before open square bracket '['
#41: FILE: blockjob.c:49:
+    /* C: */ [BLOCK_JOB_STATUS_CREATED]   = {0, 0, 1, 0, 0, 0},

ERROR: space prohibited before open square bracket '['
#42: FILE: blockjob.c:50:
+    /* R: */ [BLOCK_JOB_STATUS_RUNNING]   = {0, 0, 0, 1, 1, 1},

ERROR: space prohibited before open square bracket '['
#43: FILE: blockjob.c:51:
+    /* P: */ [BLOCK_JOB_STATUS_PAUSED]    = {0, 0, 1, 0, 1, 1},

ERROR: space prohibited before open square bracket '['
#44: FILE: blockjob.c:52:
+    /* Y: */ [BLOCK_JOB_STATUS_READY]     = {0, 0, 0, 1, 0, 1},

ERROR: space prohibited before open square bracket '['
#45: FILE: blockjob.c:53:
+    /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0},

total: 6 errors, 0 warnings, 126 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 7/14: blockjobs: ensure abort is called for cancelled jobs...
Checking PATCH 8/14: blockjobs: add commit, abort, clean helpers...
Checking PATCH 9/14: blockjobs: add prepare callback...
Checking PATCH 10/14: blockjobs: Add waiting event...
ERROR: space prohibited before open square bracket '['
#43: FILE: blockjob.c:48:
+    /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0},

ERROR: space prohibited before open square bracket '['
#44: FILE: blockjob.c:49:
+    /* C: */ [BLOCK_JOB_STATUS_CREATED]   = {0, 0, 1, 0, 0, 0, 0},

ERROR: space prohibited before open square bracket '['
#45: FILE: blockjob.c:50:
+    /* R: */ [BLOCK_JOB_STATUS_RUNNING]   = {0, 0, 0, 1, 1, 1, 1},

ERROR: space prohibited before open square bracket '['
#46: FILE: blockjob.c:51:
+    /* P: */ [BLOCK_JOB_STATUS_PAUSED]    = {0, 0, 1, 0, 1, 0, 1},

ERROR: space prohibited before open square bracket '['
#47: FILE: blockjob.c:52:
+    /* Y: */ [BLOCK_JOB_STATUS_READY]     = {0, 0, 0, 1, 0, 1, 1},

ERROR: space prohibited before open square bracket '['
#48: FILE: blockjob.c:53:
+    /* W: */ [BLOCK_JOB_STATUS_WAITING]   = {0, 0, 0, 0, 0, 0, 1},

ERROR: space prohibited before open square bracket '['
#49: FILE: blockjob.c:54:
+    /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0, 0},

total: 7 errors, 0 warnings, 106 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 11/14: blockjobs: add PENDING status and event...
ERROR: space prohibited before open square bracket '['
#44: FILE: blockjob.c:48:
+    /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0},

ERROR: space prohibited before open square bracket '['
#45: FILE: blockjob.c:49:
+    /* C: */ [BLOCK_JOB_STATUS_CREATED]   = {0, 0, 1, 0, 0, 0, 0, 0},

ERROR: space prohibited before open square bracket '['
#46: FILE: blockjob.c:50:
+    /* R: */ [BLOCK_JOB_STATUS_RUNNING]   = {0, 0, 0, 1, 1, 1, 1, 1},

ERROR: space prohibited before open square bracket '['
#47: FILE: blockjob.c:51:
+    /* P: */ [BLOCK_JOB_STATUS_PAUSED]    = {0, 0, 1, 0, 1, 0, 0, 1},

ERROR: space prohibited before open square bracket '['
#48: FILE: blockjob.c:52:
+    /* Y: */ [BLOCK_JOB_STATUS_READY]     = {0, 0, 0, 1, 0, 1, 1, 1},

ERROR: space prohibited before open square bracket '['
#49: FILE: blockjob.c:53:
+    /* W: */ [BLOCK_JOB_STATUS_WAITING]   = {0, 0, 0, 0, 0, 0, 1, 1},

ERROR: space prohibited before open square bracket '['
#50: FILE: blockjob.c:54:
+    /* D: */ [BLOCK_JOB_STATUS_PENDING]   = {0, 0, 0, 0, 0, 0, 0, 1},

ERROR: space prohibited before open square bracket '['
#51: FILE: blockjob.c:55:
+    /* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {1, 0, 0, 0, 0, 0, 0, 0},

total: 8 errors, 0 warnings, 107 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 12/14: blockjobs: add block-job-finalize...
Checking PATCH 13/14: blockjobs: Expose manual property...
Checking PATCH 14/14: iotests: test manual job dismissal...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Re: [Qemu-devel] [RFC v3 00/14] blockjobs: add explicit job management
Posted by John Snow 6 years, 2 months ago

On 01/26/2018 09:05 PM, 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.
> 
> Furthermore, it's not viable to have graph changes when the monitor
> isn't looking either. We need at least another event for that.
> 
> This series is a rough sketch for the WAITING, PENDING and CONCLUDED
> events that accompany an expanded job management scheme that a management
> client can opt-in to.
> 
> V3:
>  - Added WAITING and PENDING events
>  - Added block_job_finalize verb
>  - Added .pending() callback for jobs
>  - Tweaked how .commit/.abort work
> 
> V2:
>  - Added tests!
>  - Changed property name (Jeff, Paolo)
> 
> RFC / Known problems:
> - I need a lot more tests.
> - Jobs need to actually implement their .pending callback for this to be of any
>   actual use.
> - Mirror needs to be refactored to use the commit/abort/pending/clean callbacks
>   to fulfill the promise made by "no graph changes without user authorization"
>   that PENDING is supposed to offer
> - Jobs beyond backup will be able to opt-in to the new management scheme in the
>   next version.
> - V4 will include a forced synchronicity for jobs in a transaction; i.e. all
>   jobs will be forced to use either the old or new styles, but not a mix.
> 
> Please take a look and shout loudly if I'm wandering in the wrong direction.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch block-job-reap
> https://github.com/jnsnow/qemu/tree/block-job-reap
> 
> This version is tagged block-job-reap-v3:
> https://github.com/jnsnow/qemu/releases/tag/block-job-reap-v3
> 
> John Snow (14):
>   blockjobs: add manual property
>   blockjobs: Add status enum
>   blockjobs: add state transition table
>   blockjobs: RFC add block_job_verb permission table
>   blockjobs: add block_job_dismiss
>   blockjobs: add CONCLUDED state
>   blockjobs: ensure abort is called for cancelled jobs
>   blockjobs: add commit, abort, clean helpers
>   blockjobs: add prepare callback
>   blockjobs: Add waiting event
>   blockjobs: add PENDING status and event
>   blockjobs: add block-job-finalize
>   blockjobs: Expose manual property
>   iotests: test manual job dismissal
> 
>  block/backup.c               |  22 ++--
>  block/commit.c               |   2 +-
>  block/mirror.c               |   2 +-
>  block/replication.c          |   5 +-
>  block/stream.c               |   2 +-
>  block/trace-events           |   2 +
>  blockdev.c                   |  42 ++++++-
>  blockjob.c                   | 279 ++++++++++++++++++++++++++++++++++++++++---
>  include/block/block_int.h    |   9 +-
>  include/block/blockjob.h     |  38 ++++++
>  include/block/blockjob_int.h |  12 +-
>  qapi/block-core.json         | 135 +++++++++++++++++++--
>  tests/qemu-iotests/056       | 241 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/056.out   |   4 +-
>  tests/test-bdrv-drain.c      |   4 +-
>  tests/test-blockjob-txn.c    |   2 +-
>  tests/test-blockjob.c        |   4 +-
>  17 files changed, 750 insertions(+), 55 deletions(-)
> 

NACK

There are a lot of changes I've already made to this series based on
Kevin's recommendations; please wait for the next revision.

And a lot of bugs in this series I've already found.

--js