Patchwork [14/47] stream: add on-error argument

login
register
mail settings
Submitter Paolo Bonzini
Date July 24, 2012, 11:03 a.m.
Message ID <1343127865-16608-15-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/172863/
State New
Headers show

Comments

Paolo Bonzini - July 24, 2012, 11:03 a.m.
This patch adds support for error management to streaming.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/stream.c   |   28 +++++++++++++++++++++++++++-
 block_int.h      |    3 ++-
 blockdev.c       |   11 ++++++++---
 hmp.c            |    3 ++-
 qapi-schema.json |   10 ++++++++--
 qmp-commands.hx  |    2 +-
 6 files changed, 48 insertions(+), 9 deletions(-)
Eric Blake - July 31, 2012, 6:40 p.m.
On 07/24/2012 05:03 AM, Paolo Bonzini wrote:
> This patch adds support for error management to streaming.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -1656,17 +1656,23 @@
>  #
>  # @speed:  #optional the maximum speed, in bytes per second
>  #
> +# @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.

>  #          If @speed is invalid, InvalidParameter
> +#          If @on_error is not supported, InvalidParameter

s/on_error/on-error/ to match the rest of the docs
Kevin Wolf - Aug. 1, 2012, 10:29 a.m.
Am 24.07.2012 13:03, schrieb Paolo Bonzini:
> This patch adds support for error management to streaming.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/stream.c   |   28 +++++++++++++++++++++++++++-
>  block_int.h      |    3 ++-
>  blockdev.c       |   11 ++++++++---
>  hmp.c            |    3 ++-
>  qapi-schema.json |   10 ++++++++--
>  qmp-commands.hx  |    2 +-
>  6 files changed, 48 insertions(+), 9 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index b3ede44..03cae14 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -31,6 +31,7 @@ typedef struct StreamBlockJob {
>      BlockJob common;
>      RateLimit limit;
>      BlockDriverState *base;
> +    BlockdevOnError on_error;
>      char backing_file_id[1024];
>  } StreamBlockJob;
>  
> @@ -78,6 +79,7 @@ static void coroutine_fn stream_run(void *opaque)
>      BlockDriverState *bs = s->common.bs;
>      BlockDriverState *base = s->base;
>      int64_t sector_num, end;
> +    int error = 0;
>      int ret = 0;
>      int n = 0;
>      void *buf;
> @@ -136,7 +138,19 @@ wait:
>              ret = stream_populate(bs, sector_num, n, buf);
>          }
>          if (ret < 0) {
> -            break;
> +            BlockErrorAction action =
> +                block_job_error_action(&s->common, s->common.bs, s->on_error,
> +                                       true, -ret);
> +            if (action == BDRV_ACTION_STOP) {
> +                n = 0;
> +                continue;
> +            }
> +            if (error == 0) {
> +                error = ret;
> +            }

I'm not sure we should return an error at the end of the job for
BDRV_ACTION_IGNORE. If it's used for guest block devices, there's no
sign of an error either (except the guest reading garbage, of course).
But it's hard to discuss a feature for which there is no clear use case
anyway... Maybe it's something you could use to rescue what's still
accessible on a corrupted image or so.

In any case we should document it somewhere. Your commit message in
patch 13 probably belongs somewhere in the docs.

>  void stream_start(BlockDriverState *bs, BlockDriverState *base,
>                    const char *base_id, int64_t speed,
> +                  BlockdevOnError on_error,
>                    BlockDriverCompletionFunc *cb,
>                    void *opaque, Error **errp)
>  {
>      StreamBlockJob *s;
>  
> +    if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
> +         on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
> +        !bdrv_iostatus_is_enabled(bs)) {
> +        error_set(errp, QERR_INVALID_PARAMETER, "on-error");
> +        return;
> +    }

Hm, this is an interesting one. bdrv_iostatus_is_enabled() returns true
for a block device that is (or was once) attached to virtio-blk, IDE or
scsi-disk. Which made sense so far because only these devices would
actually set the status.

Now with block jobs, we have other places that can set the status. And
we have images that don't belong to any device, but can still get errors
(mirror target). Maybe it would make sense to just enable the iostatus
here instead of failing?

Kevin
Paolo Bonzini - Aug. 1, 2012, 11:11 a.m.
Il 01/08/2012 12:29, Kevin Wolf ha scritto:
>> > +    if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
>> > +         on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
>> > +        !bdrv_iostatus_is_enabled(bs)) {
>> > +        error_set(errp, QERR_INVALID_PARAMETER, "on-error");
>> > +        return;
>> > +    }
> Hm, this is an interesting one. bdrv_iostatus_is_enabled() returns true
> for a block device that is (or was once) attached to virtio-blk, IDE or
> scsi-disk. Which made sense so far because only these devices would
> actually set the status.
> 
> Now with block jobs, we have other places that can set the status. And
> we have images that don't belong to any device, but can still get errors
> (mirror target). Maybe it would make sense to just enable the iostatus
> here instead of failing?

