[Qemu-devel] [PATCH v8 00/12] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Qemu-devel] [PATCH v8 00/12] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD

[Fam Zheng]

This series adds for point-in-time snapshot NBD exporting based on
blockdev-backup (variant of drive-backup with existing device as target).

We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
export it through built in NBD server. The steps are as below:

 1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>

    (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
    providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
    used r/w by guest. Whether or not setting backing file in the image file
    doesn't matter, as we are going to override the backing hd in the next
    step)

 2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2

    (where source-drive is the running BlockDriverState name for
    RUNNING-VM.img. This patch implements "backing=" option to override
    backing_hd for added drive)

 3. (QMP) blockdev-backup device=source-drive sync=none target=target0

    (this is the QMP command introduced by this series, which use a named
    device as target of drive-backup)

 4. (QMP) nbd-server-add device=target0

When image fleecing done:

 1. (QMP) block-job-cancel device=source-drive

 2. (HMP) drive_del target0

 3. (SHELL) rm BACKUP.qcow2

v8: Address comments from Markus, Kevin and Ian. Thanks for the review!

    Cover letter fixed "block-job-cancel". (Ian)

    Dropped "[v7 07/10] block: Pass error in bdrv_snapshot_create". (Markus)
    Dropped "[v7 08/10] block: Add checks of blocker in block operations". (Markus)
    They are neither necessary for this series nor significant improvements.

    [01/10] qapi: Add BlockOperationType enum
            Since 2.0. (Kevin, Markus)
    [02/10] block: Introduce op_blockers to BlockDriverState
            Commit message reword. (Markus)
            Move bdrv_op_blocker_is_empty() here.
    [03/10] block: Replace in_use with operation blocker
            Moved to the front of series.
            Reword commit message. (Markus)
            Move memcpy in bdrv_move_feature_fields() into 02/10. (Markus)
            Don't drop in_use asserts, convert them. (Markus)
            Drop unused local_err. (Markus)
            Simplify bdrv_op_blocker_is_empty(). (Markus)
            Fix error message for do_drive_del() and dataplane. (Markus)
            Remember to error_free for dataplane op blocker. (Markus)
            Drop whitespace change. (Markus)
            Mechanical convert of in_use for block_job_create().
    [04/10] block: Add bdrv_set_backing_hd()
            New. Separate the common part for setting backing_hd.
    [05/10] block: Add backing_blocker in BlockDriverState
            New.
    [06/10] block: Parse "backing" option to reference existing BDS
            Make use of 04/10.
            We need to delete "backing" from qdict, otherwise QMP complains
            with an supported option, not dropping it. (Markus)
            Move error_free(bs->backing_blocker) in bdrv_close(). (Markus)
            Reword comment for backing_blocker. (Markus)
            03/10 retained the logic of in_use assert, this is no longer true
            since backing_hd has a blocker on it. So drop it. (Markus)
    [07/10] block: Support dropping active in bdrv_drop_intermediate
            Make use of 04/10. (Kevin)
            Pass NULL, NULL instead of empty string to bdrv_change_backing_file. (Kevin)
    [08/10] stream: Use bdrv_drop_intermediate and drop close_unused_images
    [09/10] qmp: Add command 'blockdev-backup'
            Since 1.8 -> Since 2.0.
    [10/10] block: Allow backup on referenced named BlockDriverState

v7: Fix typo in cover letter "ide0-hd0". (Markus)
    Rebase to current qemu.git. (Resolved a few trivial contextual conflict)

v6: Address Paolo's comments, (except for bitmask):
    - Add blocker for all backing_hd references, a relatively big change, some
      patches are reordered.
    - Introduce a few other necessary patches.
    - Move two snapshot checks into bdrv_snapshot_*.

    The interface is unchanged.

Fam Zheng (12):
  blkdebug: Use QLIST_FOREACH_SAFE to resume IO
  qapi: Add BlockOperationType enum
  block: Introduce op_blockers to BlockDriverState
  block: Replace in_use with operation blocker
  block: Move op_blocker check from block_job_create to its caller
  block: Add bdrv_set_backing_hd()
  block: Add backing_blocker in BlockDriverState
  block: Parse "backing" option to reference existing BDS
  block: Support dropping active in bdrv_drop_intermediate
  stream: Use bdrv_drop_intermediate and drop close_unused_images
  qmp: Add command 'blockdev-backup'
  block: Allow backup on referenced named BlockDriverState

 block-migration.c               |   7 +-
 block.c                         | 291 ++++++++++++++++++++++++++--------------
 block/backup.c                  |  21 +++
 block/blkdebug.c                |   8 +-
 block/commit.c                  |   1 +
 block/stream.c                  |  28 +---
 blockdev.c                      |  70 ++++++++--
 blockjob.c                      |  14 +-
 hw/block/dataplane/virtio-blk.c |  18 ++-
 include/block/block.h           |  10 +-
 include/block/block_int.h       |   9 +-
 include/block/blockjob.h        |   3 +
 qapi-schema.json                |  99 ++++++++++++++
 qmp-commands.hx                 |  44 ++++++
 14 files changed, 465 insertions(+), 158 deletions(-)

-- 
1.8.5.1

[Qemu-devel] [PATCH v8 01/12] blkdebug: Use QLIST_FOREACH_SAFE to resume IO

[Fam Zheng]

Qemu-iotest 030 was broken.

When the coroutine runs and finishes, it will remove itself from the req
list, so let's use safe version of foreach to avoid use after free.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/blkdebug.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 37cf028..957be2c 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -594,9 +594,9 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
 static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)
 {
     BDRVBlkdebugState *s = bs->opaque;
-    BlkdebugSuspendedReq *r;
+    BlkdebugSuspendedReq *r, *next;
 
-    QLIST_FOREACH(r, &s->suspended_reqs, next) {
+    QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) {
         if (!strcmp(r->tag, tag)) {
             qemu_coroutine_enter(r->co, NULL);
             return 0;
@@ -609,7 +609,7 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
                                             const char *tag)
 {
     BDRVBlkdebugState *s = bs->opaque;
-    BlkdebugSuspendedReq *r;
+    BlkdebugSuspendedReq *r, *r_next;
     BlkdebugRule *rule, *next;
     int i, ret = -ENOENT;
 
@@ -622,7 +622,7 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
             }
         }
     }
-    QLIST_FOREACH(r, &s->suspended_reqs, next) {
+    QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, r_next) {
         if (!strcmp(r->tag, tag)) {
             qemu_coroutine_enter(r->co, NULL);
             ret = 0;
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 02/12] qapi: Add BlockOperationType enum

[Fam Zheng]

This adds the enum of all the operations that can be taken on a block
device.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qapi-schema.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index d6f8615..8e982a2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1440,6 +1440,56 @@
   'data': ['commit', 'stream', 'mirror', 'backup'] }
 
 ##
