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

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

[Manos Pitsidianakis]

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

Based-on: <20170825132028.6184-1-el13635@mail.ntua.gr>

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. 

v3:
  fix suggestions by berto
  new commit: remove throttle-groups list

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 (7):
  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/throttle-groups.c: remove throttle-groups list
  block: remove BlockBackendPublic
  qemu-iotests: add 191 for legacy throttling interface

 include/block/block.h           |   2 +-
 include/block/block_int.h       |  17 +++++
 include/block/blockjob_int.h    |   4 +-
 include/block/throttle-groups.h |   2 +
 include/sysemu/block-backend.h  |  16 +---
 block.c                         |  26 ++++++-
 block/block-backend.c           | 158 ++++++++++++++++++++++++----------------
 block/commit.c                  |   4 +-
 block/mirror.c                  |   2 +-
 block/qapi.c                    |  24 +++---
 block/throttle-groups.c         | 145 ++++++++++++++++++++----------------
 block/throttle.c                |   8 ++
 block/vvfat.c                   |   2 +-
 blockdev.c                      | 136 ++++++++++++++++++++++++++++++----
 blockjob.c                      |  19 ++---
 tests/test-blockjob.c           |   9 +--
 tests/test-throttle.c           |  22 ++++--
 tests/qemu-iotests/191          | 138 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/191.out      |   5 ++
 tests/qemu-iotests/group        |   1 +
 20 files changed, 538 insertions(+), 202 deletions(-)
 create mode 100644 tests/qemu-iotests/191
 create mode 100644 tests/qemu-iotests/191.out

-- 
2.11.0

[Qemu-devel] [PATCH v3 1/7] 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.

All implicit filters will have either bs->file or bs->backing set. This
is a needed assumption so we can know which direction we will skip down
the graph.

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

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7571c0aaaf..9e0f70e055 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -699,6 +699,21 @@ static inline BlockDriverState *backing_bs(BlockDriverState *bs)
     return bs->backing ? bs->backing->bs : NULL;
 }
 
+static inline BlockDriverState *file_bs(BlockDriverState *bs)
+{
+    return bs->file ? bs->file->bs : NULL;
+}
+
+/* This is a convenience function to get either bs->file->bs or
+ * bs->backing->bs * */
+static inline BlockDriverState *child_bs(BlockDriverState *bs)
+{
+    BlockDriverState *backing = backing_bs(bs);
+    BlockDriverState *file = file_bs(bs);
+    assert(!(file && backing));
+    return backing ?: file;
+}
+
 
 /* Essential block drivers which must always be statically linked into qemu, and
  * which therefore can be accessed without using bdrv_find_format() */
@@ -980,4 +995,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 */
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);
 
-- 
2.11.0

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

[Alberto Garcia]

On Fri 25 Aug 2017 03:23:26 PM CEST, Manos Pitsidianakis wrote:

> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
> +{
> +    BlockDriverState *backing = backing_bs(bs);
> +    BlockDriverState *file = file_bs(bs);
> +    assert(!(file && backing));
> +    return backing ?: file;
> +}

I'm still not sure how useful it is to have a separate function for
this, instead of putting it inside bdrv_get_first_explicit(), but anyway

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

Berto

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

[Kevin Wolf]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> 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.
> 
> All implicit filters will have either bs->file or bs->backing set. This
> is a needed assumption so we can know which direction we will skip down
> the graph.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  include/block/block_int.h | 17 +++++++++++++++++
>  block.c                   | 10 ++++++++++
>  block/qapi.c              | 14 +++++---------
>  blockdev.c                | 34 ++++++++++++++++++++++++++++++++++
>  4 files changed, 66 insertions(+), 9 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 7571c0aaaf..9e0f70e055 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -699,6 +699,21 @@ static inline BlockDriverState *backing_bs(BlockDriverState *bs)
>      return bs->backing ? bs->backing->bs : NULL;
>  }
>  
> +static inline BlockDriverState *file_bs(BlockDriverState *bs)
> +{
> +    return bs->file ? bs->file->bs : NULL;
> +}
> +
> +/* This is a convenience function to get either bs->file->bs or
> + * bs->backing->bs * */
> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
> +{
> +    BlockDriverState *backing = backing_bs(bs);
> +    BlockDriverState *file = file_bs(bs);
> +    assert(!(file && backing));
> +    return backing ?: file;
> +}