I'm not sure what would happen, so I preferred to be safe.

The right solution would be "support iostatus in sd and friends, and
drop bdrv_iostatus_is_enabled altogether", of course...

Paolo
Kevin Wolf - Aug. 1, 2012, 11:45 a.m.
Am 01.08.2012 13:11, schrieb Paolo Bonzini:
> Il 01/08/2012 12:29, Kevin Wolf ha scritto:
>>>> +    if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
>>>> +         on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
>>>> +        !bdrv_iostatus_is_enabled(bs)) {
>>>> +        error_set(errp, QERR_INVALID_PARAMETER, "on-error");
>>>> +        return;
>>>> +    }
>> Hm, this is an interesting one. bdrv_iostatus_is_enabled() returns true
>> for a block device that is (or was once) attached to virtio-blk, IDE or
>> scsi-disk. Which made sense so far because only these devices would
>> actually set the status.
>>
>> Now with block jobs, we have other places that can set the status. And
>> we have images that don't belong to any device, but can still get errors
>> (mirror target). Maybe it would make sense to just enable the iostatus
>> here instead of failing?
> 
> I'm not sure what would happen, so I preferred to be safe.
> 
> The right solution would be "support iostatus in sd and friends, and
> drop bdrv_iostatus_is_enabled altogether", of course...

Support it _for_ sd and friends, but support it _in_ the block layer.
What's missing before this can happen is a VMState for the block layer
so that requeued requests can be migrated. Breaks migration
compatibility, obviously.

Kevin

Patch

diff --git a/block/stream.c b/block/stream.c
index b3ede44..03cae14 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -31,6 +31,7 @@  typedef struct StreamBlockJob {
     BlockJob common;
     RateLimit limit;
     BlockDriverState *base;
+    BlockdevOnError on_error;
     char backing_file_id[1024];
 } StreamBlockJob;
 
@@ -78,6 +79,7 @@  static void coroutine_fn stream_run(void *opaque)
     BlockDriverState *bs = s->common.bs;
     BlockDriverState *base = s->base;
     int64_t sector_num, end;
+    int error = 0;
     int ret = 0;
     int n = 0;
     void *buf;
@@ -136,7 +138,19 @@  wait:
             ret = stream_populate(bs, sector_num, n, buf);
         }
         if (ret < 0) {
-            break;
+            BlockErrorAction action =
+                block_job_error_action(&s->common, s->common.bs, s->on_error,
+                                       true, -ret);
+            if (action == BDRV_ACTION_STOP) {
+                n = 0;
+                continue;
+            }
+            if (error == 0) {
+                error = ret;
+            }
+            if (action == BDRV_ACTION_REPORT) {
+                break;
+            }
         }
         ret = 0;
 
@@ -148,6 +162,9 @@  wait:
         bdrv_disable_copy_on_read(bs);
     }
 
+    /* Do not remove the backing file if an error was there but ignored.  */
+    ret = error;
+
     if (!block_job_is_cancelled(&s->common) && sector_num == end && ret == 0) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
