diff mbox

[3/9] block jobs: Use BdrvChild callbacks for iostatus operations

Message ID 1458675397-24956-4-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf March 22, 2016, 7:36 p.m. UTC
The block jobs currently modify the target BB's error handling options
and require that the source BB's iostatus is enabled in order to
implement the per-job error options. It's obvious that this is something
between ugly, adventurous and plain wrong, so we should ideally fix this
instead of thinking about how to handle multiple BBs in this case.

Unfortunately we have a chicken-and-egg problem here: In order to fix
the problem, the block jobs need their own BBs. This requires multiple
BBs per BDS, which in turn means that BDS.blk must be removed. Removing
BDS.blk means that the jobs already need their own BB. The only other
options is that we lift the iostatus operations to the BdrvChild level
and deal with multiple BBs now.

So even though we don't want to have these callbacks in the end (because
they indicate broken logic), this patch converts the iostatus operations
for block jobs targets to BdrvChild callbacks; if more than one parent
implements iostatus operations, they are called for each of them.

Once the conversion is completed, block jobs will get their own internal
BB and the callbacks can be removed again.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c            | 20 +++++------
 block/block-backend.c     | 42 +++++++++++++++++++++++
 block/commit.c            |  2 +-
 block/mirror.c            | 21 ++++++------
 block/stream.c            |  2 +-
 blockjob.c                | 85 +++++++++++++++++++++++++++++++++++++++++++++--
 include/block/block_int.h | 10 ++++++
 include/block/blockjob.h  |  9 +++++
 8 files changed, 165 insertions(+), 26 deletions(-)

Comments

Max Reitz March 29, 2016, 6:15 p.m. UTC | #1
On 22.03.2016 20:36, Kevin Wolf wrote:
> The block jobs currently modify the target BB's error handling options
> and require that the source BB's iostatus is enabled in order to
> implement the per-job error options. It's obvious that this is something
> between ugly, adventurous and plain wrong, so we should ideally fix this
> instead of thinking about how to handle multiple BBs in this case.
> 
> Unfortunately we have a chicken-and-egg problem here: In order to fix
> the problem, the block jobs need their own BBs. This requires multiple
> BBs per BDS, which in turn means that BDS.blk must be removed. Removing
> BDS.blk means that the jobs already need their own BB. The only other
> options is that we lift the iostatus operations to the BdrvChild level
> and deal with multiple BBs now.
> 
> So even though we don't want to have these callbacks in the end (because
> they indicate broken logic), this patch converts the iostatus operations
> for block jobs targets to BdrvChild callbacks; if more than one parent
> implements iostatus operations, they are called for each of them.
> 
> Once the conversion is completed, block jobs will get their own internal
> BB and the callbacks can be removed again.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c            | 20 +++++------
>  block/block-backend.c     | 42 +++++++++++++++++++++++
>  block/commit.c            |  2 +-
>  block/mirror.c            | 21 ++++++------
>  block/stream.c            |  2 +-
>  blockjob.c                | 85 +++++++++++++++++++++++++++++++++++++++++++++--
>  include/block/block_int.h | 10 ++++++
>  include/block/blockjob.h  |  9 +++++
>  8 files changed, 165 insertions(+), 26 deletions(-)

[...]

> diff --git a/block/block-backend.c b/block/block-backend.c
> index c4c0dc0..03e68a7 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -100,6 +100,39 @@ static const char *blk_root_get_name(BdrvChild *child)
>      return blk_name(child->opaque);
>  }
>  
> +static void blk_root_iostatus_reset(BdrvChild *child)
> +{
> +    return blk_iostatus_reset(child->opaque);

The C11 standard (draft) says in 6.8.6.4 paragraph 1 that:
> A return statement with an expression shall not appear in a function
> whose return type is void.

Even if it is a void expression, it still is an expression. Also, the
semantics of a return statement with an expression is as follows
(paragraph 3):
> If a return statement with an expression is executed, the value of
> the expression is returned to the caller as the value of the function
> call expression.

However, 6.3.2.2 ("void") says:
> The (nonexistent) value of a void expression (an expression that has
> type void) shall not be used in any way

Therefore, the value of a void expression cannot be returned through a
return instruction because such a value does not exist.

I looked it up because I remember you had quite a bit of knowledge
regarding corner cases of the void types. The patch looks good overall,
so if you can point me to why this is allowed or that it's something we
already do somewhere else in qemu, then I'll give my R-b.

Max
diff mbox

Patch

diff --git a/block/backup.c b/block/backup.c
index 10397e2..016741e 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -220,9 +220,8 @@  static void backup_iostatus_reset(BlockJob *job)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common);
 
