Patchwork [PULL,for,Kevin,00/19] Block job improvements part 1

login
register
mail settings
Submitter Paolo Bonzini
Date Sept. 28, 2012, 3:22 p.m.
Message ID <1348845782-15073-1-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/187801/
State New
Headers show

Comments

Paolo Bonzini - Sept. 28, 2012, 3:22 p.m.
Kevin,

the following changes since commit ac05f3492421caeb05809ffa02c6198ede179e43:

  add a boot parameter to set reboot timeout (2012-09-25 20:05:04 -0500)

are available in the git repository at:

  git://github.com/bonzini/qemu.git blkmirror-job-1.3-part1

for you to fetch changes up to ed306a929f16fda8a85561430b1ac370bde65097:

  qemu-iotests: add tests for streaming error handling (2012-09-27 15:11:22 +0200)

This message has the diff from my post of Sep 25th.  You reviewed 18 patches,
these are 19 because of the new "qmp: add 'busy' member to BlockJobInfo" patch.

----------------------------------------------------------------
Jeff Cody (1):
      blockdev: rename block_stream_cb to a generic block_job_cb

Paolo Bonzini (18):
      qerror/block: introduce QERR_BLOCK_JOB_NOT_ACTIVE
      block: fix documentation of block_job_cancel_sync
      block: move job APIs to separate files
      block: add block_job_query
      qmp: add 'busy' member to BlockJobInfo
      block: add support for job pause/resume
      qmp: add block-job-pause and block-job-resume
      qemu-iotests: add test for pausing a streaming operation
      block: rename block_job_complete to block_job_completed
      iostatus: rename BlockErrorAction, BlockQMPEventAction
      iostatus: move BlockdevOnError declaration to QAPI
      iostatus: change is_read to a bool
      iostatus: reorganize io error code
      block: introduce block job error
      stream: add on-error argument
      blkdebug: process all set_state rules in the old state
      qemu-iotests: map underscore to dash in QMP argument names
      qemu-iotests: add tests for streaming error handling

 Makefile.objs                 |   5 +-
 QMP/qmp-events.txt            |  22 ++++
 block.c                       | 187 ++++++++----------------------
 block.h                       |  20 ++--
 block/Makefile.objs           |   3 +-
 block/blkdebug.c              |  12 +-
 block/stream.c                |  33 +++++-
 block_int.h                   | 162 ++------------------------
 blockdev.c                    |  86 +++++++++-----
 blockjob.c                    | 249 ++++++++++++++++++++++++++++++++++++++++
 blockjob.h                    | 243 +++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx               |  35 +++++-
 hmp.c                         |  26 ++++-
 hmp.h                         |   2 +
 hw/fdc.c                      |   4 +-
 hw/ide/core.c                 |  22 +---
 hw/ide/pci.c                  |   4 +-
 hw/scsi-disk.c                |  25 ++--
 hw/scsi-generic.c             |   4 +-
 hw/virtio-blk.c               |  23 ++--
 monitor.c                     |   1 +
 monitor.h                     |   1 +
 qapi-schema.json              |  91 ++++++++++++++-
 qemu-tool.c                   |   6 +
 qerror.h                      |   6 +
 qmp-commands.hx               |  14 ++-
 tests/qemu-iotests/030        | 260 +++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out    |   4 +-
 tests/qemu-iotests/group      |   2 +-
 tests/qemu-iotests/iotests.py |  15 ++-
 trace-events                  |   4 +-
 31 file modificati, 1152 inserzioni(+), 419 rimozioni(-)
 create mode 100644 blockjob.c
 create mode 100644 blockjob.h
Eric Blake - Sept. 28, 2012, 3:57 p.m.
On 09/28/2012 09:22 AM, Paolo Bonzini wrote:
> Kevin,
> 
> the following changes since commit ac05f3492421caeb05809ffa02c6198ede179e43:
> 
>   add a boot parameter to set reboot timeout (2012-09-25 20:05:04 -0500)
> 
> are available in the git repository at:
> 
>   git://github.com/bonzini/qemu.git blkmirror-job-1.3-part1
> 
> for you to fetch changes up to ed306a929f16fda8a85561430b1ac370bde65097:
> 
>   qemu-iotests: add tests for streaming error handling (2012-09-27 15:11:22 +0200)
> 
> This message has the diff from my post of Sep 25th.  You reviewed 18 patches,
> these are 19 because of the new "qmp: add 'busy' member to BlockJobInfo" patch.