I still think you should just get the only child and not restrict it to
bs->file or bs->backing.

child = QLIST_FIRST(&bs->children);
assert(!QLIST_NEXT(child, next));
return child->bs;

file_bs() isn't needed at all then. And Berto is probably
right that this is simple enough that it can just be inlined in
bdrv_get_first_explicit().

> 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);
> +
> [...]

Most, if not all, of the commands that you change accept both node names
and BlockBackend names, which function as aliases for their root node.
If a node name is given, the user claims that they know the graph
structure and it is supposed to be exact. We should not skip the
implicit filter node then. (For example, for creating an external
snapshot, creating the snapshot above the filter is meaningful. It
requires that the user knows about the filter node, but by using a node
name they tell us that they do.)

Please make sure that your qemu-iotests case covers all of the cases for
which you add a bdrv_get_first_explicit() call in this patch. So far, we
seem to be completely missing:

* Create external snapshot
* Create internal snapshot
* Delete internal snapshot
* blockdev-backup
* blockdev-mirror

All of them should cover the case where a BlockBackend name is given as
well as the cases with a node name.

Kevin

[Qemu-devel] [PATCH v3 2/7] 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.

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

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);
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);
-- 
2.11.0

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

[Kevin Wolf]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> 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.
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v3 3/7] 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>
---
 include/block/blockjob_int.h |  4 +--
 blockdev.c                   | 65 +++++++++++++++++++++++++++++++++++++++-----
 blockjob.c                   | 19 ++++++-------
 tests/test-blockjob.c        |  9 +++---
 4 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05c0d..a4ab7f3d59 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -112,8 +112,8 @@ 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 for external
+ *          jobs and %NULL for internal jobs.
  * @job_type: The class object for the newly-created job.
  * @bs: The block
  * @perm, @shared_perm: Permissions to request for @bs
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..1231be5ce3 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -622,20 +622,17 @@ 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. */
+        if (!job_id) {
+            error_setg(errp, "A job ID must be specified");
+            return NULL;
+        }
         if (!id_wellformed(job_id)) {
             error_setg(errp, "Invalid job ID '%s'", job_id);
             return NULL;
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 23bdf1a932..fc41ea2147 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -93,7 +93,7 @@ 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 */
+    /* No job ID provided */
     job[0] = do_test_id(blk[0], NULL, false);
 
     /* These are all invalid job IDs */
@@ -119,16 +119,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

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

[Alberto Garcia]

On Fri 25 Aug 2017 03:23:28 PM CEST, Manos Pitsidianakis wrote:
> 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.

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

Berto

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

[Kevin Wolf]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v3 4/7] 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>
---
 include/block/throttle-groups.h |   1 +
 include/sysemu/block-backend.h  |   6 +-
 block/block-backend.c           | 134 +++++++++++++++++++++++++---------------
 block/qapi.c                    |  10 +--
 block/throttle.c                |   8 +++
 blockdev.c                      |  37 ++++++++---
 tests/test-throttle.c           |  19 +++---
 7 files changed, 140 insertions(+), 75 deletions(-)

diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index e2fd0513c4..8493540766 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -81,5 +81,6 @@ void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
  * mutex.
  */
 bool throttle_group_exists(const char *name);
+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/block/block-backend.c b/block/block-backend.c
index c51fb8c8aa..693ad27fc9 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"
@@ -319,7 +320,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) {
@@ -634,13 +635,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);
 
@@ -661,12 +656,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;
 }
 
@@ -1024,13 +1013,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;
@@ -1051,11 +1033,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;
@@ -1723,13 +1700,8 @@ 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);
-            throttle_group_attach_aio_context(tgm, new_context);
-        }
         bdrv_set_aio_context(bs, new_context);
     }
 }
@@ -1948,45 +1920,101 @@ 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;
+    ThrottleState *ts;
+
+    bdrv_drained_begin(bs);
+
+    /* Force creation of group in case it doesn't exist */
+    ts = throttle_group_incref(group);
+    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:
+    throttle_group_unref(ts);
+    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) {
@@ -1997,19 +2025,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 5ee1e2a7ca..7804ded709 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -35,6 +35,14 @@ static QemuOptsList throttle_opts = {
     },
 };
 
+static BlockDriver bdrv_throttle;
+
+ThrottleGroupMember *throttle_get_tgm(BlockDriverState *bs)
+{
+    assert(bs->drv == &bdrv_throttle);
+    return (ThrottleGroupMember *)bs->opaque;
+}
+
 static int throttle_configure_tgm(BlockDriverState *bs,
                                   ThrottleGroupMember *tgm,
                                   QDict *options, Error **errp)
diff --git a/blockdev.c b/blockdev.c
index 6ffa5b0b04..3f76eed2aa 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,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
     BlockDriverState *bs;
     BlockBackend *blk;
     AioContext *aio_context;
+    Error *local_err = NULL;
 
     blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
                       arg->has_id ? arg->id : NULL,
@@ -2704,18 +2712,29 @@ 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);
+        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;
+            }
         } else if (arg->has_group) {
-            blk_io_limits_update_group(blk, arg->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;
+            }
         }
         /* 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 */
+    } 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/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

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

[Alberto Garcia]

On Fri 25 Aug 2017 03:23:29 PM CEST, Manos Pitsidianakis wrote:
> 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>

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

Berto

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

[Stefan Hajnoczi]

On Fri, Aug 25, 2017 at 04:23:29PM +0300, Manos Pitsidianakis wrote:
>  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);

Is this really necessary?  bdrv_replace_node() also takes a temporary
reference:

  bdrv_ref(to);
  bdrv_replace_child_noperm(c, to);
  bdrv_unref(from);

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

[Kevin Wolf]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> 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>

This patch doesn't apply cleanly any more and needs a rebase.

>  /* 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;
> +    ThrottleState *ts;
> +
> +    bdrv_drained_begin(bs);

bs can be NULL:

$ x86_64-softmmu/qemu-system-x86_64 -drive media=cdrom,bps=1024
Segmentation fault (core dumped)

>  static void blk_root_drained_begin(BdrvChild *child)
>  {
> +    ThrottleGroupMember *tgm;
>      BlockBackend *blk = child->opaque;
>  
>      if (++blk->quiesce_counter == 1) {
> @@ -1997,19 +2025,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);
> +    }

We shouldn't really need any throttling code in
blk_root_drained_begin/end any more now because the throttle node will
be drained. If this code is necessary, a bdrv_drain() on an explicit
throttle node will work differently from one on an implicit one.

Unfortunately, this seems to be true about the throttle node. Implicit
throttle nodes will keep ignoring the throttle limit in order to
complete the drain request quickly, where as explicit throttle nodes
will process their requests at the configured speed before the drain
request can be completed.

This doesn't feel right to me, both should behave the same.

Kevin

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

[Manos Pitsidianakis]

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

On Thu, Sep 07, 2017 at 03:26:11PM +0200, Kevin Wolf wrote:
>We shouldn't really need any throttling code in
>blk_root_drained_begin/end any more now because the throttle node will
>be drained. If this code is necessary, a bdrv_drain() on an explicit
>throttle node will work differently from one on an implicit one.
>
>Unfortunately, this seems to be true about the throttle node. Implicit
>throttle nodes will keep ignoring the throttle limit in order to
>complete the drain request quickly, where as explicit throttle nodes
>will process their requests at the configured speed before the drain
>request can be completed.
>
>This doesn't feel right to me, both should behave the same.
>
>Kevin
>

I suppose we can implement bdrv_co_drain and increase io_limits_disabled 
from inside the driver. And then remove the implicit filter logic from 
blk_root_drained_begin. But there's no _end callback equivalent so we 
can't decrease io_limits_disabled at the end of the drain. So I think 
there are two options:

- make a bdrv_co_drain_end cb and recurse in blk_root_drain_end for all 
  children to call it. Old behavior of I/O bursts (?) during a drain is 
  kept.
- remove io_limits_disabled and let throttled requests obey limits 
  during a drain

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

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

[Kevin Wolf]

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

Am 08.09.2017 um 17:44 hat Manos Pitsidianakis geschrieben:
> On Thu, Sep 07, 2017 at 03:26:11PM +0200, Kevin Wolf wrote:
> > We shouldn't really need any throttling code in
> > blk_root_drained_begin/end any more now because the throttle node will
> > be drained. If this code is necessary, a bdrv_drain() on an explicit
> > throttle node will work differently from one on an implicit one.
> > 
> > Unfortunately, this seems to be true about the throttle node. Implicit
> > throttle nodes will keep ignoring the throttle limit in order to
> > complete the drain request quickly, where as explicit throttle nodes
> > will process their requests at the configured speed before the drain
> > request can be completed.
> > 
> > This doesn't feel right to me, both should behave the same.
> > 
> > Kevin
> > 
> 
> I suppose we can implement bdrv_co_drain and increase io_limits_disabled
> from inside the driver. And then remove the implicit filter logic from
> blk_root_drained_begin. But there's no _end callback equivalent so we can't
> decrease io_limits_disabled at the end of the drain. So I think there are
> two options:
> 
> - make a bdrv_co_drain_end cb and recurse in blk_root_drain_end for all
> children to call it. Old behavior of I/O bursts (?) during a drain is  kept.

This is the solution I was thinking of. It was always odd to have a
drained_begin/end pair in the external interface and in BdrvChildRole,
but not in BlockDriver. So it was to be expected that we'd need this
sooner or later.

> - remove io_limits_disabled and let throttled requests obey limits  during a
> drain

This was discussed earlier (at least when the disable code was
introduced in BlockBackend, but I think actually more than once), and
even though everyone agreed that ignoring the limits is ugly, we
seem to have come to the conclusion that it's the least bad option.
blk_drain() blocks and makes everything else hang, so we don't want it
to wait for several seconds.

Kevin

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

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

[Manos Pitsidianakis]

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

On Fri, Sep 08, 2017 at 06:00:11PM +0200, Kevin Wolf wrote:
>Am 08.09.2017 um 17:44 hat Manos Pitsidianakis geschrieben:
>> On Thu, Sep 07, 2017 at 03:26:11PM +0200, Kevin Wolf wrote:
>> > We shouldn't really need any throttling code in
>> > blk_root_drained_begin/end any more now because the throttle node will
>> > be drained. If this code is necessary, a bdrv_drain() on an explicit
>> > throttle node will work differently from one on an implicit one.
>> >
>> > Unfortunately, this seems to be true about the throttle node. Implicit
>> > throttle nodes will keep ignoring the throttle limit in order to
>> > complete the drain request quickly, where as explicit throttle nodes
>> > will process their requests at the configured speed before the drain
>> > request can be completed.
>> >
>> > This doesn't feel right to me, both should behave the same.
>> >
>> > Kevin
>> >
>>
>> I suppose we can implement bdrv_co_drain and increase io_limits_disabled
>> from inside the driver. And then remove the implicit filter logic from
>> blk_root_drained_begin. But there's no _end callback equivalent so we can't
>> decrease io_limits_disabled at the end of the drain. So I think there are
>> two options:
>>
>> - make a bdrv_co_drain_end cb and recurse in blk_root_drain_end for all
>> children to call it. Old behavior of I/O bursts (?) during a drain is  kept.
>
>This is the solution I was thinking of. It was always odd to have a
>drained_begin/end pair in the external interface and in BdrvChildRole,
>but not in BlockDriver. So it was to be expected that we'd need this
>sooner or later.
>
>> - remove io_limits_disabled and let throttled requests obey limits  during a
>> drain
>
>This was discussed earlier (at least when the disable code was
>introduced in BlockBackend, but I think actually more than once), and
>even though everyone agreed that ignoring the limits is ugly, we
>seem to have come to the conclusion that it's the least bad option.
>blk_drain() blocks and makes everything else hang, so we don't want it
>to wait for several seconds.
>
>Kevin

That makes sense. I will look into this and resubmit the series with 
this additional change.

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

[Qemu-devel] [PATCH v3 5/7] block/throttle-groups.c: remove throttle-groups list

[Manos Pitsidianakis]

block/throttle-groups.c uses a list to keep track of all groups and make
sure all names are unique.

This patch moves all ThrottleGroup objects under the root container.
While block/throttle.c requires that all throttle groups are created
with object-add or -object, the legacy throttling interface still used
the throttle_groups list. By using the root container we get the name
collision check for free and have all groups, legacy or not, available
through the qom-get/qom-set commands. Legacy groups are marked and freed
when they have no users left instead of waiting for an explicit
object-del.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 include/block/throttle-groups.h |   1 +
 block/block-backend.c           |  15 +++--
 block/throttle-groups.c         | 145 +++++++++++++++++++++++-----------------
 tests/test-throttle.c           |   3 +
 4 files changed, 96 insertions(+), 68 deletions(-)

diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 8493540766..13fbc63f1e 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -58,6 +58,7 @@ typedef struct ThrottleGroupMember {
 
 const char *throttle_group_get_name(ThrottleGroupMember *tgm);
 
+void throttle_group_new_legacy(const char *name, Error **errp);
 ThrottleState *throttle_group_incref(const char *name);
 void throttle_group_unref(ThrottleState *ts);
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 693ad27fc9..65f458ce8f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1957,12 +1957,18 @@ void blk_io_limits_enable(BlockBackend *blk, const char *group,  Error **errp)
     BlockDriverState *bs = blk_bs(blk), *throttle_node;
     QDict *options = qdict_new();
     Error *local_err = NULL;
-    ThrottleState *ts;
-
-    bdrv_drained_begin(bs);
 
     /* Force creation of group in case it doesn't exist */
-    ts = throttle_group_incref(group);
+    if (!throttle_group_exists(group)) {
+        throttle_group_new_legacy(group, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    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,
@@ -1987,7 +1993,6 @@ void blk_io_limits_enable(BlockBackend *blk, const char *group,  Error **errp)
     assert(throttle_node->refcnt == 1);
 
 end:
-    throttle_group_unref(ts);
     bdrv_drained_end(bs);
     blk_get_public(blk)->throttle_node = throttle_node;
 }
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 454bfcb067..238b648489 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -33,8 +33,10 @@
 #include "qapi-visit.h"
 #include "qom/object.h"
 #include "qom/object_interfaces.h"
+#include "qapi/qobject-input-visitor.h"
 
 static void throttle_group_obj_init(Object *obj);
+static bool throttle_group_can_be_deleted(UserCreatable *uc, Error **errp);
 static void throttle_group_obj_complete(UserCreatable *obj, Error **errp);
 
 /* The ThrottleGroup structure (with its ThrottleState) is shared
@@ -66,6 +68,7 @@ typedef struct ThrottleGroup {
 
     /* refuse individual property change if initialization is complete */
     bool is_initialized;
+    bool legacy;
     char *name; /* This is constant during the lifetime of the group */
 
     QemuMutex lock; /* This lock protects the following four fields */
@@ -74,34 +77,47 @@ typedef struct ThrottleGroup {
     ThrottleGroupMember *tokens[2];
     bool any_timer_armed[2];
     QEMUClockType clock_type;
-
-    /* This field is protected by the global QEMU mutex */
-    QTAILQ_ENTRY(ThrottleGroup) list;
 } ThrottleGroup;
 
-/* This is protected by the global QEMU mutex */
-static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
-    QTAILQ_HEAD_INITIALIZER(throttle_groups);
 
+struct ThrottleGroupQuery {
+    const char *name;
+    ThrottleGroup *result;
+};
 
-/* This function reads throttle_groups and must be called under the global
- * mutex.
+static int query_throttle_groups_foreach(Object *obj, void *data)
+{
+    ThrottleGroup *tg;
+    struct ThrottleGroupQuery *query = data;
+
+    tg = (ThrottleGroup *)object_dynamic_cast(obj, TYPE_THROTTLE_GROUP);
+    if (!tg) {
+        return 0;
+    }
+
+    if (!g_strcmp0(query->name, tg->name)) {
+        query->result = tg;
+        return 1;
+    }
+
+    return 0;
+}
+
+
+/* This function reads the QOM root container and must be called under the
+ * global mutex.
  */
 static ThrottleGroup *throttle_group_by_name(const char *name)
 {
-    ThrottleGroup *iter;
+    struct ThrottleGroupQuery query = { name = name };
 
     /* Look for an existing group with that name */
-    QTAILQ_FOREACH(iter, &throttle_groups, list) {
-        if (!g_strcmp0(name, iter->name)) {
-            return iter;
-        }
-    }
-
-    return NULL;
+    object_child_foreach(object_get_objects_root(),
+                         query_throttle_groups_foreach, &query);
+    return query.result;
 }
 
-/* This function reads throttle_groups and must be called under the global
+/* This function must be called under the global
  * mutex.
  */
 bool throttle_group_exists(const char *name)
@@ -109,34 +125,49 @@ bool throttle_group_exists(const char *name)
     return throttle_group_by_name(name) != NULL;
 }
 
+/*
+ * Create a new ThrottleGroup, insert it in the object root container so that
+ * we can refer to it by id and set tg->legacy to true
+ *
+ * This function edits the QOM root container and must be called under the
+ * global mutex.
+ *
+ * @name: the name of the ThrottleGroup.
+ * @errp: Error object. Will be set if @name collides with a non-ThrottleGroup
+ *        QOM object
+ */
+void throttle_group_new_legacy(const char *name, Error **errp)
+{
+    ThrottleGroup *tg = NULL;
+
+    /* Create an empty property qdict. Caller is responsible for
+     * setting up limits */
+    QDict *pdict = qdict_new();
+    Visitor *v = qobject_input_visitor_new(QOBJECT(pdict));
+
+    /* tg will have a ref count of 2, one for the object root container
+     * and one for the caller */
+    tg = THROTTLE_GROUP(user_creatable_add_type(TYPE_THROTTLE_GROUP,
+                name, pdict, v, errp));
+    visit_free(v);
+    QDECREF(pdict);
+    if (!tg) {
+        return;
+    }
+    tg->legacy = true;
+}
+
 /* Increments the reference count of a ThrottleGroup given its name.
  *
- * If no ThrottleGroup is found with the given name a new one is
- * created.
- *
- * This function edits throttle_groups and must be called under the global
- * mutex.
- *
  * @name: the name of the ThrottleGroup
  * @ret:  the ThrottleState member of the ThrottleGroup
  */
 ThrottleState *throttle_group_incref(const char *name)
 {
-    ThrottleGroup *tg = NULL;
-
-    /* Look for an existing group with that name */
-    tg = throttle_group_by_name(name);
-
-    if (tg) {
-        object_ref(OBJECT(tg));
-    } else {
-        /* Create a new one if not found */
-        /* new ThrottleGroup obj will have a refcnt = 1 */
-        tg = THROTTLE_GROUP(object_new(TYPE_THROTTLE_GROUP));
-        tg->name = g_strdup(name);
-        throttle_group_obj_complete(USER_CREATABLE(tg), &error_abort);
-    }
+    ThrottleGroup *tg = throttle_group_by_name(name);
 
+    assert(tg);
+    object_ref(OBJECT(tg));
     return &tg->ts;
 }
 
@@ -145,8 +176,8 @@ ThrottleState *throttle_group_incref(const char *name)
  * When the reference count reaches zero the ThrottleGroup is
  * destroyed.
  *
- * This function edits throttle_groups and must be called under the global
- * mutex.
+ * This function edits the QOM root container and must be called under the
+ * global mutex.
  *
  * @ts:  The ThrottleGroup to unref, given by its ThrottleState member
  */
@@ -154,6 +185,13 @@ void throttle_group_unref(ThrottleState *ts)
 {
     ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
     object_unref(OBJECT(tg));
+    /* ThrottleGroups will always have an extra reference from their container,
+     * so accessing it now is safe */
+    if (tg->legacy && OBJECT(tg)->ref == 1) {
+        tg->legacy = false;
+        /* Drop object created from legacy interface manually */
+        user_creatable_del(tg->name, &error_abort);
+    }
 }
 
 /* Get the name from a ThrottleGroupMember's group. The name (and the pointer)
@@ -490,14 +528,10 @@ static void write_timer_cb(void *opaque)
 }
 
 /* Register a ThrottleGroupMember from the throttling group, also initializing
- * its timers and updating its throttle_state pointer to point to it. If a
- * throttling group with that name does not exist yet, it will be created.
- *
- * This function edits throttle_groups and must be called under the global
- * mutex.
+ * its timers and updating its throttle_state pointer to point to it.
  *
  * @tgm:       the ThrottleGroupMember to insert
- * @groupname: the name of the group
+ * @groupname: the name of the group. It must already exist.
  * @ctx:       the AioContext to use
  */
 void throttle_group_register_tgm(ThrottleGroupMember *tgm,
@@ -690,8 +724,6 @@ static ThrottleParamInfo properties[] = {
     }
 };
 
-/* This function edits throttle_groups and must be called under the global
- * mutex */
 static void throttle_group_obj_init(Object *obj)
 {
     ThrottleGroup *tg = THROTTLE_GROUP(obj);
@@ -702,49 +734,36 @@ static void throttle_group_obj_init(Object *obj)
         tg->clock_type = QEMU_CLOCK_VIRTUAL;
     }
     tg->is_initialized = false;
+    tg->legacy = false;
     qemu_mutex_init(&tg->lock);
     throttle_init(&tg->ts);
     QLIST_INIT(&tg->head);
 }
 
-/* This function edits throttle_groups and must be called under the global
- * mutex */
 static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
 {
     ThrottleGroup *tg = THROTTLE_GROUP(obj);
     ThrottleConfig cfg;
 
-    /* set group name to object id if it exists */
+    /* set group name to object id */
     if (!tg->name && tg->parent_obj.parent) {
         tg->name = object_get_canonical_path_component(OBJECT(obj));
     }
     /* We must have a group name at this point */
     assert(tg->name);
 
-    /* error if name is duplicate */
-    if (throttle_group_exists(tg->name)) {
-        error_setg(errp, "A group with this name already exists");
-        return;
-    }
-
     /* check validity */
     throttle_get_config(&tg->ts, &cfg);
     if (!throttle_is_valid(&cfg, errp)) {
         return;
     }
     throttle_config(&tg->ts, tg->clock_type, &cfg);
-    QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
     tg->is_initialized = true;
 }
 
-/* This function edits throttle_groups and must be called under the global
- * mutex */
 static void throttle_group_obj_finalize(Object *obj)
 {
     ThrottleGroup *tg = THROTTLE_GROUP(obj);
-    if (tg->is_initialized) {
-        QTAILQ_REMOVE(&throttle_groups, tg, list);
-    }
     qemu_mutex_destroy(&tg->lock);
     g_free(tg->name);
 }