+# @BlockOperationType
+#
+# Type of a block operation. (since 2.0)
+#
+# @backup-source: As a backup source. See the 'drive-backup' command.
+#
+# @backup-target: As a backup target. See the 'drive-backup' command.
+#
+# @change: See the 'change' command.
+#
+# @commit: See the 'block-commit' command.
+#
+# @dataplane: The virtio-blk dataplane feature.
+#
+# @drive-del: See the 'drive_del' HMP command.
+#
+# @eject: See the 'eject' command.
+#
+# @external-snapshot: See the 'blockdev-snapshot-sync' command.
+#
+# @internal-snapshot: See the 'blockdev-snapshot-internal-sync' command.
+#
+# @internal-snapshot-delete: See the 'blockdev-snapshot-delete-internal-sync' command.
+#
+# @mirror: See the 'drive-mirror' command.
+#
+# @resize: See the 'block-resize' command.
+#
+# @stream: See the 'block-stream' command.
+#
+# Since: 2.0
+##
+{ 'enum': 'BlockOpType',
+  'data': [
+    'backup-source',
+    'backup-target',
+    'change',
+    'commit',
+    'dataplane',
+    'drive-del',
+    'eject',
+    'external-snapshot',
+    'internal-snapshot',
+    'internal-snapshot-delete',
+    'mirror',
+    'resize',
+    'stream'
+] }
+
+##
 # @BlockJobInfo:
 #
 # Information about a long-running block device operation.
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 03/12] block: Introduce op_blockers to BlockDriverState

[Fam Zheng]

BlockDriverState.op_blockers is an array of lists with BLOCK_OP_TYPE_MAX
elements. Each list is a list of blockers of an operation type
(BlockOpType), that marks this BDS as currently blocked for a certain
type of operation with reason errors stored in the list. The rule of
usage is:

 * BDS user who wants to take an operation should check if there's any
   blocker of the type with bdrv_op_is_blocked().

 * BDS user who wants to block certain types of operation, should call
   bdrv_op_block (or bdrv_op_block_all to block all types of operations,
   which is similar to the existing bdrv_set_in_use()).

 * A blocker is only referenced by op_blockers, so the lifecycle is
   managed by caller, and shouldn't be lost until unblock, so typically
   a caller does these:

   - Allocate a blocker with error_setg or similar, call bdrv_op_block()
     to block some operations.
   - Hold the blocker, do his job.
   - Unblock operations that it blocked, with the same reason pointer
     passed to bdrv_op_unblock().
   - Release the blocker with error_free().

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 71 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  7 +++++
 include/block/block_int.h |  5 ++++
 3 files changed, 83 insertions(+)

diff --git a/block.c b/block.c
index 13f001a..74547af 100644
--- a/block.c
+++ b/block.c
@@ -1627,6 +1627,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     /* keep the same entry in bdrv_states */
     pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
             bs_src->device_name);
+    memcpy(bs_dest->op_blockers, bs_src->op_blockers,
+           sizeof(bs_dest->op_blockers));
     bs_dest->list = bs_src->list;
 }
 
@@ -4629,6 +4631,75 @@ void bdrv_unref(BlockDriverState *bs)
     }
 }
 
+struct BdrvOpBlocker {
+    Error *reason;
+    QLIST_ENTRY(BdrvOpBlocker) list;
+};
+
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
+{
+    BdrvOpBlocker *blocker;
+    assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+    if (!QLIST_EMPTY(&bs->op_blockers[op])) {
+        blocker = QLIST_FIRST(&bs->op_blockers[op]);
+        if (errp) {
+            *errp = error_copy(blocker->reason);
+        }
+        return true;
+    }
+    return false;
+}
+
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+    BdrvOpBlocker *blocker;
+    assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+
+    blocker = g_malloc0(sizeof(BdrvOpBlocker));
+    blocker->reason = reason;
+    QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
+}
+
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+    BdrvOpBlocker *blocker, *next;
+    assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+    QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
+        if (blocker->reason == reason) {
+            QLIST_REMOVE(blocker, list);
+            g_free(blocker);
+        }
+    }
+}
+
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason)
+{
+    int i;
+    for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+        bdrv_op_block(bs, i, reason);
+    }
+}
+
+void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
+{
+    int i;
+    for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+        bdrv_op_unblock(bs, i, reason);
+    }
+}
+
+bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
+{
+    int i;
+
+    for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+        if (!QLIST_EMPTY(&bs->op_blockers[i])) {
+            return false;
+        }
+    }
+    return true;
+}
+
 void bdrv_set_in_use(BlockDriverState *bs, int in_use)
 {
     assert(bs->in_use != in_use);
diff --git a/include/block/block.h b/include/block/block.h
index 36efaea..890af1a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -442,6 +442,13 @@ void bdrv_unref(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason);
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason);
+void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason);
+bool bdrv_op_blocker_is_empty(BlockDriverState *bs);
+
 #ifdef CONFIG_LINUX_AIO
 int raw_get_aio_fd(BlockDriverState *bs);
 #else
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8b132d7..458acd6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -252,6 +252,8 @@ typedef struct BlockLimits {
     int opt_transfer_length;
 } BlockLimits;
 
+typedef struct BdrvOpBlocker BdrvOpBlocker;
+
 /*
  * Note: the function bdrv_append() copies and swaps contents of
  * BlockDriverStates, so if you add new fields to this struct, please
@@ -333,6 +335,9 @@ struct BlockDriverState {
 
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
 
+    /* operation blockers */
+    QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX];
+
     /* long-running background operation */
     BlockJob *job;
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 04/12] block: Replace in_use with operation blocker

[Fam Zheng]

This drops BlockDriverState.in_use with op_blockers:

  - Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1).
  - Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0).
  - Check bdrv_op_is_blocked() in place of bdrv_in_use(bs).
    The specific types are used, e.g. in place of starting block backup,
    bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...).
  - Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).