-    if (s->target->blk) {
-        blk_iostatus_reset(s->target->blk);
-    }
+    /* FIXME Only touch iostatus of a job-owned BB */
+    bdrv_iostatus_reset(s->target);
 }
 
 static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
@@ -402,10 +401,9 @@  static void coroutine_fn backup_run(void *opaque)
 
     job->done_bitmap = bitmap_new(end);
 
-    if (target->blk) {
-        blk_set_on_error(target->blk, on_target_error, on_target_error);
-        blk_iostatus_enable(target->blk);
-    }
+    /* FIXME Only touch on_error and iostatus of a job-owned BB */
+    bdrv_set_on_error(target, on_target_error, on_target_error);
+    bdrv_iostatus_enable(target);
 
     bdrv_add_before_write_notifier(bs, &before_write);
 
@@ -482,9 +480,9 @@  static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_unlock(&job->flush_rwlock);
     g_free(job->done_bitmap);
 
-    if (target->blk) {
-        blk_iostatus_disable(target->blk);
-    }
+    /* FIXME Only touch iostatus of a job-owned BB */
+    bdrv_iostatus_disable(target);
+
     bdrv_op_unblock_all(target, job->common.blocker);
 
     data = g_malloc(sizeof(*data));
@@ -515,7 +513,7 @@  void backup_start(BlockDriverState *bs, BlockDriverState *target,
 
     if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
          on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
-        (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+        !bdrv_iostatus_is_enabled(bs)) {
         error_setg(errp, QERR_INVALID_PARAMETER, "on-source-error");
         return;
     }
diff --git a/block/block-backend.c b/block/block-backend.c
index c4c0dc0..03e68a7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -100,6 +100,39 @@  static const char *blk_root_get_name(BdrvChild *child)
     return blk_name(child->opaque);
 }
 
+static void blk_root_iostatus_reset(BdrvChild *child)
+{
+    return blk_iostatus_reset(child->opaque);
+}
+
+static void blk_root_iostatus_enable(BdrvChild *child)
+{
+    return blk_iostatus_enable(child->opaque);
+}
+
+static void blk_root_iostatus_disable(BdrvChild *child)
+{
+    return blk_iostatus_disable(child->opaque);
+}
+
+static bool blk_root_iostatus_is_enabled(BdrvChild *child)
+{
+    return blk_iostatus_is_enabled(child->opaque);
+}
+
+static void blk_root_iostatus_set_err(BdrvChild *child, int error)
+{
+    return blk_iostatus_set_err(child->opaque, error);
+}
+
+static void blk_root_set_on_error(BdrvChild *child,
+                                  BlockdevOnError on_read_error,
+                                  BlockdevOnError on_write_error)
+{
+    return blk_set_on_error(child->opaque, on_read_error, on_write_error);
+}
+
+
 static const BdrvChildRole child_root = {
     .inherit_options    = blk_root_inherit_options,
 
@@ -108,6 +141,15 @@  static const BdrvChildRole child_root = {
     .get_name           = blk_root_get_name,
 
     .drain_queue        = blk_drain_throttling_queue,
+
+    /* FIXME Remove these callbacks. Children setting the iostatus are wrong,
+     * it should be controlled by the parents. */
+    .iostatus_reset     = blk_root_iostatus_reset,
+    .iostatus_enable    = blk_root_iostatus_enable,
+    .iostatus_disable   = blk_root_iostatus_disable,
+    .iostatus_is_enabled = blk_root_iostatus_is_enabled,
+    .iostatus_set_err   = blk_root_iostatus_set_err,
+    .set_on_error       = blk_root_set_on_error,
 };
 
 /*
diff --git a/block/commit.c b/block/commit.c
index 446a3ae..5e45195 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -215,7 +215,7 @@  void commit_start(BlockDriverState *bs, BlockDriverState *base,
 
     if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
          on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
-        (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+        !bdrv_iostatus_is_enabled(bs)) {
         error_setg(errp, "Invalid parameter combination");
         return;
     }
diff --git a/block/mirror.c b/block/mirror.c
index da18c0b..b9a93fd 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -689,9 +689,9 @@  immediate_exit:
     g_free(s->cow_bitmap);
     g_free(s->in_flight_bitmap);
     bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
-    if (s->target->blk) {
-        blk_iostatus_disable(s->target->blk);
-    }
+
+    /* FIXME Only touch iostatus of a job-owned BB */
+    bdrv_iostatus_disable(s->target);
 
     data = g_malloc(sizeof(*data));
     data->ret = ret;
@@ -716,9 +716,8 @@  static void mirror_iostatus_reset(BlockJob *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
 
-    if (s->target->blk) {
-        blk_iostatus_reset(s->target->blk);
-    }
+    /* FIXME Only touch iostatus of a job-owned BB */
+    bdrv_iostatus_reset(s->target);
 }
 
 static void mirror_complete(BlockJob *job, Error **errp)
@@ -802,7 +801,7 @@  static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
 
     if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
          on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
-        (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+        !bdrv_iostatus_is_enabled(bs)) {
         error_setg(errp, QERR_INVALID_PARAMETER, "on-source-error");
         return;
     }
@@ -855,10 +854,10 @@  static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
 
     bdrv_op_block_all(s->target, s->common.blocker);
 
-    if (s->target->blk) {
-        blk_set_on_error(s->target->blk, on_target_error, on_target_error);
-        blk_iostatus_enable(s->target->blk);
-    }
+    /* FIXME Only touch on_error and iostatus of a job-owned BB */
+    bdrv_set_on_error(s->target, on_target_error, on_target_error);
+    bdrv_iostatus_enable(s->target);
+
     s->common.co = qemu_coroutine_create(mirror_run);
     trace_mirror_start(bs, s, s->common.co, opaque);
     qemu_coroutine_enter(s->common.co, s);
diff --git a/block/stream.c b/block/stream.c
index cafaa07..d74f97f 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -224,7 +224,7 @@  void stream_start(BlockDriverState *bs, BlockDriverState *base,
 
     if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
          on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
-        (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+        !bdrv_iostatus_is_enabled(bs)) {
         error_setg(errp, QERR_INVALID_PARAMETER, "on-error");
         return;
     }
diff --git a/blockjob.c b/blockjob.c
index 9fc37ca..e6edd81 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -50,6 +50,87 @@  struct BlockJobTxn {
     int refcnt;
 };
 
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_reset(BlockDriverState *bs)
+{
+    BdrvChild *c;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role->iostatus_reset) {
+            c->role->iostatus_reset(c);
+        }
+    }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_enable(BlockDriverState *bs)
+{
+    BdrvChild *c;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role->iostatus_enable) {
+            c->role->iostatus_enable(c);
+        }
+    }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_disable(BlockDriverState *bs)
+{
+    BdrvChild *c;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role->iostatus_disable) {
+            c->role->iostatus_disable(c);
+        }
+    }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+bool bdrv_iostatus_is_enabled(BlockDriverState *bs)
+{
+    BdrvChild *c;
+    bool ret = false;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role->iostatus_is_enabled) {
+            ret = c->role->iostatus_is_enabled(c);
+            if (!ret) {
+                return false;
+            }
+        }
+    }
+
+    /* Return false if no BB exists; true if BBs exist and all of them have
+     * iostatus enabled */
+    return ret;
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
+{
+    BdrvChild *c;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role->iostatus_set_err) {
+            c->role->iostatus_set_err(c, error);
+        }
+    }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
+                       BlockdevOnError on_write_error)
+{
+    BdrvChild *c;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role->set_on_error) {
+            c->role->set_on_error(c, on_read_error, on_write_error);
+        }
+    }
+}
+
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)
@@ -443,8 +524,8 @@  BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
         job->user_paused = true;
         block_job_pause(job);
         block_job_iostatus_set_err(job, error);
-        if (bs->blk && bs != job->bs) {
-            blk_iostatus_set_err(bs->blk, error);
+        if (bs != job->bs) {
+            bdrv_iostatus_set_err(bs, error);
         }
     }
     return action;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 195abe8..9358949 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -361,6 +361,16 @@  struct BdrvChildRole {
     const char* (*get_name)(BdrvChild *child);
 
     bool (*drain_queue)(BdrvChild *child);
+
+    /* FIXME Remove these callbacks. Children setting the iostatus are wrong,
+     * it should be controlled by the parents. */
+    void (*iostatus_reset)(BdrvChild *child);
+    void (*iostatus_enable)(BdrvChild *child);
+    void (*iostatus_disable)(BdrvChild *child);
+    bool (*iostatus_is_enabled)(BdrvChild *child);
+    void (*iostatus_set_err)(BdrvChild *child, int error);
+    void (*set_on_error)(BdrvChild *child, BlockdevOnError on_read_error,
+                         BlockdevOnError on_write_error);
 };
 
 extern const BdrvChildRole child_file;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 8bedc49..33a42c3 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -448,4 +448,13 @@  void block_job_txn_unref(BlockJobTxn *txn);
  */
 void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job);
 
+/* TODO These are functions that shouldn't exist. Remove all of their users. */
+void bdrv_iostatus_reset(BlockDriverState *bs);
+void bdrv_iostatus_enable(BlockDriverState *bs);
+void bdrv_iostatus_disable(BlockDriverState *bs);
+bool bdrv_iostatus_is_enabled(BlockDriverState *bs);
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
+void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
+                       BlockdevOnError on_write_error);
+
 #endif