@@ -183,11 +200,19 @@  static BlockJobType stream_job_type = {
 
 void stream_start(BlockDriverState *bs, BlockDriverState *base,
                   const char *base_id, int64_t speed,
+                  BlockdevOnError on_error,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp)
 {
     StreamBlockJob *s;
 
+    if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
+         on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
+        !bdrv_iostatus_is_enabled(bs)) {
+        error_set(errp, QERR_INVALID_PARAMETER, "on-error");
+        return;
+    }
+
     s = block_job_create(&stream_job_type, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
@@ -198,6 +223,7 @@  void stream_start(BlockDriverState *bs, BlockDriverState *base,
         pstrcpy(s->backing_file_id, sizeof(s->backing_file_id), base_id);
     }
 
+    s->on_error = on_error;
     s->common.co = qemu_coroutine_create(stream_run);
     trace_stream_start(bs, base, s, s->common.co, opaque);
     qemu_coroutine_enter(s->common.co, s);
diff --git a/block_int.h b/block_int.h
index 92c106a..d30ff7f 100644
--- a/block_int.h
+++ b/block_int.h
@@ -289,6 +289,7 @@  void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
  * @base_id: The file name that will be written to @bs as the new
  * backing file if the job completes.  Ignored if @base is %NULL.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @on_error: The action to take upon error.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
  * @errp: Error object.
@@ -300,7 +301,7 @@  void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
  * @base_id in the written image and to @base in the live BlockDriverState.
  */
 void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                  const char *base_id, int64_t speed,
+                  const char *base_id, int64_t speed, BlockdevOnError on_error,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp);
 
diff --git a/blockdev.c b/blockdev.c
index fccbe3d..49ade14 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1088,13 +1088,18 @@  static void block_stream_cb(void *opaque, int ret)
 }
 
 void qmp_block_stream(const char *device, bool has_base,
-                      const char *base, bool has_speed,
-                      int64_t speed, Error **errp)
+                      const char *base, bool has_speed, int64_t speed,
+                      bool has_on_error, BlockdevOnError on_error,
+                      Error **errp)
 {
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
     Error *local_err = NULL;
 
+    if (!has_on_error) {
+        on_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+
     bs = bdrv_find(device);
     if (!bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
@@ -1110,7 +1115,7 @@  void qmp_block_stream(const char *device, bool has_base,
     }
 
     stream_start(bs, base_bs, base, has_speed ? speed : 0,
-                 block_stream_cb, bs, &local_err);
+                 on_error, block_stream_cb, bs, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
         return;
diff --git a/hmp.c b/hmp.c
index a4c6629..5d4e6ac 100644
--- a/hmp.c
+++ b/hmp.c
@@ -845,7 +845,8 @@  void hmp_block_stream(Monitor *mon, const QDict *qdict)
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
     qmp_block_stream(device, base != NULL, base,
-                     qdict_haskey(qdict, "speed"), speed, &error);
+                     qdict_haskey(qdict, "speed"), speed,
+                     BLOCKDEV_ON_ERROR_REPORT, true, &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index d7191f3..6133d90 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1656,17 +1656,23 @@ 
 #
 # @speed:  #optional the maximum speed, in bytes per second
 #
+# @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.
+#
 # Returns: Nothing on success
 #          If streaming is already active on this device, DeviceInUse
 #          If @device does not exist, DeviceNotFound
 #          If image streaming is not supported by this device, NotSupported
 #          If @base does not exist, BaseNotFound
 #          If @speed is invalid, InvalidParameter
+#          If @on_error is not supported, InvalidParameter
 #
 # Since: 1.1
 ##
-{ 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str',
-                                       '*speed': 'int' } }
+{ 'command': 'block-stream',
+  'data': { 'device': 'str', '*base': 'str', '*speed': 'int',
+            '*on-error': 'BlockdevOnError' } }
 
 ##
 # @block-job-set-speed:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index f0c98a1..7026a4a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -718,7 +718,7 @@  EQMP
 
     {
         .name       = "block-stream",
-        .args_type  = "device:B,base:s?,speed:o?",
+        .args_type  = "device:B,base:s?,speed:o?,on-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_block_stream,
     },