Note: there is only bdrv_op_block_all and bdrv_op_unblock_all callers at
this moment. So although the checks are specific to op types, this
changes can still be seen as identical logic with previously with
in_use. The difference is error message are improved because of blocker
error info.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block-migration.c               |  7 +++++--
 block.c                         | 24 +++++++-----------------
 blockdev.c                      | 15 ++++++---------
 blockjob.c                      | 14 +++++++++-----
 hw/block/dataplane/virtio-blk.c | 18 ++++++++++++------
 include/block/block.h           |  2 --
 include/block/block_int.h       |  1 -
 include/block/blockjob.h        |  3 +++
 8 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 897fdba..bf9a25f 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -59,6 +59,7 @@ typedef struct BlkMigDevState {
     unsigned long *aio_bitmap;
     int64_t completed_sectors;
     BdrvDirtyBitmap *dirty_bitmap;
+    Error *blocker;
 } BlkMigDevState;
 
 typedef struct BlkMigBlock {
@@ -346,7 +347,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
         bmds->completed_sectors = 0;
         bmds->shared_base = block_mig_state.shared_base;
         alloc_aio_bitmap(bmds);
-        bdrv_set_in_use(bs, 1);
+        error_setg(&bmds->blocker, "block device is in use by migration");
+        bdrv_op_block_all(bs, bmds->blocker);
         bdrv_ref(bs);
 
         block_mig_state.total_sector_sum += sectors;
@@ -584,7 +586,8 @@ static void blk_mig_cleanup(void)
     blk_mig_lock();
     while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
         QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
-        bdrv_set_in_use(bmds->bs, 0);
+        bdrv_op_unblock_all(bmds->bs, bmds->blocker);
+        error_free(bmds->blocker);
         bdrv_unref(bmds->bs);
         g_free(bmds->aio_bitmap);
         g_free(bmds);
diff --git a/block.c b/block.c
index 74547af..28814f7 100644
--- a/block.c
+++ b/block.c
@@ -1621,7 +1621,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     bs_dest->refcnt             = bs_src->refcnt;
 
     /* job */
-    bs_dest->in_use             = bs_src->in_use;
     bs_dest->job                = bs_src->job;
 
     /* keep the same entry in bdrv_states */
@@ -1653,7 +1652,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
     assert(bs_new->job == NULL);
     assert(bs_new->dev == NULL);
-    assert(bs_new->in_use == 0);
+    assert(bdrv_op_blocker_is_empty(bs_new));
     assert(bs_new->io_limits_enabled == false);
     assert(!throttle_have_timer(&bs_new->throttle_state));
 
@@ -1672,7 +1671,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     /* Check a few fields that should remain attached to the device */
     assert(bs_new->dev == NULL);
     assert(bs_new->job == NULL);
-    assert(bs_new->in_use == 0);
+    assert(bdrv_op_blocker_is_empty(bs_new));
     assert(bs_new->io_limits_enabled == false);
     assert(!throttle_have_timer(&bs_new->throttle_state));
 
@@ -1709,7 +1708,7 @@ static void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
     assert(!bs->job);
-    assert(!bs->in_use);
+    assert(bdrv_op_blocker_is_empty(bs));
     assert(!bs->refcnt);
     assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
@@ -1891,7 +1890,8 @@ int bdrv_commit(BlockDriverState *bs)
         return -ENOTSUP;
     }
 
-    if (bdrv_in_use(bs) || bdrv_in_use(bs->backing_hd)) {
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, NULL) ||
+        bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, NULL)) {
         return -EBUSY;
     }
 
@@ -2919,8 +2919,9 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     if (bs->read_only)
         return -EACCES;
-    if (bdrv_in_use(bs))
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
         return -EBUSY;