@@ -881,7 +900,7 @@ static void throttle_group_get_limits(Object *obj, Visitor *v,
 
 static bool throttle_group_can_be_deleted(UserCreatable *uc, Error **errp)
 {
-    return OBJECT(uc)->ref == 1;
+    return OBJECT(uc)->ref == 1 && THROTTLE_GROUP(uc)->legacy == false;
 }
 
 static void throttle_group_obj_class_init(ObjectClass *klass, void *class_data)
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index eef2b1c707..927117ecab 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -609,6 +609,9 @@ static void test_groups(void)
     g_assert(tgm2->throttle_state == NULL);
     g_assert(tgm3->throttle_state == NULL);
 
+    throttle_group_new_legacy("foo", &error_fatal);
+    throttle_group_new_legacy("bar", &error_fatal);
+
     throttle_group_register_tgm(tgm1, "bar", blk_get_aio_context(blk1));
     throttle_group_register_tgm(tgm2, "foo", blk_get_aio_context(blk2));
     throttle_group_register_tgm(tgm3, "bar", blk_get_aio_context(blk3));
-- 
2.11.0

Re: [Qemu-devel] [PATCH v3 5/7] block/throttle-groups.c: remove throttle-groups list

[Alberto Garcia]

On Fri 25 Aug 2017 03:23:30 PM CEST, Manos Pitsidianakis wrote:
> @@ -1957,12 +1957,18 @@ void blk_io_limits_enable(BlockBackend *blk, const char *group,  Error **errp)
>      BlockDriverState *bs = blk_bs(blk), *throttle_node;
>      QDict *options = qdict_new();
>      Error *local_err = NULL;
> -    ThrottleState *ts;
> -
> -    bdrv_drained_begin(bs);
>  
>      /* Force creation of group in case it doesn't exist */
> -    ts = throttle_group_incref(group);
> +    if (!throttle_group_exists(group)) {
> +        throttle_group_new_legacy(group, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }

After this, the reference count of the new group is 2 (as is already
explained in throttle_group_new_legacy()).

> +    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,

And the implicit throttle node will hold and extra reference, making it
3.

How do you delete the legacy throttle group then? block_set_io_throttle
with everything set to 0 disables I/O limits and destroys the node, but
the reference count is still 2 and the group is not destroyed.

> +static int query_throttle_groups_foreach(Object *obj, void *data)
> +{
> +    ThrottleGroup *tg;
> +    struct ThrottleGroupQuery *query = data;
> +
> +    tg = (ThrottleGroup *)object_dynamic_cast(obj, TYPE_THROTTLE_GROUP);
> +    if (!tg) {
> +        return 0;
> +    }
> +
> +    if (!g_strcmp0(query->name, tg->name)) {
> +        query->result = tg;
> +        return 1;
> +    }
> +
> +    return 0;
> +}

If you want you can make this a bit shorter if you merge both ifs:

   if (tg && !g_strcmp0(query->name, tg->name)) {
       query->result = tg;
       return 1;
   }

   return 0;

> +void throttle_group_new_legacy(const char *name, Error **errp)
> +{
> +    ThrottleGroup *tg = NULL;

No need to initialize tg here.

>  ThrottleState *throttle_group_incref(const char *name)
>  {

I think that you can make this one static and remove it from
throttle-groups.h (no one else is using it). Same with
throttle_group_unref.

Berto

[Qemu-devel] [PATCH v3 6/7] 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.

Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 include/sysemu/block-backend.h | 12 +-----------
 block/block-backend.c          | 43 +++++++++++++++++++-----------------------
 block/qapi.c                   |  4 ++--
 blockdev.c                     |  4 ++--
 4 files changed, 24 insertions(+), 39 deletions(-)

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);
diff --git a/block/block-backend.c b/block/block-backend.c
index 65f458ce8f..c70e1c5cf7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -37,7 +37,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 */
@@ -320,7 +323,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) {
@@ -615,19 +618,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;
 }
 
 /*
@@ -1920,15 +1915,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);
 
@@ -1944,7 +1939,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);
@@ -1994,7 +1989,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)
@@ -2002,11 +1997,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)) {
         return;
@@ -2030,8 +2025,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);
         }
@@ -2044,8 +2039,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 3f76eed2aa..df8316b1c3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2712,7 +2712,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);
@@ -2730,7 +2730,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
         }
         /* Set the new throttling configuration */
         blk_set_io_limits(blk, &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
-- 
2.11.0

[Qemu-devel] [PATCH v3 7/7] qemu-iotests: add 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.

Reviewed-by: Alberto Garcia <berto@igalia.com>
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 fee98440a5..ff732f0385 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -187,4 +187,5 @@
 188 rw auto quick
 189 rw auto
 190 rw auto quick
+191 rw auto quick
 192 rw auto quick
-- 
2.11.0