I reviewed the interdiff, and it appears to address all the concerns
from v3 for these patches, other than my request to include 'type' as
part of the new event, possibly as an enum rather than freeform string.
 But since we agreed to defer that to a followup patch, it does not hold
up this series.  I also did one last sanity read through the patches.

Series:
Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf - Sept. 28, 2012, 6:23 p.m.
Am 28.09.2012 17:22, schrieb Paolo Bonzini:
> Kevin,
> 
> the following changes since commit ac05f3492421caeb05809ffa02c6198ede179e43:
> 
>   add a boot parameter to set reboot timeout (2012-09-25 20:05:04 -0500)
> 
> are available in the git repository at:
> 
>   git://github.com/bonzini/qemu.git blkmirror-job-1.3-part1
> 
> for you to fetch changes up to ed306a929f16fda8a85561430b1ac370bde65097:
> 
>   qemu-iotests: add tests for streaming error handling (2012-09-27 15:11:22 +0200)
> 
> This message has the diff from my post of Sep 25th.  You reviewed 18 patches,
> these are 19 because of the new "qmp: add 'busy' member to BlockJobInfo" patch.
> 
> ----------------------------------------------------------------
> Jeff Cody (1):
>       blockdev: rename block_stream_cb to a generic block_job_cb
> 
> Paolo Bonzini (18):
>       qerror/block: introduce QERR_BLOCK_JOB_NOT_ACTIVE
>       block: fix documentation of block_job_cancel_sync
>       block: move job APIs to separate files
>       block: add block_job_query
>       qmp: add 'busy' member to BlockJobInfo
>       block: add support for job pause/resume
>       qmp: add block-job-pause and block-job-resume
>       qemu-iotests: add test for pausing a streaming operation
>       block: rename block_job_complete to block_job_completed
>       iostatus: rename BlockErrorAction, BlockQMPEventAction
>       iostatus: move BlockdevOnError declaration to QAPI
>       iostatus: change is_read to a bool
>       iostatus: reorganize io error code
>       block: introduce block job error
>       stream: add on-error argument
>       blkdebug: process all set_state rules in the old state
>       qemu-iotests: map underscore to dash in QMP argument names
>       qemu-iotests: add tests for streaming error handling
> 
>  Makefile.objs                 |   5 +-
>  QMP/qmp-events.txt            |  22 ++++
>  block.c                       | 187 ++++++++----------------------
>  block.h                       |  20 ++--
>  block/Makefile.objs           |   3 +-
>  block/blkdebug.c              |  12 +-
>  block/stream.c                |  33 +++++-
>  block_int.h                   | 162 ++------------------------
>  blockdev.c                    |  86 +++++++++-----
>  blockjob.c                    | 249 ++++++++++++++++++++++++++++++++++++++++
>  blockjob.h                    | 243 +++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx               |  35 +++++-
>  hmp.c                         |  26 ++++-
>  hmp.h                         |   2 +
>  hw/fdc.c                      |   4 +-
>  hw/ide/core.c                 |  22 +---
>  hw/ide/pci.c                  |   4 +-
>  hw/scsi-disk.c                |  25 ++--
>  hw/scsi-generic.c             |   4 +-
>  hw/virtio-blk.c               |  23 ++--
>  monitor.c                     |   1 +
>  monitor.h                     |   1 +
>  qapi-schema.json              |  91 ++++++++++++++-
>  qemu-tool.c                   |   6 +
>  qerror.h                      |   6 +
>  qmp-commands.hx               |  14 ++-
>  tests/qemu-iotests/030        | 260 +++++++++++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/030.out    |   4 +-
>  tests/qemu-iotests/group      |   2 +-
>  tests/qemu-iotests/iotests.py |  15 ++-
>  trace-events                  |   4 +-
>  31 file modificati, 1152 inserzioni(+), 419 rimozioni(-)
>  create mode 100644 blockjob.c
>  create mode 100644 blockjob.h

Thanks, applied all to the block branch.

Kevin
Paolo Bonzini - Oct. 1, 2012, 1:37 p.m.
Il 28/09/2012 20:23, Kevin Wolf ha scritto:
> Am 28.09.2012 17:22, schrieb Paolo Bonzini:
>> Kevin,
>>
>> the following changes since commit ac05f3492421caeb05809ffa02c6198ede179e43:
>>
>>   add a boot parameter to set reboot timeout (2012-09-25 20:05:04 -0500)
>>
>> are available in the git repository at:
>>
>>   git://github.com/bonzini/qemu.git blkmirror-job-1.3-part1
>>
>> for you to fetch changes up to ed306a929f16fda8a85561430b1ac370bde65097:
>>
>>   qemu-iotests: add tests for streaming error handling (2012-09-27 15:11:22 +0200)
>>
>> This message has the diff from my post of Sep 25th.  You reviewed 18 patches,
>> these are 19 because of the new "qmp: add 'busy' member to BlockJobInfo" patch.
>>
>> ----------------------------------------------------------------
>> Jeff Cody (1):
>>       blockdev: rename block_stream_cb to a generic block_job_cb
>>
>> Paolo Bonzini (18):
>>       qerror/block: introduce QERR_BLOCK_JOB_NOT_ACTIVE
>>       block: fix documentation of block_job_cancel_sync
>>       block: move job APIs to separate files
>>       block: add block_job_query
>>       qmp: add 'busy' member to BlockJobInfo
>>       block: add support for job pause/resume
>>       qmp: add block-job-pause and block-job-resume
>>       qemu-iotests: add test for pausing a streaming operation
>>       block: rename block_job_complete to block_job_completed
>>       iostatus: rename BlockErrorAction, BlockQMPEventAction
>>       iostatus: move BlockdevOnError declaration to QAPI
>>       iostatus: change is_read to a bool
>>       iostatus: reorganize io error code
>>       block: introduce block job error
>>       stream: add on-error argument
>>       blkdebug: process all set_state rules in the old state
>>       qemu-iotests: map underscore to dash in QMP argument names
>>       qemu-iotests: add tests for streaming error handling
>>
>>  Makefile.objs                 |   5 +-
>>  QMP/qmp-events.txt            |  22 ++++
>>  block.c                       | 187 ++++++++----------------------
>>  block.h                       |  20 ++--
>>  block/Makefile.objs           |   3 +-
>>  block/blkdebug.c              |  12 +-
>>  block/stream.c                |  33 +++++-
>>  block_int.h                   | 162 ++------------------------
>>  blockdev.c                    |  86 +++++++++-----
>>  blockjob.c                    | 249 ++++++++++++++++++++++++++++++++++++++++
>>  blockjob.h                    | 243 +++++++++++++++++++++++++++++++++++++++
>>  hmp-commands.hx               |  35 +++++-
>>  hmp.c                         |  26 ++++-
>>  hmp.h                         |   2 +
>>  hw/fdc.c                      |   4 +-
>>  hw/ide/core.c                 |  22 +---
>>  hw/ide/pci.c                  |   4 +-
>>  hw/scsi-disk.c                |  25 ++--
>>  hw/scsi-generic.c             |   4 +-
>>  hw/virtio-blk.c               |  23 ++--
>>  monitor.c                     |   1 +
>>  monitor.h                     |   1 +
>>  qapi-schema.json              |  91 ++++++++++++++-
>>  qemu-tool.c                   |   6 +
>>  qerror.h                      |   6 +
>>  qmp-commands.hx               |  14 ++-
>>  tests/qemu-iotests/030        | 260 +++++++++++++++++++++++++++++++++++++++++-
>>  tests/qemu-iotests/030.out    |   4 +-
>>  tests/qemu-iotests/group      |   2 +-
>>  tests/qemu-iotests/iotests.py |  15 ++-
>>  trace-events                  |   4 +-
>>  31 file modificati, 1152 inserzioni(+), 419 rimozioni(-)
>>  create mode 100644 blockjob.c
>>  create mode 100644 blockjob.h
> 
> Thanks, applied all to the block branch.

Great, I rebased and only got mostly trivial conflicts.  I pushed the
outcome to the branch blkmirror-job-20120925-rebase of my github
repository.  Do I need to repost, or are you going to continue your
review on the previous patchset?  The next obvious points to step are:

- part 2: "qemu-iotests: add mirroring test case" or "qemu-iotests: add
testcases for mirroring on-source-error/on-target-error"

- part 3: "mirror: perform COW if the cluster size is bigger than the
granularity" or "mirror: allow customizing the granularity"

- part 4: "mirror: support arbitrarily-sized iterations"

Paolo

Patch

diff --git a/blockjob.c b/blockjob.c
index 5dd9c1e..b5c16f3 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -198,6 +198,7 @@  BlockJobInfo *block_job_query(BlockJob *job)
     info->type      = g_strdup(job->job_type->job_type);
     info->device    = g_strdup(bdrv_get_device_name(job->bs));
     info->len       = job->len;
+    info->busy      = job->busy;
     info->paused    = job->paused;
     info->offset    = job->offset;
     info->speed     = job->speed;
diff --git a/blockjob.h b/blockjob.h
index 45cc03d..c2261a9 100644
--- a/blockjob.h
+++ b/blockjob.h
@@ -199,7 +199,8 @@  void block_job_resume(BlockJob *job);
  * block_job_is_paused:
  * @job: The job being queried.
  *
- * Returns whether the job is currently paused.
+ * Returns whether the job is currently paused, or will pause
+ * as soon as it reaches a sleeping point.
  */
 bool block_job_is_paused(BlockJob *job);
 
@@ -207,12 +208,10 @@  bool block_job_is_paused(BlockJob *job);
  * block_job_cancel_sync:
  * @job: The job to be canceled.
  *
- * Synchronously cancel the job and wait for it to reach a quiescent
- * state.  Note that the completion callback will still be called
- * asynchronously, hence it is *not* valid to call #bdrv_delete
- * immediately after #block_job_cancel_sync.  Users of block jobs
- * will usually protect the BlockDriverState objects with a reference
- * count, should this be a concern.
+ * Synchronously cancel the job.  The completion callback is called
+ * before the function returns.  The job may actually complete
+ * instead of canceling itself; the circumstances under which this
+ * happens depend on the kind of job that is active.
  *
  * Returns the return value from the job if the job actually completed
  * during the call, or -ECANCELED if it was canceled.
diff --git a/qapi-schema.json b/qapi-schema.json
index 8719a9d..d31ff0e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1100,11 +1100,11 @@ 
 # @ignore: ignore the error, only report a QMP event (BLOCK_IO_ERROR
 #          or BLOCK_JOB_ERROR)
 #
+# @enospc: same as @stop on ENOSPC, same as @report otherwise.
+#
 # @stop: for guest operations, stop the virtual machine;
 #        for jobs, pause the job
 #
-# @enospc: same as @stop on ENOSPC, same as @report otherwise.
-#
 # Since: 1.3
 ##
 { 'enum': 'BlockdevOnError',
@@ -1121,19 +1121,23 @@ 
 #
 # @len: the maximum progress value
 #
-# @paused: whether the job is paused (since 1.2)
+# @busy: false if the job is known to be in a quiescent state, with
+#        no pending I/O.  Since 1.3.
+#
+# @paused: whether the job is paused or, if @busy is true, will
+#          pause itself as soon as possible.  Since 1.3.
 #
 # @offset: the current progress value
 #
 # @speed: the rate limit, bytes per second
 #
-# @io-status: the status of the job (since 1.2)
+# @io-status: the status of the job (since 1.3)
 #
 # Since: 1.1
 ##
 { 'type': 'BlockJobInfo',
   'data': {'type': 'str', 'device': 'str', 'len': 'int',
-           'offset': 'int', 'paused': 'bool', 'speed': 'int',
+           'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int',
            'io-status': 'BlockDeviceIoStatus'} }
 
 ##
@@ -1833,7 +1837,7 @@ 
 #
 # @on-error: #optional the action to take on an error (default report).
 #            'stop' and 'enospc' can only be used if the block device
-#            supports io-status (see BlockInfo).  Since 1.2.
+#            supports io-status (see BlockInfo).  Since 1.3.
 #
 # Returns: Nothing on success
 #          If @device does not exist, DeviceNotFound
@@ -1886,7 +1890,8 @@ 
 #
 # @device: the device name
 #
-# @force: #optional whether to allow cancellation of a paused job (default false)
+# @force: #optional whether to allow cancellation of a paused job (default
+#         false).  Since 1.3.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive