[Qemu-devel] [PATCH v2 0/6] block: remove legacy I/O throttling

[Qemu-devel] [PATCH v2 0/6] block: remove legacy I/O throttling

[Manos Pitsidianakis]

This series depends on my other series 'add throttle block driver filter'
currently on v4.

Replacing the current I/O interface means the user will use the same options as
before and QEMU will create a hidden throttle filter node beneath the device's
BlockBackend. 

v2:
  new commit: require job-id when device is a node name
  new commit: remove BlockBackendPublic
  new commit: add dedicated iotest
  cleanup reference counting in block/block-backend.c functions
  add new function to get filter child bs
  take ownership of options in bdrv_new_open_driver()

Manos Pitsidianakis (6):
  block: skip implicit nodes in snapshots, blockjobs
  block: add options parameter to bdrv_new_open_driver()
  block: require job-id when device is a node name
  block: remove legacy I/O throttling
  block: add iotest 191 for legacy throttling interface
  block: remove BlockBackendPublic

 block.c                         |  26 ++++++-
 block/block-backend.c           | 152 +++++++++++++++++++++++-----------------
 block/commit.c                  |   4 +-
 block/mirror.c                  |   2 +-
 block/qapi.c                    |  24 +++----
 block/throttle.c                |   8 +++
 block/vvfat.c                   |   2 +-
 blockdev.c                      | 148 +++++++++++++++++++++++++++++++++-----
 blockjob.c                      |  16 ++---
 include/block/block.h           |   2 +-
 include/block/block_int.h       |   9 +++
 include/block/blockjob_int.h    |   3 +-
 include/block/throttle-groups.h |   1 +
 include/sysemu/block-backend.h  |  16 +----
 tests/qemu-iotests/191          | 138 ++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/191.out      |   5 ++
 tests/qemu-iotests/group        |   1 +
 tests/test-blockjob.c           |  10 +--
 tests/test-throttle.c           |  19 ++---
 19 files changed, 440 insertions(+), 146 deletions(-)
 create mode 100644 tests/qemu-iotests/191
 create mode 100644 tests/qemu-iotests/191.out

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/6] block: skip implicit nodes in snapshots, blockjobs

[Manos Pitsidianakis]

Implicit filter nodes added at the top of nodes can interfere with block
jobs. This is not a problem when they are added by other jobs since
adding another job will issue a QERR_DEVICE_IN_USE, but it can happen in
the next commit which introduces an implicitly created throttle filter
node below BlockBackend, which we want to be skipped during automatic
operations on the graph since the user does not necessarily know about
their existence.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c                   | 10 ++++++++++
 block/qapi.c              | 14 +++++---------
 blockdev.c                | 34 ++++++++++++++++++++++++++++++++++
 include/block/block_int.h |  9 +++++++++
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 3615a6809e..e35d546c08 100644
--- a/block.c
+++ b/block.c
@@ -4945,3 +4945,13 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
 
     return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
 }
+
+/* Get first explicit node down a bs chain. */
+BlockDriverState *bdrv_get_first_explicit(BlockDriverState *bs)
+{
+    while (bs && bs->drv && bs->implicit) {
+        bs = child_bs(bs);
+        assert(bs);
+    }
+    return bs;
+}
diff --git a/block/qapi.c b/block/qapi.c
index 7fa2437923..847b044d13 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -147,9 +147,8 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 
         /* Skip automatically inserted nodes that the user isn't aware of for
          * query-block (blk != NULL), but not for query-named-block-nodes */
-        while (blk && bs0->drv && bs0->implicit) {
-            bs0 = backing_bs(bs0);
-            assert(bs0);
+        if (blk) {
+            bs0 = bdrv_get_first_explicit(bs0);
         }
     }
 
@@ -336,9 +335,7 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     char *qdev;
 
     /* Skip automatically inserted nodes that the user isn't aware of */
-    while (bs && bs->drv && bs->implicit) {
-        bs = backing_bs(bs);
-    }
+    bs = bdrv_get_first_explicit(bs);
 
     info->device = g_strdup(blk_name(blk));
     info->type = g_strdup("unknown");
@@ -465,9 +462,8 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
     /* Skip automatically inserted nodes that the user isn't aware of in
      * a BlockBackend-level command. Stay at the exact node for a node-level
      * command. */
-    while (blk_level && bs->drv && bs->implicit) {
-        bs = backing_bs(bs);
-        assert(bs);
+    if (blk_level) {
+        bs = bdrv_get_first_explicit(bs);
     }
 
     if (bdrv_get_node_name(bs)[0]) {
diff --git a/blockdev.c b/blockdev.c
index 23475abb72..fc7b65c3f0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1300,6 +1300,10 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
     if (!bs) {
         return NULL;
     }
+
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -1508,6 +1512,9 @@ static void internal_snapshot_prepare(BlkActionState *common,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
@@ -1664,6 +1671,9 @@ static void external_snapshot_prepare(BlkActionState *common,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    state->old_bs = bdrv_get_first_explicit(state->old_bs);
+
     /* Acquire AioContext now so any threads operating on old_bs stop */
     state->aio_context = bdrv_get_aio_context(state->old_bs);
     aio_context_acquire(state->aio_context);
@@ -1844,6 +1854,9 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
@@ -1908,6 +1921,9 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     target = bdrv_lookup_bs(backup->target, backup->target, errp);
     if (!target) {
         return;
@@ -2988,6 +3004,9 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3095,6 +3114,9 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3209,6 +3231,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         return NULL;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3484,6 +3509,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3638,6 +3666,9 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     target_bs = bdrv_lookup_bs(target, target, errp);
     if (!target_bs) {
         return;
@@ -3786,6 +3817,9 @@ void qmp_change_backing_file(const char *device,
         return;
     }
 
+    /* Skip implicit filter nodes */
+    bs = bdrv_get_first_explicit(bs);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7571c0aaaf..8353b83e86 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -699,6 +699,13 @@ static inline BlockDriverState *backing_bs(BlockDriverState *bs)
     return bs->backing ? bs->backing->bs : NULL;
 }
 
+static inline BlockDriverState *child_bs(BlockDriverState *bs)
+{
+    BdrvChild *child = QLIST_FIRST(&bs->children);
+    assert(child && !QLIST_NEXT(child, next));
+    return child->bs;
+}
+
 
 /* Essential block drivers which must always be statically linked into qemu, and
  * which therefore can be accessed without using bdrv_find_format() */
@@ -980,4 +987,6 @@ void bdrv_dec_in_flight(BlockDriverState *bs);
 
 void blockdev_close_all_bdrv_states(void);
 
+BlockDriverState *bdrv_get_first_explicit(BlockDriverState *bs);
+
 #endif /* BLOCK_INT_H */
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/6] block: add options parameter to bdrv_new_open_driver()

[Manos Pitsidianakis]

Allow passing a QDict *options parameter to bdrv_new_open_driver() so
that it can be used if a driver needs it upon creation. The previous
behaviour (empty bs->options and bs->explicit_options) remains when
options is NULL.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c               | 16 +++++++++++++---
 block/commit.c        |  4 ++--
 block/mirror.c        |  2 +-
 block/vvfat.c         |  2 +-
 include/block/block.h |  2 +-
 5 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index e35d546c08..2de1c29eb3 100644
--- a/block.c
+++ b/block.c
@@ -1153,16 +1153,26 @@ open_failed:
     return ret;
 }
 
+/*
+ * If options is not NULL, its ownership is transferred to the block layer. The
+ * caller must use QINCREF() if they wish to keep ownership.
+ */
 BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
-                                       int flags, Error **errp)
+                                       int flags, QDict *options, Error **errp)
 {
     BlockDriverState *bs;
     int ret;
 
     bs = bdrv_new();
     bs->open_flags = flags;
-    bs->explicit_options = qdict_new();
-    bs->options = qdict_new();
+    if (options) {
+        bs->explicit_options = qdict_clone_shallow(options);
+        bs->options = qdict_clone_shallow(options);
+        QDECREF(options);
+    } else {
+        bs->explicit_options = qdict_new();
+        bs->options = qdict_new();
+    }
     bs->opaque = NULL;
 
     update_options_from_flags(bs->options, flags);
diff --git a/block/commit.c b/block/commit.c
index c7857c3321..539e23c3f8 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -342,7 +342,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     /* Insert commit_top block node above top, so we can block consistent read
      * on the backing chain below it */
     commit_top_bs = bdrv_new_open_driver(&bdrv_commit_top, filter_node_name, 0,
-                                         errp);
+                                         NULL, errp);
     if (commit_top_bs == NULL) {
         goto fail;
     }
@@ -494,7 +494,7 @@ int bdrv_commit(BlockDriverState *bs)
     backing_file_bs = backing_bs(bs);
 
     commit_top_bs = bdrv_new_open_driver(&bdrv_commit_top, NULL, BDRV_O_RDWR,
-                                         &local_err);
+                                         NULL, &local_err);
     if (commit_top_bs == NULL) {
         error_report_err(local_err);
         goto ro_cleanup;
diff --git a/block/mirror.c b/block/mirror.c
index c9a6a3ca86..e1a160e6ea 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1164,7 +1164,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
      * reads on the top, while disabling it in the intermediate nodes, and make
      * the backing chain writable. */
     mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
-                                         BDRV_O_RDWR, errp);
+                                         BDRV_O_RDWR, NULL, errp);
     if (mirror_top_bs == NULL) {
         return;
     }