+    }
     ret = drv->bdrv_truncate(bs, offset);
     if (ret == 0) {
         ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
@@ -4700,17 +4701,6 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
     return true;
 }
 
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
-{
-    assert(bs->in_use != in_use);
-    bs->in_use = in_use;
-}
-
-int bdrv_in_use(BlockDriverState *bs)
-{
-    return bs->in_use;
-}
-
 void bdrv_iostatus_enable(BlockDriverState *bs)
 {
     bs->iostatus_enabled = true;
diff --git a/blockdev.c b/blockdev.c
index 44755e1..29958bc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1224,8 +1224,8 @@ static void external_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
-    if (bdrv_in_use(state->old_bs)) {
-        error_set(errp, QERR_DEVICE_IN_USE, device);
+    if (bdrv_op_is_blocked(state->old_bs,
+                           BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
         return;
     }
 
@@ -1441,8 +1441,7 @@ exit:
 
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {
-    if (bdrv_in_use(bs)) {
-        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
         return;
     }
     if (!bdrv_dev_has_removable_media(bs)) {
@@ -1643,7 +1642,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
         qerror_report(QERR_DEVICE_NOT_FOUND, id);
         return -1;
     }
-    if (bdrv_in_use(bs)) {
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
         qerror_report(QERR_DEVICE_IN_USE, id);
         return -1;
     }
@@ -1881,8 +1880,7 @@ void qmp_drive_backup(const char *device, const char *target,
         }
     }
 
-    if (bdrv_in_use(bs)) {
-        error_set(errp, QERR_DEVICE_IN_USE, device);
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
         return;
     }
 
@@ -2011,8 +2009,7 @@ void qmp_drive_mirror(const char *device, const char *target,
         }
     }
 
-    if (bdrv_in_use(bs)) {
-        error_set(errp, QERR_DEVICE_IN_USE, device);
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR, errp)) {
         return;
     }
 
diff --git a/blockjob.c b/blockjob.c
index 9e5fd5c..f1ff036 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -41,14 +41,16 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
 {
     BlockJob *job;
 
-    if (bs->job || bdrv_in_use(bs)) {
+    if (bs->job || !bdrv_op_blocker_is_empty(bs)) {
         error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
         return NULL;
     }
     bdrv_ref(bs);
-    bdrv_set_in_use(bs, 1);
-
     job = g_malloc0(driver->instance_size);
+    error_setg(&job->blocker, "block device is in use by block job: %s",
+               BlockJobType_lookup[driver->job_type]);
+    bdrv_op_block_all(bs, job->blocker);
+
     job->driver        = driver;
     job->bs            = bs;
     job->cb            = cb;
@@ -63,8 +65,9 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
         block_job_set_speed(job, speed, &local_err);
         if (error_is_set(&local_err)) {
             bs->job = NULL;
+            bdrv_op_unblock_all(bs, job->blocker);
+            error_free(job->blocker);
             g_free(job);
-            bdrv_set_in_use(bs, 0);
             error_propagate(errp, local_err);
             return NULL;
         }
@@ -79,8 +82,9 @@ void block_job_completed(BlockJob *job, int ret)
     assert(bs->job == job);
     job->cb(job->opaque, ret);
     bs->job = NULL;
+    bdrv_op_unblock_all(bs, job->blocker);
+    error_free(job->blocker);
     g_free(job);
-    bdrv_set_in_use(bs, 0);
 }
 
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index f2d7350..dde58e5 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -69,6 +69,9 @@ struct VirtIOBlockDataPlane {
                                              queue */
 
     unsigned int num_reqs;
+
+    /* Operation blocker on BDS */
+    Error *blocker;
 };
 
 /* Raise an interrupt to signal guest, if necessary */
@@ -385,6 +388,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
 {
     VirtIOBlockDataPlane *s;
     int fd;
+    Error *local_err = NULL;
 
     *dataplane = NULL;
 
@@ -406,8 +410,10 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     /* If dataplane is (re-)enabled while the guest is running there could be
      * block jobs that can conflict.
      */
-    if (bdrv_in_use(blk->conf.bs)) {
-        error_report("cannot start dataplane thread while device is in use");
+    if (bdrv_op_is_blocked(blk->conf.bs, BLOCK_OP_TYPE_DATAPLANE, &local_err)) {
+        error_report("cannot start dataplane thread: %s",
+                      error_get_pretty(local_err));
+        error_free(local_err);
         return false;
     }
 
@@ -422,9 +428,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     s->vdev = vdev;
     s->fd = fd;
     s->blk = blk;
-
-    /* Prevent block operations that conflict with data plane thread */
-    bdrv_set_in_use(blk->conf.bs, 1);
+    error_setg(&s->blocker, "block device is in use by data plane");
+    bdrv_op_block_all(blk->conf.bs, s->blocker);
 
     *dataplane = s;
     return true;
@@ -437,7 +442,8 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     }
 
     virtio_blk_data_plane_stop(s);
-    bdrv_set_in_use(s->blk->conf.bs, 0);
+    bdrv_op_unblock_all(s->blk->conf.bs, s->blocker);
+    error_free(s->blocker);
     g_free(s);
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index 890af1a..ac8976e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -439,8 +439,6 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
-void bdrv_set_in_use(BlockDriverState *bs, int in_use);
-int bdrv_in_use(BlockDriverState *bs);
 
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 458acd6..2f6556d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -330,7 +330,6 @@ struct BlockDriverState {
     char device_name[32];
     QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
     int refcnt;
-    int in_use; /* users other than guest access, eg. block migration */
     QTAILQ_ENTRY(BlockDriverState) list;
 
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index d76de62..c0a7875 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -106,6 +106,9 @@ struct BlockJob {
     /** The completion function that will be called when the job completes.  */
     BlockDriverCompletionFunc *cb;
 
+    /** Block other operations when block job is running */
+    Error *blocker;
+
     /** The opaque value that is passed to the completion function.  */
     void *opaque;
 };
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 05/12] block: Move op_blocker check from block_job_create to its caller

[Fam Zheng]

It makes no sense to check for "any" blocker on bs, we are here only
because of the mechanical conversion from in_use to op_blockers. Remove
it now, and let the callers check specific operation types. Backup and
mirror already have it, add checker to stream and commit.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 8 ++++++++
 blockjob.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 29958bc..f342a80 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1754,6 +1754,10 @@ void qmp_block_stream(const char *device, bool has_base,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
+        return;
+    }
+
     if (base) {
         base_bs = bdrv_find_backing_image(bs, base);
         if (base_bs == NULL) {
@@ -1794,6 +1798,10 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
+        return;
+    }
+
     /* default top_bs is the active layer */
     top_bs = bs;
 
diff --git a/blockjob.c b/blockjob.c
index f1ff036..21e21c0 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -41,7 +41,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
 {
     BlockJob *job;
 
-    if (bs->job || !bdrv_op_blocker_is_empty(bs)) {
+    if (bs->job) {
         error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
         return NULL;
     }
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 06/12] block: Add bdrv_set_backing_hd()

[Fam Zheng]

This is the common but non-trivial steps to assign or change the
backing_hd of BDS.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c               | 29 +++++++++++++++++++++++------
 include/block/block.h |  1 +
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 28814f7..63a5918 100644
--- a/block.c
+++ b/block.c
@@ -958,6 +958,24 @@ fail:
     return ret;
 }
 
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
+{
+    if (bs->backing_hd) {
+        bdrv_unref(bs->backing_hd);
+    }
+
+    bs->backing_hd = backing_hd;
+    if (!backing_hd) {
+        return;
+    }
+    bdrv_ref(bs->backing_hd);
+
+    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+            bs->backing_hd->file->filename);
+    pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+            bs->backing_hd->drv->format_name);
+}
+
 /*
  * Opens the backing file for a BlockDriverState if not yet open
  *
@@ -971,6 +989,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
     char backing_filename[PATH_MAX];
     int back_flags, ret;
     BlockDriver *back_drv = NULL;
+    BlockDriverState *backing_hd;
     Error *local_err = NULL;
 
     if (bs->backing_hd != NULL) {
@@ -994,7 +1013,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
                                        sizeof(backing_filename));
     }
 
-    bs->backing_hd = bdrv_new("");
+    backing_hd = bdrv_new("");
 
     if (bs->backing_format[0] != '\0') {
         back_drv = bdrv_find_format(bs->backing_format);
@@ -1004,20 +1023,18 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
     back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
                                     BDRV_O_COPY_ON_READ);
 
-    ret = bdrv_open(bs->backing_hd,
+    ret = bdrv_open(backing_hd,
                     *backing_filename ? backing_filename : NULL, options,
                     back_flags, back_drv, &local_err);
     if (ret < 0) {
-        bdrv_unref(bs->backing_hd);
-        bs->backing_hd = NULL;
+        bdrv_unref(backing_hd);
         bs->open_flags |= BDRV_O_NO_BACKING;
         error_setg(errp, "Could not open backing file: %s",
                    error_get_pretty(local_err));
         error_free(local_err);
         return ret;
     }
-    pstrcpy(bs->backing_file, sizeof(bs->backing_file),
-            bs->backing_hd->file->filename);
+    bdrv_set_backing_hd(bs, backing_hd);
     return 0;
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index ac8976e..20bfcd9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -185,6 +185,7 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename,
                    QDict *options, int flags, Error **errp);
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
 int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
               int flags, BlockDriver *drv, Error **errp);
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 07/12] block: Add backing_blocker in BlockDriverState

[Fam Zheng]

This makes use of op_blocker and blocks all the operations except for
commit target, on each BlockDriverState->backing_hd.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 13 +++++++++++++
 include/block/block_int.h |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/block.c b/block.c
index 63a5918..b3993d7 100644
--- a/block.c
+++ b/block.c
@@ -961,7 +961,13 @@ fail:
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
 {
     if (bs->backing_hd) {
+        assert(error_is_set(&bs->backing_blocker));
+        bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
         bdrv_unref(bs->backing_hd);
+    } else {
+        error_setg(&bs->backing_blocker,
+                   "device is used as backing hd of '%s'",
+                   bs->device_name);
     }
 
     bs->backing_hd = backing_hd;
@@ -970,6 +976,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     }
     bdrv_ref(bs->backing_hd);
 
+    bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
+    /* Otherwise we won't be able to commit due to check in bdrv_commit */
+    bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
+                    bs->backing_blocker);
     pstrcpy(bs->backing_file, sizeof(bs->backing_file),
             bs->backing_hd->file->filename);
     pstrcpy(bs->backing_format, sizeof(bs->backing_format),
@@ -1476,6 +1486,9 @@ void bdrv_close(BlockDriverState *bs)
 
     if (bs->drv) {
         if (bs->backing_hd) {
+            assert(error_is_set(&bs->backing_blocker));
+            bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
+            error_free(bs->backing_blocker);
             bdrv_unref(bs->backing_hd);
             bs->backing_hd = NULL;
         }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 2f6556d..1ac17d5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -341,6 +341,9 @@ struct BlockDriverState {
     BlockJob *job;
 
     QDict *options;
+
+    /* The error object in use for blocking operations on backing_hd */
+    Error *backing_blocker;
 };
 
 int get_tmp_filename(char *filename, int size);
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 08/12] block: Parse "backing" option to reference existing BDS

[Fam Zheng]

Now it's safe to allow reference for backing_hd in the interface.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index b3993d7..fba7148 100644
--- a/block.c
+++ b/block.c
@@ -1191,11 +1191,25 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     /* If there is a backing file, use it */
     if ((flags & BDRV_O_NO_BACKING) == 0) {
         QDict *backing_options;
-
-        qdict_extract_subqdict(options, &backing_options, "backing.");
-        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
-        if (ret < 0) {
-            goto close_and_fail;
+        const char *backing_name;
+        BlockDriverState *backing_hd;
+
+        backing_name = qdict_get_try_str(options, "backing");
+        qdict_del(options, "backing");
+        if (backing_name) {
+            backing_hd = bdrv_find(backing_name);
+            if (!backing_hd) {
+                error_set(&local_err, QERR_DEVICE_NOT_FOUND, backing_name);
+                ret = -ENOENT;
+                goto close_and_fail;
+            }
+            bdrv_set_backing_hd(bs, backing_hd);
+        } else {
+            qdict_extract_subqdict(options, &backing_options, "backing.");
+            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
+            if (ret < 0) {
+                goto close_and_fail;
+            }
         }
     }
 
@@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
     assert(bs_new->job == NULL);
     assert(bs_new->dev == NULL);
-    assert(bdrv_op_blocker_is_empty(bs_new));
     assert(bs_new->io_limits_enabled == false);
     assert(!throttle_have_timer(&bs_new->throttle_state));
 
@@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
     /* Check a few fields that should remain attached to the device */
     assert(bs_new->dev == NULL);
     assert(bs_new->job == NULL);
-    assert(bdrv_op_blocker_is_empty(bs_new));
     assert(bs_new->io_limits_enabled == false);
     assert(!throttle_have_timer(&bs_new->throttle_state));
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 09/12] block: Support dropping active in bdrv_drop_intermediate

[Fam Zheng]

Dropping intermediate could be useful both for commit and stream, and
BDS refcnt plus bdrv_swap could do most of the job nicely. It also needs
to work with op blockers.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c        | 142 +++++++++++++++++++++++++--------------------------------
 block/commit.c |   1 +
 2 files changed, 63 insertions(+), 80 deletions(-)

diff --git a/block.c b/block.c
index fba7148..e5e7b0b 100644
--- a/block.c
+++ b/block.c
@@ -2176,114 +2176,96 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
     return overlay;
 }
 
-typedef struct BlkIntermediateStates {
-    BlockDriverState *bs;
-    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
-} BlkIntermediateStates;
-
-
 /*
- * Drops images above 'base' up to and including 'top', and sets the image
- * above 'top' to have base as its backing file.
+ * Drops images above 'base' up to and including 'top', and sets new 'base'
+ * as backing_hd of top_overlay (the image orignally has 'top' as backing
+ * file). top_overlay may be NULL if 'top' is active, no such update needed.
+ * Requires that the top_overlay to 'top' is opened r/w.
+ *
+ * 1) This will convert the following chain:
+ * ... <- base <- ... <- top <- overlay <-... <- active
+ *
+ * to
  *
- * Requires that the overlay to 'top' is opened r/w, so that the backing file
- * information in 'bs' can be properly updated.
+ * ... <- base <- overlay <- active
  *
- * E.g., this will convert the following chain:
- * bottom <- base <- intermediate <- top <- active
+ * 2) It is allowed for bottom==base, in which case it converts:
+ *
+ * ... <- base <- ... <- top <- overlay <- ... <- active
  *
  * to
  *
- * bottom <- base <- active
+ * base <- overlay <- active
  *
- * It is allowed for bottom==base, in which case it converts:
+ * 2) It also allows active==top, in which case it converts:
  *
- * base <- intermediate <- top <- active
+ * ... <- base <- ... <- top (active)
  *
  * to
  *
- * base <- active
+ * base == active == top, i.e. only base and lower remains: *top == *base when
+ * return.
+ *
+ * 3) If base==NULL, it will drop all the BDS below overlay and set its
+ * backing_hd to NULL. I.e.:
+ *
+ * base(NULL) <- ... <- overlay <- ... <- active
  *
- * Error conditions:
- *  if active == top, that is considered an error
+ * to
  *
+ * overlay <- ... <- active
  */
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
                            BlockDriverState *base)
 {
-    BlockDriverState *intermediate;
-    BlockDriverState *base_bs = NULL;
-    BlockDriverState *new_top_bs = NULL;
-    BlkIntermediateStates *intermediate_state, *next;
-    int ret = -EIO;
-
-    QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
-    QSIMPLEQ_INIT(&states_to_delete);
+    BlockDriverState *drop_start, *overlay;
+    int ret = -EINVAL;
 
-    if (!top->drv || !base->drv) {
+    if (!top->drv || (base && !base->drv)) {
         goto exit;
     }
-
-    new_top_bs = bdrv_find_overlay(active, top);
-
-    if (new_top_bs == NULL) {
-        /* we could not find the image above 'top', this is an error */
-        goto exit;
-    }
-
-    /* special case of new_top_bs->backing_hd already pointing to base - nothing
-     * to do, no intermediate images */
-    if (new_top_bs->backing_hd == base) {
+    if (top == base) {
         ret = 0;
-        goto exit;
-    }
-
-    intermediate = top;
-
-    /* now we will go down through the list, and add each BDS we find
-     * into our deletion queue, until we hit the 'base'
-     */
-    while (intermediate) {
-        intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
-        intermediate_state->bs = intermediate;
-        QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
-
-        if (intermediate->backing_hd == base) {
-            base_bs = intermediate->backing_hd;
-            break;
+    } else if (top == active) {
+        assert(base);
+        drop_start = active->backing_hd;
+        bdrv_swap(active, base);
+        base->backing_hd = NULL;
+        bdrv_unref(drop_start);
+        ret = 0;
+    } else {
+        /* If there's an overlay, its backing_hd points to top's BDS now,
+         * the top image is dropped but this BDS structure is kept and swapped
+         * with base, this way we keep the pointers valid after dropping top */
+        overlay = bdrv_find_overlay(active, top);
+        if (!overlay) {
+            goto exit;
+        }
+        if (base) {
+            ret = bdrv_change_backing_file(overlay, base->filename,
+                                           base->drv->format_name);
+        } else {
+            ret = bdrv_change_backing_file(overlay, NULL, NULL);
+        }
+        if (ret) {
+            goto exit;
+        }
+        if (base) {
+            drop_start = top->backing_hd;
+            bdrv_swap(top, base);
+            /* Break the loop formed by bdrv_swap */
+            bdrv_set_backing_hd(base, NULL);
+        } else {
+            bdrv_set_backing_hd(overlay, NULL);
+            drop_start = top;
         }
-        intermediate = intermediate->backing_hd;
-    }
-    if (base_bs == NULL) {
-        /* something went wrong, we did not end at the base. safely
-         * unravel everything, and exit with error */
-        goto exit;
-    }
-
-    /* success - we can delete the intermediate states, and link top->base */
-    ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
-                                   base_bs->drv ? base_bs->drv->format_name : "");
-    if (ret) {
-        goto exit;
-    }
-    new_top_bs->backing_hd = base_bs;
-
 
-    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
-        /* so that bdrv_close() does not recursively close the chain */
-        intermediate_state->bs->backing_hd = NULL;
-        bdrv_unref(intermediate_state->bs);
+        bdrv_unref(drop_start);
     }
-    ret = 0;
-
 exit:
-    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
-        g_free(intermediate_state);
-    }
     return ret;
 }
 
-
 static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
                                    size_t size)
 {
diff --git a/block/commit.c b/block/commit.c
index d4090cb..4d8cd05 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -142,6 +142,7 @@ wait:
     if (!block_job_is_cancelled(&s->common) && sector_num == end) {
         /* success */
         ret = bdrv_drop_intermediate(active, top, base);
+        base = top;
     }
 
 exit_free_buf:
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 10/12] stream: Use bdrv_drop_intermediate and drop close_unused_images

[Fam Zheng]

This reuses the new bdrv_drop_intermediate.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/stream.c | 28 +---------------------------
 1 file changed, 1 insertion(+), 27 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 46bec7d..9cdcf0e 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -51,32 +51,6 @@ static int coroutine_fn stream_populate(BlockDriverState *bs,
     return bdrv_co_copy_on_readv(bs, sector_num, nb_sectors, &qiov);
 }
 
-static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
-                                const char *base_id)
-{
-    BlockDriverState *intermediate;
-    intermediate = top->backing_hd;
-
-    /* Must assign before bdrv_delete() to prevent traversing dangling pointer
-     * while we delete backing image instances.
-     */
-    top->backing_hd = base;
-
-    while (intermediate) {
-        BlockDriverState *unused;
-
-        /* reached base */
-        if (intermediate == base) {
-            break;
-        }
-
-        unused = intermediate;
-        intermediate = intermediate->backing_hd;
-        unused->backing_hd = NULL;
-        bdrv_unref(unused);
-    }
-}
-
 static void coroutine_fn stream_run(void *opaque)
 {
     StreamBlockJob *s = opaque;
@@ -190,7 +164,7 @@ wait:
             }
         }
         ret = bdrv_change_backing_file(bs, base_id, base_fmt);
-        close_unused_images(bs, base, base_id);
+        bdrv_drop_intermediate(bs, bs->backing_hd, base);
     }
 
     qemu_vfree(buf);
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 11/12] qmp: Add command 'blockdev-backup'

[Fam Zheng]

Similar to drive-backup, but this command uses a device id as target
instead of creating/opening an image file.

Also add blocker on target bs, since the target is also a named device
now.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c   | 21 +++++++++++++++++++++
 blockdev.c       | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+)

diff --git a/block/backup.c b/block/backup.c
index 0198514..c8fe1a9 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -339,6 +339,7 @@ static void coroutine_fn backup_run(void *opaque)
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
+    bdrv_op_unblock_all(target, job->common.blocker);
     bdrv_unref(target);
 
     block_job_completed(&job->common, ret);
@@ -364,6 +365,24 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bs->device_name);
+        return;
+    }
+
+    if (!bdrv_is_inserted(target)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target->device_name);
+        return;
+    }
+
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
+        return;
+    }
+
+    if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
+        return;
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -377,6 +396,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    bdrv_op_block_all(target, job->common.blocker);
+
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->target = target;
diff --git a/blockdev.c b/blockdev.c
index f342a80..1bf9a16 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1872,6 +1872,8 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
+    /* Although backup_run has this check too, we need to use bs->drv below, so
+     * do an early check redundantly. */
     if (!bdrv_is_inserted(bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
@@ -1888,6 +1890,7 @@ void qmp_drive_backup(const char *device, const char *target,
         }
     }
 
+    /* Early check to avoid creating target */
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
         return;
     }
@@ -1946,6 +1949,50 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 }
 
+void qmp_blockdev_backup(const char *device, const char *target,
+                         enum MirrorSyncMode sync,
+                         bool has_speed, int64_t speed,
+                         bool has_on_source_error,
+                         BlockdevOnError on_source_error,
+                         bool has_on_target_error,
+                         BlockdevOnError on_target_error,
+                         Error **errp)
+{
+    BlockDriverState *bs;
+    BlockDriverState *target_bs;
+    Error *local_err = NULL;
+
+    if (!has_speed) {
+        speed = 0;
+    }
+    if (!has_on_source_error) {
+        on_source_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+    if (!has_on_target_error) {
+        on_target_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    target_bs = bdrv_find(target);
+    if (!target_bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+        return;
+    }
+
+    bdrv_ref(target_bs);
+    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+                 block_job_cb, bs, &local_err);
+    if (local_err != NULL) {
+        bdrv_unref(target_bs);
+        error_propagate(errp, local_err);
+    }
+}
+
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
 
 void qmp_drive_mirror(const char *device, const char *target,
diff --git a/qapi-schema.json b/qapi-schema.json
index 8e982a2..ecea383 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1869,6 +1869,40 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @BlockdevBackup
+#
+# @device: the name of the device which should be copied.
+#
+# @target: the name of the backup target device.
+#
+# @sync: what parts of the disk image should be copied to the destination
+#        (all the disk, only the sectors allocated in the topmost image, or
+#        only new I/O).
+#
+# @speed: #optional the maximum speed, in bytes per second.
+#
+# @on-source-error: #optional the action to take on an error on the source,
+#                   default 'report'.  'stop' and 'enospc' can only be used
+#                   if the block device supports io-status (see BlockInfo).
+#
+# @on-target-error: #optional the action to take on an error on the target,
+#                   default 'report' (no limitations, since this applies to
+#                   a different block device than @device).
+#
+# Note that @on-source-error and @on-target-error only affect background I/O.
+# If an error occurs during a guest write request, the device's rerror/werror
+# actions will be used.
+#
+# Since: 2.0
+##
+{ 'type': 'BlockdevBackup',
+  'data': { 'device': 'str', 'target': 'str',
+            'sync': 'MirrorSyncMode',
+            '*speed': 'int',
+            '*on-source-error': 'BlockdevOnError',
+            '*on-target-error': 'BlockdevOnError' } }
+
+##
 # @Abort
 #
 # This action can be used to test transaction failure.
@@ -2058,6 +2092,21 @@
 { 'command': 'drive-backup', 'data': 'DriveBackup' }
 
 ##
+# @blockdev-backup
+#
+# Block device version for drive-backup command. Use existing device as target
+# of backup.
+#
+# For the arguments, see the documentation of BlockdevBackup.
+#
+# Returns: Nothing on success.
+#          If @device or @target is not a valid block device, DeviceNotFound.
+#
+# Since 2.0
+##
+{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
+
+##
 # @drive-mirror
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fba15cd..32e2b10 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -963,6 +963,50 @@ Example:
                                                "sync": "full",
                                                "target": "backup.img" } }
 <- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "blockdev-backup",
+        .args_type  = "sync:s,device:B,target:B,speed:i?,"
+                      "on-source-error:s?,on-target-error:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
+    },
+
+SQMP
+blockdev-backup
+------------
+
+The device version of drive-backup: this command takes a existing named device
+as backup target.
+
+Arguments:
+
+- "device": the name of the device which should be copied.
+            (json-string)
+- "target": the target of the new image. If the file exists, or if it is a
+            device, the existing file/device will be used as the new
+            destination.  If it does not exist, a new file will be created.
+            (json-string)
+- "sync": what parts of the disk image should be copied to the destination;
+          possibilities include "full" for all the disk, "top" for only the
+          sectors allocated in the topmost image, or "none" to only replicate
+          new I/O (MirrorSyncMode).
+- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "on-source-error": the action to take on an error on the source, default
+                     'report'.  'stop' and 'enospc' can only be used
+                     if the block device supports io-status.
+                     (BlockdevOnError, optional)
+- "on-target-error": the action to take on an error on the target, default
+                     'report' (no limitations, since this applies to
+                     a different block device than device).
+                     (BlockdevOnError, optional)
+
+Example:
+-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
+                                                  "target": "tgt-id" } }
+<- { "return": {} }
+
 EQMP
 
     {
-- 
1.8.5.1

[Qemu-devel] [PATCH v8 12/12] block: Allow backup on referenced named BlockDriverState

[Fam Zheng]

Drive backup is a read only operation on source bs. We want to allow
this specific case to enable image-fleecing. Note that when
image-fleecing job starts, the job still add its blocker to source bs,
and any other operation on it will be blocked by that.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index e5e7b0b..2496e7d 100644
--- a/block.c
+++ b/block.c
@@ -980,6 +980,8 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     /* Otherwise we won't be able to commit due to check in bdrv_commit */
     bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
                     bs->backing_blocker);
+    bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_BACKUP_SOURCE,
+                    bs->backing_blocker);
     pstrcpy(bs->backing_file, sizeof(bs->backing_file),
             bs->backing_hd->file->filename);
     pstrcpy(bs->backing_format, sizeof(bs->backing_format),
-- 
1.8.5.1

Re: [Qemu-devel] [PATCH v8 06/12] block: Add bdrv_set_backing_hd()

[Stefan Hajnoczi]

On Fri, Dec 13, 2013 at 03:35:14PM +0800, Fam Zheng wrote:
> +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
> +{
> +    if (bs->backing_hd) {
> +        bdrv_unref(bs->backing_hd);
> +    }
> +
> +    bs->backing_hd = backing_hd;
> +    if (!backing_hd) {
> +        return;
> +    }

Maybe we should also do:
bs->backing_file[0] = '\0';
bs->backing_format[0] = '\0';

Re: [Qemu-devel] [PATCH v8 07/12] block: Add backing_blocker in BlockDriverState

[Stefan Hajnoczi]

On Fri, Dec 13, 2013 at 03:35:15PM +0800, Fam Zheng wrote:
> @@ -1476,6 +1486,9 @@ void bdrv_close(BlockDriverState *bs)
>  
>      if (bs->drv) {
>          if (bs->backing_hd) {
> +            assert(error_is_set(&bs->backing_blocker));
> +            bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> +            error_free(bs->backing_blocker);
>              bdrv_unref(bs->backing_hd);
>              bs->backing_hd = NULL;
>          }

This if statement duplicates bdrv_set_backing_hd() code.  I suggest:

if (bs->backing_hd) {
    bdrv_set_backing_hd(bs, NULL);
}

bdrv_set_backing_hd() needs to be modified to call
error_free(bs->backing_blocker) when backing_hd is NULL.

Re: [Qemu-devel] [PATCH v8 08/12] block: Parse "backing" option to reference existing BDS

[Stefan Hajnoczi]

On Fri, Dec 13, 2013 at 03:35:16PM +0800, Fam Zheng wrote:
> diff --git a/block.c b/block.c
> index b3993d7..fba7148 100644
> --- a/block.c
> +++ b/block.c
> @@ -1191,11 +1191,25 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      /* If there is a backing file, use it */
>      if ((flags & BDRV_O_NO_BACKING) == 0) {
>          QDict *backing_options;
> -
> -        qdict_extract_subqdict(options, &backing_options, "backing.");
> -        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> -        if (ret < 0) {
> -            goto close_and_fail;
> +        const char *backing_name;
> +        BlockDriverState *backing_hd;
> +
> +        backing_name = qdict_get_try_str(options, "backing");
> +        qdict_del(options, "backing");

This causes a use-after-free since backing_name is a const char pointer
to the qdict element!

> +        if (backing_name) {
> +            backing_hd = bdrv_find(backing_name);
> +            if (!backing_hd) {
> +                error_set(&local_err, QERR_DEVICE_NOT_FOUND, backing_name);
> +                ret = -ENOENT;
> +                goto close_and_fail;
> +            }
> +            bdrv_set_backing_hd(bs, backing_hd);
> +        } else {
> +            qdict_extract_subqdict(options, &backing_options, "backing.");
> +            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> +            if (ret < 0) {
> +                goto close_and_fail;
> +            }
>          }

Seems like users can specify backing=foo backing.file=/tmp/a and we
ignore backing.file.  Is it useful to silently ignore the backing.
subdict?  The user may have given useless options by mistake.  An error
would help prevent weird options combinations.

>      }
>  
> @@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>      assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
>      assert(bs_new->job == NULL);
>      assert(bs_new->dev == NULL);
> -    assert(bdrv_op_blocker_is_empty(bs_new));
>      assert(bs_new->io_limits_enabled == false);
>      assert(!throttle_have_timer(&bs_new->throttle_state));
>  
> @@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>      /* Check a few fields that should remain attached to the device */
>      assert(bs_new->dev == NULL);
>      assert(bs_new->job == NULL);
> -    assert(bdrv_op_blocker_is_empty(bs_new));
>      assert(bs_new->io_limits_enabled == false);
>      assert(!throttle_have_timer(&bs_new->throttle_state));

Why are these hunks part of this patch?  I guess it makes sense *not* to
check for blockers in bdrv_swap().  Instead the high-level functions in
blockdev.c and elsewhere should check blockers.

Re: [Qemu-devel] [PATCH v8 09/12] block: Support dropping active in bdrv_drop_intermediate

[Stefan Hajnoczi]

On Fri, Dec 13, 2013 at 03:35:17PM +0800, Fam Zheng wrote:
> + * 1) This will convert the following chain:
> + * ... <- base <- ... <- top <- overlay <-... <- active
> + *
> + * to
>   *
> - * Requires that the overlay to 'top' is opened r/w, so that the backing file
> - * information in 'bs' can be properly updated.
> + * ... <- base <- overlay <- active
>   *
> - * E.g., this will convert the following chain:
> - * bottom <- base <- intermediate <- top <- active
> + * 2) It is allowed for bottom==base, in which case it converts:
> + *
> + * ... <- base <- ... <- top <- overlay <- ... <- active

If bottom==base then I think it should be:

base <- ... <- top <- overlay <- ... <- active

(Dropping the "... <- " prefix)

Re: [Qemu-devel] [PATCH v8 02/12] qapi: Add BlockOperationType enum

[Stefan Hajnoczi]

On Fri, Dec 13, 2013 at 03:35:10PM +0800, Fam Zheng wrote:
> This adds the enum of all the operations that can be taken on a block
> device.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qapi-schema.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d6f8615..8e982a2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1440,6 +1440,56 @@
>    'data': ['commit', 'stream', 'mirror', 'backup'] }
>  
>  ##
> +# @BlockOperationType
> +#
> +# Type of a block operation. (since 2.0)

Why is this exposed in qapi-schema.json?  The blockers concept is
internal to QEMU and not exposed via QMP.

Re: [Qemu-devel] [PATCH v8 02/12] qapi: Add BlockOperationType enum

[Fam Zheng]

On 2014年01月03日 18:09, Stefan Hajnoczi wrote:
> On Fri, Dec 13, 2013 at 03:35:10PM +0800, Fam Zheng wrote:
>> This adds the enum of all the operations that can be taken on a block
>> device.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   qapi-schema.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index d6f8615..8e982a2 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1440,6 +1440,56 @@
>>     'data': ['commit', 'stream', 'mirror', 'backup'] }
>>
>>   ##
>> +# @BlockOperationType
>> +#
>> +# Type of a block operation. (since 2.0)
>
> Why is this exposed in qapi-schema.json?  The blockers concept is
> internal to QEMU and not exposed via QMP.
>

I plan to add it into block information (query-block, for example). But 
in follow up patches. It could be useful for user to check which 
commands/operations are possible without trial-and-fail, when we put 
more and more complicated state and configuration into the BDS graph.

Fam

Re: [Qemu-devel] [PATCH v8 02/12] qapi: Add BlockOperationType enum

[Stefan Hajnoczi]

On Wed, Jan 08, 2014 at 10:28:59AM +0800, Fam Zheng wrote:
> On 2014年01月03日 18:09, Stefan Hajnoczi wrote:
> >On Fri, Dec 13, 2013 at 03:35:10PM +0800, Fam Zheng wrote:
> >>This adds the enum of all the operations that can be taken on a block
> >>device.
> >>
> >>Signed-off-by: Fam Zheng <famz@redhat.com>
> >>---
> >>  qapi-schema.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 50 insertions(+)
> >>
> >>diff --git a/qapi-schema.json b/qapi-schema.json
> >>index d6f8615..8e982a2 100644
> >>--- a/qapi-schema.json
> >>+++ b/qapi-schema.json
> >>@@ -1440,6 +1440,56 @@
> >>    'data': ['commit', 'stream', 'mirror', 'backup'] }
> >>
> >>  ##
> >>+# @BlockOperationType
> >>+#
> >>+# Type of a block operation. (since 2.0)
> >
> >Why is this exposed in qapi-schema.json?  The blockers concept is
> >internal to QEMU and not exposed via QMP.
> >
> 
> I plan to add it into block information (query-block, for example).
> But in follow up patches. It could be useful for user to check which
> commands/operations are possible without trial-and-fail, when we put
> more and more complicated state and configuration into the BDS
> graph.

Can you make it internal for now and expose via QMP if necessary later?

I think we shouldn't commit to public APIs unless they are ready and
provide a useful service, since we cannot change them.

Stefan

Re: [Qemu-devel] [PATCH v8 02/12] qapi: Add BlockOperationType enum

[Fam Zheng]

On 2014年01月08日 11:26, Stefan Hajnoczi wrote:
> On Wed, Jan 08, 2014 at 10:28:59AM +0800, Fam Zheng wrote:
>> On 2014年01月03日 18:09, Stefan Hajnoczi wrote:
>>> On Fri, Dec 13, 2013 at 03:35:10PM +0800, Fam Zheng wrote:
>>>> This adds the enum of all the operations that can be taken on a block
>>>> device.
>>>>
>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>>   qapi-schema.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 50 insertions(+)
>>>>
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index d6f8615..8e982a2 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -1440,6 +1440,56 @@
>>>>     'data': ['commit', 'stream', 'mirror', 'backup'] }
>>>>
>>>>   ##
>>>> +# @BlockOperationType
>>>> +#
>>>> +# Type of a block operation. (since 2.0)
>>>
>>> Why is this exposed in qapi-schema.json?  The blockers concept is
>>> internal to QEMU and not exposed via QMP.
>>>
>>
>> I plan to add it into block information (query-block, for example).
>> But in follow up patches. It could be useful for user to check which
>> commands/operations are possible without trial-and-fail, when we put
>> more and more complicated state and configuration into the BDS
>> graph.
>
> Can you make it internal for now and expose via QMP if necessary later?
>
> I think we shouldn't commit to public APIs unless they are ready and
> provide a useful service, since we cannot change them.
>

OK, works for me.

Fam

Re: [Qemu-devel] [PATCH v8 08/12] block: Parse "backing" option to reference existing BDS

[Fam Zheng]

On 2014年01月03日 17:19, Stefan Hajnoczi wrote:
>>       }
>>
>> @@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>>       assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
>>       assert(bs_new->job == NULL);
>>       assert(bs_new->dev == NULL);
>> -    assert(bdrv_op_blocker_is_empty(bs_new));
>>       assert(bs_new->io_limits_enabled == false);
>>       assert(!throttle_have_timer(&bs_new->throttle_state));
>>
>> @@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>>       /* Check a few fields that should remain attached to the device */
>>       assert(bs_new->dev == NULL);
>>       assert(bs_new->job == NULL);
>> -    assert(bdrv_op_blocker_is_empty(bs_new));
>>       assert(bs_new->io_limits_enabled == false);
>>       assert(!throttle_have_timer(&bs_new->throttle_state));
>
> Why are these hunks part of this patch?  I guess it makes sense *not* to
> check for blockers in bdrv_swap().  Instead the high-level functions in
> blockdev.c and elsewhere should check blockers.
>

The two checks are here because of the mechanical replace of in_use. 
They are removed because it is no longer true for some valid cases, e.g 
with "block-commit". So we need these hunks here, or do this as a 
preceding in the series.

Will update the commit message and keep it as is.

Fam