[Qemu-devel] [PATCH v2 0/8] blockdev: Further BlockBackend work

[Qemu-devel] [PATCH v2 0/8] blockdev: Further BlockBackend work

[Max Reitz]

Overall, this series adds more uses for BlockBackends and makes the code
follow the "reference theory" more closely, in that any BlockBackend
created (through -drive or blockdev-add) has a reference count of 1, and
that reference should be held by the monitor. This is reflected here by
introducing an explicit list of monitor-owned BlockBackends, which in
turn allows us to remove bdrv_states, and, perhaps most importantly,
blk_hide_on_behalf_of_do_drive_del().

Also, it lays groundwork for a nice series that will make bdrv_open()
return a BDS pointer.


This series depends on v8 (or any later version) of my series
"block: Rework bdrv_close_all()" (or any later version). With v7 you
will get a contextual conflict in patch 5 which should be simple to
resolve, however.

(No, v8 is not yet out, what I am referring to as that is basically v7
 with a fix for the following:
 http://lists.nongnu.org/archive/html/qemu-block/2015-11/msg00330.html)


v2:
- Mostly a rebase, complicated by the fact that the underlying series
  have changed a lot in the meantime.


Although I don't suppose anyone will care much, here's that
backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/8:[----] [-C] 'block: Add blk_name_taken()'
002/8:[----] [-C] 'block: Add blk_next_inserted()'
003/8:[----] [-C] 'block: Add blk_commit_all() and blk_invalidate_cache_all()'
004/8:[0073] [FC] 'block: Use BlockBackend more'
005/8:[0004] [FC] 'blockdev: Add list of monitor-owned BlockBackends'
006/8:[down] 'blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()'
      (renamed from "..._on_behalf_of_do_drive_del()")
007/8:[0213] [FC] 'block: Move some bdrv_*_all() functions to BB'
008/8:[0027] [FC] 'block: Remove bdrv_states'



Max Reitz (8):
  block: Add blk_name_taken()
  block: Add blk_next_inserted()
  block: Add blk_commit_all() and blk_invalidate_cache_all()
  block: Use BlockBackend more
  blockdev: Add list of monitor-owned BlockBackends
  blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()
  block: Move some bdrv_*_all() functions to BB
  block: Remove bdrv_states

 block.c                        |  82 ++------------
 block/block-backend.c          | 245 ++++++++++++++++++++++++++++++++++-------
 block/io.c                     |  88 ---------------
 block/qapi.c                   |  13 ++-
 blockdev.c                     |  29 ++---
 cpus.c                         |   7 +-
 include/block/block.h          |   5 -
 include/block/block_int.h      |   4 -
 include/sysemu/block-backend.h |   8 +-
 migration/block.c              |   7 +-
 migration/migration.c          |   3 +-
 migration/savevm.c             |  66 ++++++-----
 monitor.c                      |  15 ++-
 qemu-char.c                    |   3 +-
 qemu-io.c                      |   2 +-
 qmp.c                          |   9 +-
 stubs/Makefile.objs            |   2 +-
 stubs/bdrv-commit-all.c        |   7 --
 stubs/blk-commit-all.c         |   7 ++
 xen-mapcache.c                 |   3 +-
 20 files changed, 322 insertions(+), 283 deletions(-)
 delete mode 100644 stubs/bdrv-commit-all.c
 create mode 100644 stubs/blk-commit-all.c

-- 
2.6.2

[Qemu-devel] [PATCH v2 1/8] block: Add blk_name_taken()

[Max Reitz]

There may be BlockBackends which are not returned by blk_by_name(), but
do exist and have a name. blk_name_taken() allows testing whether a
specific name is in use already, independent of whether the BlockBackend
with that name is accessible through blk_by_name().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                        |  4 ++--
 block/block-backend.c          | 19 ++++++++++++++++++-
 include/sysemu/block-backend.h |  1 +
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index e3d5aea..faad9fb 100644
--- a/block.c
+++ b/block.c
@@ -777,8 +777,8 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
         return;
     }
 
-    /* takes care of avoiding namespaces collisions */
-    if (blk_by_name(node_name)) {
+    /* takes care of avoiding namespace collisions */
+    if (blk_name_taken(node_name)) {
         error_setg(errp, "node-name=%s is conflicting with a device id",
                    node_name);
         goto out;
diff --git a/block/block-backend.c b/block/block-backend.c
index 7373093..7128adc 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -86,7 +86,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
         error_setg(errp, "Invalid device name");
         return NULL;
     }
-    if (blk_by_name(name)) {
+    if (blk_name_taken(name)) {
         error_setg(errp, "Device with id '%s' already exists", name);
         return NULL;
     }
@@ -279,6 +279,23 @@ BlockBackend *blk_by_name(const char *name)
 }
 
 /*
+ * This function should be used to check whether a certain BlockBackend name is
+ * already taken; blk_by_name() will only search in the list of monitor-owned
+ * BlockBackends which is not necessarily complete.
+ */
+bool blk_name_taken(const char *name)
+{
+    BlockBackend *blk;
+
+    QTAILQ_FOREACH(blk, &blk_backends, link) {
+        if (!strcmp(name, blk->name)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+/*
  * Return the BlockDriverState attached to @blk if any, else null.
  */
 BlockDriverState *blk_bs(BlockBackend *blk)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9fb3e54..0593231 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -71,6 +71,7 @@ void blk_unref(BlockBackend *blk);
 void blk_remove_all_bs(void);
 const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
+bool blk_name_taken(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
-- 
2.6.2

[Qemu-devel] [PATCH v2 2/8] block: Add blk_next_inserted()

[Max Reitz]

This function skips to the next BlockBackend for which blk_is_inserted()
is true.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/block-backend.c          | 15 +++++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 7128adc..ef387f8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -252,6 +252,21 @@ BlockBackend *blk_next(BlockBackend *blk)
 }
 
 /*
+ * Like blk_next(), but skips all non-inserted BlockBackends (that is,
+ * BlockBackends for which blk_is_inserted() returns false)
+ */
+BlockBackend *blk_next_inserted(BlockBackend *blk)
+{
+    while ((blk = blk_next(blk)) != NULL) {
+        if (blk_is_inserted(blk)) {
+            break;
+        }
+    }
+
+    return blk;
+}
+
+/*
  * Return @blk's name, a non-null string.
  * Wart: the name is empty iff @blk has been hidden with
  * blk_hide_on_behalf_of_hmp_drive_del().
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 0593231..967c768 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -73,6 +73,7 @@ const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 bool blk_name_taken(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
+BlockBackend *blk_next_inserted(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
-- 
2.6.2

[Qemu-devel] [PATCH v2 3/8] block: Add blk_commit_all() and blk_invalidate_cache_all()

[Max Reitz]

These functions will be changed to iterate through the BDS trees as
defined by the BlockBackends instead of the list of root BDS, therefore
prepare moving their code to the BlockBackend level.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/block-backend.c          | 10 ++++++++++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index ef387f8..b3d086d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1331,3 +1331,13 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
 {
     return &blk->root_state;
 }
+
+int blk_commit_all(void)
+{
+    return bdrv_commit_all();
+}
+
+void blk_invalidate_cache_all(Error **errp)
+{
+    bdrv_invalidate_cache_all(errp);
+}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 967c768..ee36f93 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -129,8 +129,10 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
 int blk_co_flush(BlockBackend *blk);
 int blk_flush(BlockBackend *blk);
 int blk_flush_all(void);
+int blk_commit_all(void);
 void blk_drain(BlockBackend *blk);
 void blk_drain_all(void);
+void blk_invalidate_cache_all(Error **errp);
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
                       BlockdevOnError on_write_error);
 BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read);
-- 
2.6.2

[Qemu-devel] [PATCH v2 4/8] block: Use BlockBackend more

[Max Reitz]

Replace bdrv_drain_all(), bdrv_commmit_all(), bdrv_flush_all(),
bdrv_invalidate_cache_all(), bdrv_next() and occurrences of bdrv_states
by their BlockBackend equivalents.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 10 ++++----
 block/block-backend.c |  8 +++----
 block/qapi.c          | 13 ++++++++--
 blockdev.c            | 12 ++++++----
 cpus.c                |  7 +++---
 migration/block.c     |  7 ++++--
 migration/migration.c |  3 ++-
 migration/savevm.c    | 66 ++++++++++++++++++++++++++++++---------------------
 monitor.c             | 15 +++++++-----
 qemu-char.c           |  3 ++-
 qemu-io.c             |  2 +-
 qmp.c                 |  9 ++++---
 xen-mapcache.c        |  3 ++-
 13 files changed, 95 insertions(+), 63 deletions(-)

diff --git a/block.c b/block.c
index faad9fb..f8c1e99 100644
--- a/block.c
+++ b/block.c
@@ -1717,7 +1717,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 
     assert(bs_queue != NULL);
 
-    bdrv_drain_all();
+    blk_drain_all();
 
     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, &local_err)) {
@@ -1974,7 +1974,7 @@ void bdrv_close_all(void)
 
     /* Drop references from requests still in flight, such as canceled block
      * jobs whose AIO context has not been polled yet */
-    bdrv_drain_all();
+    blk_drain_all();
 
     blockdev_close_all_bdrv_states();
     blk_remove_all_bs();
@@ -3908,14 +3908,14 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
  */
 bool bdrv_is_first_non_filter(BlockDriverState *candidate)
 {
-    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
     /* walk down the bs forest recursively */
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+    while ((blk = blk_next_inserted(blk)) != NULL) {
         bool perm;
 
         /* try to recurse in this top level bs */
-        perm = bdrv_recurse_is_first_non_filter(bs, candidate);
+        perm = bdrv_recurse_is_first_non_filter(blk_bs(blk), candidate);
 
         /* candidate is the first non filter */
         if (perm) {
diff --git a/block/block-backend.c b/block/block-backend.c
index b3d086d..c89340e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -282,10 +282,10 @@ const char *blk_name(BlockBackend *blk)
  */
 BlockBackend *blk_by_name(const char *name)
 {
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
 
     assert(name);
-    QTAILQ_FOREACH(blk, &blk_backends, link) {
+    while ((blk = blk_next(blk)) != NULL) {
         if (!strcmp(name, blk->name)) {
             return blk;
         }
@@ -360,9 +360,9 @@ DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
  */
 BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 {
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
 
-    QTAILQ_FOREACH(blk, &blk_backends, link) {
+    while ((blk = blk_next(blk)) != NULL) {
         if (blk->legacy_dinfo == dinfo) {
             return blk;
         }
diff --git a/block/qapi.c b/block/qapi.c
index 89d4274..82727fb 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -405,15 +405,24 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
 {
     BlockStatsList *head = NULL, **p_next = &head;
     BlockDriverState *bs = NULL;
+    BlockBackend *blk = NULL;
 
     /* Just to be safe if query_nodes is not always initialized */
     query_nodes = has_query_nodes && query_nodes;
 
-    while ((bs = query_nodes ? bdrv_next_node(bs) : bdrv_next(bs))) {
+    while (query_nodes ? (bs = bdrv_next_node(bs)) != NULL
+                       : (blk = blk_next_inserted(blk)) != NULL)
+    {
         BlockStatsList *info = g_malloc0(sizeof(*info));
-        AioContext *ctx = bdrv_get_aio_context(bs);
+        AioContext *ctx = blk ? blk_get_aio_context(blk)
+                              : bdrv_get_aio_context(bs);
 
         aio_context_acquire(ctx);
+
+        if (blk) {
+            bs = blk_bs(blk);
+        }
+
         info->value = bdrv_query_stats(bs, !query_nodes);
         aio_context_release(ctx);
 
diff --git a/blockdev.c b/blockdev.c
index 93a70b4..97e6897 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1134,7 +1134,7 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
     int ret;
 
     if (!strcmp(device, "all")) {
-        ret = bdrv_commit_all();
+        ret = blk_commit_all();
     } else {
         BlockDriverState *bs;
         AioContext *aio_context;
@@ -1953,7 +1953,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
     QSIMPLEQ_INIT(&snap_bdrv_states);
 
     /* drain all i/o before any operations */
-    bdrv_drain_all();
+    blk_drain_all();
 
     /* We don't do anything in this loop that commits us to the operations */
     while (NULL != dev_entry) {
@@ -2581,7 +2581,7 @@ void qmp_block_resize(bool has_device, const char *device,
     }
 
     /* complete all in-flight operations before resizing the device */
-    bdrv_drain_all();
+    blk_drain_all();
 
     ret = bdrv_truncate(bs, size);
     switch (ret) {
@@ -3576,12 +3576,14 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 {
     BlockJobInfoList *head = NULL, **p_next = &head;
     BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
-    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        AioContext *aio_context = blk_get_aio_context(blk);
 
         aio_context_acquire(aio_context);
 
+        bs = blk_bs(blk);
         if (bs->job) {
             BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
             elem->value = block_job_query(bs->job);
diff --git a/cpus.c b/cpus.c
index 877bd70..46ef045 100644
--- a/cpus.c
+++ b/cpus.c
@@ -29,6 +29,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "exec/gdbstub.h"
 #include "sysemu/dma.h"
 #include "sysemu/kvm.h"
@@ -725,8 +726,8 @@ static int do_vm_stop(RunState state)
         qapi_event_send_stop(&error_abort);
     }
 
-    bdrv_drain_all();
-    ret = bdrv_flush_all();
+    blk_drain_all();
+    ret = blk_flush_all();
 
     return ret;
 }
@@ -1417,7 +1418,7 @@ int vm_stop_force_state(RunState state)
         runstate_set(state);
         /* Make sure to return an error if the flush in a previous vm_stop()
          * failed. */
-        return bdrv_flush_all();
+        return blk_flush_all();
     }
 }
 
diff --git a/migration/block.c b/migration/block.c
index cf9d9f8..9b28f46 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -349,6 +349,7 @@ static void unset_dirty_tracking(void)
 static void init_blk_migration(QEMUFile *f)
 {
     BlockDriverState *bs;
+    BlockBackend *blk = NULL;
     BlkMigDevState *bmds;
     int64_t sectors;
 
@@ -360,7 +361,9 @@ static void init_blk_migration(QEMUFile *f)
     block_mig_state.bulk_completed = 0;
     block_mig_state.zero_blocks = migrate_zero_blocks();
 
-    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        bs = blk_bs(blk);
+
         if (bdrv_is_read_only(bs)) {
             continue;
         }
@@ -596,7 +599,7 @@ static void block_migration_cleanup(void *opaque)
     BlkMigDevState *bmds;
     BlkMigBlock *blk;
 
-    bdrv_drain_all();
+    blk_drain_all();
 
     unset_dirty_tracking();
 
diff --git a/migration/migration.c b/migration/migration.c
index f99d3ea..74ea067 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -19,6 +19,7 @@
 #include "migration/migration.h"
 #include "migration/qemu-file.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "block/block.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/sockets.h"
@@ -296,7 +297,7 @@ static void process_incoming_migration_co(void *opaque)
     }
 
     /* Make sure all file formats flush their mutable metadata */
-    bdrv_invalidate_cache_all(&local_err);
+    blk_invalidate_cache_all(&local_err);
     if (local_err) {
         migrate_generate_event(MIGRATION_STATUS_FAILED);
         error_report_err(local_err);
diff --git a/migration/savevm.c b/migration/savevm.c
index e05158d..02690ec 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -34,6 +34,7 @@
 #include "net/net.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "qemu/timer.h"
 #include "audio/audio.h"
 #include "migration/migration.h"
@@ -1239,10 +1240,10 @@ out:
 
 static BlockDriverState *find_vmstate_bs(void)
 {
-    BlockDriverState *bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            return bs;
+    BlockBackend *blk = NULL;
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        if (bdrv_can_snapshot(blk_bs(blk))) {
+            return blk_bs(blk);
         }
     }
     return NULL;
@@ -1254,11 +1255,13 @@ static BlockDriverState *find_vmstate_bs(void)
 static int del_existing_snapshots(Monitor *mon, const char *name)
 {
     BlockDriverState *bs;
+    BlockBackend *blk = NULL;
     QEMUSnapshotInfo sn1, *snapshot = &sn1;
     Error *err = NULL;
 
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        bs = blk_bs(blk);
+
         if (bdrv_can_snapshot(bs) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0) {
             bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
@@ -1280,6 +1283,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
 void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
+    BlockBackend *blk = NULL;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
     int ret;
     QEMUFile *f;
@@ -1291,16 +1295,14 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     Error *local_err = NULL;
 
     /* Verify if there is a device that doesn't support snapshots and is writable */
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-
-        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        if (blk_is_read_only(blk)) {
             continue;
         }
 
-        if (!bdrv_can_snapshot(bs)) {
+        if (!bdrv_can_snapshot(blk_bs(blk))) {
             monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
-                               bdrv_get_device_name(bs));
+                           blk_name(blk));
             return;
         }
     }
@@ -1364,15 +1366,17 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
 
     /* create the snapshots */
 
-    bs1 = NULL;
-    while ((bs1 = bdrv_next(bs1))) {
+    blk = NULL;
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        bs1 = blk_bs(blk);
+
         if (bdrv_can_snapshot(bs1)) {
             /* Write VM state size only to the image that contains the state */
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
             ret = bdrv_snapshot_create(bs1, sn);
             if (ret < 0) {
                 monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                               bdrv_get_device_name(bs1));
+                               blk_name(blk));
             }
         }
     }
@@ -1412,6 +1416,7 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
 
 int load_vmstate(const char *name)
 {
+    BlockBackend *blk;
     BlockDriverState *bs, *bs_vm_state;
     QEMUSnapshotInfo sn;
     QEMUFile *f;
@@ -1435,12 +1440,12 @@ int load_vmstate(const char *name)
 
     /* Verify if there is any device that doesn't support snapshots and is
     writable and check if the requested snapshot is available too. */
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-
-        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+    blk = NULL;
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        if (blk_is_read_only(blk)) {
             continue;
         }
+        bs = blk_bs(blk);
 
         if (!bdrv_can_snapshot(bs)) {
             error_report("Device '%s' is writable but does not support snapshots.",
@@ -1457,10 +1462,12 @@ int load_vmstate(const char *name)
     }
 
     /* Flush all IO requests so they don't interfere with the new state.  */
-    bdrv_drain_all();
+    blk_drain_all();
+
+    blk = NULL;
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        bs = blk_bs(blk);
 
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs)) {
             ret = bdrv_snapshot_goto(bs, name);
             if (ret < 0) {
@@ -1495,6 +1502,7 @@ int load_vmstate(const char *name)
 void hmp_delvm(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs;
+    BlockBackend *blk = NULL;
     Error *err;
     const char *name = qdict_get_str(qdict, "name");
 
@@ -1503,8 +1511,9 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        bs = blk_bs(blk);
+
         if (bdrv_can_snapshot(bs)) {
             err = NULL;
             bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
@@ -1512,7 +1521,7 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
                 monitor_printf(mon,
                                "Error while deleting snapshot on device '%s':"
                                " %s\n",
-                               bdrv_get_device_name(bs),
+                               blk_name(blk),
                                error_get_pretty(err));
                 error_free(err);
             }
@@ -1523,6 +1532,7 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
+    BlockBackend *blk;
     QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
     int nb_sns, i, ret, available;
     int total;
@@ -1550,9 +1560,11 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     for (i = 0; i < nb_sns; i++) {
         sn = &sn_tab[i];
         available = 1;
-        bs1 = NULL;
+        blk = NULL;
+
+        while ((blk = blk_next_inserted(blk)) != NULL) {
+            bs1 = blk_bs(blk);
 
-        while ((bs1 = bdrv_next(bs1))) {
             if (bdrv_can_snapshot(bs1) && bs1 != bs) {
                 ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
                 if (ret < 0) {
diff --git a/monitor.c b/monitor.c
index 3295840..4600f2d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -41,6 +41,7 @@
 #include "ui/console.h"
 #include "ui/input.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/block-backend.h"
 #include "audio/audio.h"
 #include "disas/disas.h"
 #include "sysemu/balloon.h"
@@ -3402,16 +3403,18 @@ void host_net_remove_completion(ReadLineState *rs, int nb_args, const char *str)
 static void vm_completion(ReadLineState *rs, const char *str)
 {
     size_t len;
-    BlockDriverState *bs = NULL;
+    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
     len = strlen(str);
     readline_set_completion_index(rs, len);
-    while ((bs = bdrv_next(bs))) {
+    while ((blk = blk_next_inserted(blk)) != NULL) {
         SnapshotInfoList *snapshots, *snapshot;
-        AioContext *ctx = bdrv_get_aio_context(bs);
+        AioContext *ctx = blk_get_aio_context(blk);
         bool ok = false;
 
         aio_context_acquire(ctx);
+        bs = blk_bs(blk);
         if (bdrv_can_snapshot(bs)) {
             ok = bdrv_query_snapshot_info_list(bs, &snapshots, NULL) == 0;
         }
@@ -3460,7 +3463,7 @@ static void monitor_find_completion_by_table(Monitor *mon,
     int i;
     const char *ptype, *str, *name;
     const mon_cmd_t *cmd;
-    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
     if (nb_args <= 1) {
         /* command completion */
@@ -3515,8 +3518,8 @@ static void monitor_find_completion_by_table(Monitor *mon,
         case 'B':
             /* block device name completion */
             readline_set_completion_index(mon->rs, strlen(str));
-            for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
-                name = bdrv_get_device_name(bs);
+            while ((blk = blk_next(blk)) != NULL) {
+                name = blk_name(blk);
                 if (str[0] == '\0' ||
                     !strncmp(name, str, strlen(str))) {
                     readline_add_completion(mon->rs, name);
diff --git a/qemu-char.c b/qemu-char.c
index 5448b0f..28ea364 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -24,6 +24,7 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "sysemu/char.h"
@@ -501,7 +502,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
                  break;
             }
         case 's':
-            bdrv_commit_all();
+            blk_commit_all();
             break;
         case 'b':
             qemu_chr_be_event(chr, CHR_EVENT_BREAK);
diff --git a/qemu-io.c b/qemu-io.c
index 269f17c..3a9707e 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -493,7 +493,7 @@ int main(int argc, char **argv)
     /*
      * Make sure all outstanding requests complete before the program exits.
      */
-    bdrv_drain_all();
+    blk_drain_all();
 
     blk_unref(qemuio_blk);
     g_free(readline_state);
diff --git a/qmp.c b/qmp.c
index ddc63ea..4ca295d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -171,8 +171,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
 void qmp_cont(Error **errp)
 {
     Error *local_err = NULL;
-    BlockBackend *blk;
-    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
     if (runstate_needs_reset()) {
         error_setg(errp, "Resetting the Virtual Machine is required");
@@ -181,11 +180,11 @@ void qmp_cont(Error **errp)
         return;
     }
 
-    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+    while ((blk = blk_next(blk)) != NULL) {
         blk_iostatus_reset(blk);
     }
-    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
-        bdrv_add_key(bs, NULL, &local_err);
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        bdrv_add_key(blk_bs(blk), NULL, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
diff --git a/xen-mapcache.c b/xen-mapcache.c
index 97fece2..66634bc 100644
--- a/xen-mapcache.c
+++ b/xen-mapcache.c
@@ -14,6 +14,7 @@
 
 #include "hw/xen/xen_backend.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/block-backend.h"
 #include "qemu/bitmap.h"
 
 #include <xen/hvm/params.h>
@@ -419,7 +420,7 @@ void xen_invalidate_map_cache(void)
     MapCacheRev *reventry;
 
     /* Flush pending AIO before destroying the mapcache */
-    bdrv_drain_all();
+    blk_drain_all();
 
     mapcache_lock();
 
-- 
2.6.2

[Qemu-devel] [PATCH v2 5/8] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz]

The monitor does hold references to some BlockBackends so it should have
a list of those BBs; blk_backends is a different list, as it contains
references to all BBs (after a follow-up patch, that is), and that
should not be changed because we do need such a list.

monitor_remove_blk() is idempotent so that we can call it in
blockdev_auto_del() without having to care whether it had been called in
do_drive_del() before. monitor_add_blk() is idempotent for symmetry
reasons (monitor_remove_blk() is, so it would be strange for
monitor_add_blk() not to be).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 34 +++++++++++++++++++++++++++++++++-
 blockdev.c                     |  8 ++++++++
 include/sysemu/block-backend.h |  2 ++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index c89340e..e35d84f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -29,6 +29,8 @@ struct BlockBackend {
     BlockDriverState *bs;
     DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
     QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
+    QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */
+    bool in_monitor_list;
 
     void *dev;                  /* attached device model, if any */
     /* TODO change to DeviceState when all users are qdevified */
@@ -70,6 +72,11 @@ static void drive_info_del(DriveInfo *dinfo);
 static QTAILQ_HEAD(, BlockBackend) blk_backends =
     QTAILQ_HEAD_INITIALIZER(blk_backends);
 
+/* All BlockBackends referenced by the monitor and which are iterated through by
+ * blk_next() */
+static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
+    QTAILQ_HEAD_INITIALIZER(monitor_block_backends);
+
 /*
  * Create a new BlockBackend with @name, with a reference count of one.
  * @name must not be null or empty.
@@ -248,7 +255,8 @@ void blk_remove_all_bs(void)
  */
 BlockBackend *blk_next(BlockBackend *blk)
 {
-    return blk ? QTAILQ_NEXT(blk, link) : QTAILQ_FIRST(&blk_backends);
+    return blk ? QTAILQ_NEXT(blk, monitor_link)
+               : QTAILQ_FIRST(&monitor_block_backends);
 }
 
 /*
@@ -267,6 +275,30 @@ BlockBackend *blk_next_inserted(BlockBackend *blk)
 }
 
 /*
+ * Add a BlockBackend into the list of backends referenced by the monitor.
+ * Strictly for use by blockdev.c.
+ */
+void monitor_add_blk(BlockBackend *blk)
+{
+    if (!blk->in_monitor_list) {
+        QTAILQ_INSERT_TAIL(&monitor_block_backends, blk, monitor_link);
+        blk->in_monitor_list = true;
+    }
+}
+
+/*
+ * Remove a BlockBackend from the list of backends referenced by the monitor.
+ * Strictly for use by blockdev.c.
+ */
+void monitor_remove_blk(BlockBackend *blk)
+{
+    if (blk->in_monitor_list) {
+        QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
+        blk->in_monitor_list = false;
+    }
+}
+
+/*
  * Return @blk's name, a non-null string.
  * Wart: the name is empty iff @blk has been hidden with
  * blk_hide_on_behalf_of_hmp_drive_del().
diff --git a/blockdev.c b/blockdev.c
index 97e6897..f5dde19 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -146,6 +146,7 @@ void blockdev_auto_del(BlockBackend *blk)
     DriveInfo *dinfo = blk_legacy_dinfo(blk);
 
     if (dinfo && dinfo->auto_del) {
+        monitor_remove_blk(blk);
         blk_unref(blk);
     }
 }
@@ -606,6 +607,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
 
     blk_set_on_error(blk, on_read_error, on_write_error);
 
+    monitor_add_blk(blk);
+
 err_no_bs_opts:
     qemu_opts_del(opts);
     return blk;
@@ -2528,6 +2531,8 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
         blk_remove_bs(blk);
     }
 
+    monitor_remove_blk(blk);
+
     /* if we have a device attached to this BlockDriverState
      * then we need to make the drive anonymous until the device
      * can be removed.  If this is a drive with no device backing
@@ -3486,6 +3491,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     if (bs && bdrv_key_required(bs)) {
         if (blk) {
+            monitor_remove_blk(blk);
             blk_unref(blk);
         } else {
             QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
@@ -3515,6 +3521,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
     }
 
     if (has_id) {
+        /* blk_by_name() never returns a BB that is not owned by the monitor */
         blk = blk_by_name(id);
         if (!blk) {
             error_setg(errp, "Cannot find block backend %s", id);
@@ -3562,6 +3569,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
     }
 
     if (blk) {
+        monitor_remove_blk(blk);
         blk_unref(blk);
     } else {
         QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index ee36f93..9a3d8db 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -74,6 +74,8 @@ BlockBackend *blk_by_name(const char *name);
 bool blk_name_taken(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
 BlockBackend *blk_next_inserted(BlockBackend *blk);
+void monitor_add_blk(BlockBackend *blk);
+void monitor_remove_blk(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
-- 
2.6.2

[Qemu-devel] [PATCH v2 6/8] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()

[Max Reitz]

This function first removed the BlockBackend from the blk_backends list
and cleared its name so it would no longer be found by blk_name(); since
blk_next() now iterates through monitor_block_backends (which the BB is
removed from in hmp_drive_del()), this is no longer necessary.

Second, bdrv_make_anon() was called on the BDS. This was intended for
cases where the BDS was owned by that BB alone; in which case the BDS
will no longer exist at this point thanks to the blk_remove_bs() in
hmp_drive_del().

Therefore, this function does nothing useful anymore. Remove it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 25 ++-----------------------
 blockdev.c                     |  1 -
 include/sysemu/block-backend.h |  2 --
 3 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index e35d84f..609a045 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -68,7 +68,7 @@ static const AIOCBInfo block_backend_aiocb_info = {
 
 static void drive_info_del(DriveInfo *dinfo);
 
-/* All the BlockBackends (except for hidden ones) */
+/* All the BlockBackends */
 static QTAILQ_HEAD(, BlockBackend) blk_backends =
     QTAILQ_HEAD_INITIALIZER(blk_backends);
 
@@ -180,10 +180,7 @@ static void blk_delete(BlockBackend *blk)
         g_free(blk->root_state.throttle_group);
         throttle_group_unref(blk->root_state.throttle_state);
     }
-    /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */
-    if (blk->name[0]) {
-        QTAILQ_REMOVE(&blk_backends, blk, link);
-    }
+    QTAILQ_REMOVE(&blk_backends, blk, link);
     g_free(blk->name);
     drive_info_del(blk->legacy_dinfo);
     g_free(blk);
@@ -403,24 +400,6 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 }
 
 /*
- * Hide @blk.
- * @blk must not have been hidden already.
- * Make attached BlockDriverState, if any, anonymous.
- * Once hidden, @blk is invisible to all functions that don't receive
- * it as argument.  For example, blk_by_name() won't return it.
- * Strictly for use by do_drive_del().
- * TODO get rid of it!
- */
-void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
-{
-    QTAILQ_REMOVE(&blk_backends, blk, link);
-    blk->name[0] = 0;
-    if (blk->bs) {
-        bdrv_make_anon(blk->bs);
-    }
-}
-
-/*
  * Disassociates the currently associated BlockDriverState from @blk.
  */
 void blk_remove_bs(BlockBackend *blk)
diff --git a/blockdev.c b/blockdev.c
index f5dde19..934b9d8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2539,7 +2539,6 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
      * then we can just get rid of the block driver state right here.
      */
     if (blk_get_attached_dev(blk)) {
-        blk_hide_on_behalf_of_hmp_drive_del(blk);
         /* Further I/O must not pause the guest */
         blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
                          BLOCKDEV_ON_ERROR_REPORT);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9a3d8db..5208c08 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -81,8 +81,6 @@ BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
 
-void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk);
-
 void blk_iostatus_enable(BlockBackend *blk);
 bool blk_iostatus_is_enabled(const BlockBackend *blk);
 BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
-- 
2.6.2

[Qemu-devel] [PATCH v2 7/8] block: Move some bdrv_*_all() functions to BB

[Max Reitz]

Move bdrv_drain_all(), bdrv_commit_all(), bdrv_flush_all() and
bdrv_invalidate_cache_all() to BB.

The only operation left is bdrv_close_all(), which cannot be moved to
the BB because it should not only close all BBs, but also all
monitor-owned BDSs.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                 |  38 -------------
 block/block-backend.c   | 138 +++++++++++++++++++++++++++++++++++++++++++-----
 block/io.c              |  88 ------------------------------
 include/block/block.h   |   4 --
 stubs/Makefile.objs     |   2 +-
 stubs/bdrv-commit-all.c |   7 ---
 stubs/blk-commit-all.c  |   7 +++
 7 files changed, 134 insertions(+), 150 deletions(-)
 delete mode 100644 stubs/bdrv-commit-all.c
 create mode 100644 stubs/blk-commit-all.c

diff --git a/block.c b/block.c
index f8c1e99..c80675e 100644
--- a/block.c
+++ b/block.c
@@ -2297,26 +2297,6 @@ ro_cleanup:
     return ret;
 }
 
-int bdrv_commit_all(void)
-{
-    BlockDriverState *bs;
-
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        aio_context_acquire(aio_context);
-        if (bs->drv && bs->backing) {
-            int ret = bdrv_commit(bs);
-            if (ret < 0) {
-                aio_context_release(aio_context);
-                return ret;
-            }
-        }
-        aio_context_release(aio_context);
-    }
-    return 0;
-}
-
 /*
  * Return values:
  * 0        - success
@@ -3074,24 +3054,6 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
     }
 }
 
-void bdrv_invalidate_cache_all(Error **errp)
-{
-    BlockDriverState *bs;
-    Error *local_err = NULL;
-
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        aio_context_acquire(aio_context);
-        bdrv_invalidate_cache(bs, &local_err);
-        aio_context_release(aio_context);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
-    }
-}
-
 /**************************************************************/
 /* removable device support */
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 609a045..b0bf3b2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -409,6 +409,9 @@ void blk_remove_bs(BlockBackend *blk)
     notifier_list_notify(&blk->remove_bs_notifiers, blk);
 
     blk_update_root_state(blk);
+    /* After this function, the BDS may longer be accessible to blk_*_all()
+     * functions */
+    bdrv_drain(blk->bs);
 
     blk->bs->blk = NULL;
     bdrv_unref(blk->bs);
@@ -902,11 +905,6 @@ int blk_flush(BlockBackend *blk)
     return bdrv_flush(blk->bs);
 }
 
-int blk_flush_all(void)
-{
-    return bdrv_flush_all();
-}
-
 void blk_drain(BlockBackend *blk)
 {
     if (blk->bs) {
@@ -914,11 +912,6 @@ void blk_drain(BlockBackend *blk)
     }
 }
 
-void blk_drain_all(void)
-{
-    bdrv_drain_all();
-}
-
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
                       BlockdevOnError on_write_error)
 {
@@ -1343,12 +1336,133 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
     return &blk->root_state;
 }
 
+/*
+ * Wait for pending requests to complete across all BlockBackends
+ *
+ * This function does not flush data to disk, use blk_flush_all() for that
+ * after calling this function.
+ */
+void blk_drain_all(void)
+{
+    /* Always run first iteration so any pending completion BHs run */
+    bool busy = true;
+    BlockBackend *blk;
+    GSList *aio_ctxs = NULL, *ctx;
+
+    QTAILQ_FOREACH(blk, &blk_backends, link) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+
+        aio_context_acquire(aio_context);
+        if (blk_is_inserted(blk) && blk->bs->job) {
+            block_job_pause(blk->bs->job);
+        }
+        aio_context_release(aio_context);
+
+        if (!g_slist_find(aio_ctxs, aio_context)) {
+            aio_ctxs = g_slist_prepend(aio_ctxs, aio_context);
+        }
+    }
+
+    /* Note that completion of an asynchronous I/O operation can trigger any
+     * number of other I/O operations on other devices---for example a
+     * coroutine can submit an I/O request to another device in response to
+     * request completion.  Therefore we must keep looping until there was no
+     * more activity rather than simply draining each device independently.
+     */
+    while (busy) {
+        busy = false;
+
+        for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
+            AioContext *aio_context = ctx->data;
+
+            aio_context_acquire(aio_context);
+            QTAILQ_FOREACH(blk, &blk_backends, link) {
+                if (blk_is_inserted(blk) &&
+                    aio_context == blk_get_aio_context(blk))
+                {
+                    bdrv_flush_io_queue(blk->bs);
+                    if (bdrv_requests_pending(blk->bs)) {
+                        busy = true;
+                        aio_poll(aio_context, busy);
+                    }
+                }
+            }
+            busy |= aio_poll(aio_context, false);
+            aio_context_release(aio_context);
+        }
+    }
+
+    QTAILQ_FOREACH(blk, &blk_backends, link) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+
+        aio_context_acquire(aio_context);
+        if (blk_is_inserted(blk) && blk->bs->job) {
+            block_job_resume(blk->bs->job);
+        }
+        aio_context_release(aio_context);
+    }
+    g_slist_free(aio_ctxs);
+}
+
+
 int blk_commit_all(void)
 {
-    return bdrv_commit_all();
+    BlockBackend *blk;
+
+    QTAILQ_FOREACH(blk, &blk_backends, link) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+
+        aio_context_acquire(aio_context);
+        if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) {
+            int ret = bdrv_commit(blk->bs);
+            if (ret < 0) {
+                aio_context_release(aio_context);
+                return ret;
+            }
+        }
+        aio_context_release(aio_context);
+    }
+    return 0;
+}
+
+int blk_flush_all(void)
+{
+    BlockBackend *blk;
+    int result = 0;
+
+    QTAILQ_FOREACH(blk, &blk_backends, link) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+        int ret;
+
+        aio_context_acquire(aio_context);
+        if (blk_is_inserted(blk)) {
+            ret = blk_flush(blk);
+            if (ret < 0 && !result) {
+                result = ret;
+            }
+        }
+        aio_context_release(aio_context);
+    }
+
+    return result;
 }
 
 void blk_invalidate_cache_all(Error **errp)
 {
-    bdrv_invalidate_cache_all(errp);
+    BlockBackend *blk;
+    Error *local_err = NULL;
+
+    QTAILQ_FOREACH(blk, &blk_backends, link) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+
+        aio_context_acquire(aio_context);
+        if (blk_is_inserted(blk)) {
+            blk_invalidate_cache(blk, &local_err);
+        }
+        aio_context_release(aio_context);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
 }
diff --git a/block/io.c b/block/io.c
index 8dcad3b..110f7b7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -259,74 +259,6 @@ void bdrv_drain(BlockDriverState *bs)
     }
 }
 
-/*
- * Wait for pending requests to complete across all BlockDriverStates
- *
- * This function does not flush data to disk, use bdrv_flush_all() for that
- * after calling this function.
- */
-void bdrv_drain_all(void)
-{
-    /* Always run first iteration so any pending completion BHs run */
-    bool busy = true;
-    BlockDriverState *bs = NULL;
-    GSList *aio_ctxs = NULL, *ctx;
-
-    while ((bs = bdrv_next(bs))) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        aio_context_acquire(aio_context);
-        if (bs->job) {
-            block_job_pause(bs->job);
-        }
-        aio_context_release(aio_context);
-
-        if (!g_slist_find(aio_ctxs, aio_context)) {
-            aio_ctxs = g_slist_prepend(aio_ctxs, aio_context);
-        }
-    }
-
-    /* Note that completion of an asynchronous I/O operation can trigger any
-     * number of other I/O operations on other devices---for example a
-     * coroutine can submit an I/O request to another device in response to
-     * request completion.  Therefore we must keep looping until there was no
-     * more activity rather than simply draining each device independently.
-     */
-    while (busy) {
-        busy = false;
-
-        for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
-            AioContext *aio_context = ctx->data;
-            bs = NULL;
-
-            aio_context_acquire(aio_context);
-            while ((bs = bdrv_next(bs))) {
-                if (aio_context == bdrv_get_aio_context(bs)) {
-                    bdrv_flush_io_queue(bs);
-                    if (bdrv_requests_pending(bs)) {
-                        busy = true;
-                        aio_poll(aio_context, busy);
-                    }
-                }
-            }
-            busy |= aio_poll(aio_context, false);
-            aio_context_release(aio_context);
-        }
-    }
-
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        aio_context_acquire(aio_context);
-        if (bs->job) {
-            block_job_resume(bs->job);
-        }
-        aio_context_release(aio_context);
-    }
-    g_slist_free(aio_ctxs);
-}
-
 /**
  * Remove an active request from the tracked requests list
  *
@@ -1417,26 +1349,6 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
                              BDRV_REQ_ZERO_WRITE | flags);
 }
 
-int bdrv_flush_all(void)
-{
-    BlockDriverState *bs = NULL;
-    int result = 0;
-
-    while ((bs = bdrv_next(bs))) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-        int ret;
-
-        aio_context_acquire(aio_context);
-        ret = bdrv_flush(bs);
-        if (ret < 0 && !result) {
-            result = ret;
-        }
-        aio_context_release(aio_context);
-    }
-
-    return result;
-}
-
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
diff --git a/include/block/block.h b/include/block/block.h
index 03551d3..f47c45a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -268,7 +268,6 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
 void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
 int bdrv_commit(BlockDriverState *bs);
-int bdrv_commit_all(void);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
@@ -355,15 +354,12 @@ BlockAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
 
 /* Invalidate any cached metadata used by image formats */
 void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
-void bdrv_invalidate_cache_all(Error **errp);
 
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
-int bdrv_flush_all(void);
 void bdrv_close_all(void);
 void bdrv_drain(BlockDriverState *bs);
-void bdrv_drain_all(void);
 
 int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index c1f02be..f872ba0 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,5 +1,5 @@
 stub-obj-y += arch-query-cpu-def.o
-stub-obj-y += bdrv-commit-all.o
+stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-close-all-bdrv-states.o
 stub-obj-y += clock-warp.o
 stub-obj-y += cpu-get-clock.o
diff --git a/stubs/bdrv-commit-all.c b/stubs/bdrv-commit-all.c
deleted file mode 100644
index a8e0a95..0000000
--- a/stubs/bdrv-commit-all.c
+++ /dev/null
@@ -1,7 +0,0 @@
-#include "qemu-common.h"
-#include "block/block.h"
-
-int bdrv_commit_all(void)
-{
-    return 0;
-}
diff --git a/stubs/blk-commit-all.c b/stubs/blk-commit-all.c
new file mode 100644
index 0000000..90b59e5
--- /dev/null
+++ b/stubs/blk-commit-all.c
@@ -0,0 +1,7 @@
+#include "qemu-common.h"
+#include "sysemu/block-backend.h"
+
+int blk_commit_all(void)
+{
+    return 0;
+}
-- 
2.6.2

[Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_states

[Max Reitz]

Every entry in this list should be a root BDS and as such either be
anchored to a BlockBackend or be owned by the monitor.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   | 30 +-----------------------------
 blockdev.c                |  8 --------
 include/block/block.h     |  1 -
 include/block/block_int.h |  4 ----
 4 files changed, 1 insertion(+), 42 deletions(-)

diff --git a/block.c b/block.c
index c80675e..5b02990 100644
--- a/block.c
+++ b/block.c
@@ -73,8 +73,6 @@ struct BdrvDirtyBitmap {
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
-struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
-
 static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
 
@@ -247,10 +245,7 @@ void bdrv_register(BlockDriver *bdrv)
 
 BlockDriverState *bdrv_new_root(void)
 {
-    BlockDriverState *bs = bdrv_new();
-
-    QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
-    return bs;
+    return bdrv_new();
 }
 
 BlockDriverState *bdrv_new(void)
@@ -2015,17 +2010,6 @@ void bdrv_close_all(void)
    Also, NULL terminate the device_name to prevent double remove */
 void bdrv_make_anon(BlockDriverState *bs)
 {
-    /*
-     * Take care to remove bs from bdrv_states only when it's actually
-     * in it.  Note that bs->device_list.tqe_prev is initially null,
-     * and gets set to non-null by QTAILQ_INSERT_TAIL().  Establish
-     * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
-     * resetting it to null on remove.
-     */
-    if (bs->device_list.tqe_prev) {
-        QTAILQ_REMOVE(&bdrv_states, bs, device_list);
-        bs->device_list.tqe_prev = NULL;
-    }
     if (bs->node_name[0] != '\0') {
         QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
     }
@@ -2062,10 +2046,6 @@ static void change_parent_backing_link(BlockDriverState *from,
     }
     if (from->blk) {
         blk_set_bs(from->blk, to);
-        if (!to->device_list.tqe_prev) {
-            QTAILQ_INSERT_BEFORE(from, to, device_list);
-        }
-        QTAILQ_REMOVE(&bdrv_states, from, device_list);
     }
 }
 
@@ -2745,14 +2725,6 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs)
     return QTAILQ_NEXT(bs, node_list);
 }
 
-BlockDriverState *bdrv_next(BlockDriverState *bs)
-{
-    if (!bs) {
-        return QTAILQ_FIRST(&bdrv_states);
-    }
-    return QTAILQ_NEXT(bs, device_list);
-}
-
 const char *bdrv_get_node_name(const BlockDriverState *bs)
 {
     return bs->node_name;
diff --git a/blockdev.c b/blockdev.c
index 934b9d8..f9c376f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2143,12 +2143,6 @@ void qmp_blockdev_remove_medium(const char *device, Error **errp)
         goto out;
     }
 
-    /* This follows the convention established by bdrv_make_anon() */
-    if (bs->device_list.tqe_prev) {
-        QTAILQ_REMOVE(&bdrv_states, bs, device_list);
-        bs->device_list.tqe_prev = NULL;
-    }
-
     blk_remove_bs(blk);
 
 out:
@@ -2187,8 +2181,6 @@ static void qmp_blockdev_insert_anon_medium(const char *device,
     }
 
     blk_insert_bs(blk, bs);
-
-    QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
 }
 
 void qmp_blockdev_insert_medium(const char *device, const char *node_name,
diff --git a/include/block/block.h b/include/block/block.h
index f47c45a..1abfc70 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -394,7 +394,6 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
                                  Error **errp);
 bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
 BlockDriverState *bdrv_next_node(BlockDriverState *bs);
-BlockDriverState *bdrv_next(BlockDriverState *bs);
 int bdrv_is_encrypted(BlockDriverState *bs);
 int bdrv_key_required(BlockDriverState *bs);
 int bdrv_set_key(BlockDriverState *bs, const char *key);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7408eef..bc186b6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -420,8 +420,6 @@ struct BlockDriverState {
     char node_name[32];
     /* element of the list of named nodes building the graph */
     QTAILQ_ENTRY(BlockDriverState) node_list;
-    /* element of the list of "drives" the guest sees */
-    QTAILQ_ENTRY(BlockDriverState) device_list;
     /* element of the list of all BlockDriverStates (all_bdrv_states) */
     QTAILQ_ENTRY(BlockDriverState) bs_list;
     /* element of the list of monitor-owned BDS */
@@ -478,8 +476,6 @@ extern BlockDriver bdrv_file;
 extern BlockDriver bdrv_raw;
 extern BlockDriver bdrv_qcow2;
 
-extern QTAILQ_HEAD(BdrvStates, BlockDriverState) bdrv_states;
-
 /**
  * bdrv_setup_io_funcs:
  *
-- 
2.6.2

Re: [Qemu-devel] [PATCH v2 0/8] blockdev: Further BlockBackend work

[Eric Blake]

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

[side question]

On 11/09/2015 08:27 PM, Max Reitz wrote:
> Overall, this series adds more uses for BlockBackends and makes the code
> follow the "reference theory" more closely, in that any BlockBackend
> created (through -drive or blockdev-add) has a reference count of 1, and
> that reference should be held by the monitor. This is reflected here by
> introducing an explicit list of monitor-owned BlockBackends, which in
> turn allows us to remove bdrv_states, and, perhaps most importantly,
> blk_hide_on_behalf_of_do_drive_del().
> 

> Although I don't suppose anyone will care much, here's that
> backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/8:[----] [-C] 'block: Add blk_name_taken()'
> 002/8:[----] [-C] 'block: Add blk_next_inserted()'
> 003/8:[----] [-C] 'block: Add blk_commit_all() and blk_invalidate_cache_all()'
> 004/8:[0073] [FC] 'block: Use BlockBackend more'
> 005/8:[0004] [FC] 'blockdev: Add list of monitor-owned BlockBackends'
> 006/8:[down] 'blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()'
>       (renamed from "..._on_behalf_of_do_drive_del()")

This is a nice note. Is there any way to make 'git backport-diff'
automatically do this, or does it have to be done manually when renaming
patches?

> 007/8:[0213] [FC] 'block: Move some bdrv_*_all() functions to BB'
> 008/8:[0027] [FC] 'block: Remove bdrv_states'
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v2 0/8] blockdev: Further BlockBackend work

[Max Reitz]

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

On 10.11.2015 04:45, Eric Blake wrote:
> [side question]
> 
> On 11/09/2015 08:27 PM, Max Reitz wrote:
>> Overall, this series adds more uses for BlockBackends and makes the code
>> follow the "reference theory" more closely, in that any BlockBackend
>> created (through -drive or blockdev-add) has a reference count of 1, and
>> that reference should be held by the monitor. This is reflected here by
>> introducing an explicit list of monitor-owned BlockBackends, which in
>> turn allows us to remove bdrv_states, and, perhaps most importantly,
>> blk_hide_on_behalf_of_do_drive_del().
>>
> 
>> Although I don't suppose anyone will care much, here's that
>> backport-diff against v1:
>>
>> Key:
>> [----] : patches are identical
>> [####] : number of functional differences between upstream/downstream patch
>> [down] : patch is downstream-only
>> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>>
>> 001/8:[----] [-C] 'block: Add blk_name_taken()'
>> 002/8:[----] [-C] 'block: Add blk_next_inserted()'
>> 003/8:[----] [-C] 'block: Add blk_commit_all() and blk_invalidate_cache_all()'
>> 004/8:[0073] [FC] 'block: Use BlockBackend more'
>> 005/8:[0004] [FC] 'blockdev: Add list of monitor-owned BlockBackends'
>> 006/8:[down] 'blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()'
>>       (renamed from "..._on_behalf_of_do_drive_del()")
> 
> This is a nice note. Is there any way to make 'git backport-diff'
> automatically do this, or does it have to be done manually when renaming
> patches?

As far as I know, git backport-diff simply looks for matching patches
based on the title alone, so I don't think there's a way for it to
automatically figure out name changes.

I don't know of a way of telling it that a particular "downstream" patch
belongs to another particular "upstream" patch, though. So in general,
if I do want an accurate diff even for renamed patches (I did not here,
because v1 has been more than eight months ago), I just diff manually
and count the changes or temporarily rename the patch on the current
(new) branch.

Max


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

Re: [Qemu-devel] [PATCH v2 0/8] blockdev: Further BlockBackend work

[Jeff Cody]

On Tue, Nov 10, 2015 at 04:49:12AM +0100, Max Reitz wrote:
> On 10.11.2015 04:45, Eric Blake wrote:
> > [side question]
> > 
> > On 11/09/2015 08:27 PM, Max Reitz wrote:
> >> Overall, this series adds more uses for BlockBackends and makes the code
> >> follow the "reference theory" more closely, in that any BlockBackend
> >> created (through -drive or blockdev-add) has a reference count of 1, and
> >> that reference should be held by the monitor. This is reflected here by
> >> introducing an explicit list of monitor-owned BlockBackends, which in
> >> turn allows us to remove bdrv_states, and, perhaps most importantly,
> >> blk_hide_on_behalf_of_do_drive_del().
> >>
> > 
> >> Although I don't suppose anyone will care much, here's that
> >> backport-diff against v1:
> >>
> >> Key:
> >> [----] : patches are identical
> >> [####] : number of functional differences between upstream/downstream patch
> >> [down] : patch is downstream-only
> >> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> >>
> >> 001/8:[----] [-C] 'block: Add blk_name_taken()'
> >> 002/8:[----] [-C] 'block: Add blk_next_inserted()'
> >> 003/8:[----] [-C] 'block: Add blk_commit_all() and blk_invalidate_cache_all()'
> >> 004/8:[0073] [FC] 'block: Use BlockBackend more'
> >> 005/8:[0004] [FC] 'blockdev: Add list of monitor-owned BlockBackends'
> >> 006/8:[down] 'blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()'
> >>       (renamed from "..._on_behalf_of_do_drive_del()")
> > 
> > This is a nice note. Is there any way to make 'git backport-diff'
> > automatically do this, or does it have to be done manually when renaming
> > patches?
> 
> As far as I know, git backport-diff simply looks for matching patches
> based on the title alone, so I don't think there's a way for it to
> automatically figure out name changes.
> 
> I don't know of a way of telling it that a particular "downstream" patch
> belongs to another particular "upstream" patch, though. So in general,
> if I do want an accurate diff even for renamed patches (I did not here,
> because v1 has been more than eight months ago), I just diff manually
> and count the changes or temporarily rename the patch on the current
> (new) branch.
> 
> Max
> 

Yeah, it is a limitation of the "match-by-title" method.  I suppose we
could add a couple different matching "engines", some of which may
work better for some scenarios rather than others (for instance,
looking for cherry-pick lines, etc..).  Maybe even go so far as try
and match patches based on similar content.

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/8] block: Add blk_next_inserted()

[Alberto Garcia]

On Tue 10 Nov 2015 04:27:43 AM CET, Max Reitz <mreitz@redhat.com> wrote:
> This function skips to the next BlockBackend for which blk_is_inserted()
> is true.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

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

Berto

Re: [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_states

[Kevin Wolf]

Am 10.11.2015 um 04:27 hat Max Reitz geschrieben:
> Every entry in this list should be a root BDS and as such either be
> anchored to a BlockBackend or be owned by the monitor.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   | 30 +-----------------------------
>  blockdev.c                |  8 --------
>  include/block/block.h     |  1 -
>  include/block/block_int.h |  4 ----
>  4 files changed, 1 insertion(+), 42 deletions(-)

>  void bdrv_make_anon(BlockDriverState *bs)
>  {
> -    /*
> -     * Take care to remove bs from bdrv_states only when it's actually
> -     * in it.  Note that bs->device_list.tqe_prev is initially null,
> -     * and gets set to non-null by QTAILQ_INSERT_TAIL().  Establish
> -     * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
> -     * resetting it to null on remove.
> -     */
> -    if (bs->device_list.tqe_prev) {
> -        QTAILQ_REMOVE(&bdrv_states, bs, device_list);
> -        bs->device_list.tqe_prev = NULL;
> -    }
>      if (bs->node_name[0] != '\0') {
>          QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
>      }

bdrv_make_anon() has only a single user at this point and the remaining
part after this patch is so small that we could consider inlining it.

Kevin

Re: [Qemu-devel] [PATCH v2 7/8] block: Move some bdrv_*_all() functions to BB

[Kevin Wolf]

Am 10.11.2015 um 04:27 hat Max Reitz geschrieben:
> Move bdrv_drain_all(), bdrv_commit_all(), bdrv_flush_all() and
> bdrv_invalidate_cache_all() to BB.
> 
> The only operation left is bdrv_close_all(), which cannot be moved to
> the BB because it should not only close all BBs, but also all
> monitor-owned BDSs.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

bdrv_commit_all() and bdrv_flush_all() are relatively obvious. They are
meant to commit/flush changes made by a guest, so moving to the BB level
seems right.

I'm not so sure about bdrv_invalidate_cache_all(). Even if a BB isn't
attached to a BDS, we still need to invalidate the caches of any images
that have been opened before migration has completed, including those
images that are only owned by the monitor.

Similarly I'm concerned about bdrv_drain_all(). Anything outside the
block layer should certainly call blk_drain_all() because it can only be
interested in those images that it can see. The calls in block.c,
however, look like they should consider BDSes without a BB as well. (The
monitor, which can hold direct BDS references, may be another valid user
of a bdrv_drain_all that also covers BDSes without a BB.)

And block.c calling blk_*() functions smells a bit fishy anyway.

Kevin

Re: [Qemu-devel] [PATCH v2 7/8] block: Move some bdrv_*_all() functions to BB

[Max Reitz]

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

On 01.12.2015 17:01, Kevin Wolf wrote:
> Am 10.11.2015 um 04:27 hat Max Reitz geschrieben:
>> Move bdrv_drain_all(), bdrv_commit_all(), bdrv_flush_all() and
>> bdrv_invalidate_cache_all() to BB.
>>
>> The only operation left is bdrv_close_all(), which cannot be moved to
>> the BB because it should not only close all BBs, but also all
>> monitor-owned BDSs.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> bdrv_commit_all() and bdrv_flush_all() are relatively obvious. They are
> meant to commit/flush changes made by a guest, so moving to the BB level
> seems right.

OK, I wanted to just follow this comment, but since monitor_bdrv_states
is local to blockdev.c (and I consider that correct) that would mean
either making it global (which I wouldn't like) or keeping bdrv_states
(which I wouldn't like either, not least because dropping bdrv_states
allows us to drop bdrv_new_root(), which may or may not be crucial to
the follow-up series to this one).

So, I'll be trying to defend what this patch does for now.

> I'm not so sure about bdrv_invalidate_cache_all(). Even if a BB isn't
> attached to a BDS, we still need to invalidate the caches of any images
> that have been opened before migration has completed, including those
> images that are only owned by the monitor.

I consider BB-less BDSs to be in a temporary state between blockdev-add
and binding them to a device, or between unbinding them from a device
and blockdev-del. So I don't even know if we should allow them to be
migrated.

Anyway, in my opinion, a BB-less BDS should be totally static.
Invalidating its cache shouldn't do anything. Instead, its cache should
be invalidated when it is detached from a BB (just like this patch adds
a bdrv_drain() call to blk_remove_bs()).

> Similarly I'm concerned about bdrv_drain_all(). Anything outside the
> block layer should certainly call blk_drain_all() because it can only be
> interested in those images that it can see. The calls in block.c,
> however, look like they should consider BDSes without a BB as well. (The
> monitor, which can hold direct BDS references, may be another valid user
> of a bdrv_drain_all that also covers BDSes without a BB.)

Similarly, in my opinion, bdrv_drain() shouldn't do anything on a
BB-less BDS. That is why this patch adds a bdrv_drain() call to
blk_remove_bs() (a bdrv_invalidate_cache() call is missing yet, though).

But I just remembered that block jobs don't use BBs... Did I mention
before how wrong I think that is?

Now I'm torn between trying to make block jobs use BBs once more
(because I really think BB-less BDSs should not have any active I/O) and
doing some ugly things with bdrv_states and/or bdrv_monitor_states. I'll
have to think about it.

> And block.c calling blk_*() functions smells a bit fishy anyway.

I don't find it any more fishy than single-BDS functions calling
bdrv_*_all() functions. And that is, not fishy at all. If some function
wants to drain all I/O and we define I/O to always originate from a BB,
then I find the only reasonable approach to call blk_drain_all().

Max


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

Re: [Qemu-devel] [PATCH v2 7/8] block: Move some bdrv_*_all() functions to BB

[Kevin Wolf]

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

Am 05.12.2015 um 00:15 hat Max Reitz geschrieben:
> On 01.12.2015 17:01, Kevin Wolf wrote:
> > Am 10.11.2015 um 04:27 hat Max Reitz geschrieben:
> >> Move bdrv_drain_all(), bdrv_commit_all(), bdrv_flush_all() and
> >> bdrv_invalidate_cache_all() to BB.
> >>
> >> The only operation left is bdrv_close_all(), which cannot be moved to
> >> the BB because it should not only close all BBs, but also all
> >> monitor-owned BDSs.
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> > 
> > bdrv_commit_all() and bdrv_flush_all() are relatively obvious. They are
> > meant to commit/flush changes made by a guest, so moving to the BB level
> > seems right.
> 
> OK, I wanted to just follow this comment, but since monitor_bdrv_states
> is local to blockdev.c (and I consider that correct) that would mean
> either making it global (which I wouldn't like) or keeping bdrv_states
> (which I wouldn't like either, not least because dropping bdrv_states
> allows us to drop bdrv_new_root(), which may or may not be crucial to
> the follow-up series to this one).
> 
> So, I'll be trying to defend what this patch does for now.
> 
> > I'm not so sure about bdrv_invalidate_cache_all(). Even if a BB isn't
> > attached to a BDS, we still need to invalidate the caches of any images
> > that have been opened before migration has completed, including those
> > images that are only owned by the monitor.
> 
> I consider BB-less BDSs to be in a temporary state between blockdev-add
> and binding them to a device, or between unbinding them from a device
> and blockdev-del. So I don't even know if we should allow them to be
> migrated.

If we don't want to, we should add a migration blocker while such a BDS
exists.

I don't think that would be right, though. Definitely not in the strict
way you phrased it ("binding them to a device"), but probably also not
if you interpret "device" as any kind of user, including block jobs or
NBD servers; actually, I think even the monitor would belong in this
list, but then you always have "something" attached, otherwise the
refcount becomes zero and the BDS is deleted.

If you don't include the monitor, though, you prevent entirely
reasonable use cases like opening upfront multiple images for a device
with removable media and then changing between them later on, which
leaves some BDSes unattached while another medium is inserted.

I think BDSes without a BB attached should still be first-class
citizens.

> Anyway, in my opinion, a BB-less BDS should be totally static.
> Invalidating its cache shouldn't do anything. Instead, its cache should
> be invalidated when it is detached from a BB (just like this patch adds
> a bdrv_drain() call to blk_remove_bs()).

Are you sure you remember what bdrv_invalidate_cache() is for? Its job
is dropping stale cache contents after a mere bdrv_open() when migrating
with shared storage. It is wrong to ever use this after actually using
(instead of just opening) an image.

The order is like this:

1. Start destination qemu -incoming. This opens the image and
   potentially reads in some metadata (qcow2 L1 table, etc.)
2. Source keeps writing to the image.
3. Migration completes. Source flushes the image and stops writing.
4. Destination must call bdrv_invalidate_cache() to make sure it gets
   all the updates from step 2 instead of using stale data from step 1.

Note how there is no detaching from a BB involved at all.

> > Similarly I'm concerned about bdrv_drain_all(). Anything outside the
> > block layer should certainly call blk_drain_all() because it can only be
> > interested in those images that it can see. The calls in block.c,
> > however, look like they should consider BDSes without a BB as well. (The
> > monitor, which can hold direct BDS references, may be another valid user
> > of a bdrv_drain_all that also covers BDSes without a BB.)
> 
> Similarly, in my opinion, bdrv_drain() shouldn't do anything on a
> BB-less BDS. That is why this patch adds a bdrv_drain() call to
> blk_remove_bs() (a bdrv_invalidate_cache() call is missing yet, though).
> 
> But I just remembered that block jobs don't use BBs... Did I mention
> before how wrong I think that is?
> 
> Now I'm torn between trying to make block jobs use BBs once more
> (because I really think BB-less BDSs should not have any active I/O) and
> doing some ugly things with bdrv_states and/or bdrv_monitor_states. I'll
> have to think about it.

I think we first need to allow multiple BBs per BDS, so that block jobs
can create their own BB.

And anyway, this seems to be a great start for discussing our whole BB
model in person on Friday. As I said, I think what we're currently doing
is wrong. I am concerned that introducing a model that actually makes
sense is hard to do in a compatible way, but I'm getting less and less
confident that doing it like we do now is the right conclusion from that.

> > And block.c calling blk_*() functions smells a bit fishy anyway.
> 
> I don't find it any more fishy than single-BDS functions calling
> bdrv_*_all() functions. And that is, not fishy at all. If some function
> wants to drain all I/O and we define I/O to always originate from a BB,
> then I find the only reasonable approach to call blk_drain_all().

bdrv_*_all() calls are always fishy, at least if done outside shutdown.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]