[Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations

[Kevin Wolf <kwolf@redhat.com>]

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/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
-- 
1.8.3.1