[Qemu-devel] [PATCH 00/20] block: Rework bdrv_close_all()

[Qemu-devel] [PATCH 00/20] block: Rework bdrv_close_all()

[Max Reitz]

Currently, bdrv_close_all() force-closes all BDSs with a BlockBackend,
which can lead to data corruption (see the iotest added in the final
patch of this series) and is most certainly very ugly.

This series reworks bdrv_close_all() to notify all owners of a
BlockBackend that they should release their reference (and additionally
the monitor releases all its references to BB-less BDSs). This way,
force-closing becomes unnecessary.

Also, blk_hide_on_behalf_of_do_drive_del() is removed. Yay!


This series depends on my series "blockdev: BlockBackend and media" (or
any later version).


Max Reitz (21):
  block: Guard remaining unsafe blk_bs() callers
  quorum: Fix close path
  block: Add bdrv_close_all() notifiers
  block: Add bdrv_close_all() handlers
  block: Remove per-BDS close notifiers
  block: Use blk_remove_bs() in blk_delete()
  blockdev: Use blk_remove_bs() in do_drive_del()
  block: Make bdrv_close() static
  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_do_drive_del()
  block: Make bdrv_drain_one() public
  block: Move some bdrv_*_all() functions to BB
  block: Remove bdrv_states
  blockdev: Keep track of monitor-owned BDS
  block: Strip down bdrv_close_all()
  iotests: Add "wait" functionality to _cleanup_qemu
  iotests: Add test for multiple BB on BDS tree

 block.c                        | 166 ++++-------------------
 block/block-backend.c          | 297 ++++++++++++++++++++++++++++++++---------
 block/qapi.c                   |  13 +-
 block/quorum.c                 |   3 +-
 block/snapshot.c               |   3 +-
 blockdev-nbd.c                 |  36 +----
 blockdev.c                     | 139 +++++++++++++++----
 cpus.c                         |   7 +-
 device-hotplug.c               |   2 +-
 hw/block/xen_disk.c            |  20 ++-
 hw/ide/piix.c                  |   1 -
 include/block/block.h          |   9 +-
 include/block/block_int.h      |   8 +-
 include/sysemu/block-backend.h |  23 ++--
 include/sysemu/blockdev.h      |   3 +
 migration/block.c              |  10 +-
 migration/migration.c          |   4 +-
 monitor.c                      |  13 +-
 nbd.c                          |  37 ++++-
 qemu-char.c                    |   3 +-
 qemu-img.c                     |  39 +++---
 qemu-io.c                      |   8 +-
 qemu-nbd.c                     |   6 +-
 qmp.c                          |  14 +-
 savevm.c                       |  66 +++++----
 stubs/Makefile.objs            |   2 +-
 stubs/bdrv-commit-all.c        |   7 -
 stubs/blk-commit-all.c         |   7 +
 tests/qemu-iotests/117         |  86 ++++++++++++
 tests/qemu-iotests/117.out     |  14 ++
 tests/qemu-iotests/common.qemu |  12 +-
 tests/qemu-iotests/group       |   1 +
 xen-mapcache.c                 |   3 +-
 33 files changed, 681 insertions(+), 381 deletions(-)
 delete mode 100644 stubs/bdrv-commit-all.c
 create mode 100644 stubs/blk-commit-all.c
 create mode 100755 tests/qemu-iotests/117
 create mode 100644 tests/qemu-iotests/117.out

-- 
2.1.0

[Qemu-devel] [PATCH 01/21] block: Guard remaining unsafe blk_bs() callers

[Max Reitz]

There are cases where it is probably (!) not necessary to check whether
the return value of blk_bs() is non-NULL, and those are places after
blk_new_open(). In every other place, though, there has to be some check
to make sure that the return value of blk_bs() is non-NULL before it is
used.

This patch adds that check to hopefully all of the remaining places.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c          | 18 ++++++++++--------
 hw/block/xen_disk.c |  4 +++-
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8ed2fec..9c5b6ac 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -121,14 +121,16 @@ void blockdev_mark_auto_del(BlockBackend *blk)
         return;
     }
 
-    aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
+    if (bs) {
+        aio_context = bdrv_get_aio_context(bs);
+        aio_context_acquire(aio_context);
 
-    if (bs->job) {
-        block_job_cancel(bs->job);
-    }
+        if (bs->job) {
+            block_job_cancel(bs->job);
+        }
 
-    aio_context_release(aio_context);
+        aio_context_release(aio_context);
+    }
 
     dinfo->auto_del = 1;
 }
@@ -228,8 +230,8 @@ bool drive_check_orphaned(void)
             dinfo->type != IF_NONE) {
             fprintf(stderr, "Warning: Orphaned drive without device: "
                     "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
-                    blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
-                    dinfo->bus, dinfo->unit);
+                    blk_name(blk), blk_bs(blk) ? blk_bs(blk)->filename : "",
+                    if_name[dinfo->type], dinfo->bus, dinfo->unit);
             rs = true;
         }
     }
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 1b0257c..665e706 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -933,9 +933,11 @@ static int blk_connect(struct XenDevice *xendev)
     blk_attach_dev_nofail(blkdev->blk, blkdev);
     blkdev->file_size = blk_getlength(blkdev->blk);
     if (blkdev->file_size < 0) {
+        BlockDriverState *bs = blk_bs(blkdev->blk);
+        const char *drv_name = bs ? bdrv_get_format_name(bs) : NULL;
         xen_be_printf(&blkdev->xendev, 1, "blk_getlength: %d (%s) | drv %s\n",
                       (int)blkdev->file_size, strerror(-blkdev->file_size),
-                      bdrv_get_format_name(blk_bs(blkdev->blk)) ?: "-");
+                      drv_name ?: "-");
         blkdev->file_size = 0;
     }
 
-- 
2.1.0

[Qemu-devel] [PATCH 02/21] quorum: Fix close path

[Max Reitz]

bdrv_unref() can lead to bdrv_close(), which in turn will result in
bdrv_drain_all(). This function will later be called blk_drain_all() and
iterate only over the BlockBackends for which blk_is_inserted() holds
true; therefore, bdrv_is_inserted() and thus quorum_is_inserted() will
probably be called.

This patch makes quorum_is_inserted() aware of the fact that some
children may have been closed already.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/quorum.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/quorum.c b/block/quorum.c
index 7811c4a..b249d75 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1005,6 +1005,7 @@ static void quorum_close(BlockDriverState *bs)
 
     for (i = 0; i < s->num_children; i++) {
         bdrv_unref(s->bs[i]);
+        s->bs[i] = NULL;
     }
 
     g_free(s->bs);
@@ -1070,7 +1071,7 @@ static int quorum_is_inserted(BlockDriverState *bs)
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        if (!bdrv_is_inserted(s->bs[i])) {
+        if (s->bs[i] && !bdrv_is_inserted(s->bs[i])) {
             return 0;
         }
     }
-- 
2.1.0

[Qemu-devel] [PATCH 03/21] block: Add bdrv_close_all() notifiers

[Max Reitz]

This adds a list of notifiers to be invoked on bdrv_close_all().

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

diff --git a/block.c b/block.c
index b7e631c..3adb724 100644
--- a/block.c
+++ b/block.c
@@ -104,6 +104,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
+static NotifierList close_all_notifiers =
+    NOTIFIER_LIST_INITIALIZER(close_all_notifiers);
+
 #ifdef _WIN32
 static int is_windows_drive_prefix(const char *filename)
 {
@@ -1913,6 +1916,8 @@ void bdrv_close_all(void)
 {
     BlockDriverState *bs;
 
+    notifier_list_notify(&close_all_notifiers, NULL);
+
     QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
@@ -1922,6 +1927,11 @@ void bdrv_close_all(void)
     }
 }
 
+void bdrv_add_close_all_notifier(Notifier *notifier)
+{
+    notifier_list_add(&close_all_notifiers, notifier);
+}
+
 /* Check if any requests are in-flight (including throttled requests) */
 static bool bdrv_requests_pending(BlockDriverState *bs)
 {
diff --git a/include/block/block.h b/include/block/block.h
index fa9dfd5..301baac 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -539,4 +539,6 @@ void bdrv_io_plug(BlockDriverState *bs);
 void bdrv_io_unplug(BlockDriverState *bs);
 void bdrv_flush_io_queue(BlockDriverState *bs);
 
+void bdrv_add_close_all_notifier(Notifier *notifier);
+
 #endif
-- 
2.1.0

[Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Max Reitz]

Every time a reference to a BlockBackend is taken, a notifier for
bdrv_close_all() has to be deposited so the reference holder can
relinquish its reference when bdrv_close_all() is called. That notifier
should be revoked on a bdrv_unref() call.

Add a Notifier * parameter to all the functions changing the reference
count of a BlockBackend: blk_new(), blk_new_with_bs(), blk_new_open(),
blk_ref() and blk_unref().

By dropping the manual reference handling in hw/block/xen_disk.c, the
blk_unref() in hw/ide/piix.c can be dropped as well.

NBD used close notifiers so far; this patch changes it to use the
bdrv_close_all() notifier as well. The change in behavior is the
following: First, the close notifier was of course called indirectly
through bdrv_close_all(), so this case will not change.

Second, as bdrv_close() could not be called because of a bdrv_delete()
(NBD holds a reference to the BB which in turn holds a BDS reference),
the only other way for the notifier to be called was because of an
eject. However, this was wrong: With the NBD attached to some BB, it
should stay attached until the NBD export is removed or the server
stopped; medium ejection should have nothing to do with this (other than
the NBD server not being able to read data while the virtual is open, if
there is one (and there is one if the NBD server is attached to the same
BB as a guest device with a tray is; which is correct behavior because
attaching an NBD server to a guest-used BB should result in the NBD
server behaving exactly the same as the guest device)).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 69 ++++++++++++++++++++++++++++++++++++------
 blockdev-nbd.c                 | 36 +---------------------
 blockdev.c                     | 62 ++++++++++++++++++++++++++++++++++---
 device-hotplug.c               |  2 +-
 hw/block/xen_disk.c            | 16 +++++++---
 hw/ide/piix.c                  |  1 -
 include/sysemu/block-backend.h | 13 +++++---
 include/sysemu/blockdev.h      |  3 ++
 nbd.c                          | 37 ++++++++++++++++++++--
 qemu-img.c                     | 39 ++++++++++++------------
 qemu-io.c                      |  6 ++--
 qemu-nbd.c                     |  6 ++--
 12 files changed, 202 insertions(+), 88 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 7c18f8d..9f14f9b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -74,7 +74,8 @@ static QTAILQ_HEAD(, BlockBackend) blk_backends =
  * Store an error through @errp on failure, unless it's null.
  * Return the new BlockBackend on success, null on failure.
  */
-BlockBackend *blk_new(const char *name, Error **errp)
+BlockBackend *blk_new(const char *name, Notifier *close_all_notifier,
+                      Error **errp)
 {
     BlockBackend *blk;
 
@@ -97,6 +98,9 @@ BlockBackend *blk_new(const char *name, Error **errp)
     blk = g_new0(BlockBackend, 1);
     blk->name = g_strdup(name);
     blk->refcnt = 1;
+    if (close_all_notifier) {
+        bdrv_add_close_all_notifier(close_all_notifier);
+    }
     QTAILQ_INSERT_TAIL(&blk_backends, blk, link);
     return blk;
 }
@@ -105,12 +109,13 @@ BlockBackend *blk_new(const char *name, Error **errp)
  * Create a new BlockBackend with a new BlockDriverState attached.
  * Otherwise just like blk_new(), which see.
  */
-BlockBackend *blk_new_with_bs(const char *name, Error **errp)
+BlockBackend *blk_new_with_bs(const char *name, Notifier *close_all_notifier,
+                              Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
 
-    blk = blk_new(name, errp);
+    blk = blk_new(name, close_all_notifier, errp);
     if (!blk) {
         return NULL;
     }
@@ -135,12 +140,12 @@ BlockBackend *blk_new_with_bs(const char *name, Error **errp)
  */
 BlockBackend *blk_new_open(const char *name, const char *filename,
                            const char *reference, QDict *options, int flags,
-                           Error **errp)
+                           Notifier *close_all_notifier, Error **errp)
 {
     BlockBackend *blk;
     int ret;
 
-    blk = blk_new_with_bs(name, errp);
+    blk = blk_new_with_bs(name, close_all_notifier, errp);
     if (!blk) {
         QDECREF(options);
         return NULL;
@@ -148,7 +153,7 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
 
     ret = bdrv_open(&blk->bs, filename, reference, options, flags, NULL, errp);
     if (ret < 0) {
-        blk_unref(blk);
+        blk_unref(blk, close_all_notifier);
         return NULL;
     }
 
@@ -188,9 +193,13 @@ static void drive_info_del(DriveInfo *dinfo)
  * Increment @blk's reference count.
  * @blk must not be null.
  */
-void blk_ref(BlockBackend *blk)
+void blk_ref(BlockBackend *blk, Notifier *close_all_notifier)
 {
     blk->refcnt++;
+
+    if (close_all_notifier) {
+        bdrv_add_close_all_notifier(close_all_notifier);
+    }
 }
 
 /*
@@ -198,8 +207,12 @@ void blk_ref(BlockBackend *blk)
  * If this drops it to zero, destroy @blk.
  * For convenience, do nothing if @blk is null.
  */
-void blk_unref(BlockBackend *blk)
+void blk_unref(BlockBackend *blk, Notifier *close_all_notifier)
 {
+    if (close_all_notifier) {
+        notifier_remove(close_all_notifier);
+    }
+
     if (blk) {
         assert(blk->refcnt > 0);
         if (!--blk->refcnt) {
@@ -347,6 +360,21 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
     bs->blk = blk;
 }
 
+typedef struct DevCloseAllNotifier {
+    Notifier n;
+    BlockBackend *blk;
+    QTAILQ_ENTRY(DevCloseAllNotifier) next;
+} DevCloseAllNotifier;
+
+static QTAILQ_HEAD(, DevCloseAllNotifier) close_all_notifiers =
+    QTAILQ_HEAD_INITIALIZER(close_all_notifiers);
+
+static void dev_close_all_notifier(Notifier *n, void *data)
+{
+    DevCloseAllNotifier *can = DO_UPCAST(DevCloseAllNotifier, n, n);
+    blk_detach_dev(can->blk, can->blk->dev);
+}
+
 /*
  * Attach device model @dev to @blk.
  * Return 0 on success, -EBUSY when a device model is attached already.
@@ -354,10 +382,19 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
 int blk_attach_dev(BlockBackend *blk, void *dev)
 /* TODO change to DeviceState *dev when all users are qdevified */
 {
+    DevCloseAllNotifier *can;
+
     if (blk->dev) {
         return -EBUSY;
     }
-    blk_ref(blk);
+
+    can = g_new0(DevCloseAllNotifier, 1);
+    can->n.notify = dev_close_all_notifier;
+    can->blk = blk;
+
+    QTAILQ_INSERT_TAIL(&close_all_notifiers, can, next);
+
+    blk_ref(blk, &can->n);
     blk->dev = dev;
     blk_iostatus_reset(blk);
     return 0;
@@ -382,12 +419,24 @@ void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
 void blk_detach_dev(BlockBackend *blk, void *dev)
 /* TODO change to DeviceState *dev when all users are qdevified */
 {
+    DevCloseAllNotifier *can;
+
     assert(blk->dev == dev);
     blk->dev = NULL;
     blk->dev_ops = NULL;
     blk->dev_opaque = NULL;
     blk->guest_block_size = 512;
-    blk_unref(blk);
+
+    QTAILQ_FOREACH(can, &close_all_notifiers, next) {
+        if (can->blk == blk) {
+            break;
+        }
+    }
+    assert(can);
+
+    blk_unref(blk, &can->n);
+    QTAILQ_REMOVE(&close_all_notifiers, can, next);
+    g_free(can);
 }
 
 /*
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 22e95d1..eb5f9a0 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -47,36 +47,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
     }
 }
 
-/* Hook into the BlockDriverState notifiers to close the export when
- * the file is closed.
- */
-typedef struct NBDCloseNotifier {
-    Notifier n;
-    NBDExport *exp;
-    QTAILQ_ENTRY(NBDCloseNotifier) next;
-} NBDCloseNotifier;
-
-static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
-    QTAILQ_HEAD_INITIALIZER(close_notifiers);
-
-static void nbd_close_notifier(Notifier *n, void *data)
-{
-    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
-
-    notifier_remove(&cn->n);
-    QTAILQ_REMOVE(&close_notifiers, cn, next);
-
-    nbd_export_close(cn->exp);
-    nbd_export_put(cn->exp);
-    g_free(cn);
-}
-
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
                         Error **errp)
 {
     BlockBackend *blk;
     NBDExport *exp;
-    NBDCloseNotifier *n;
 
     if (server_fd == -1) {
         error_setg(errp, "NBD server not running");
@@ -108,20 +83,11 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL);
 
     nbd_export_set_name(exp, device);
-
-    n = g_new0(NBDCloseNotifier, 1);
-    n->n.notify = nbd_close_notifier;
-    n->exp = exp;
-    blk_add_close_notifier(blk, &n->n);
-    QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
 }
 
 void qmp_nbd_server_stop(Error **errp)
 {
-    while (!QTAILQ_EMPTY(&close_notifiers)) {
-        NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers);
-        nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp));
-    }
+    nbd_export_close_all();
 
     if (server_fd != -1) {
         qemu_set_fd_handler2(server_fd, NULL, NULL, NULL, NULL);
diff --git a/blockdev.c b/blockdev.c
index 9c5b6ac..fd3f50d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -47,6 +47,15 @@
 #include "trace.h"
 #include "sysemu/arch_init.h"
 
+typedef struct BlockdevCloseAllNotifier {
+    Notifier n;
+    BlockBackend *blk;
+    QTAILQ_ENTRY(BlockdevCloseAllNotifier) next;
+} BlockdevCloseAllNotifier;
+
+static QTAILQ_HEAD(, BlockdevCloseAllNotifier) close_all_notifiers =
+    QTAILQ_HEAD_INITIALIZER(close_all_notifiers);
+
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
     [IF_IDE] = "ide",
@@ -78,6 +87,37 @@ static int if_max_devs[IF_COUNT] = {
     [IF_SCSI] = 7,
 };
 
+static void monitor_blk_unref_with_can(BlockBackend *blk,
+                                       BlockdevCloseAllNotifier *can)
+{
+    if (can) {
+        assert(can->blk == blk);
+    } else {
+        QTAILQ_FOREACH(can, &close_all_notifiers, next) {
+            if (can->blk == blk) {
+                break;
+            }
+        }
+        assert(can);
+    }
+
+    blk_unref(blk, &can->n);
+    QTAILQ_REMOVE(&close_all_notifiers, can, next);
+    g_free(can);
+}
+
+void monitor_blk_unref(BlockBackend *blk)
+{
+    monitor_blk_unref_with_can(blk, NULL);
+}
+
+static void blockdev_close_all_notifier(Notifier *n, void *data)
+{
+    BlockdevCloseAllNotifier *can = DO_UPCAST(BlockdevCloseAllNotifier, n, n);
+
+    monitor_blk_unref_with_can(can->blk, can);
+}
+
 /**
  * Boards may call this to offer board-by-board overrides
  * of the default, global values.
@@ -140,7 +180,7 @@ void blockdev_auto_del(BlockBackend *blk)
     DriveInfo *dinfo = blk_legacy_dinfo(blk);
 
     if (dinfo && dinfo->auto_del) {
-        blk_unref(blk);
+        monitor_blk_unref(blk);
     }
 }
 
@@ -460,6 +500,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     bool has_driver_specific_opts;
     BlockdevDetectZeroesOptions detect_zeroes =
         BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
+    BlockdevCloseAllNotifier *can;
 
     /* Check common options by copying from bs_opts to opts, all other options
      * stay in bs_opts for processing by bdrv_open(). */
@@ -536,14 +577,21 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     }
 
     /* init */
+    can = g_new0(BlockdevCloseAllNotifier, 1);
+    can->n.notify = blockdev_close_all_notifier;
+
     if ((!file || !*file) && !has_driver_specific_opts) {
         BlockBackendRootState *blk_rs;
 
-        blk = blk_new(qemu_opts_id(opts), errp);
+        blk = blk_new(qemu_opts_id(opts), &can->n, errp);
         if (!blk) {
+            g_free(can);
             goto early_err;
         }
 
+        can->blk = blk;
+        QTAILQ_INSERT_TAIL(&close_all_notifiers, can, next);
+
         blk_rs = blk_get_root_state(blk);
         blk_rs->open_flags    = bdrv_flags;
         blk_rs->read_only     = !(bdrv_flags & BDRV_O_RDWR);
@@ -561,12 +609,16 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         }
 
         blk = blk_new_open(qemu_opts_id(opts), file, NULL, bs_opts, bdrv_flags,
-                           errp);
+                           &can->n, errp);
         if (!blk) {
+            g_free(can);
             goto err_no_bs_opts;
         }
         bs = blk_bs(blk);
 
+        can->blk = blk;
+        QTAILQ_INSERT_TAIL(&close_all_notifiers, can, next);
+
         bs->detect_zeroes = detect_zeroes;
 
         /* disk I/O throttling */
@@ -2250,7 +2302,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
         blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
                          BLOCKDEV_ON_ERROR_REPORT);
     } else {
-        blk_unref(blk);
+        monitor_blk_unref(blk);
     }
 
     aio_context_release(aio_context);
@@ -3174,7 +3226,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     if (bs && bdrv_key_required(bs)) {
         if (blk) {
-            blk_unref(blk);
+            monitor_blk_unref(blk);
         } else {
             bdrv_unref(bs);
         }
diff --git a/device-hotplug.c b/device-hotplug.c
index 9e38cc4..aa05a71 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -77,6 +77,6 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
 
 err:
     if (dinfo) {
-        blk_unref(blk_by_legacy_dinfo(dinfo));
+        monitor_blk_unref(blk_by_legacy_dinfo(dinfo));
     }
 }
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 665e706..3f80cbe 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -909,8 +909,10 @@ static int blk_connect(struct XenDevice *xendev)
 
         /* setup via xenbus -> create new block driver instance */
         xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
+        /* No need to give a close_all_notifier because blk_attach_dev_nofail()
+         * and blk_detach_dev() will keep track of our reference */
         blkdev->blk = blk_new_open(blkdev->dev, blkdev->filename, NULL, options,
-                                   qflags, &local_err);
+                                   qflags, NULL, &local_err);
         if (!blkdev->blk) {
             xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
                           error_get_pretty(local_err));
@@ -926,11 +928,16 @@ static int blk_connect(struct XenDevice *xendev)
             blkdev->blk = NULL;
             return -1;
         }
-        /* blkdev->blk is not create by us, we get a reference
-         * so we can blk_unref() unconditionally */
-        blk_ref(blkdev->blk);
     }
+
     blk_attach_dev_nofail(blkdev->blk, blkdev);
+    if (!blkdev->dinfo) {
+        /* blkdev->blk has just been created; blk_attach_dev_nofail() counts
+         * the reference blkdev->blk, so we do not have to keep track (which
+         * allows us to ignore bdrv_close_all()) */
+        blk_unref(blkdev->blk, NULL);
+    }
+
     blkdev->file_size = blk_getlength(blkdev->blk);
     if (blkdev->file_size < 0) {
         BlockDriverState *bs = blk_bs(blkdev->blk);
@@ -1033,7 +1040,6 @@ static void blk_disconnect(struct XenDevice *xendev)
 
     if (blkdev->blk) {
         blk_detach_dev(blkdev->blk, blkdev);
-        blk_unref(blkdev->blk);
         blkdev->blk = NULL;
     }
     xen_be_unbind_evtchn(&blkdev->xendev);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index b0172fb..becf0f5 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -184,7 +184,6 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev)
                 blk_detach_dev(blk, ds);
             }
             pci_ide->bus[di->bus].ifs[di->unit].blk = NULL;
-            blk_unref(blk);
         }
     }
     qdev_reset_all(DEVICE(dev));
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c8b47f7..12a05df 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -13,6 +13,7 @@
 #ifndef BLOCK_BACKEND_H
 #define BLOCK_BACKEND_H
 
+#include "qemu/notify.h"
 #include "qemu/typedefs.h"
 #include "qapi/error.h"
 
@@ -60,13 +61,15 @@ typedef struct BlockDevOps {
     void (*resize_cb)(void *opaque);
 } BlockDevOps;
 
-BlockBackend *blk_new(const char *name, Error **errp);
-BlockBackend *blk_new_with_bs(const char *name, Error **errp);
+BlockBackend *blk_new(const char *name, Notifier *close_all_notifier,
+                      Error **errp);
+BlockBackend *blk_new_with_bs(const char *name, Notifier *close_all_notifier,
+                              Error **errp);
 BlockBackend *blk_new_open(const char *name, const char *filename,
                            const char *reference, QDict *options, int flags,
-                           Error **errp);
-void blk_ref(BlockBackend *blk);
-void blk_unref(BlockBackend *blk);
+                           Notifier *close_all_notifier, Error **errp);
+void blk_ref(BlockBackend *blk, Notifier *close_all_notifier);
+void blk_unref(BlockBackend *blk, Notifier *close_all_notifier);
 const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 2a34332..b6091e0 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -67,4 +67,7 @@ DriveInfo *add_init_drive(const char *opts);
 
 void do_commit(Monitor *mon, const QDict *qdict);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+
+void monitor_blk_unref(BlockBackend *blk);
+
 #endif
diff --git a/nbd.c b/nbd.c
index 53cf82b..d1dc350 100644
--- a/nbd.c
+++ b/nbd.c
@@ -956,9 +956,25 @@ static void blk_aio_detach(void *opaque)
     exp->ctx = NULL;
 }
 
+typedef struct NBDCloseNotifier {
+    Notifier n;
+    NBDExport *exp;
+    QTAILQ_ENTRY(NBDCloseNotifier) next;
+} NBDCloseNotifier;
+
+static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
+    QTAILQ_HEAD_INITIALIZER(close_notifiers);
+
+static void nbd_close_notifier(Notifier *n, void *data)
+{
+    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
+    nbd_export_close(cn->exp);
+}
+
 NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
                           uint32_t nbdflags, void (*close)(NBDExport *))
 {
+    NBDCloseNotifier *n = g_new0(NBDCloseNotifier, 1);
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
     exp->refcount = 1;
     QTAILQ_INIT(&exp->clients);
@@ -968,7 +984,12 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
     exp->size = size == -1 ? blk_getlength(blk) : size;
     exp->close = close;
     exp->ctx = blk_get_aio_context(blk);
-    blk_ref(blk);
+
+    n->n.notify = nbd_close_notifier;
+    n->exp = exp;
+    QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
+
+    blk_ref(blk, &n->n);
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
     /*
      * NBD exports are used for non-shared storage migration.  Make sure
@@ -1014,6 +1035,7 @@ void nbd_export_set_name(NBDExport *exp, const char *name)
 
 void nbd_export_close(NBDExport *exp)
 {
+    NBDCloseNotifier *n;
     NBDClient *client, *next;
 
     nbd_export_get(exp);
@@ -1022,11 +1044,22 @@ void nbd_export_close(NBDExport *exp)
     }
     nbd_export_set_name(exp, NULL);
     nbd_export_put(exp);
+
     if (exp->blk) {
+        QTAILQ_FOREACH(n, &close_notifiers, next) {
+            if (n->exp == exp) {
+                break;
+            }
+        }
+        assert(n);
+
         blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
                                         blk_aio_detach, exp);
-        blk_unref(exp->blk);
+        blk_unref(exp->blk, &n->n);
         exp->blk = NULL;
+
+        QTAILQ_REMOVE(&close_notifiers, n, next);
+        g_free(n);
     }
 }
 
diff --git a/qemu-img.c b/qemu-img.c
index 8b4139e..55c5262 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -300,7 +300,7 @@ static BlockBackend *img_open(const char *id, const char *filename,
         qdict_put_obj(options, "driver", QOBJECT(qstring_from_str(fmt)));
     }
 
-    blk = blk_new_open(id, filename, NULL, options, flags, &local_err);
+    blk = blk_new_open(id, filename, NULL, options, flags, NULL, &local_err);
     if (!blk) {
         error_report("Could not open '%s': %s", filename,
                      error_get_pretty(local_err));
@@ -322,7 +322,7 @@ static BlockBackend *img_open(const char *id, const char *filename,
     }
     return blk;
 fail:
-    blk_unref(blk);
+    blk_unref(blk, NULL);
     return NULL;
 }
 
@@ -711,7 +711,7 @@ static int img_check(int argc, char **argv)
 
 fail:
     qapi_free_ImageCheck(check);
-    blk_unref(blk);
+    blk_unref(blk, NULL);
     return ret;
 }
 
@@ -878,7 +878,7 @@ unref_backing:
 done:
     qemu_progress_end();
 
-    blk_unref(blk);
+    blk_unref(blk, NULL);
 
     if (local_err) {
         qerror_report_err(local_err);
@@ -1294,9 +1294,9 @@ static int img_compare(int argc, char **argv)
 out:
     qemu_vfree(buf1);
     qemu_vfree(buf2);
-    blk_unref(blk2);
+    blk_unref(blk2, NULL);
 out2:
-    blk_unref(blk1);
+    blk_unref(blk1, NULL);
 out3:
     qemu_progress_end();
     return ret;
@@ -1867,11 +1867,11 @@ out:
     qemu_opts_free(create_opts);
     qemu_vfree(buf);
     qemu_opts_del(sn_opts);
-    blk_unref(out_blk);
+    blk_unref(out_blk, NULL);
     g_free(bs);
     if (blk) {
         for (bs_i = 0; bs_i < bs_n; bs_i++) {
-            blk_unref(blk[bs_i]);
+            blk_unref(blk[bs_i], NULL);
         }
         g_free(blk);
     }
@@ -2006,7 +2006,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
         if (err) {
             error_report("%s", error_get_pretty(err));
             error_free(err);
-            blk_unref(blk);
+            blk_unref(blk, NULL);
             goto err;
         }
 
@@ -2015,7 +2015,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
         *last = elem;
         last = &elem->next;
 
-        blk_unref(blk);
+        blk_unref(blk, NULL);
 
         filename = fmt = NULL;
         if (chain) {
@@ -2303,7 +2303,7 @@ static int img_map(int argc, char **argv)
     dump_map_entry(output_format, &curr, NULL);
 
 out:
-    blk_unref(blk);
+    blk_unref(blk, NULL);
     return ret < 0;
 }
 
@@ -2427,7 +2427,7 @@ static int img_snapshot(int argc, char **argv)
     }
 
     /* Cleanup */
-    blk_unref(blk);
+    blk_unref(blk, NULL);
     if (ret) {
         return 1;
     }
@@ -2544,7 +2544,7 @@ static int img_rebase(int argc, char **argv)
 
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
         blk_old_backing = blk_new_open("old_backing", backing_name, NULL,
-                                       options, src_flags, &local_err);
+                                       options, src_flags, NULL, &local_err);
         if (!blk_old_backing) {
             error_report("Could not open old backing file '%s': %s",
                          backing_name, error_get_pretty(local_err));
@@ -2562,7 +2562,8 @@ static int img_rebase(int argc, char **argv)
             }
 
             blk_new_backing = blk_new_open("new_backing", out_baseimg, NULL,
-                                           options, src_flags, &local_err);
+                                           options, src_flags, NULL,
+                                           &local_err);
             if (!blk_new_backing) {
                 error_report("Could not open new backing file '%s': %s",
                              out_baseimg, error_get_pretty(local_err));
@@ -2741,11 +2742,11 @@ out:
     qemu_progress_end();
     /* Cleanup */
     if (!unsafe) {
-        blk_unref(blk_old_backing);
-        blk_unref(blk_new_backing);
+        blk_unref(blk_old_backing, NULL);
+        blk_unref(blk_new_backing, NULL);
     }
 
-    blk_unref(blk);
+    blk_unref(blk, NULL);
     if (ret) {
         return 1;
     }
@@ -2868,7 +2869,7 @@ static int img_resize(int argc, char **argv)
         break;
     }
 out:
-    blk_unref(blk);
+    blk_unref(blk, NULL);
     if (ret) {
         return 1;
     }
@@ -3006,7 +3007,7 @@ static int img_amend(int argc, char **argv)
 out:
     qemu_progress_end();
 
-    blk_unref(blk);
+    blk_unref(blk, NULL);
     qemu_opts_del(opts);
     qemu_opts_free(create_opts);
     g_free(options);
diff --git a/qemu-io.c b/qemu-io.c
index bde97f8..ffbe0f1 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -38,7 +38,7 @@ static ReadLineState *readline_state;
 
 static int close_f(BlockBackend *blk, int argc, char **argv)
 {
-    blk_unref(qemuio_blk);
+    blk_unref(qemuio_blk, NULL);
     qemuio_blk = NULL;
     return 0;
 }
@@ -60,7 +60,7 @@ static int openfile(char *name, int flags, QDict *opts)
         return 1;
     }
 
-    qemuio_blk = blk_new_open("hda", name, NULL, opts, flags, &local_err);
+    qemuio_blk = blk_new_open("hda", name, NULL, opts, flags, NULL, &local_err);
     if (!qemuio_blk) {
         fprintf(stderr, "%s: can't open%s%s: %s\n", progname,
                 name ? " device " : "", name ?: "",
@@ -476,7 +476,7 @@ int main(int argc, char **argv)
      */
     bdrv_drain_all();
 
-    blk_unref(qemuio_blk);
+    blk_unref(qemuio_blk, NULL);
     g_free(readline_state);
     return 0;
 }
diff --git a/qemu-nbd.c b/qemu-nbd.c
index fdc9590..716053b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -689,7 +689,8 @@ int main(int argc, char **argv)
     }
 
     srcpath = argv[optind];
-    blk = blk_new_open("hda", srcpath, NULL, options, flags, &local_err);
+    /* the reference will be passed on nbd_export_new() */
+    blk = blk_new_open("hda", srcpath, NULL, options, flags, NULL, &local_err);
     if (!blk) {
         errx(EXIT_FAILURE, "Failed to blk_new_open '%s': %s", argv[optind],
              error_get_pretty(local_err));
@@ -723,7 +724,9 @@ int main(int argc, char **argv)
         }
     }
 
+    /* nbd_export_new() takes the reference to blk */
     exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed);
+    blk_unref(blk, NULL);
 
     if (sockpath) {
         fd = unix_socket_incoming(sockpath);
@@ -768,7 +771,6 @@ int main(int argc, char **argv)
         }
     } while (state != TERMINATED);
 
-    blk_unref(blk);
     if (sockpath) {
         unlink(sockpath);
     }
-- 
2.1.0

[Qemu-devel] [PATCH 05/21] block: Remove per-BDS close notifiers

[Max Reitz]

They are unused, and bdrv_close_all() is a perfectly valid replacement.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                        | 7 -------
 block/block-backend.c          | 7 -------
 include/block/block.h          | 1 -
 include/block/block_int.h      | 2 --
 include/sysemu/block-backend.h | 1 -
 5 files changed, 18 deletions(-)

diff --git a/block.c b/block.c
index 3adb724..0c6ab96 100644
--- a/block.c
+++ b/block.c
@@ -374,7 +374,6 @@ BlockDriverState *bdrv_new(void)
     for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
         QLIST_INIT(&bs->op_blockers[i]);
     }
-    notifier_list_init(&bs->close_notifiers);
     notifier_with_return_list_init(&bs->before_write_notifiers);
     qemu_co_queue_init(&bs->throttled_reqs[0]);
     qemu_co_queue_init(&bs->throttled_reqs[1]);
@@ -384,11 +383,6 @@ BlockDriverState *bdrv_new(void)
     return bs;
 }
 
-void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify)
-{
-    notifier_list_add(&bs->close_notifiers, notify);
-}
-
 BlockDriver *bdrv_find_format(const char *format_name)
 {
     BlockDriver *drv1;
@@ -1866,7 +1860,6 @@ void bdrv_close(BlockDriverState *bs)
     bdrv_drain_all(); /* complete I/O */
     bdrv_flush(bs);
     bdrv_drain_all(); /* in case flush left pending I/O */
-    notifier_list_notify(&bs->close_notifiers, bs);
 
     if (bs->drv) {
         if (bs->backing_hd) {
diff --git a/block/block-backend.c b/block/block-backend.c
index 9f14f9b..b825d8e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1098,13 +1098,6 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
     }
 }
 
-void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
-{
-    if (blk->bs) {
-        bdrv_add_close_notifier(blk->bs, notify);
-    }
-}
-
 void blk_io_plug(BlockBackend *blk)
 {
     if (blk->bs) {
diff --git a/include/block/block.h b/include/block/block.h
index 301baac..2b7b3d6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -192,7 +192,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 void bdrv_close(BlockDriverState *bs);
-void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 91d21c5..9842655 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -350,8 +350,6 @@ struct BlockDriverState {
     BlockDriverState *backing_hd;
     BlockDriverState *file;
 
-    NotifierList close_notifiers;
-
     /* Callback before write request is processed */
     NotifierWithReturnList before_write_notifiers;
 
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 12a05df..f1da495 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -159,7 +159,6 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
                                                                   void *),
                                      void (*detach_aio_context)(void *),
                                      void *opaque);
-void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
 void blk_io_plug(BlockBackend *blk);
 void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
-- 
2.1.0

[Qemu-devel] [PATCH 06/21] block: Use blk_remove_bs() in blk_delete()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index b825d8e..a7264a0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -164,12 +164,7 @@ static void blk_delete(BlockBackend *blk)
 {
     assert(!blk->refcnt);
     assert(!blk->dev);
-    if (blk->bs) {
-        assert(blk->bs->blk == blk);
-        blk->bs->blk = NULL;
-        bdrv_unref(blk->bs);
-        blk->bs = NULL;
-    }
+    blk_remove_bs(blk);
     /* Avoid double-remove after blk_hide_on_behalf_of_do_drive_del() */
     if (blk->name[0]) {
         QTAILQ_REMOVE(&blk_backends, blk, link);
@@ -333,6 +328,8 @@ void blk_remove_bs(BlockBackend *blk)
         return;
     }
 
+    assert(blk->bs->blk == blk);
+
     blk_update_root_state(blk);
 
     bdrv_unref(blk->bs);
-- 
2.1.0

[Qemu-devel] [PATCH 07/21] blockdev: Use blk_remove_bs() in do_drive_del()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index fd3f50d..ccf5469 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2285,11 +2285,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
 
     /* quiesce block driver; prevent further io */
-    bdrv_drain_all();
-    if (bs) {
-        bdrv_flush(bs);
-        bdrv_close(bs);
-    }
+    blk_remove_bs(blk);
 
     /* if we have a device attached to this BlockDriverState
      * then we need to make the drive anonymous until the device
-- 
2.1.0

[Qemu-devel] [PATCH 08/21] block: Make bdrv_close() static

[Max Reitz]

There are no users of bdrv_close() left, except for one of bdrv_open()'s
failure paths, bdrv_close_all() and bdrv_delete(), and that is good.
Make bdrv_close() static so nobody makes the mistake of directly using
bdrv_close() again.

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

diff --git a/block.c b/block.c
index 0c6ab96..0cd6457 100644
--- a/block.c
+++ b/block.c
@@ -107,6 +107,8 @@ static int use_bdrv_whitelist;
 static NotifierList close_all_notifiers =
     NOTIFIER_LIST_INITIALIZER(close_all_notifiers);
 
+static void bdrv_close(BlockDriverState *bs);
+
 #ifdef _WIN32
 static int is_windows_drive_prefix(const char *filename)
 {
@@ -1850,7 +1852,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 }
 
 
-void bdrv_close(BlockDriverState *bs)
+static void bdrv_close(BlockDriverState *bs)
 {
     BdrvAioNotifier *ban, *ban_next;
 
diff --git a/include/block/block.h b/include/block/block.h
index 2b7b3d6..6688920 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -191,7 +191,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                         BlockReopenQueue *queue, Error **errp);
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
-void bdrv_close(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
 int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
-- 
2.1.0

[Qemu-devel] [PATCH 09/21] 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>
---
 block.c                        |  2 +-
 block/block-backend.c          | 19 ++++++++++++++++++-
 include/sysemu/block-backend.h |  1 +
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 0cd6457..8eef0c5 100644
--- a/block.c
+++ b/block.c
@@ -898,7 +898,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
     }
 
     /* takes care of avoiding namespaces collisions */
-    if (blk_by_name(node_name)) {
+    if (blk_name_taken(node_name)) {
         error_setg(errp, "node-name=%s is conflicting with a device id",
                    node_name);
         return;
diff --git a/block/block-backend.c b/block/block-backend.c
index a7264a0..4467a8c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -84,7 +84,7 @@ BlockBackend *blk_new(const char *name, Notifier *close_all_notifier,
         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;
     }
@@ -259,6 +259,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 f1da495..7322db6 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -72,6 +72,7 @@ void blk_ref(BlockBackend *blk, Notifier *close_all_notifier);
 void blk_unref(BlockBackend *blk, Notifier *close_all_notifier);
 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.1.0

[Qemu-devel] [PATCH 10/21] 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>
---
 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 4467a8c..3ff6679 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -232,6 +232,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_do_drive_del().
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 7322db6..e1645ae 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -74,6 +74,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.1.0

[Qemu-devel] [PATCH 11/21] 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>
---
 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 3ff6679..27c9b99 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1233,3 +1233,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 e1645ae..73b26ad 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -127,7 +127,9 @@ 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_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.1.0

[Qemu-devel] [PATCH 12/21] 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               | 22 ++++++++---------
 block/block-backend.c |  8 +++----
 block/qapi.c          | 13 ++++++++--
 block/snapshot.c      |  3 ++-
 blockdev.c            | 14 ++++++-----
 cpus.c                |  7 +++---
 migration/block.c     | 10 +++++---
 migration/migration.c |  4 ++--
 monitor.c             | 13 ++++++----
 qemu-char.c           |  3 ++-
 qemu-io.c             |  2 +-
 qmp.c                 | 14 +++++------
 savevm.c              | 66 ++++++++++++++++++++++++++++++---------------------
 xen-mapcache.c        |  3 ++-
 14 files changed, 107 insertions(+), 75 deletions(-)

diff --git a/block.c b/block.c
index 8eef0c5..ca6a587 100644
--- a/block.c
+++ b/block.c
@@ -1689,7 +1689,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)) {
@@ -1859,9 +1859,9 @@ static void bdrv_close(BlockDriverState *bs)
     if (bs->job) {
         block_job_cancel_sync(bs->job);
     }
-    bdrv_drain_all(); /* complete I/O */
+    blk_drain_all(); /* complete I/O */
     bdrv_flush(bs);
-    bdrv_drain_all(); /* in case flush left pending I/O */
+    blk_drain_all(); /* in case flush left pending I/O */
 
     if (bs->drv) {
         if (bs->backing_hd) {
@@ -1909,15 +1909,15 @@ static void bdrv_close(BlockDriverState *bs)
 
 void bdrv_close_all(void)
 {
-    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
     notifier_list_notify(&close_all_notifiers, NULL);
 
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        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);
-        bdrv_close(bs);
+        bdrv_close(blk_bs(blk));
         aio_context_release(aio_context);
     }
 }
@@ -5690,7 +5690,7 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
 
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
 {
-    bdrv_drain_all(); /* ensure there are no in-flight requests */
+    blk_drain_all(); /* ensure there are no in-flight requests */
 
     bdrv_detach_aio_context(bs);
 
@@ -5793,14 +5793,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 27c9b99..f136fc8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -262,10 +262,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;
         }
@@ -323,9 +323,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 db42a6e..79ca32a 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -393,15 +393,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/block/snapshot.c b/block/snapshot.c
index 698e1a1..e92e2d4 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -24,6 +24,7 @@
 
 #include "block/snapshot.h"
 #include "block/block_int.h"
+#include "sysemu/block-backend.h"
 
 QemuOptsList internal_snapshot_opts = {
     .name = "snapshot",
@@ -238,7 +239,7 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
     }
 
     /* drain all pending i/o before deleting snapshot */
-    bdrv_drain_all();
+    blk_drain_all();
 
     if (drv->bdrv_snapshot_delete) {
         return drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp);
diff --git a/blockdev.c b/blockdev.c
index ccf5469..22d7ceb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1156,7 +1156,7 @@ void do_commit(Monitor *mon, const QDict *qdict)
     int ret;
 
     if (!strcmp(device, "all")) {
-        ret = bdrv_commit_all();
+        ret = blk_commit_all();
     } else {
         bs = bdrv_find(device);
         if (!bs) {
@@ -1830,7 +1830,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) {
@@ -2341,7 +2341,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) {
@@ -2508,7 +2508,7 @@ void qmp_block_commit(const char *device,
     bs = blk_bs(blk);
 
     /* drain all i/o before commits */
-    bdrv_drain_all();
+    blk_drain_all();
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, errp)) {
         goto out;
@@ -3238,12 +3238,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 3a5323b..ccfd299 100644
--- a/cpus.c
+++ b/cpus.c
@@ -28,6 +28,7 @@
 #include "monitor/monitor.h"
 #include "qapi/qmp/qerror.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "exec/gdbstub.h"
 #include "sysemu/dma.h"
 #include "sysemu/kvm.h"
@@ -620,8 +621,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;
 }
@@ -1319,7 +1320,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 0c76106..ec435db 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -23,6 +23,7 @@
 #include "migration/block.h"
 #include "migration/migration.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/block-backend.h"
 #include <assert.h>
 
 #define BLOCK_SIZE                       (1 << 20)
@@ -348,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;
 
@@ -359,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;
         }
@@ -456,7 +460,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
         blk_mig_lock();
         if (bmds_aio_inflight(bmds, sector)) {
             blk_mig_unlock();
-            bdrv_drain_all();
+            blk_drain_all();
         } else {
             blk_mig_unlock();
         }
@@ -596,7 +600,7 @@ static void blk_mig_cleanup(void)
     BlkMigDevState *bmds;
     BlkMigBlock *blk;
 
-    bdrv_drain_all();
+    blk_drain_all();
 
     unset_dirty_tracking();
 
diff --git a/migration/migration.c b/migration/migration.c
index b3adbc6..f08cd6d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -19,7 +19,7 @@
 #include "monitor/monitor.h"
 #include "migration/qemu-file.h"
 #include "sysemu/sysemu.h"
-#include "block/block.h"
+#include "sysemu/block-backend.h"
 #include "qemu/sockets.h"
 #include "migration/block.h"
 #include "qemu/thread.h"
@@ -104,7 +104,7 @@ static void process_incoming_migration_co(void *opaque)
     qemu_announce_self();
 
     /* Make sure all file formats flush their mutable metadata */
-    bdrv_invalidate_cache_all(&local_err);
+    blk_invalidate_cache_all(&local_err);
     if (local_err) {
         qerror_report_err(local_err);
         error_free(local_err);
diff --git a/monitor.c b/monitor.c
index 7e4f605..25ea682 100644
--- a/monitor.c
+++ b/monitor.c
@@ -40,6 +40,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"
@@ -4611,13 +4612,15 @@ 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;
 
+        bs = blk_bs(blk);
         if (!bdrv_can_snapshot(bs)) {
             continue;
         }
@@ -4664,7 +4667,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 */
@@ -4717,8 +4720,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 98d4342..4d3fb60 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/timer.h"
 #include "sysemu/char.h"
 #include "hw/usb.h"
@@ -523,7 +524,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 ffbe0f1..6970dfc 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -474,7 +474,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, NULL);
     g_free(readline_state);
diff --git a/qmp.c b/qmp.c
index d414118..9402555 100644
--- a/qmp.c
+++ b/qmp.c
@@ -151,8 +151,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
 
 void qmp_cont(Error **errp)
 {
-    BlockBackend *blk;
-    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
     if (runstate_needs_reset()) {
         error_setg(errp, "Resetting the Virtual Machine is required");
@@ -161,14 +160,13 @@ 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)) {
-        if (bdrv_key_required(bs)) {
-            error_set(errp, QERR_DEVICE_ENCRYPTED,
-                      bdrv_get_device_name(bs),
-                      bdrv_get_encrypted_filename(bs));
+    while ((blk = blk_next_inserted(blk)) != NULL) {
+        if (bdrv_key_required(blk_bs(blk))) {
+            error_set(errp, QERR_DEVICE_ENCRYPTED, blk_name(blk),
+                      bdrv_get_encrypted_filename(blk_bs(blk)));
             return;
         }
     }
diff --git a/savevm.c b/savevm.c
index 08ec678..ec4c2df 100644
--- a/savevm.c
+++ b/savevm.c
@@ -30,6 +30,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"
@@ -1001,10 +1002,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;
@@ -1016,11 +1017,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);
@@ -1042,6 +1045,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
 void do_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;
@@ -1052,16 +1056,14 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     const char *name = qdict_get_try_str(qdict, "name");
 
     /* 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;
         }
     }
@@ -1118,15 +1120,17 @@ void do_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));
             }
         }
     }
@@ -1165,6 +1169,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;
@@ -1188,12 +1193,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.",
@@ -1210,10 +1215,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) {
@@ -1246,6 +1253,7 @@ int load_vmstate(const char *name)
 void do_delvm(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs;
+    BlockBackend *blk = NULL;
     Error *err;
     const char *name = qdict_get_str(qdict, "name");
 
@@ -1254,8 +1262,9 @@ void do_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);
@@ -1263,7 +1272,7 @@ void do_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);
             }
@@ -1274,6 +1283,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
 void do_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;
@@ -1301,9 +1311,11 @@ void do_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/xen-mapcache.c b/xen-mapcache.c
index 8cefd0c..1a98a69 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>
@@ -421,7 +422,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.1.0

[Qemu-devel] [PATCH 13/21] 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.

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

diff --git a/block/block-backend.c b/block/block-backend.c
index f136fc8..c5ca0f4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -28,6 +28,7 @@ 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 */
 
     void *dev;                  /* attached device model, if any */
     /* TODO change to DeviceState when all users are qdevified */
@@ -67,6 +68,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.
@@ -228,7 +234,8 @@ void blk_unref(BlockBackend *blk, Notifier *close_all_notifier)
  */
 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);
 }
 
 /*
@@ -247,6 +254,24 @@ 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)
+{
+    QTAILQ_INSERT_TAIL(&monitor_block_backends, blk, monitor_link);
+}
+
+/*
+ * Remove a BlockBackend from the list of backends referenced by the monitor.
+ * Strictly for use by blockdev.c.
+ */
+void monitor_remove_blk(BlockBackend *blk)
+{
+    QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
+}
+
+/*
  * Return @blk's name, a non-null string.
  * Wart: the name is empty iff @blk has been hidden with
  * blk_hide_on_behalf_of_do_drive_del().
diff --git a/blockdev.c b/blockdev.c
index 22d7ceb..0f965bd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -90,6 +90,8 @@ static int if_max_devs[IF_COUNT] = {
 static void monitor_blk_unref_with_can(BlockBackend *blk,
                                        BlockdevCloseAllNotifier *can)
 {
+    BlockBackend *blk1 = NULL;
+
     if (can) {
         assert(can->blk == blk);
     } else {
@@ -101,6 +103,15 @@ static void monitor_blk_unref_with_can(BlockBackend *blk,
         assert(can);
     }
 
+    /* Thanks to do_drive_del() magic, the BlockBackend is not necessarily
+     * listed at this point */
+    while ((blk1 = blk_next(blk1)) != NULL) {
+        if (blk1 == blk) {
+            monitor_remove_blk(blk);
+            break;
+        }
+    }
+
     blk_unref(blk, &can->n);
     QTAILQ_REMOVE(&close_all_notifiers, can, next);
     g_free(can);
@@ -634,6 +645,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;
@@ -2294,6 +2307,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
      */
     if (blk_get_attached_dev(blk)) {
         blk_hide_on_behalf_of_do_drive_del(blk);
+        monitor_remove_blk(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 73b26ad..1683732 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -75,6 +75,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.1.0

[Qemu-devel] [PATCH 14/21] blockdev: Remove blk_hide_on_behalf_of_do_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 do_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
do_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 c5ca0f4..aeedabc 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -64,7 +64,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);
 
@@ -171,10 +171,7 @@ static void blk_delete(BlockBackend *blk)
     assert(!blk->refcnt);
     assert(!blk->dev);
     blk_remove_bs(blk);
-    /* Avoid double-remove after blk_hide_on_behalf_of_do_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);
@@ -359,24 +356,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_do_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 0f965bd..9db66fd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2306,7 +2306,6 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
      * then we can just get rid of the block driver state right here.
      */
     if (blk_get_attached_dev(blk)) {
-        blk_hide_on_behalf_of_do_drive_del(blk);
         monitor_remove_blk(blk);
         /* Further I/O must not pause the guest */
         blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 1683732..960e57d 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -82,8 +82,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_do_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.1.0

[Qemu-devel] [PATCH 15/21] block: Make bdrv_drain_one() public

[Max Reitz]

We will need it in block/block-backend.c.

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

diff --git a/block.c b/block.c
index ca6a587..d4a3c79 100644
--- a/block.c
+++ b/block.c
@@ -1948,7 +1948,7 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
     return false;
 }
 
-static bool bdrv_drain_one(BlockDriverState *bs)
+bool bdrv_drain_one(BlockDriverState *bs)
 {
     bool bs_busy;
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9842655..262b635 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -428,6 +428,8 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
 void bdrv_set_io_limits(BlockDriverState *bs,
                         ThrottleConfig *cfg);
 
+bool bdrv_drain_one(BlockDriverState *bs);
+
 
 /**
  * bdrv_add_before_write_notifier:
-- 
2.1.0

[Qemu-devel] [PATCH 16/21] 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                 |  97 --------------------------------------------
 block/block-backend.c   | 104 ++++++++++++++++++++++++++++++++++++++++++------
 include/block/block.h   |   4 --
 stubs/Makefile.objs     |   2 +-
 stubs/bdrv-commit-all.c |   7 ----
 stubs/blk-commit-all.c  |   7 ++++
 6 files changed, 100 insertions(+), 121 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 d4a3c79..00fe705 100644
--- a/block.c
+++ b/block.c
@@ -1976,37 +1976,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.
- *
- * 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 be arbitrarily complex and a constant flow of I/O can come until the
- * coroutine is complete.  Because of this, it is not possible to have a
- * function to drain a single device's I/O queue.
- */
-void bdrv_drain_all(void)
-{
-    /* Always run first iteration so any pending completion BHs run */
-    bool busy = true;
-    BlockDriverState *bs;
-
-    while (busy) {
-        busy = false;
-
-        QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-            AioContext *aio_context = bdrv_get_aio_context(bs);
-
-            aio_context_acquire(aio_context);
-            busy |= bdrv_drain_one(bs);
-            aio_context_release(aio_context);
-        }
-    }
-}
-
 /* make a BlockDriverState anonymous by removing from bdrv_state and
  * graph_bdrv_state list.
    Also, NULL terminate the device_name to prevent double remove */
@@ -2299,26 +2268,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_hd) {
-            int ret = bdrv_commit(bs);
-            if (ret < 0) {
-                aio_context_release(aio_context);
-                return ret;
-            }
-        }
-        aio_context_release(aio_context);
-    }
-    return 0;
-}
-
 /**
  * Remove an active request from the tracked requests list
  *
@@ -3765,14 +3714,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;
@@ -3789,26 +3730,6 @@ int bdrv_get_flags(BlockDriverState *bs)
     return bs->open_flags;
 }
 
-int bdrv_flush_all(void)
-{
-    BlockDriverState *bs;
-    int result = 0;
-
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        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;
-}
-
 int bdrv_has_zero_init_1(BlockDriverState *bs)
 {
     return 1;
@@ -4963,24 +4884,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;
-        }
-    }
-}
-
 int bdrv_flush(BlockDriverState *bs)
 {
     Coroutine *co;
diff --git a/block/block-backend.c b/block/block-backend.c
index aeedabc..9a00645 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -367,6 +367,9 @@ void blk_remove_bs(BlockBackend *blk)
     assert(blk->bs->blk == blk);
 
     blk_update_root_state(blk);
+    /* After this function, the BDS may longer be accessible to blk_*_all()
+     * functions */
+    blk_drain_all();
 
     bdrv_unref(blk->bs);
     blk->bs->blk = NULL;
@@ -883,16 +886,6 @@ int blk_flush(BlockBackend *blk)
     return bdrv_flush(blk->bs);
 }
 
-int blk_flush_all(void)
-{
-    return bdrv_flush_all();
-}
-
-void blk_drain_all(void)
-{
-    bdrv_drain_all();
-}
-
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
                       BlockdevOnError on_write_error)
 {
@@ -1238,12 +1231,99 @@ 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.
+ *
+ * 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 be arbitrarily complex and a constant flow of I/O can come until the
+ * coroutine is complete.  Because of this, it is not possible to have a
+ * function to drain a single device's I/O queue.
+ */
+void blk_drain_all(void)
+{
+    /* Always run first iteration so any pending completion BHs run */
+    bool busy = true;
+    BlockBackend *blk;
+
+    while (busy) {
+        busy = false;
+
+        QTAILQ_FOREACH(blk, &blk_backends, link) {
+            AioContext *aio_context = blk_get_aio_context(blk);
+
+            if (!blk_is_inserted(blk)) {
+                continue;
+            }
+
+            aio_context_acquire(aio_context);
+            busy |= bdrv_drain_one(blk->bs);
+            aio_context_release(aio_context);
+        }
+    }
+}
+
 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_hd) {
+            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/include/block/block.h b/include/block/block.h
index 6688920..1e1039f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -235,7 +235,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);
@@ -321,15 +320,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 5e347d0..56eb2e8 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 += chr-baum-init.o
 stub-obj-y += chr-msmouse.o
 stub-obj-y += chr-testdev.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.1.0

[Qemu-devel] [PATCH 17/21] 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                   | 21 +--------------------
 include/block/block.h     |  1 -
 include/block/block_int.h |  2 --
 3 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/block.c b/block.c
index 00fe705..64d1524 100644
--- a/block.c
+++ b/block.c
@@ -88,9 +88,6 @@ static void coroutine_fn bdrv_co_do_rw(void *opaque);
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
 
-static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
-    QTAILQ_HEAD_INITIALIZER(bdrv_states);
-
 static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
 
@@ -360,10 +357,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)
@@ -1981,17 +1975,6 @@ void bdrv_drain(BlockDriverState *bs)
    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);
     }
@@ -2032,8 +2015,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     /* job */
     bs_dest->job                = bs_src->job;
 
-    /* keep the same entry in bdrv_states */
-    bs_dest->device_list = bs_src->device_list;
     bs_dest->blk = bs_src->blk;
 
     memcpy(bs_dest->op_blockers, bs_src->op_blockers,
diff --git a/include/block/block.h b/include/block/block.h
index 1e1039f..217482f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -357,7 +357,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 262b635..9005938 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -380,8 +380,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;
     QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
     int refcnt;
 
-- 
2.1.0

[Qemu-devel] [PATCH 18/21] blockdev: Keep track of monitor-owned BDS

[Max Reitz]

...and release the reference on bdrv_close_all().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   |  2 ++
 blockdev.c                | 24 ++++++++++++++++++++++++
 include/block/block_int.h |  2 ++
 3 files changed, 28 insertions(+)

diff --git a/block.c b/block.c
index 64d1524..f096577 100644
--- a/block.c
+++ b/block.c
@@ -2015,6 +2015,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     /* job */
     bs_dest->job                = bs_src->job;
 
+    /* keep the same entry in the list of monitor-owned BDS */
+    bs_dest->monitor_list = bs_src->monitor_list;
     bs_dest->blk = bs_src->blk;
 
     memcpy(bs_dest->op_blockers, bs_src->op_blockers,
diff --git a/blockdev.c b/blockdev.c
index 9db66fd..2801aea 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -56,6 +56,11 @@ typedef struct BlockdevCloseAllNotifier {
 static QTAILQ_HEAD(, BlockdevCloseAllNotifier) close_all_notifiers =
     QTAILQ_HEAD_INITIALIZER(close_all_notifiers);
 
+static Notifier *close_all_bdrv_states_notifier;
+
+static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
+    QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
+
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
     [IF_IDE] = "ide",
@@ -129,6 +134,17 @@ static void blockdev_close_all_notifier(Notifier *n, void *data)
     monitor_blk_unref_with_can(can->blk, can);
 }
 
+static void blockdev_close_all_bdrv_states(Notifier *n, void *data)
+{
+    BlockDriverState *bds, *next_bds;
+
+    QTAILQ_FOREACH_SAFE(bds, &monitor_bdrv_states, monitor_list, next_bds) {
+        bdrv_unref(bds);
+    }
+
+    g_free(n);
+}
+
 /**
  * Boards may call this to offer board-by-board overrides
  * of the default, global values.
@@ -3231,6 +3247,14 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
         if (!bs) {
             goto fail;
         }
+
+        QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
+        if (!close_all_bdrv_states_notifier) {
+            close_all_bdrv_states_notifier = g_new0(Notifier, 1);
+            close_all_bdrv_states_notifier->notify =
+                blockdev_close_all_bdrv_states;
+            bdrv_add_close_all_notifier(close_all_bdrv_states_notifier);
+        }
     }
 
     if (bs && bdrv_key_required(bs)) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9005938..61c848a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -380,6 +380,8 @@ 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 monitor-owned BDS */
+    QTAILQ_ENTRY(BlockDriverState) monitor_list;
     QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
     int refcnt;
 
-- 
2.1.0

[Qemu-devel] [PATCH 19/21] block: Strip down bdrv_close_all()

[Max Reitz]

References to all BlockBackends and BlockDriverStates are now released
by their respective holders; force-closing them is no longer necessary.

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

diff --git a/block.c b/block.c
index f096577..747b92f 100644
--- a/block.c
+++ b/block.c
@@ -1903,17 +1903,8 @@ static void bdrv_close(BlockDriverState *bs)
 
 void bdrv_close_all(void)
 {
-    BlockBackend *blk = NULL;
-
     notifier_list_notify(&close_all_notifiers, NULL);
-
-    while ((blk = blk_next_inserted(blk)) != NULL) {
-        AioContext *aio_context = blk_get_aio_context(blk);
-
-        aio_context_acquire(aio_context);
-        bdrv_close(blk_bs(blk));
-        aio_context_release(aio_context);
-    }
+    blk_close_all_finalize();
 }
 
 void bdrv_add_close_all_notifier(Notifier *notifier)
diff --git a/block/block-backend.c b/block/block-backend.c
index 9a00645..242dea4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -220,6 +220,14 @@ void blk_unref(BlockBackend *blk, Notifier *close_all_notifier)
 }
 
 /*
+ * For use by bdrv_close_all()
+ */
+void blk_close_all_finalize(void)
+{
+    assert(QTAILQ_EMPTY(&blk_backends));
+}
+
+/*
  * Return the BlockBackend after @blk.
  * If @blk is null, return the first one.
  * Else, return @blk's next sibling, which may be null.
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 960e57d..1a05dbb 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -70,6 +70,7 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
                            Notifier *close_all_notifier, Error **errp);
 void blk_ref(BlockBackend *blk, Notifier *close_all_notifier);
 void blk_unref(BlockBackend *blk, Notifier *close_all_notifier);
+void blk_close_all_finalize(void);
 const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 bool blk_name_taken(const char *name);
-- 
2.1.0

[Qemu-devel] [PATCH 20/21] iotests: Add "wait" functionality to _cleanup_qemu

[Max Reitz]

The qemu process does not always need to be killed, just waiting for it
can be fine, too. This introduces a way to do so.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.qemu | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 8e618b5..4e1996c 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -187,13 +187,23 @@ function _launch_qemu()
 
 
 # Silenty kills the QEMU process
+#
+# If $wait is set to anything other than the empty string, the process will not
+# be killed but only waited for, and any output will be forwarded to stdout. If
+# $wait is empty, the process will be killed and all output will be suppressed.
 function _cleanup_qemu()
 {
     # QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
     for i in "${!QEMU_OUT[@]}"
     do
-        kill -KILL ${QEMU_PID[$i]} 2>/dev/null
+        if [ -z "${wait}" ]; then
+            kill -KILL ${QEMU_PID[$i]} 2>/dev/null
+        fi
         wait ${QEMU_PID[$i]} 2>/dev/null # silent kill
+        if [ -n "${wait}" ]; then
+            cat <&${QEMU_OUT[$i]} | _filter_testdir | _filter_qemu \
+                                  | _filter_qemu_io | _filter_qmp
+        fi
         rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
         eval "exec ${QEMU_IN[$i]}<&-"   # close file descriptors
         eval "exec ${QEMU_OUT[$i]}<&-"
-- 
2.1.0

[Qemu-devel] [PATCH 21/21] iotests: Add test for multiple BB on BDS tree

[Max Reitz]

This adds a test for having multiple BlockBackends in one BDS tree. In
this case, there is one BB for the protocol BDS and one BB for the
format BDS in a simple two-BDS tree (with the protocol BDS and BB added
first).

When bdrv_close_all() is executed, no cached data from any BDS should be
lost; the protocol BDS may not be closed until the format BDS is closed.
Otherwise, metadata updates may be lost.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/117     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/117.out | 14 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 101 insertions(+)
 create mode 100755 tests/qemu-iotests/117
 create mode 100644 tests/qemu-iotests/117.out

diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117
new file mode 100755
index 0000000..7881fc7
--- /dev/null
+++ b/tests/qemu-iotests/117
@@ -0,0 +1,86 @@
+#!/bin/bash
+#
+# Test case for shared BDS between backend trees
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# 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/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+_make_test_img 64k
+
+_launch_qemu
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'blockdev-add',
+       'arguments': { 'options': { 'id': 'protocol',
+                                   'driver': 'file',
+                                   'filename': '$TEST_IMG' } } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'blockdev-add',
+       'arguments': { 'options': { 'id': 'format',
+                                   'driver': '$IMGFMT',
+                                   'file': 'protocol' } } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'qemu-io format \"write -P 42 0 64k\"' } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'quit' }" \
+    'return'
+
+wait=1 _cleanup_qemu
+
+_check_test_img
+
+$QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/117.out b/tests/qemu-iotests/117.out
new file mode 100644
index 0000000..f52dc1a
--- /dev/null
+++ b/tests/qemu-iotests/117.out
@@ -0,0 +1,14 @@
+QA output created by 117
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+{"return": {}}
+{"return": {}}
+{"return": {}}
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+No errors were found on the image.
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0b3722e..1b08adc 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -116,4 +116,5 @@
 113 rw auto quick
 114 rw auto quick
 116 rw auto quick
+117 rw auto
 118 rw auto
-- 
2.1.0

Re: [Qemu-devel] [PATCH 00/20] block: Rework bdrv_close_all()

[Max Reitz]

@subject: Sorry, should be 00/21 of course. I messed up when copying the 
subject and the text to this final version.

Max

Re: [Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Paolo Bonzini]

On 26/01/2015 20:27, Max Reitz wrote:
> However, this was wrong: With the NBD attached to some BB, it
> should stay attached until the NBD export is removed or the server
> stopped; medium ejection should have nothing to do with this (other than
> the NBD server not being able to read data while the virtual is open, if
> there is one (and there is one if the NBD server is attached to the same
> BB as a guest device with a tray is; which is correct behavior because
> attaching an NBD server to a guest-used BB should result in the NBD
> server behaving exactly the same as the guest device)).

Not entirely; use of close notifiers was on purpose.  We do not want the
NBD client to start reading data from a different medium without noticing.

If the NBD server is attached to the BDS, it should keep serving the BDS.

If the NBD server is attached to the BB, the clients must have a way to
notice the medium change, and so far the solution was to close the
connection.  I'd be wary to change this, though I understand where you
come from.

Paolo

Re: [Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Max Reitz]

On 2015-01-26 at 15:40, Paolo Bonzini wrote:
>
> On 26/01/2015 20:27, Max Reitz wrote:
>> However, this was wrong: With the NBD attached to some BB, it
>> should stay attached until the NBD export is removed or the server
>> stopped; medium ejection should have nothing to do with this (other than
>> the NBD server not being able to read data while the virtual is open, if
>> there is one (and there is one if the NBD server is attached to the same
>> BB as a guest device with a tray is; which is correct behavior because
>> attaching an NBD server to a guest-used BB should result in the NBD
>> server behaving exactly the same as the guest device)).
> Not entirely; use of close notifiers was on purpose.  We do not want the
> NBD client to start reading data from a different medium without noticing.
>
> If the NBD server is attached to the BDS, it should keep serving the BDS.

The problem is that it is no longer attached to the BDS, but to the BB.

> If the NBD server is attached to the BB, the clients must have a way to
> notice the medium change, and so far the solution was to close the
> connection.  I'd be wary to change this, though I understand where you
> come from.

Yes, I understand. However, I think the NBD server has to be attached to 
a separate BB if medium changes should not be allowed; but this would 
break backwards compatibility...

I think to retain compatibility we could either just do what we always 
did (although I find it wrong), or we could simply set up an eject 
blocker when attaching an NBD server to a BB. What do you think?

Max

Re: [Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Paolo Bonzini]

On 26/01/2015 21:43, Max Reitz wrote:
>> If the NBD server is attached to the BDS, it should keep serving the BDS.
> 
> The problem is that it is no longer attached to the BDS, but to the BB.

That's not necessarily a problem. :)  It is the cause of the problem though.

Is it possible to attach two BBs to the same BDS?

Because part of the solution could be to introduce a new blockdev-serve
command that takes a BDS, creates a BB and exports that BB.

> I think to retain compatibility we could either just do what we always
> did (although I find it wrong), or we could simply set up an eject
> blocker when attaching an NBD server to a BB. What do you think?

An eject blocker would also break backwards-compatibility though.  What
about an eject notifier?  Would that concept make sense?

Paolo

Re: [Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Max Reitz]

On 2015-01-26 at 16:10, Paolo Bonzini wrote:
>
> On 26/01/2015 21:43, Max Reitz wrote:
>>> If the NBD server is attached to the BDS, it should keep serving the BDS.
>> The problem is that it is no longer attached to the BDS, but to the BB.
> That's not necessarily a problem. :)  It is the cause of the problem though.
>
> Is it possible to attach two BBs to the same BDS?
>
> Because part of the solution could be to introduce a new blockdev-serve
> command that takes a BDS, creates a BB and exports that BB.
>
>> I think to retain compatibility we could either just do what we always
>> did (although I find it wrong), or we could simply set up an eject
>> blocker when attaching an NBD server to a BB. What do you think?
> An eject blocker would also break backwards-compatibility though.  What
> about an eject notifier?  Would that concept make sense?

It does make sense (in that it is the way I would implement "just do 
what we always did"), but I just don't like it for the fact that it 
makes NBD a special snowflake. I can live with it, though.

Max

Re: [Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Paolo Bonzini]

On 26/01/2015 22:13, Max Reitz wrote:
>>>
>> An eject blocker would also break backwards-compatibility though.  What
>> about an eject notifier?  Would that concept make sense?
> 
> It does make sense (in that it is the way I would implement "just do
> what we always did"), but I just don't like it for the fact that it
> makes NBD a special snowflake. I can live with it, though.

Yes, it's weird.  But this is just the backwards-compatible solution.

I'm okay with implementing only the new solution, but:

- the old QMP (and HMP?) commands must be removed

- the new command probably must not reuse the same BB as the guest, and
I am not sure that this is possible.

Paolo

Re: [Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Eric Blake]

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

On 01/26/2015 02:22 PM, Paolo Bonzini wrote:
> 
> 
> On 26/01/2015 22:13, Max Reitz wrote:
>>>>
>>> An eject blocker would also break backwards-compatibility though.  What
>>> about an eject notifier?  Would that concept make sense?
>>
>> It does make sense (in that it is the way I would implement "just do
>> what we always did"), but I just don't like it for the fact that it
>> makes NBD a special snowflake. I can live with it, though.
> 
> Yes, it's weird.  But this is just the backwards-compatible solution.
> 
> I'm okay with implementing only the new solution, but:
> 
> - the old QMP (and HMP?) commands must be removed

Back-compat is a bear to figure out. From libvirt's perspective, it is
okay to require a newer libvirt in order to drive newer qemu (the new
libvirt knowing how to probe whether old or new commands exist, and use
the right one); but it is still awkward any time upgrading qemu without
libvirt causes things to break gratuitously.

> 
> - the new command probably must not reuse the same BB as the guest, and
> I am not sure that this is possible.

We've had that design goal in the back of our minds for some time
(sharing a single BDS among multiple devices) - but I don't think it has
actually happened yet, so if this is the first time we make it happen,
there may be lots of details to get right.  But it makes the most sense
(exporting and NBD disk is a form of creating a _new_ BB - distinct from
a guest-visible device, but both uses are definitely backends; and
sharing the same BDS among both backends makes total sense, so that the
drive visible to the guest can change medium without invalidating the
NBD serving up the old contents).

-- 
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 01/21] block: Guard remaining unsafe blk_bs() callers

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> There are cases where it is probably (!) not necessary to check whether
> the return value of blk_bs() is non-NULL, and those are places after
> blk_new_open(). In every other place, though, there has to be some check
> to make sure that the return value of blk_bs() is non-NULL before it is
> used.
> 
> This patch adds that check to hopefully all of the remaining places.

I did not audit for places you might have missed, but concur that these
needed it.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c          | 18 ++++++++++--------
>  hw/block/xen_disk.c |  4 +++-
>  2 files changed, 13 insertions(+), 9 deletions(-)


Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 02/21] quorum: Fix close path

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> bdrv_unref() can lead to bdrv_close(), which in turn will result in
> bdrv_drain_all(). This function will later be called blk_drain_all() and
> iterate only over the BlockBackends for which blk_is_inserted() holds
> true; therefore, bdrv_is_inserted() and thus quorum_is_inserted() will
> probably be called.
> 
> This patch makes quorum_is_inserted() aware of the fact that some
> children may have been closed already.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/quorum.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

>      for (i = 0; i < s->num_children; i++) {
> -        if (!bdrv_is_inserted(s->bs[i])) {
> +        if (s->bs[i] && !bdrv_is_inserted(s->bs[i])) {
>              return 0;

May have a minor conflict if you fix the earlier series to use bool for
this function's return type.  But that should not get in the way of:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 03/21] block: Add bdrv_close_all() notifiers

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> This adds a list of notifiers to be invoked on bdrv_close_all().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 10 ++++++++++
>  include/block/block.h |  2 ++
>  2 files changed, 12 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
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 04/21] block: Add bdrv_close_all() handlers

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> Every time a reference to a BlockBackend is taken, a notifier for
> bdrv_close_all() has to be deposited so the reference holder can
> relinquish its reference when bdrv_close_all() is called. That notifier
> should be revoked on a bdrv_unref() call.
> 

In addition to the design question about whether NBD exports should be
their own new BB,

> @@ -198,8 +207,12 @@ void blk_ref(BlockBackend *blk)
>   * If this drops it to zero, destroy @blk.
>   * For convenience, do nothing if @blk is null.
>   */
> -void blk_unref(BlockBackend *blk)
> +void blk_unref(BlockBackend *blk, Notifier *close_all_notifier)
>  {
> +    if (close_all_notifier) {
> +        notifier_remove(close_all_notifier);
> +    }
> +
>      if (blk) {

Does removing a notifier when blk is NULL make sense?

-- 
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 04/21] block: Add bdrv_close_all() handlers

[Max Reitz]

On 2015-01-28 at 17:44, Eric Blake wrote:
> On 01/26/2015 12:27 PM, Max Reitz wrote:
>> Every time a reference to a BlockBackend is taken, a notifier for
>> bdrv_close_all() has to be deposited so the reference holder can
>> relinquish its reference when bdrv_close_all() is called. That notifier
>> should be revoked on a bdrv_unref() call.
>>
> In addition to the design question about whether NBD exports should be
> their own new BB,
>
>> @@ -198,8 +207,12 @@ void blk_ref(BlockBackend *blk)
>>    * If this drops it to zero, destroy @blk.
>>    * For convenience, do nothing if @blk is null.
>>    */
>> -void blk_unref(BlockBackend *blk)
>> +void blk_unref(BlockBackend *blk, Notifier *close_all_notifier)
>>   {
>> +    if (close_all_notifier) {
>> +        notifier_remove(close_all_notifier);
>> +    }
>> +
>>       if (blk) {
> Does removing a notifier when blk is NULL make sense?

Considering the pattern is probably "allocate notifier; if that fails, 
goto fail; create BB; if that fails, goto fail; ...; fail: blk_unref(); 
free notifier", the is indeed a case where the notifier may be non-NULL 
but not yet registered (if "create BB" failed). Therefore, to simplify 
the caller code, notifier_remove() should only be called if blk is not 
NULL, right.

Will fix.

Max

Re: [Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Max Reitz]

On 2015-01-28 at 17:05, Eric Blake wrote:
> On 01/26/2015 02:22 PM, Paolo Bonzini wrote:
>>
>> On 26/01/2015 22:13, Max Reitz wrote:
>>>> An eject blocker would also break backwards-compatibility though.  What
>>>> about an eject notifier?  Would that concept make sense?
>>> It does make sense (in that it is the way I would implement "just do
>>> what we always did"), but I just don't like it for the fact that it
>>> makes NBD a special snowflake. I can live with it, though.
>> Yes, it's weird.  But this is just the backwards-compatible solution.
>>
>> I'm okay with implementing only the new solution, but:
>>
>> - the old QMP (and HMP?) commands must be removed
> Back-compat is a bear to figure out. From libvirt's perspective, it is
> okay to require a newer libvirt in order to drive newer qemu (the new
> libvirt knowing how to probe whether old or new commands exist, and use
> the right one); but it is still awkward any time upgrading qemu without
> libvirt causes things to break gratuitously.
>
>> - the new command probably must not reuse the same BB as the guest, and
>> I am not sure that this is possible.
> We've had that design goal in the back of our minds for some time
> (sharing a single BDS among multiple devices) - but I don't think it has
> actually happened yet, so if this is the first time we make it happen,
> there may be lots of details to get right.  But it makes the most sense
> (exporting and NBD disk is a form of creating a _new_ BB - distinct from
> a guest-visible device, but both uses are definitely backends; and
> sharing the same BDS among both backends makes total sense, so that the
> drive visible to the guest can change medium without invalidating the
> NBD serving up the old contents).

Well, I've looked up the discussion Markus, Kevin and me were having; 
our result was that some users may find it useful to have an own BB for 
an NBD server, while others may want to re-use an existing BB. The 
former would be requested by creating an NBD server on a node-name 
instead of giving a device name; if given a device name, NBD will reuse 
that BB, and will detach itself on eject.

Somehow I lost track of that final detail (I blame Christmas and New 
Year), so that's indeed what we decided upon. I will implement it in v2 
(an eject notifier for BBs, which is then used by NBD).

For the sake of completeness: Currently, it is impossible to have 
multiple BBs per BDS, so we cannot yet implement having a separate BB 
for the NBD server. This issue is however unrelated to this series, so 
we should be fine for now.

Max

Re: [Qemu-devel] [PATCH 04/21] block: Add bdrv_close_all() handlers

[Paolo Bonzini]

On 28/01/2015 23:56, Max Reitz wrote:
> Somehow I lost track of that final detail (I blame Christmas and New
> Year), so that's indeed what we decided upon. I will implement it in v2
> (an eject notifier for BBs, which is then used by NBD).

Yeah, I was thinking again about it and an eject notifier really sounds
like the best plan.

Paolo

Re: [Qemu-devel] [PATCH 05/21] block: Remove per-BDS close notifiers

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> They are unused, and bdrv_close_all() is a perfectly valid replacement.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                        | 7 -------
>  block/block-backend.c          | 7 -------
>  include/block/block.h          | 1 -
>  include/block/block_int.h      | 2 --
>  include/sysemu/block-backend.h | 1 -
>  5 files changed, 18 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 06/21] block: Use blk_remove_bs() in blk_delete()

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/block-backend.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 07/21] blockdev: Use blk_remove_bs() in do_drive_del()

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 08/21] block: Make bdrv_close() static

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> There are no users of bdrv_close() left, except for one of bdrv_open()'s
> failure paths, bdrv_close_all() and bdrv_delete(), and that is good.
> Make bdrv_close() static so nobody makes the mistake of directly using
> bdrv_close() again.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 4 +++-
>  include/block/block.h | 1 -
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 09/21] block: Add blk_name_taken()

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> 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>
> ---
>  block.c                        |  2 +-
>  block/block-backend.c          | 19 ++++++++++++++++++-
>  include/sysemu/block-backend.h |  1 +
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0cd6457..8eef0c5 100644
> --- a/block.c
> +++ b/block.c
> @@ -898,7 +898,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
>      }
>  
>      /* takes care of avoiding namespaces collisions */
> -    if (blk_by_name(node_name)) {
> +    if (blk_name_taken(node_name)) {

While you are here, s/namespaces/namespace/

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 10/21] block: Add blk_next_inserted()

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> This function skips to the next BlockBackend for which blk_is_inserted()
> is true.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/block-backend.c          | 15 +++++++++++++++
>  include/sysemu/block-backend.h |  1 +
>  2 files changed, 16 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 11/21] block: Add blk_commit_all() and blk_invalidate_cache_all()

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> 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>
> ---
>  block/block-backend.c          | 10 ++++++++++
>  include/sysemu/block-backend.h |  2 ++
>  2 files changed, 12 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 12/21] block: Use BlockBackend more

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> 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               | 22 ++++++++---------
>  block/block-backend.c |  8 +++----
>  block/qapi.c          | 13 ++++++++--
>  block/snapshot.c      |  3 ++-
>  blockdev.c            | 14 ++++++-----
>  cpus.c                |  7 +++---
>  migration/block.c     | 10 +++++---
>  migration/migration.c |  4 ++--
>  monitor.c             | 13 ++++++----
>  qemu-char.c           |  3 ++-
>  qemu-io.c             |  2 +-
>  qmp.c                 | 14 +++++------
>  savevm.c              | 66 ++++++++++++++++++++++++++++++---------------------
>  xen-mapcache.c        |  3 ++-
>  14 files changed, 107 insertions(+), 75 deletions(-)
> 

> +++ b/block/qapi.c
> @@ -393,15 +393,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)

Don't we still want to list empty BBs in the query (that is,
intentionally list that there are no stats because there is no medium)
rather than silently omitting them?


> @@ -348,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;
>  
> @@ -359,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);
> +

What happens if someone initiates a live migration, then inserts the
medium?  It looks like this will skip over the drive that was empty at
the time the migration started.


-- 
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 13/21] blockdev: Add list of monitor-owned BlockBackends

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> 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.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/block-backend.c          | 27 ++++++++++++++++++++++++++-
>  blockdev.c                     | 14 ++++++++++++++
>  include/sysemu/block-backend.h |  2 ++
>  3 files changed, 42 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 14/21] blockdev: Remove blk_hide_on_behalf_of_do_drive_del()

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> 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 do_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
> do_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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 15/21] block: Make bdrv_drain_one() public

[Eric Blake]

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

On 01/26/2015 12:27 PM, Max Reitz wrote:
> We will need it in block/block-backend.c.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   | 2 +-
>  include/block/block_int.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
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 12/21] block: Use BlockBackend more

[Max Reitz]

On 2015-01-29 at 20:12, Eric Blake wrote:
> On 01/26/2015 12:27 PM, Max Reitz wrote:
>> 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               | 22 ++++++++---------
>>   block/block-backend.c |  8 +++----
>>   block/qapi.c          | 13 ++++++++--
>>   block/snapshot.c      |  3 ++-
>>   blockdev.c            | 14 ++++++-----
>>   cpus.c                |  7 +++---
>>   migration/block.c     | 10 +++++---
>>   migration/migration.c |  4 ++--
>>   monitor.c             | 13 ++++++----
>>   qemu-char.c           |  3 ++-
>>   qemu-io.c             |  2 +-
>>   qmp.c                 | 14 +++++------
>>   savevm.c              | 66 ++++++++++++++++++++++++++++++---------------------
>>   xen-mapcache.c        |  3 ++-
>>   14 files changed, 107 insertions(+), 75 deletions(-)
>>
>> +++ b/block/qapi.c
>> @@ -393,15 +393,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)
> Don't we still want to list empty BBs in the query (that is,
> intentionally list that there are no stats because there is no medium)
> rather than silently omitting them?

Well, maybe, but this patch does not change that behavior. Actually, the 
behavior is changed in the "BlockBackend and media" series, and I guess 
it's probably the patch "blockdev: Do not create BDS for empty drive". 
As of that series in general, there is no BDS for empty drives any more 
and subsequently there are no blockstats any more.

So if anywhere, I'd need to fix it in that series, probably in the "Move 
BlockAcctStats into BlockBackend" patch.

>> @@ -348,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;
>>   
>> @@ -359,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);
>> +
> What happens if someone initiates a live migration, then inserts the
> medium?  It looks like this will skip over the drive that was empty at
> the time the migration started.

Well, to me it looks like its your own fault then. Also, there is no 
difference between using blk_next_inserted() like here and using 
blk_next() + blk_is_inserted(), so you probably can still insert a 
medium during this loop. Maybe. I'm not sure, but I don't think anything 
changed from the old bdrv_next() behavior.

(blk_bext_inserted() iterates over all the BlockBackends and if the one 
you're looking for is not inserted by the time the BB is iterated over, 
it is skipped; if it is inserted, it is not skipped. It doesn't matter 
whether it was inserted at the beginning.)

Max