diff --git a/block/vvfat.c b/block/vvfat.c
index a9e207f7f0..6c59473baf 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3194,7 +3194,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
 #endif
 
     backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
-                                   &error_abort);
+                                   NULL, &error_abort);
     *(void**) backing->opaque = s;
 
     bdrv_set_backing_hd(s->bs, backing, &error_abort);
diff --git a/include/block/block.h b/include/block/block.h
index ab80195378..d1f03cb48b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -263,7 +263,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
 BlockDriverState *bdrv_open(const char *filename, const char *reference,
                             QDict *options, int flags, Error **errp);
 BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
-                                       int flags, Error **errp);
+                                       int flags, QDict *options, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs,
                                     QDict *options, int flags);
-- 
2.11.0

[Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name

[Manos Pitsidianakis]

With implicit filter nodes on the top of the graph it is not possible to
generate job-ids with the name of the device in block_job_create()
anymore, since the job's bs will not be a child_root.

Instead we can require that job-id is not NULL in block_job_create(),
and check that a job-id has been set in the callers of
block_job_create() in blockdev.c. It is more consistent to require an
explicit job-id when the device parameter in the job creation command,
eg

 { "execute": "drive-backup",
   "arguments": { "device": "drive0",
                  "sync": "full",
                  "target": "backup.img" } }

is not a BlockBackend name, instead of automatically getting it from the
root BS if device is a node name. That information is lost after calling
block_job_create(), so we can do it in its caller instead.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 blockdev.c                   | 65 +++++++++++++++++++++++++++++++++++++++-----
 blockjob.c                   | 16 ++++-------
 include/block/blockjob_int.h |  3 +-
 tests/test-blockjob.c        | 10 ++-----
 4 files changed, 67 insertions(+), 27 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index fc7b65c3f0..6ffa5b0b04 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3004,6 +3004,16 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         return;
     }
 
+    /* Always require a job-id when device is a node name */
+    if (!has_job_id) {
+        if (blk_by_name(device)) {
+            job_id = device;
+        } else {
+            error_setg(errp, "An explicit job ID is required for this node");
+            return;
+        }
+    }
+
     /* Skip implicit filter nodes */
     bs = bdrv_get_first_explicit(bs);
 
@@ -3058,7 +3068,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     /* backing_file string overrides base bs filename */
     base_name = has_backing_file ? backing_file : base_name;
 
-    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+    stream_start(job_id, bs, base_bs, base_name,
                  has_speed ? speed : 0, on_error, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -3117,6 +3127,16 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
     /* Skip implicit filter nodes */
     bs = bdrv_get_first_explicit(bs);
 
+    /* Always require a job-id when device is a node name */
+    if (!has_job_id) {
+        if (blk_by_name(device)) {
+            job_id = device;
+        } else {
+            error_setg(errp, "An explicit job ID is required for this node");
+            return;
+        }
+    }
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -3171,7 +3191,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
                              " but 'top' is the active layer");
             goto out;
         }
-        commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
+        commit_active_start(job_id, bs, base_bs,
                             BLOCK_JOB_DEFAULT, speed, on_error,
                             filter_node_name, NULL, NULL, false, &local_err);
     } else {
@@ -3179,7 +3199,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
         if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
             goto out;
         }
-        commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
+        commit_start(job_id, bs, base_bs, top_bs, speed,
                      on_error, has_backing_file ? backing_file : NULL,
                      filter_node_name, &local_err);
     }
@@ -3220,7 +3240,13 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
     if (!backup->has_job_id) {
-        backup->job_id = NULL;
+        /* Always require a job-id when device is a node name */
+        if (blk_by_name(backup->device)) {
+            backup->job_id = backup->device;
+        } else {
+            error_setg(errp, "An explicit job ID is required for this node");
+            return NULL;
+        }
     }
     if (!backup->has_compress) {
         backup->compress = false;
@@ -3366,7 +3392,13 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
         backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT;
     }
     if (!backup->has_job_id) {
-        backup->job_id = NULL;
+        /* Always require a job-id when device is a node name */
+        if (blk_by_name(backup->device)) {
+            backup->job_id = backup->device;
+        } else {
+            error_setg(errp, "An explicit job ID is required for this node");
+            return NULL;
+        }
     }
     if (!backup->has_compress) {
         backup->compress = false;
@@ -3509,6 +3541,16 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         return;
     }
 
+    /* Always require a job-id when device is a node name */
+    if (!arg->has_job_id) {
+        if (blk_by_name(arg->device)) {
+            arg->job_id = arg->device;
+        } else {
+            error_setg(errp, "An explicit job ID is required for this node");
+            return;
+        }
+    }
+
     /* Skip implicit filter nodes */
     bs = bdrv_get_first_explicit(bs);
 
@@ -3624,7 +3666,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 
     bdrv_set_aio_context(target_bs, aio_context);
 
-    blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
+    blockdev_mirror_common(arg->job_id, bs, target_bs,
                            arg->has_replaces, arg->replaces, arg->sync,
                            backing_mode, arg->has_speed, arg->speed,
                            arg->has_granularity, arg->granularity,
@@ -3674,12 +3716,21 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
         return;
     }
 
+    /* Always require a job-id when device is a node name */
+    if (!has_job_id) {
+        if (blk_by_name(device)) {
+            job_id = device;
+        } else {
+            error_setg(errp, "An explicit job ID is required for this node");
+            return;
+        }
+    }
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
     bdrv_set_aio_context(target_bs, aio_context);
 
-    blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
+    blockdev_mirror_common(job_id, bs, target_bs,
                            has_replaces, replaces, sync, backing_mode,
                            has_speed, speed,
                            has_granularity, granularity,
diff --git a/blockjob.c b/blockjob.c
index 70a78188b7..ba26890ae9 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -622,20 +622,14 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
         return NULL;
     }
 
-    if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
-        job_id = bdrv_get_device_name(bs);
-        if (!*job_id) {
-            error_setg(errp, "An explicit job ID is required for this node");
-            return NULL;
-        }
-    }
-
-    if (job_id) {
-        if (flags & BLOCK_JOB_INTERNAL) {
+    if (flags & BLOCK_JOB_INTERNAL) {
+        if (job_id) {
             error_setg(errp, "Cannot specify job ID for internal block job");
             return NULL;
         }
-
+    } else {
+        /* Require job-id. */
+        assert(job_id);
         if (!id_wellformed(job_id)) {
             error_setg(errp, "Invalid job ID '%s'", job_id);
             return NULL;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05c0d..ff906808a6 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -112,8 +112,7 @@ struct BlockJobDriver {
 
 /**
  * block_job_create:
- * @job_id: The id of the newly-created job, or %NULL to have one
- * generated automatically.
+ * @job_id: The id of the newly-created job, must be non %NULL.
  * @job_type: The class object for the newly-created job.
  * @bs: The block
  * @perm, @shared_perm: Permissions to request for @bs
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 23bdf1a932..83ce5f63c7 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -93,9 +93,6 @@ static void test_job_ids(void)
     blk[1] = create_blk("drive1");
     blk[2] = create_blk("drive2");
 
-    /* No job ID provided and the block backend has no name */
-    job[0] = do_test_id(blk[0], NULL, false);
-
     /* These are all invalid job IDs */
     job[0] = do_test_id(blk[0], "0id", false);
     job[0] = do_test_id(blk[0], "",    false);
@@ -119,16 +116,15 @@ static void test_job_ids(void)
     block_job_early_fail(job[0]);
     job[1] = do_test_id(blk[1], "id0", true);
 
-    /* No job ID specified, defaults to the backend name ('drive1') */
     block_job_early_fail(job[1]);
-    job[1] = do_test_id(blk[1], NULL, true);
+    job[1] = do_test_id(blk[1], "drive1", true);
 
     /* Duplicate job ID */
     job[2] = do_test_id(blk[2], "drive1", false);
 
-    /* The ID of job[2] would default to 'drive2' but it is already in use */
+    /* The ID of job[2] is already in use */
     job[0] = do_test_id(blk[0], "drive2", true);
-    job[2] = do_test_id(blk[2], NULL, false);
+    job[2] = do_test_id(blk[2], "drive2", false);
 
     /* This one is valid */
     job[2] = do_test_id(blk[2], "id_2", true);
-- 
2.11.0

[Qemu-devel] [PATCH v2 4/6] block: remove legacy I/O throttling

[Manos Pitsidianakis]

This commit removes all I/O throttling from block/block-backend.c. In
order to support the existing interface, it is changed to use the
block/throttle.c filter driver.

The throttle filter node that is created by the legacy interface is
stored in a 'throttle_node' field in the BlockBackendPublic of the
device. The legacy throttle node is managed by the legacy interface
completely. More advanced configurations with the filter drive are
possible using the QMP API, but these will be ignored by the legacy
interface.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/block-backend.c           | 133 ++++++++++++++++++++++++----------------
 block/qapi.c                    |  10 +--
 block/throttle.c                |   8 +++
 blockdev.c                      |  49 +++++++++++----
 include/block/throttle-groups.h |   1 +
 include/sysemu/block-backend.h  |   6 +-
 tests/test-throttle.c           |  19 +++---
 7 files changed, 146 insertions(+), 80 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index df0200fc49..61983b7393 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -15,6 +15,7 @@
 #include "block/block_int.h"
 #include "block/blockjob.h"
 #include "block/throttle-groups.h"
+#include "qemu/throttle-options.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
 #include "qapi-event.h"
@@ -282,7 +283,7 @@ static void blk_delete(BlockBackend *blk)
     assert(!blk->refcnt);
     assert(!blk->name);
     assert(!blk->dev);
-    if (blk->public.throttle_group_member.throttle_state) {
+    if (blk->public.throttle_node) {
         blk_io_limits_disable(blk);
     }
     if (blk->root) {
@@ -593,13 +594,7 @@ BlockBackend *blk_by_public(BlockBackendPublic *public)
  */
 void blk_remove_bs(BlockBackend *blk)
 {
-    ThrottleTimers *tt;
-
     notifier_list_notify(&blk->remove_bs_notifiers, blk);
-    if (blk->public.throttle_group_member.throttle_state) {
-        tt = &blk->public.throttle_group_member.throttle_timers;
-        throttle_timers_detach_aio_context(tt);
-    }
 
     blk_update_root_state(blk);
 
@@ -620,12 +615,6 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
     bdrv_ref(bs);
 
     notifier_list_notify(&blk->insert_bs_notifiers, blk);
-    if (blk->public.throttle_group_member.throttle_state) {
-        throttle_timers_attach_aio_context(
-            &blk->public.throttle_group_member.throttle_timers,
-            bdrv_get_aio_context(bs));
-    }
-
     return 0;
 }
 
@@ -983,13 +972,6 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
     }
 
     bdrv_inc_in_flight(bs);
-
-    /* throttling disk I/O */
-    if (blk->public.throttle_group_member.throttle_state) {
-        throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member,
-                bytes, false);
-    }
-
     ret = bdrv_co_preadv(blk->root, offset, bytes, qiov, flags);
     bdrv_dec_in_flight(bs);
     return ret;
@@ -1010,11 +992,6 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
     }
 
     bdrv_inc_in_flight(bs);
-    /* throttling disk I/O */
-    if (blk->public.throttle_group_member.throttle_state) {
-        throttle_group_co_io_limits_intercept(&blk->public.throttle_group_member,
-                bytes, true);
-    }
 
     if (!blk->enable_write_cache) {
         flags |= BDRV_REQ_FUA;
@@ -1682,16 +1659,9 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb)
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
     BlockDriverState *bs = blk_bs(blk);
-    ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
 
     if (bs) {
-        if (tgm->throttle_state) {
-            throttle_group_detach_aio_context(tgm);
-        }
         bdrv_set_aio_context(bs, new_context);
-        if (tgm->throttle_state) {
-            throttle_group_attach_aio_context(tgm, new_context);
-        }
     }
 }
 
@@ -1909,45 +1879,98 @@ int blk_commit_all(void)
 /* throttling disk I/O limits */
 void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
 {
-    throttle_group_config(&blk->public.throttle_group_member, cfg);
+    assert(blk->public.throttle_node);
+    throttle_group_config(throttle_get_tgm(blk->public.throttle_node), cfg);
 }
 
 void blk_io_limits_disable(BlockBackend *blk)
 {
-    assert(blk->public.throttle_group_member.throttle_state);
-    bdrv_drained_begin(blk_bs(blk));
-    throttle_group_unregister_tgm(&blk->public.throttle_group_member);
-    bdrv_drained_end(blk_bs(blk));
+    BlockDriverState *bs, *throttle_node;
+
+    throttle_node = blk_get_public(blk)->throttle_node;
+
+    assert(throttle_node);
+
+    bs = throttle_node->file->bs;
+    bdrv_drained_begin(bs);
+
+    /* Ref throttle_node's child bs to ensure it won't go away */
+    bdrv_ref(bs);
+
+    bdrv_child_try_set_perm(throttle_node->file, 0, BLK_PERM_ALL,
+                            &error_abort);
+    /* Replace throttle_node with bs. While throttle_node was inserted under
+     * blk, at this point it might have more than one parent, so use
+     * bdrv_replace_node(). This destroys throttle_node */
+    bdrv_replace_node(throttle_node, bs, &error_abort);
+    blk_get_public(blk)->throttle_node = NULL;
+
+    bdrv_unref(bs);
+    bdrv_drained_end(bs);
+
 }
 
 /* should be called before blk_set_io_limits if a limit is set */
-void blk_io_limits_enable(BlockBackend *blk, const char *group)
+void blk_io_limits_enable(BlockBackend *blk, const char *group,  Error **errp)
 {
-    assert(!blk->public.throttle_group_member.throttle_state);
-    throttle_group_register_tgm(&blk->public.throttle_group_member,
-                                group, blk_get_aio_context(blk));
+    BlockDriverState *bs = blk_bs(blk), *throttle_node;
+    QDict *options = qdict_new();
+    Error *local_err = NULL;
+
+    bdrv_drained_begin(bs);
+
+    qdict_set_default_str(options, "file", bs->node_name);
+    qdict_set_default_str(options, QEMU_OPT_THROTTLE_GROUP_NAME, group);
+    throttle_node = bdrv_new_open_driver(bdrv_find_format("throttle"), NULL,
+                                         bdrv_get_flags(bs), options, errp);
+    if (!throttle_node) {
+        goto end;
+    }
+    throttle_node->implicit = true;
+
+    blk_remove_bs(blk);
+    blk_insert_bs(blk, throttle_node, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        blk_insert_bs(blk, bs, &error_abort);
+        bdrv_unref(throttle_node);
+        throttle_node = NULL;
+        goto end;
+    }
+    bdrv_unref(throttle_node);
+
+    assert(throttle_node->file->bs == bs);
+    assert(throttle_node->refcnt == 1);
+
+end:
+    bdrv_drained_end(bs);
+    blk_get_public(blk)->throttle_node = throttle_node;
 }
 
-void blk_io_limits_update_group(BlockBackend *blk, const char *group)
+void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error **errp)
 {
+    ThrottleGroupMember *tgm;
+
     /* this BB is not part of any group */
-    if (!blk->public.throttle_group_member.throttle_state) {
+    if (!blk->public.throttle_node) {
         return;
     }
 
+    tgm = throttle_get_tgm(blk->public.throttle_node);
     /* this BB is a part of the same group than the one we want */
-    if (!g_strcmp0(throttle_group_get_name(&blk->public.throttle_group_member),
-                group)) {
+    if (!g_strcmp0(throttle_group_get_name(tgm),
+                   group)) {
         return;
     }
 
-    /* need to change the group this bs belong to */
+    /* need to change the group this bs belongs to */
     blk_io_limits_disable(blk);
-    blk_io_limits_enable(blk, group);
+    blk_io_limits_enable(blk, group, errp);
 }
 
 static void blk_root_drained_begin(BdrvChild *child)
 {
+    ThrottleGroupMember *tgm;
     BlockBackend *blk = child->opaque;
 
     if (++blk->quiesce_counter == 1) {
@@ -1958,19 +1981,25 @@ static void blk_root_drained_begin(BdrvChild *child)
 
     /* Note that blk->root may not be accessible here yet if we are just
      * attaching to a BlockDriverState that is drained. Use child instead. */
-
-    if (atomic_fetch_inc(&blk->public.throttle_group_member.io_limits_disabled) == 0) {
-        throttle_group_restart_tgm(&blk->public.throttle_group_member);
+    if (blk->public.throttle_node) {
+        tgm = throttle_get_tgm(blk->public.throttle_node);
+        if (atomic_fetch_inc(&tgm->io_limits_disabled) == 0) {
+            throttle_group_restart_tgm(tgm);
+        }
     }
 }
 
 static void blk_root_drained_end(BdrvChild *child)
 {
+    ThrottleGroupMember *tgm;
     BlockBackend *blk = child->opaque;
     assert(blk->quiesce_counter);
 
-    assert(blk->public.throttle_group_member.io_limits_disabled);
-    atomic_dec(&blk->public.throttle_group_member.io_limits_disabled);
+    if (blk->public.throttle_node) {
+        tgm = throttle_get_tgm(blk->public.throttle_node);
+        assert(tgm->io_limits_disabled);
+        atomic_dec(&tgm->io_limits_disabled);
+    }
 
     if (--blk->quiesce_counter == 0) {
         if (blk->dev_ops && blk->dev_ops->drained_end) {
diff --git a/block/qapi.c b/block/qapi.c
index 847b044d13..ab55db7134 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -66,11 +66,12 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 
     info->detect_zeroes = bs->detect_zeroes;
 
-    if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
+    if (blk && blk_get_public(blk)->throttle_node) {
         ThrottleConfig cfg;
-        BlockBackendPublic *blkp = blk_get_public(blk);
+        BlockDriverState *throttle_node = blk_get_public(blk)->throttle_node;
+        ThrottleGroupMember *tgm = throttle_get_tgm(throttle_node);
 
-        throttle_group_get_config(&blkp->throttle_group_member, &cfg);
+        throttle_group_get_config(tgm, &cfg);
 
         info->bps     = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
         info->bps_rd  = cfg.buckets[THROTTLE_BPS_READ].avg;
@@ -118,8 +119,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
         info->iops_size = cfg.op_size;
 
         info->has_group = true;
-        info->group =
-            g_strdup(throttle_group_get_name(&blkp->throttle_group_member));
+        info->group = g_strdup(throttle_group_get_name(tgm));
     }
 
     info->write_threshold = bdrv_write_threshold_get(bs);
diff --git a/block/throttle.c b/block/throttle.c
index 3e6cb1de7b..16861b57ff 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -38,6 +38,14 @@ static QemuOptsList throttle_opts = {
     },
 };
 
+static BlockDriver bdrv_throttle;
+
+ThrottleGroupMember *throttle_get_tgm(BlockDriverState *bs)
+{
+    assert(bs->drv == &bdrv_throttle);
+    return (ThrottleGroupMember *)bs->opaque;
+}
+
 /* Extract ThrottleConfig options. Assumes cfg is initialized and will be
  * checked for validity.
  *
diff --git a/blockdev.c b/blockdev.c
index 6ffa5b0b04..20e5513f87 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -607,7 +607,14 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         if (!throttling_group) {
             throttling_group = id;
         }
-        blk_io_limits_enable(blk, throttling_group);
+        blk_io_limits_enable(blk, throttling_group, &error);
+        if (error) {
+            error_propagate(errp, error);
+            blk_unref(blk);
+            blk = NULL;
+            goto err_no_bs_opts;
+
+        }
         blk_set_io_limits(blk, &cfg);
     }
 
@@ -2629,6 +2636,9 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
     BlockDriverState *bs;
     BlockBackend *blk;
     AioContext *aio_context;
+    BlockDriverState *throttle_node = NULL;
+    ThrottleGroupMember *tgm;
+    Error *local_err = NULL;
 
     blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
                       arg->has_id ? arg->id : NULL,
@@ -2704,18 +2714,33 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
     if (throttle_enabled(&cfg)) {
         /* Enable I/O limits if they're not enabled yet, otherwise
          * just update the throttling group. */
-        if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
-            blk_io_limits_enable(blk,
-                                 arg->has_group ? arg->group :
-                                 arg->has_device ? arg->device :
-                                 arg->id);
-        } else if (arg->has_group) {
-            blk_io_limits_update_group(blk, arg->group);
+        if (!blk_get_public(blk)->throttle_node) {
+            blk_io_limits_enable(blk, arg->has_group ? arg->group :
+                                 arg->has_device ? arg->device : arg->id,
+                                 &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                goto out;
+            }
         }
-        /* Set the new throttling configuration */
-        blk_set_io_limits(blk, &cfg);
-    } else if (blk_get_public(blk)->throttle_group_member.throttle_state) {
-        /* If all throttling settings are set to 0, disable I/O limits */
+
+        if (arg->has_group) {
+            /* move throttle node membership to arg->group */
+            blk_io_limits_update_group(blk, arg->group, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                goto out;
+            }
+        }
+
+        throttle_node = blk_get_public(blk)->throttle_node;
+        tgm = throttle_get_tgm(throttle_node);
+        throttle_group_config(tgm, &cfg);
+    } else if (blk_get_public(blk)->throttle_node) {
+        /*
+         * If all throttling settings are set to 0, disable I/O limits
+         * by deleting the legacy throttle node
+         * */
         blk_io_limits_disable(blk);
     }
 
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 82f030523f..26a8e44b43 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -76,5 +76,6 @@ void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
 void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
                                        AioContext *new_context);
 void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
+ThrottleGroupMember *throttle_get_tgm(BlockDriverState *bs);
 
 #endif
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 0e0cda7521..4a7ca53685 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -73,7 +73,7 @@ typedef struct BlockDevOps {
  * friends so that BlockBackends can be kept in lists outside block-backend.c
  * */
 typedef struct BlockBackendPublic {
-    ThrottleGroupMember throttle_group_member;
+    BlockDriverState *throttle_node;
 } BlockBackendPublic;
 
 BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm);
@@ -225,7 +225,7 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
 
 void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg);
 void blk_io_limits_disable(BlockBackend *blk);
-void blk_io_limits_enable(BlockBackend *blk, const char *group);
-void blk_io_limits_update_group(BlockBackend *blk, const char *group);
+void blk_io_limits_enable(BlockBackend *blk, const char *group, Error **errp);
+void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error **errp);
 
 #endif
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 0ea9093eee..eef2b1c707 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -594,7 +594,6 @@ static void test_groups(void)
 {
     ThrottleConfig cfg1, cfg2;
     BlockBackend *blk1, *blk2, *blk3;
-    BlockBackendPublic *blkp1, *blkp2, *blkp3;
     ThrottleGroupMember *tgm1, *tgm2, *tgm3;
 
     /* No actual I/O is performed on these devices */
@@ -602,13 +601,9 @@ static void test_groups(void)
     blk2 = blk_new(0, BLK_PERM_ALL);
     blk3 = blk_new(0, BLK_PERM_ALL);
 
-    blkp1 = blk_get_public(blk1);
-    blkp2 = blk_get_public(blk2);
-    blkp3 = blk_get_public(blk3);
-
-    tgm1 = &blkp1->throttle_group_member;
-    tgm2 = &blkp2->throttle_group_member;
-    tgm3 = &blkp3->throttle_group_member;
+    tgm1 = g_new0(ThrottleGroupMember, 1);
+    tgm2 = g_new0(ThrottleGroupMember, 1);
+    tgm3 = g_new0(ThrottleGroupMember, 1);
 
     g_assert(tgm1->throttle_state == NULL);
     g_assert(tgm2->throttle_state == NULL);
@@ -655,6 +650,14 @@ static void test_groups(void)
     g_assert(tgm1->throttle_state == NULL);
     g_assert(tgm2->throttle_state == NULL);
     g_assert(tgm3->throttle_state == NULL);
+
+    g_free(tgm1);
+    g_free(tgm2);
+    g_free(tgm3);
+
+    blk_unref(blk1);
+    blk_unref(blk2);
+    blk_unref(blk3);
 }
 
 int main(int argc, char **argv)
-- 
2.11.0

[Qemu-devel] [PATCH v2 5/6] block: add iotest 191 for legacy throttling interface

[Manos Pitsidianakis]

Check that the implicit throttle filter driver node, used for
compatibility with the legacy throttling interface on the BlockBackend
level, works.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 tests/qemu-iotests/191     | 138 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/191.out |   5 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 144 insertions(+)
 create mode 100644 tests/qemu-iotests/191
 create mode 100644 tests/qemu-iotests/191.out

diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
new file mode 100644
index 0000000000..82fd0b2fe5
--- /dev/null
+++ b/tests/qemu-iotests/191
@@ -0,0 +1,138 @@
+#!/usr/bin/env python
+#
+# Tests that the legacy throttling interface using an implicit throttle filter
+# driver node works
+#
+# Copyright (C) 2017 Manos Pitsidianakis
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+
+class TestLegacyThrottling(iotests.QMPTestCase):
+    test_img = os.path.join(iotests.test_dir, "test.img")
+    target_img = os.path.join(iotests.test_dir, "target.img")
+    base_img = os.path.join(iotests.test_dir, "base.img")
+
+    def setUp(self):
+        iotests.qemu_img("create", "-f", iotests.imgfmt, self.base_img, "1G")
+        iotests.qemu_img("create", "-f", iotests.imgfmt, self.test_img, "-b", self.base_img)
+        iotests.qemu_io("-f", iotests.imgfmt, "-c", "write -P0x5d 1M 128M", self.test_img)
+        self.vm = iotests.VM().add_drive(self.test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.do_check_throttle_node(expect=True)
+        params = {"device": "drive0",
+                  "bps": 0,
+                  "bps_rd": 0,
+                  "bps_wr": 0,
+                  "iops": 0,
+                  "iops_rd": 0,
+                  "iops_wr": 0,
+                 }
+        """
+        This must remove the implicit throttle_node
+        """
+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
+                             **params)
+        self.do_check_throttle_node(expect=False)
+        self.vm.shutdown()
+
+    def do_test_job(self, cmd, **args):
+        params = {"device": "drive0",
+                  "bps": 1024,
+                  "bps_rd": 0,
+                  "bps_wr": 0,
+                  "iops": 0,
+                  "iops_rd": 0,
+                  "iops_wr": 0,
+                 }
+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
+                             **params)
+        self.assert_qmp(result, "return", {})
+        result = self.vm.qmp(cmd, **args)
+        self.assert_qmp(result, "return", {})
+        result = self.vm.qmp("query-block-jobs")
+        self.assert_qmp(result, "return[0]/device", "drive0")
+
+    def do_check_throttle_node(self, expect):
+        result = self.vm.qmp("query-named-block-nodes")
+        for r in result["return"]:
+            if r["drv"] == "throttle":
+                    self.assertTrue(expect)
+                    return
+        if expect:
+            """ throttle_node missing! """
+            self.assertTrue(False)
+
+    def do_check_params(self, file):
+        result = self.vm.qmp("query-block")
+        self.assert_qmp(result, "return[0]/inserted/bps", 1024)
+        self.assert_qmp(result, "return[0]/inserted/drv", iotests.imgfmt)
+        self.assert_qmp(result, "return[0]/inserted/file", file)
+
+    """
+    Check that query-block reports the correct throttling parameters while
+    ignoring the implicit throttle node.
+    """
+    def test_query_block(self):
+        params = {"device": "drive0",
+                  "bps": 1024,
+                  "bps_rd": 0,
+                  "bps_wr": 0,
+                  "iops": 0,
+                  "iops_rd": 0,
+                  "iops_wr": 0,
+                 }
+        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
+                             **params)
+        self.assert_qmp(result, "return", {})
+        self.do_check_params(file=self.test_img)
+
+    """
+    Check that the throttle node doesn't get removed by block jobs, and that
+    query-block reports the correct throttling parameters
+    """
+    def test_drive_mirror(self):
+        self.do_test_job("drive-mirror", device="drive0",
+                          target=self.target_img,
+                          sync="full")
+        self.vm.event_wait("BLOCK_JOB_READY")
+        self.vm.qmp("block-job-complete", device="drive0")
+        """
+        query-block should report `target_img` now
+        """
+        self.do_check_params(file=self.target_img)
+
+    def test_drive_backup(self):
+        self.do_test_job("drive-backup", device="drive0",
+                          target=self.target_img,
+                          sync="full")
+        self.vm.event_wait("BLOCK_JOB_COMPLETED")
+        self.do_check_params(file=self.test_img)
+
+    def test_block_commit(self):
+        self.do_test_job("block-commit", device="drive0")
+        self.vm.event_wait("BLOCK_JOB_READY")
+        self.vm.qmp("block-job-complete", device="drive0")
+        """
+        query-block should report the backing file `base_img` now
+        """
+        self.do_check_params(file=self.base_img)
+
+if __name__ == "__main__":
+    iotests.main(supported_fmts=["qcow2"])
diff --git a/tests/qemu-iotests/191.out b/tests/qemu-iotests/191.out
new file mode 100644
index 0000000000..89968f35d7
--- /dev/null
+++ b/tests/qemu-iotests/191.out
@@ -0,0 +1,5 @@
+....
+----------------------------------------------------------------------
+Ran 4 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6e6afb8ba9..1b4221f26d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -187,3 +187,4 @@
 188 rw auto quick
 189 rw auto
 190 rw auto quick
+191 rw auto quick
-- 
2.11.0

[Qemu-devel] [PATCH v2 6/6] block: remove BlockBackendPublic

[Manos Pitsidianakis]

All BlockBackend level throttling (via the implicit throttle filter node) is
done in block/block-backend.c and block/throttle-groups.c doesn't know
about BlockBackends anymore. Since BlockBackendPublic is not needed anymore, remove it.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/block-backend.c          | 43 +++++++++++++++++++-----------------------
 block/qapi.c                   |  4 ++--
 blockdev.c                     |  6 +++---
 include/sysemu/block-backend.h | 12 +-----------
 4 files changed, 25 insertions(+), 40 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 61983b7393..05f6e67222 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -36,7 +36,10 @@ struct BlockBackend {
     DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
     QTAILQ_ENTRY(BlockBackend) link;         /* for block_backends */
     QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */
-    BlockBackendPublic public;
+
+    /* implicit throttle filter node for backwards compatibility with legacy
+     * throttling commands */
+    BlockDriverState *throttle_node;
 
     void *dev;                  /* attached device model, if any */
     bool legacy_dev;            /* true if dev is not a DeviceState */
@@ -283,7 +286,7 @@ static void blk_delete(BlockBackend *blk)
     assert(!blk->refcnt);
     assert(!blk->name);
     assert(!blk->dev);
-    if (blk->public.throttle_node) {
+    if (blk->throttle_node) {
         blk_io_limits_disable(blk);
     }
     if (blk->root) {
@@ -574,19 +577,11 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 }
 
 /*
- * Returns a pointer to the publicly accessible fields of @blk.
+ * Returns the throttle_node field of @blk.
  */
-BlockBackendPublic *blk_get_public(BlockBackend *blk)
+BlockDriverState *blk_get_throttle_node(BlockBackend *blk)
 {
-    return &blk->public;
-}
-
-/*
- * Returns a BlockBackend given the associated @public fields.
- */
-BlockBackend *blk_by_public(BlockBackendPublic *public)
-{
-    return container_of(public, BlockBackend, public);
+    return blk->throttle_node;
 }
 
 /*
@@ -1879,15 +1874,15 @@ int blk_commit_all(void)
 /* throttling disk I/O limits */
 void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
 {
-    assert(blk->public.throttle_node);
-    throttle_group_config(throttle_get_tgm(blk->public.throttle_node), cfg);
+    assert(blk->throttle_node);
+    throttle_group_config(throttle_get_tgm(blk->throttle_node), cfg);
 }
 
 void blk_io_limits_disable(BlockBackend *blk)
 {
     BlockDriverState *bs, *throttle_node;
 
-    throttle_node = blk_get_public(blk)->throttle_node;
+    throttle_node = blk->throttle_node;
 
     assert(throttle_node);
 
@@ -1903,7 +1898,7 @@ void blk_io_limits_disable(BlockBackend *blk)
      * blk, at this point it might have more than one parent, so use
      * bdrv_replace_node(). This destroys throttle_node */
     bdrv_replace_node(throttle_node, bs, &error_abort);
-    blk_get_public(blk)->throttle_node = NULL;
+    blk->throttle_node = NULL;
 
     bdrv_unref(bs);
     bdrv_drained_end(bs);
@@ -1944,7 +1939,7 @@ void blk_io_limits_enable(BlockBackend *blk, const char *group,  Error **errp)
 
 end:
     bdrv_drained_end(bs);
-    blk_get_public(blk)->throttle_node = throttle_node;
+    blk->throttle_node = throttle_node;
 }
 
 void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error **errp)
@@ -1952,11 +1947,11 @@ void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error **er
     ThrottleGroupMember *tgm;
 
     /* this BB is not part of any group */
-    if (!blk->public.throttle_node) {
+    if (!blk->throttle_node) {
         return;
     }
 
-    tgm = throttle_get_tgm(blk->public.throttle_node);
+    tgm = throttle_get_tgm(blk->throttle_node);
     /* this BB is a part of the same group than the one we want */
     if (!g_strcmp0(throttle_group_get_name(tgm),
                    group)) {
@@ -1981,8 +1976,8 @@ static void blk_root_drained_begin(BdrvChild *child)
 
     /* Note that blk->root may not be accessible here yet if we are just
      * attaching to a BlockDriverState that is drained. Use child instead. */
-    if (blk->public.throttle_node) {
-        tgm = throttle_get_tgm(blk->public.throttle_node);
+    if (blk->throttle_node) {
+        tgm = throttle_get_tgm(blk->throttle_node);
         if (atomic_fetch_inc(&tgm->io_limits_disabled) == 0) {
             throttle_group_restart_tgm(tgm);
         }
@@ -1995,8 +1990,8 @@ static void blk_root_drained_end(BdrvChild *child)
     BlockBackend *blk = child->opaque;
     assert(blk->quiesce_counter);
 
-    if (blk->public.throttle_node) {
-        tgm = throttle_get_tgm(blk->public.throttle_node);
+    if (blk->throttle_node) {
+        tgm = throttle_get_tgm(blk->throttle_node);
         assert(tgm->io_limits_disabled);
         atomic_dec(&tgm->io_limits_disabled);
     }
diff --git a/block/qapi.c b/block/qapi.c
index ab55db7134..2be44a6758 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -66,9 +66,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 
     info->detect_zeroes = bs->detect_zeroes;
 
-    if (blk && blk_get_public(blk)->throttle_node) {
+    if (blk && blk_get_throttle_node(blk)) {
         ThrottleConfig cfg;
-        BlockDriverState *throttle_node = blk_get_public(blk)->throttle_node;
+        BlockDriverState *throttle_node = blk_get_throttle_node(blk);
         ThrottleGroupMember *tgm = throttle_get_tgm(throttle_node);
 
         throttle_group_get_config(tgm, &cfg);
diff --git a/blockdev.c b/blockdev.c
index 20e5513f87..5c11c245b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2714,7 +2714,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
     if (throttle_enabled(&cfg)) {
         /* Enable I/O limits if they're not enabled yet, otherwise
          * just update the throttling group. */
-        if (!blk_get_public(blk)->throttle_node) {
+        if (!blk_get_throttle_node(blk)) {
             blk_io_limits_enable(blk, arg->has_group ? arg->group :
                                  arg->has_device ? arg->device : arg->id,
                                  &local_err);
@@ -2733,10 +2733,10 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
             }
         }
 
-        throttle_node = blk_get_public(blk)->throttle_node;
+        throttle_node = blk_get_throttle_node(blk);
         tgm = throttle_get_tgm(throttle_node);
         throttle_group_config(tgm, &cfg);
-    } else if (blk_get_public(blk)->throttle_node) {
+    } else if (blk_get_throttle_node(blk)) {
         /*
          * If all throttling settings are set to 0, disable I/O limits
          * by deleting the legacy throttle node
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 4a7ca53685..a05d75fa5f 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -68,14 +68,6 @@ typedef struct BlockDevOps {
     void (*drained_end)(void *opaque);
 } BlockDevOps;
 
-/* This struct is embedded in (the private) BlockBackend struct and contains
- * fields that must be public. This is in particular for QLIST_ENTRY() and
- * friends so that BlockBackends can be kept in lists outside block-backend.c
- * */
-typedef struct BlockBackendPublic {
-    BlockDriverState *throttle_node;
-} BlockBackendPublic;
-
 BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm);
 BlockBackend *blk_new_open(const char *filename, const char *reference,
                            QDict *options, int flags, Error **errp);
@@ -90,9 +82,7 @@ BlockBackend *blk_all_next(BlockBackend *blk);
 bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp);
 void monitor_remove_blk(BlockBackend *blk);
 
-BlockBackendPublic *blk_get_public(BlockBackend *blk);
-BlockBackend *blk_by_public(BlockBackendPublic *public);
-
+BlockDriverState *blk_get_throttle_node(BlockBackend *blk);
 BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
 int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 0/6] block: remove legacy I/O throttling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

On 08/09/2017 09:02 AM, Manos Pitsidianakis wrote:
> This series depends on my other series 'add throttle block driver filter'
> currently on v4.

More precisely, for patchew:
Based-on: <20170809100734.17540-1-el13635@mail.ntua.gr>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 1/6] block: skip implicit nodes in snapshots, blockjobs

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:51 PM CEST, Manos Pitsidianakis wrote:
> @@ -2988,6 +3004,9 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>          return;
>      }
>  
> +    /* Skip implicit filter nodes */
> +    bs = bdrv_get_first_explicit(bs);
> +
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);

This change works here because right now the only implicit node that we
could have in practice is the throttle node, but I wonder if this is
good enough for any kind of implicit node in general.

> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
> +{
> +    BdrvChild *child = QLIST_FIRST(&bs->children);
> +    assert(child && !QLIST_NEXT(child, next));
> +    return child->bs;
> +}

This aborts if the bs has a number of children != 1. That's not
something that I would expect from a function named like that.

Considering that you're only using it in bdrv_get_first_explicit(), why
don't you simply move the code there?

The other question is of course whether we can rely for the future on
the assumption that implicit nodes only have one children.

Berto

Re: [Qemu-devel] [PATCH v2 1/6] block: skip implicit nodes in snapshots, blockjobs

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

On Tue, Aug 15, 2017 at 03:24:38PM +0200, Alberto Garcia wrote:
>On Wed 09 Aug 2017 04:02:51 PM CEST, Manos Pitsidianakis wrote:
>> @@ -2988,6 +3004,9 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>>          return;
>>      }
>>
>> +    /* Skip implicit filter nodes */
>> +    bs = bdrv_get_first_explicit(bs);
>> +
>>      aio_context = bdrv_get_aio_context(bs);
>>      aio_context_acquire(aio_context);
>
>This change works here because right now the only implicit node that we
>could have in practice is the throttle node, but I wonder if this is
>good enough for any kind of implicit node in general.

It does feel a bit sloppy but block jobs should work the same with
implicit nodes and without, so all we can do is ignore them.

>
>> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
>> +{
>> +    BdrvChild *child = QLIST_FIRST(&bs->children);
>> +    assert(child && !QLIST_NEXT(child, next));
>> +    return child->bs;
>> +}
>
>This aborts if the bs has a number of children != 1. That's not
>something that I would expect from a function named like that.
>
>Considering that you're only using it in bdrv_get_first_explicit(), why
>don't you simply move the code there?

It felt useful to have a function that also returns file->bs (we have 
backing_bs() already) instead of doing backing_bs(bs) ? : file_bs(bs)
>
>The other question is of course whether we can rely for the future on
>the assumption that implicit nodes only have one children.

This is only to get either bs->backing or bs->file (we can't have both
anyway). I wanted to document it with something like "Used for filter 
drivers with only one child" which fits with what implicit nodes we have 
so far (mirror, commit, throttle and in the future backup)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 2/6] block: add options parameter to bdrv_new_open_driver()

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:52 PM CEST, Manos Pitsidianakis wrote:
> Allow passing a QDict *options parameter to bdrv_new_open_driver() so
> that it can be used if a driver needs it upon creation. The previous
> behaviour (empty bs->options and bs->explicit_options) remains when
> options is NULL.
>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 1/6] block: skip implicit nodes in snapshots, blockjobs

[Alberto Garcia]

On Tue 15 Aug 2017 03:50:12 PM CEST, Manos Pitsidianakis wrote:
>>> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
>>> +{
>>> +    BdrvChild *child = QLIST_FIRST(&bs->children);
>>> +    assert(child && !QLIST_NEXT(child, next));
>>> +    return child->bs;
>>> +}
>>
>>This aborts if the bs has a number of children != 1. That's not
>>something that I would expect from a function named like that.
>>
>>Considering that you're only using it in bdrv_get_first_explicit(),
>>why don't you simply move the code there?
>
> It felt useful to have a function that also returns file->bs (we have 
> backing_bs() already) instead of doing backing_bs(bs) ? : file_bs(bs)
>>
>>The other question is of course whether we can rely for the future on
>>the assumption that implicit nodes only have one children.
>
> This is only to get either bs->backing or bs->file (we can't have both
> anyway).

You can have other children (see Quorum for example).

Berto

Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:53 PM CEST, Manos Pitsidianakis wrote:
> @@ -622,20 +622,14 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>          return NULL;
>      }
>  
> -    if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
> -        job_id = bdrv_get_device_name(bs);
> -        if (!*job_id) {
> -            error_setg(errp, "An explicit job ID is required for this node");
> -            return NULL;
> -        }
> -    }
> -
> -    if (job_id) {
> -        if (flags & BLOCK_JOB_INTERNAL) {
> +    if (flags & BLOCK_JOB_INTERNAL) {
> +        if (job_id) {
>              error_setg(errp, "Cannot specify job ID for internal block job");
>              return NULL;
>          }

When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...

> -
> +    } else {
> +        /* Require job-id. */
> +        assert(job_id);
>          if (!id_wellformed(job_id)) {
>              error_setg(errp, "Invalid job ID '%s'", job_id);
>              return NULL;
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index f13ad05c0d..ff906808a6 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -112,8 +112,7 @@ struct BlockJobDriver {
>  
>  /**
>   * block_job_create:
> - * @job_id: The id of the newly-created job, or %NULL to have one
> - * generated automatically.
> + * @job_id: The id of the newly-created job, must be non %NULL.

...but here you say that it must not be NULL.

And since job_id can be NULL in some cases I think I'd replace the
assert(job_id) that you added to block_job_create() with a normal
pointer check + error_setg().

> @@ -93,9 +93,6 @@ static void test_job_ids(void)
>      blk[1] = create_blk("drive1");
>      blk[2] = create_blk("drive2");
>  
> -    /* No job ID provided and the block backend has no name */
> -    job[0] = do_test_id(blk[0], NULL, false);
> -

If you decide to handle NULL ids by returning and error instead of
asserting, we should keep a couple of tests for that scenario.

Berto

Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

On Mon, Aug 21, 2017 at 05:05:30PM +0200, Alberto Garcia wrote:
>On Wed 09 Aug 2017 04:02:53 PM CEST, Manos Pitsidianakis wrote:
>> @@ -622,20 +622,14 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>>          return NULL;
>>      }
>>
>> -    if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
>> -        job_id = bdrv_get_device_name(bs);
>> -        if (!*job_id) {
>> -            error_setg(errp, "An explicit job ID is required for this node");
>> -            return NULL;
>> -        }
>> -    }
>> -
>> -    if (job_id) {
>> -        if (flags & BLOCK_JOB_INTERNAL) {
>> +    if (flags & BLOCK_JOB_INTERNAL) {
>> +        if (job_id) {
>>              error_setg(errp, "Cannot specify job ID for internal block job");
>>              return NULL;
>>          }
>
>When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
>
>> -
>> +    } else {
>> +        /* Require job-id. */
>> +        assert(job_id);
>>          if (!id_wellformed(job_id)) {
>>              error_setg(errp, "Invalid job ID '%s'", job_id);
>>              return NULL;
>> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
>> index f13ad05c0d..ff906808a6 100644
>> --- a/include/block/blockjob_int.h
>> +++ b/include/block/blockjob_int.h
>> @@ -112,8 +112,7 @@ struct BlockJobDriver {
>>
>>  /**
>>   * block_job_create:
>> - * @job_id: The id of the newly-created job, or %NULL to have one
>> - * generated automatically.
>> + * @job_id: The id of the newly-created job, must be non %NULL.
>
>...but here you say that it must not be NULL.
>
>And since job_id can be NULL in some cases I think I'd replace the
>assert(job_id) that you added to block_job_create() with a normal
>pointer check + error_setg().
>
>> @@ -93,9 +93,6 @@ static void test_job_ids(void)
>>      blk[1] = create_blk("drive1");
>>      blk[2] = create_blk("drive2");
>>
>> -    /* No job ID provided and the block backend has no name */
>> -    job[0] = do_test_id(blk[0], NULL, false);
>> -
>
>If you decide to handle NULL ids by returning and error instead of
>asserting, we should keep a couple of tests for that scenario.
>
>Berto
>

Thanks, I will change that. What cases are you thinking of besides 
internal jobs though? And should documentation of block_job_create 
reflect that internal jobs have job_id == NULL?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name

[Alberto Garcia]

On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
>>> -    if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
>>> -        job_id = bdrv_get_device_name(bs);
>>> -        if (!*job_id) {
>>> -            error_setg(errp, "An explicit job ID is required for this node");
>>> -            return NULL;
>>> -        }
>>> -    }
>>> -
>>> -    if (job_id) {
>>> -        if (flags & BLOCK_JOB_INTERNAL) {
>>> +    if (flags & BLOCK_JOB_INTERNAL) {
>>> +        if (job_id) {
>>>              error_setg(errp, "Cannot specify job ID for internal block job");
>>>              return NULL;
>>>          }
>>
>>When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
>>
>>> -
>>> +    } else {
>>> +        /* Require job-id. */
>>> +        assert(job_id);
>>>          if (!id_wellformed(job_id)) {
>>>              error_setg(errp, "Invalid job ID '%s'", job_id);
>>>              return NULL;
>>> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
>>> index f13ad05c0d..ff906808a6 100644
>>> --- a/include/block/blockjob_int.h
>>> +++ b/include/block/blockjob_int.h
>>> @@ -112,8 +112,7 @@ struct BlockJobDriver {
>>>
>>>  /**
>>>   * block_job_create:
>>> - * @job_id: The id of the newly-created job, or %NULL to have one
>>> - * generated automatically.
>>> + * @job_id: The id of the newly-created job, must be non %NULL.
>>
>>...but here you say that it must not be NULL.
>>
>>And since job_id can be NULL in some cases I think I'd replace the
>>assert(job_id) that you added to block_job_create() with a normal
>>pointer check + error_setg().
>>
>>> @@ -93,9 +93,6 @@ static void test_job_ids(void)
>>>      blk[1] = create_blk("drive1");
>>>      blk[2] = create_blk("drive2");
>>>
>>> -    /* No job ID provided and the block backend has no name */
>>> -    job[0] = do_test_id(blk[0], NULL, false);
>>> -
>>
>>If you decide to handle NULL ids by returning and error instead of
>>asserting, we should keep a couple of tests for that scenario.
>>
>>Berto
>>
>
> Thanks, I will change that. What cases are you thinking of besides
> internal jobs though?

Both cases (external job with a NULL id and internal job with non-NULL
id), checking that block_job_create() does return an error.

> And should documentation of block_job_create reflect that internal
> jobs have job_id == NULL?

Yes, it should document the restrictions of the job_id parameter.

Berto

Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]

On Tue, Aug 22, 2017 at 11:57:28AM +0200, Alberto Garcia wrote:
>On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
>>>> -    if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
>>>> -        job_id = bdrv_get_device_name(bs);
>>>> -        if (!*job_id) {
>>>> -            error_setg(errp, "An explicit job ID is required for this node");
>>>> -            return NULL;
>>>> -        }
>>>> -    }
>>>> -
>>>> -    if (job_id) {
>>>> -        if (flags & BLOCK_JOB_INTERNAL) {
>>>> +    if (flags & BLOCK_JOB_INTERNAL) {
>>>> +        if (job_id) {
>>>>              error_setg(errp, "Cannot specify job ID for internal block job");
>>>>              return NULL;
>>>>          }
>>>
>>>When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
>>>
>>>> -
>>>> +    } else {
>>>> +        /* Require job-id. */
>>>> +        assert(job_id);
>>>>          if (!id_wellformed(job_id)) {
>>>>              error_setg(errp, "Invalid job ID '%s'", job_id);
>>>>              return NULL;
>>>> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
>>>> index f13ad05c0d..ff906808a6 100644
>>>> --- a/include/block/blockjob_int.h
>>>> +++ b/include/block/blockjob_int.h
>>>> @@ -112,8 +112,7 @@ struct BlockJobDriver {
>>>>
>>>>  /**
>>>>   * block_job_create:
>>>> - * @job_id: The id of the newly-created job, or %NULL to have one
>>>> - * generated automatically.
>>>> + * @job_id: The id of the newly-created job, must be non %NULL.
>>>
>>>...but here you say that it must not be NULL.
>>>
>>>And since job_id can be NULL in some cases I think I'd replace the
>>>assert(job_id) that you added to block_job_create() with a normal
>>>pointer check + error_setg().
>>>
>>>> @@ -93,9 +93,6 @@ static void test_job_ids(void)
>>>>      blk[1] = create_blk("drive1");
>>>>      blk[2] = create_blk("drive2");
>>>>
>>>> -    /* No job ID provided and the block backend has no name */
>>>> -    job[0] = do_test_id(blk[0], NULL, false);
>>>> -
>>>
>>>If you decide to handle NULL ids by returning and error instead of
>>>asserting, we should keep a couple of tests for that scenario.
>>>
>>>Berto
>>>
>>
>> Thanks, I will change that. What cases are you thinking of besides
>> internal jobs though?
>
>Both cases (external job with a NULL id and internal job with non-NULL
>id), checking that block_job_create() does return an error.

I thought we handled the external job case by requiring job_id is set 
before calling block_job_create(). I will check my patch again.

>
>> And should documentation of block_job_create reflect that internal
>> jobs have job_id == NULL?
>
>Yes, it should document the restrictions of the job_id parameter.
>
>Berto
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name

[Alberto Garcia]

On Tue 22 Aug 2017 12:31:26 PM CEST, Manos Pitsidianakis wrote:
> On Tue, Aug 22, 2017 at 11:57:28AM +0200, Alberto Garcia wrote:
>>On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
>>>>> -    if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
>>>>> -        job_id = bdrv_get_device_name(bs);
>>>>> -        if (!*job_id) {
>>>>> -            error_setg(errp, "An explicit job ID is required for this node");
>>>>> -            return NULL;
>>>>> -        }
>>>>> -    }
>>>>> -
>>>>> -    if (job_id) {
>>>>> -        if (flags & BLOCK_JOB_INTERNAL) {
>>>>> +    if (flags & BLOCK_JOB_INTERNAL) {
>>>>> +        if (job_id) {
>>>>>              error_setg(errp, "Cannot specify job ID for internal block job");
>>>>>              return NULL;
>>>>>          }
>>>>
>>>>When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
>>>>
>>>>> -
>>>>> +    } else {
>>>>> +        /* Require job-id. */
>>>>> +        assert(job_id);
>>>>>          if (!id_wellformed(job_id)) {
>>>>>              error_setg(errp, "Invalid job ID '%s'", job_id);
>>>>>              return NULL;
>>>>> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
>>>>> index f13ad05c0d..ff906808a6 100644
>>>>> --- a/include/block/blockjob_int.h
>>>>> +++ b/include/block/blockjob_int.h
>>>>> @@ -112,8 +112,7 @@ struct BlockJobDriver {
>>>>>
>>>>>  /**
>>>>>   * block_job_create:
>>>>> - * @job_id: The id of the newly-created job, or %NULL to have one
>>>>> - * generated automatically.
>>>>> + * @job_id: The id of the newly-created job, must be non %NULL.
>>>>
>>>>...but here you say that it must not be NULL.
>>>>
>>>>And since job_id can be NULL in some cases I think I'd replace the
>>>>assert(job_id) that you added to block_job_create() with a normal
>>>>pointer check + error_setg().
>>>>
>>>>> @@ -93,9 +93,6 @@ static void test_job_ids(void)
>>>>>      blk[1] = create_blk("drive1");
>>>>>      blk[2] = create_blk("drive2");
>>>>>
>>>>> -    /* No job ID provided and the block backend has no name */
>>>>> -    job[0] = do_test_id(blk[0], NULL, false);
>>>>> -
>>>>
>>>>If you decide to handle NULL ids by returning and error instead of
>>>>asserting, we should keep a couple of tests for that scenario.
>>>>
>>>>Berto
>>>>
>>>
>>> Thanks, I will change that. What cases are you thinking of besides
>>> internal jobs though?
>>
>>Both cases (external job with a NULL id and internal job with non-NULL
>>id), checking that block_job_create() does return an error.
>
> I thought we handled the external job case by requiring job_id is set 
> before calling block_job_create(). I will check my patch again.

Yes, that appears to be checked correctly, I don't think you can call
block_job_create() directly with a NULL id for external block job.

But I also don't see how you can create an internal job with a non-NULL
id, so you could assert() there as well.

Either assert on both places or use error_setg() in both places. I think
I prefer the latter.

Berto

Re: [Qemu-devel] [PATCH v2 4/6] block: remove legacy I/O throttling

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:54 PM CEST, Manos Pitsidianakis wrote:
> +void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error **errp)
>  {
> +    ThrottleGroupMember *tgm;
> +
>      /* this BB is not part of any group */
> -    if (!blk->public.throttle_group_member.throttle_state) {
> +    if (!blk->public.throttle_node) {
>          return;
>      }
>  
> +    tgm = throttle_get_tgm(blk->public.throttle_node);
>      /* this BB is a part of the same group than the one we want */
> -    if (!g_strcmp0(throttle_group_get_name(&blk->public.throttle_group_member),
> -                group)) {
> +    if (!g_strcmp0(throttle_group_get_name(tgm),
> +                   group)) {

You can join these two lines, no need to have 'group' in its own line.

> @@ -2629,6 +2636,9 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
>      BlockDriverState *bs;
>      BlockBackend *blk;
>      AioContext *aio_context;
> +    BlockDriverState *throttle_node = NULL;
> +    ThrottleGroupMember *tgm;
> +    Error *local_err = NULL;
>  
>      blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
>                        arg->has_id ? arg->id : NULL,
> @@ -2704,18 +2714,33 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
>      if (throttle_enabled(&cfg)) {
>          /* Enable I/O limits if they're not enabled yet, otherwise
>           * just update the throttling group. */
> -        if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
> -            blk_io_limits_enable(blk,
> -                                 arg->has_group ? arg->group :
> -                                 arg->has_device ? arg->device :
> -                                 arg->id);
> -        } else if (arg->has_group) {
> -            blk_io_limits_update_group(blk, arg->group);
> +        if (!blk_get_public(blk)->throttle_node) {
> +            blk_io_limits_enable(blk, arg->has_group ? arg->group :
> +                                 arg->has_device ? arg->device : arg->id,
> +                                 &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                goto out;
> +            }
>          }
> -        /* Set the new throttling configuration */
> -        blk_set_io_limits(blk, &cfg);
> -    } else if (blk_get_public(blk)->throttle_group_member.throttle_state) {
> -        /* If all throttling settings are set to 0, disable I/O limits */
> +
> +        if (arg->has_group) {
> +            /* move throttle node membership to arg->group */
> +            blk_io_limits_update_group(blk, arg->group, &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                goto out;
> +            }
> +        }

This used to be

   if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
      /* Enable throttling */
   } else if (arg->has_group) {
      /* Update group */
   }

but now it is

   if (!blk_get_throttle_node(blk)) {
      /* Enable throttling */
   }
   if (arg->has_group) {
      /* Update group */
   }

Now if I/O limits are not set then the code sets the same group twice.

Everything else looks fine.

Berto

Re: [Qemu-devel] [PATCH v2 6/6] block: remove BlockBackendPublic

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:56 PM CEST, Manos Pitsidianakis wrote:
> All BlockBackend level throttling (via the implicit throttle filter node) is
> done in block/block-backend.c and block/throttle-groups.c doesn't know
> about BlockBackends anymore. Since BlockBackendPublic is not needed anymore, remove it.
>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 5/6] block: add iotest 191 for legacy throttling interface

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:55 PM CEST, Manos Pitsidianakis wrote:
> Check that the implicit throttle filter driver node, used for
> compatibility with the legacy throttling interface on the BlockBackend
> level, works.
>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto