* [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD
@ 2014-05-20 6:04 Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 01/15] block: Add BlockOpType enum Fam Zheng
` (15 more replies)
0 siblings, 16 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
Thanks for Eric's and Markus' reviewing!
v20: Rebase to master, with minor improvements as listed below:
[02/15] block: Introduce op_blockers to BlockDriverState
Better error message in bdrv_op_is_blocked. (Markus)
[03/15] block: Replace in_use with operation blocker
Accurate commit message: why use bdrv_op_blocker_is_empty in
block_job_create. (Markus)
[06/15] block: Add backing_blocker in BlockDriverState
Drop error_is_set().
Call error_free() unconditionally.
[08/15] block: Support dropping active in bdrv_drop_intermediate
Fix list index in function comment. (Eric)
[13/15] block: Add blockdev-backup to transaction
Drop error_is_set().
[14/15] qemu-iotests: Test blockdev-backup in 055
Bump year in file header. (Eric)
v19: Rebase to master, resolve several contextual conflicts.
Patch 05 has the only code change difference from v18 to preserve.
assert(bs->backing_hd == NULL);
v18: Address reviewing comments from Jeff and Eric. Rebased to current master.
Side by side diff from v17: http://bit.ly/1oO2Fvt
[01/15] block: Add BlockOpType enum
Add Jeff's reviewed-by.
[02/15] block: Introduce op_blockers to BlockDriverState
Add Jeff's reviewed-by.
[03/15] block: Replace in_use with operation blocker
Add Jeff's reviewed-by.
[04/15] block: Move op_blocker check from block_job_create to its caller
Add Jeff's reviewed-by.
[05/15] block: Add bdrv_set_backing_hd()
Don't unset bs->backing_file and bs->backing_format when
backing_hd==NULL, because qcow2_close() will save these into image
header.
[08/15] block: Support dropping active in bdrv_drop_intermediate
Swap parameters for bdrv_swap:
bdrv_swap(active, base); -> bdrv_swap(base, active);
Use bdrv_set_backing_hd().
[10/15] commit: Use bdrv_drop_intermediate
New. (Jeff)
[11/15] qmp: Add command 'blockdev-backup'
Since 2.0 -> Since 2.1. (Eric)
[13/15] block: Add blockdev-backup to transaction
Comment "Since 2.1" for blockdev-backup. (Eric)
[15/15] qemu-iotests: Image fleecing test case 089
Case number 083 -> 089.
Fam Zheng (15):
block: Add BlockOpType 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
commit: Use bdrv_drop_intermediate
qmp: Add command 'blockdev-backup'
block: Allow backup on referenced named BlockDriverState
block: Add blockdev-backup to transaction
qemu-iotests: Test blockdev-backup in 055
qemu-iotests: Image fleecing test case 089
block-migration.c | 7 +-
block.c | 310 +++++++++++++++++++++++++++-------------
block/backup.c | 26 ++++
block/commit.c | 2 +-
block/mirror.c | 9 +-
block/stream.c | 42 +-----
blockdev.c | 122 ++++++++++++++--
blockjob.c | 14 +-
hw/block/dataplane/virtio-blk.c | 18 ++-
include/block/block.h | 29 +++-
include/block/block_int.h | 9 +-
include/block/blockjob.h | 3 +
qapi-schema.json | 52 +++++++
qmp-commands.hx | 44 ++++++
tests/qemu-iotests/055 | 277 +++++++++++++++++++++++++++++------
tests/qemu-iotests/055.out | 4 +-
tests/qemu-iotests/089 | 99 +++++++++++++
tests/qemu-iotests/089.out | 5 +
tests/qemu-iotests/group | 1 +
19 files changed, 856 insertions(+), 217 deletions(-)
create mode 100755 tests/qemu-iotests/089
create mode 100644 tests/qemu-iotests/089.out
--
1.9.2
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 01/15] block: Add BlockOpType enum
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-21 12:28 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 02/15] block: Introduce op_blockers to BlockDriverState Fam Zheng
` (14 subsequent siblings)
15 siblings, 1 reply; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
This adds the enum of all the operations that can be taken on a block
device.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
include/block/block.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/include/block/block.h b/include/block/block.h
index 1b119aa..5d4e663 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -159,6 +159,25 @@ typedef struct BDRVReopenState {
void *opaque;
} BDRVReopenState;
+/*
+ * Block operation types
+ */
+typedef enum BlockOpType {
+ BLOCK_OP_TYPE_BACKUP_SOURCE,
+ BLOCK_OP_TYPE_BACKUP_TARGET,
+ BLOCK_OP_TYPE_CHANGE,
+ BLOCK_OP_TYPE_COMMIT,
+ BLOCK_OP_TYPE_DATAPLANE,
+ BLOCK_OP_TYPE_DRIVE_DEL,
+ BLOCK_OP_TYPE_EJECT,
+ BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
+ BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
+ BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
+ BLOCK_OP_TYPE_MIRROR,
+ BLOCK_OP_TYPE_RESIZE,
+ BLOCK_OP_TYPE_STREAM,
+ BLOCK_OP_TYPE_MAX,
+} BlockOpType;
void bdrv_iostatus_enable(BlockDriverState *bs);
void bdrv_iostatus_reset(BlockDriverState *bs);
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 02/15] block: Introduce op_blockers to BlockDriverState
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 01/15] block: Add BlockOpType enum Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-21 12:28 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 03/15] block: Replace in_use with operation blocker Fam Zheng
` (13 subsequent siblings)
15 siblings, 1 reply; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
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>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 7 +++++
include/block/block_int.h | 5 ++++
3 files changed, 88 insertions(+)
diff --git a/block.c b/block.c
index c90c71a..bb60b07 100644
--- a/block.c
+++ b/block.c
@@ -335,6 +335,7 @@ void bdrv_register(BlockDriver *bdrv)
BlockDriverState *bdrv_new(const char *device_name, Error **errp)
{
BlockDriverState *bs;
+ int i;
if (bdrv_find(device_name)) {
error_setg(errp, "Device with id '%s' already exists",
@@ -353,6 +354,9 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp)
if (device_name[0] != '\0') {
QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
}
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ QLIST_INIT(&bs->op_blockers[i]);
+ }
bdrv_iostatus_disable(bs);
notifier_list_init(&bs->close_notifiers);
notifier_with_return_list_init(&bs->before_write_notifiers);
@@ -1911,6 +1915,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
bs_src->device_name);
bs_dest->device_list = bs_src->device_list;
+ memcpy(bs_dest->op_blockers, bs_src->op_blockers,
+ sizeof(bs_dest->op_blockers));
}
/*
@@ -5273,6 +5279,76 @@ 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) {
+ error_setg(errp, "Device '%s' is busy: %s",
+ bs->device_name, error_get_pretty(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 5d4e663..d00025b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -472,6 +472,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 9ffcb69..78662d5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -270,6 +270,8 @@ typedef struct BlockLimits {
size_t opt_mem_alignment;
} 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
@@ -360,6 +362,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.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 03/15] block: Replace in_use with operation blocker
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 01/15] block: Add BlockOpType enum Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 02/15] block: Introduce op_blockers to BlockDriverState Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-21 12:46 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller Fam Zheng
` (12 subsequent siblings)
15 siblings, 1 reply; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
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, ...).
There is one exception in block_job_create, where
bdrv_op_blocker_is_empty() is used, because we don't know the operation
type here. This doesn't matter because in a few commits away we will drop
the check and move it to callers that _do_ know the type.
- 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>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
block-migration.c | 7 +++++--
block.c | 24 +++++++-----------------
blockdev.c | 19 +++++++++----------
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, 45 insertions(+), 43 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index 56951e0..1656270 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 {
@@ -361,7 +362,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;
@@ -599,7 +601,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 bb60b07..ef9c1a7 100644
--- a/block.c
+++ b/block.c
@@ -1908,7 +1908,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 */
@@ -1951,7 +1950,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));
@@ -1970,7 +1969,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));
@@ -2015,7 +2014,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));
@@ -2197,7 +2196,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;
}
@@ -3450,8 +3450,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);
@@ -5349,17 +5350,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 7810e9f..5d950fa 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1295,8 +1295,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;
}
@@ -1518,8 +1518,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)) {
@@ -1721,14 +1720,16 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
const char *id = qdict_get_str(qdict, "id");
BlockDriverState *bs;
+ Error *local_err = NULL;
bs = bdrv_find(id);
if (!bs) {
qerror_report(QERR_DEVICE_NOT_FOUND, id);
return -1;
}
- if (bdrv_in_use(bs)) {
- qerror_report(QERR_DEVICE_IN_USE, id);
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
+ error_report("%s", error_get_pretty(local_err));
+ error_free(local_err);
return -1;
}
@@ -1984,8 +1985,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;
}
@@ -2118,8 +2118,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 cd4784f..60e72f5 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 (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 70b8a5a..e49c253 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -70,6 +70,9 @@ struct VirtIOBlockDataPlane {
queue */
unsigned int num_reqs;
+
+ /* Operation blocker on BDS */
+ Error *blocker;
};
/* Raise an interrupt to signal guest, if necessary */
@@ -350,6 +353,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
{
VirtIOBlockDataPlane *s;
int fd;
+ Error *local_err = NULL;
*dataplane = NULL;
@@ -372,9 +376,10 @@ void 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_setg(errp,
- "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;
}
@@ -406,8 +411,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
}
s->ctx = iothread_get_aio_context(s->iothread);
- /* 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;
}
@@ -420,7 +425,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);
object_unref(OBJECT(s->iothread));
g_free(s);
}
diff --git a/include/block/block.h b/include/block/block.h
index d00025b..60e79a8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -469,8 +469,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 78662d5..3518076 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -358,7 +358,6 @@ struct BlockDriverState {
QTAILQ_ENTRY(BlockDriverState) device_list;
QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
int refcnt;
- int in_use; /* users other than guest access, eg. block migration */
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.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (2 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 03/15] block: Replace in_use with operation blocker Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 11:43 ` Jeff Cody
2014-05-21 13:20 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd() Fam Zheng
` (11 subsequent siblings)
15 siblings, 2 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
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>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
blockdev.c | 8 ++++++++
blockjob.c | 2 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 5d950fa..21fc55b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1850,6 +1850,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) {
@@ -1894,6 +1898,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 60e72f5..7d84ca1 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.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd()
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (3 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-21 13:56 ` Stefan Hajnoczi
2014-05-21 14:17 ` Jeff Cody
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState Fam Zheng
` (10 subsequent siblings)
15 siblings, 2 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
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 | 36 +++++++++++++++++++++++-------------
include/block/block.h | 1 +
2 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/block.c b/block.c
index ef9c1a7..a47118b 100644
--- a/block.c
+++ b/block.c
@@ -1094,6 +1094,21 @@ fail:
return ret;
}
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
+{
+
+ bs->backing_hd = backing_hd;
+ if (!backing_hd) {
+ goto out;
+ }
+ bs->open_flags &= ~BDRV_O_NO_BACKING;
+ pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
+ pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+ backing_hd->drv ? backing_hd->drv->format_name : "");
+out:
+ bdrv_refresh_limits(bs);
+}
+
/*
* Opens the backing file for a BlockDriverState if not yet open
*
@@ -1107,6 +1122,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
char *backing_filename = g_malloc0(PATH_MAX);
int ret = 0;
BlockDriver *back_drv = NULL;
+ BlockDriverState *backing_hd;
Error *local_err = NULL;
if (bs->backing_hd != NULL) {
@@ -1129,27 +1145,26 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
}
+ backing_hd = bdrv_new("", errp);
+
if (bs->backing_format[0] != '\0') {
back_drv = bdrv_find_format(bs->backing_format);
}
assert(bs->backing_hd == NULL);
- ret = bdrv_open(&bs->backing_hd,
+ ret = bdrv_open(&backing_hd,
*backing_filename ? backing_filename : NULL, NULL, options,
bdrv_backing_flags(bs->open_flags), back_drv, &local_err);
if (ret < 0) {
- bs->backing_hd = NULL;
+ bdrv_unref(backing_hd);
+ backing_hd = NULL;
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);
goto free_exit;
}
-
- if (bs->backing_hd->file) {
- pstrcpy(bs->backing_file, sizeof(bs->backing_file),
- bs->backing_hd->file->filename);
- }
+ bdrv_set_backing_hd(bs, backing_hd);
/* Recalculate the BlockLimits with the backing file */
bdrv_refresh_limits(bs);
@@ -2002,12 +2017,7 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
/* The contents of 'tmp' will become bs_top, as we are
* swapping bs_new and bs_top contents. */
- bs_top->backing_hd = bs_new;
- bs_top->open_flags &= ~BDRV_O_NO_BACKING;
- pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file),
- bs_new->filename);
- pstrcpy(bs_top->backing_format, sizeof(bs_top->backing_format),
- bs_new->drv ? bs_new->drv->format_name : "");
+ bdrv_set_backing_hd(bs_top, bs_new);
}
static void bdrv_delete(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 60e79a8..032828a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -213,6 +213,7 @@ int bdrv_parse_discard_flags(const char *mode, int *flags);
int bdrv_open_image(BlockDriverState **pbs, const char *filename,
QDict *options, const char *bdref_key, int flags,
bool allow_none, Error **errp);
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
int bdrv_open(BlockDriverState **pbs, const char *filename,
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (4 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd() Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-21 14:03 ` Stefan Hajnoczi
2014-05-21 14:06 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 07/15] block: Parse "backing" option to reference existing BDS Fam Zheng
` (9 subsequent siblings)
15 siblings, 2 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
This makes use of op_blocker and blocks all the operations except for
commit target, on each BlockDriverState->backing_hd.
The asserts for op_blocker in bdrv_swap are removed because with this
change, the target of block commit has at least the backing blocker of
its child, so the assertion is not true. Callers should do their check.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block.c | 22 ++++++++++++++++++----
block/mirror.c | 1 +
include/block/block_int.h | 3 +++
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index a47118b..efec10a 100644
--- a/block.c
+++ b/block.c
@@ -1097,14 +1097,29 @@ fail:
void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
{
+ if (bs->backing_hd) {
+ assert(bs->backing_blocker);
+ bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
+ } else if (backing_hd) {
+ error_setg(&bs->backing_blocker,
+ "device is used as backing hd of '%s'",
+ bs->device_name);
+ }
+
bs->backing_hd = backing_hd;
if (!backing_hd) {
+ error_free(bs->backing_blocker);
goto out;
}
bs->open_flags &= ~BDRV_O_NO_BACKING;
pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
pstrcpy(bs->backing_format, sizeof(bs->backing_format),
backing_hd->drv ? backing_hd->drv->format_name : "");
+
+ 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);
out:
bdrv_refresh_limits(bs);
}
@@ -1762,8 +1777,9 @@ void bdrv_close(BlockDriverState *bs)
if (bs->drv) {
if (bs->backing_hd) {
- bdrv_unref(bs->backing_hd);
- bs->backing_hd = NULL;
+ BlockDriverState *backing_hd = bs->backing_hd;
+ bdrv_set_backing_hd(bs, NULL);
+ bdrv_unref(backing_hd);
}
bs->drv->bdrv_close(bs);
g_free(bs->opaque);
@@ -1965,7 +1981,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));
@@ -1984,7 +1999,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));
diff --git a/block/mirror.c b/block/mirror.c
index 1c38aa8..6a53d79 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -499,6 +499,7 @@ immediate_exit:
* trigger the unref from the top one */
BlockDriverState *p = s->base->backing_hd;
s->base->backing_hd = NULL;
+ bdrv_op_unblock_all(p, s->base->backing_blocker);
bdrv_unref(p);
}
}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3518076..49e5824 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -368,6 +368,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.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 07/15] block: Parse "backing" option to reference existing BDS
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (5 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 08/15] block: Support dropping active in bdrv_drop_intermediate Fam Zheng
` (8 subsequent siblings)
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
Now it's safe to allow reference for backing_hd in the interface.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index efec10a..4fc2dd7 100644
--- a/block.c
+++ b/block.c
@@ -1445,12 +1445,35 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
/* If there is a backing file, use it */
if ((flags & BDRV_O_NO_BACKING) == 0) {
QDict *backing_options;
+ const char *backing_name;
+ BlockDriverState *backing_hd;
+ backing_name = qdict_get_try_str(options, "backing");
qdict_extract_subqdict(options, &backing_options, "backing.");
- ret = bdrv_open_backing_file(bs, backing_options, &local_err);
- if (ret < 0) {
+
+ if (backing_name && qdict_size(backing_options)) {
+ error_setg(&local_err,
+ "Option \"backing\" and \"backing.*\" cannot be "
+ "used together");
+ ret = -EINVAL;
goto close_and_fail;
}
+ 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;
+ }
+ qdict_del(options, "backing");
+ bdrv_set_backing_hd(bs, backing_hd);
+ bdrv_ref(backing_hd);
+ } else {
+ ret = bdrv_open_backing_file(bs, backing_options, &local_err);
+ if (ret < 0) {
+ goto close_and_fail;
+ }
+ }
}
/* For snapshot=on, create a temporary qcow2 overlay. bs points to the
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 08/15] block: Support dropping active in bdrv_drop_intermediate
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (6 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 07/15] block: Parse "backing" option to reference existing BDS Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 09/15] stream: Use bdrv_drop_intermediate and drop close_unused_images Fam Zheng
` (7 subsequent siblings)
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
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 | 139 ++++++++++++++++++++++++++++-----------------------------
block/commit.c | 2 +-
2 files changed, 70 insertions(+), 71 deletions(-)
diff --git a/block.c b/block.c
index 4fc2dd7..e0d7984 100644
--- a/block.c
+++ b/block.c
@@ -2558,115 +2558,114 @@ 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's overlay (the image orignally has 'top' as backing file).
+ * top's overlay may be NULL if 'top' is active, no such update needed.
+ * Requires that the top's overlay to 'top' is opened r/w.
+ *
+ * 1) This will convert the following chain:
+ *
+ * ... <- base <- ... <- top <- overlay <-... <- active
*
- * Requires that the overlay to 'top' is opened r/w, so that the backing file
- * information in 'bs' can be properly updated.
+ * to
+ *
+ * ... <- base <- overlay <- active
+ *
+ * 2) It is allowed for bottom==base, in which case it converts:
*
- * E.g., this will convert the following chain:
- * bottom <- base <- intermediate <- top <- active
+ * base <- ... <- top <- overlay <- ... <- active
*
* to
*
- * bottom <- base <- active
+ * base <- overlay <- active
*
- * It is allowed for bottom==base, in which case it converts:
+ * 3) 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.
+ *
+ * 4) If base==NULL, it will drop all the BDS below overlay and set its
+ * backing_hd to NULL. I.e.:
*
- * Error conditions:
- * if active == top, that is considered an error
+ * base(NULL) <- ... <- overlay <- ... <- active
+ *
+ * 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, *bs;
+ int ret = -EINVAL;
- if (!top->drv || !base->drv) {
+ assert(active);
+ assert(top);
+ /* Verify that top is in backing chain of active */
+ bs = active;
+ while (bs && bs != top) {
+ bs = bs->backing_hd;
+ }
+ if (!bs) {
goto exit;
}
+ /* Verify that base is in backing chain of top */
+ if (base) {
+ while (bs && bs != base) {
+ bs = bs->backing_hd;
+ }
+ if (bs != base) {
+ 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 */
+ if (!top->drv || (base && !base->drv)) {
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;
+ } else if (top == active) {
+ assert(base);
+ drop_start = active->backing_hd;
+ bdrv_swap(base, active);
+ bdrv_set_backing_hd(base, NULL);
+ bdrv_unref(drop_start);
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;
- }
- 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 */
+ overlay = bdrv_find_overlay(active, top);
+ if (!overlay) {
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 : "");
+ ret = bdrv_change_backing_file(overlay,
+ base ? base->filename : NULL,
+ base ? base->drv->format_name : NULL);
if (ret) {
goto exit;
}
- new_top_bs->backing_hd = base_bs;
- bdrv_refresh_limits(new_top_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);
+ bs = overlay->backing_hd;
+ bdrv_set_backing_hd(overlay, base);
+ if (base) {
+ bdrv_ref(base);
+ }
+ if (bs) {
+ bdrv_unref(bs);
}
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 5c09f44..147da6a 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -73,6 +73,7 @@ static void coroutine_fn commit_run(void *opaque)
int bytes_written = 0;
int64_t base_len;
+ overlay_bs = bdrv_find_overlay(active, top);
ret = s->common.len = bdrv_getlength(top);
@@ -154,7 +155,6 @@ exit_restore_reopen:
if (s->base_flags != bdrv_get_flags(base)) {
bdrv_reopen(base, s->base_flags, NULL);
}
- overlay_bs = bdrv_find_overlay(active, top);
if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
}
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 09/15] stream: Use bdrv_drop_intermediate and drop close_unused_images
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (7 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 08/15] block: Support dropping active in bdrv_drop_intermediate Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 10/15] commit: Use bdrv_drop_intermediate Fam Zheng
` (6 subsequent siblings)
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
This reuses the new bdrv_drop_intermediate.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/stream.c | 42 +-----------------------------------------
1 file changed, 1 insertion(+), 41 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index dd0b4ac..1b348a2 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -32,7 +32,6 @@ typedef struct StreamBlockJob {
RateLimit limit;
BlockDriverState *base;
BlockdevOnError on_error;
- char backing_file_id[1024];
} StreamBlockJob;
static int coroutine_fn stream_populate(BlockDriverState *bs,
@@ -51,34 +50,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);
- }
-
- bdrv_refresh_limits(top);
-}
-
static void coroutine_fn stream_run(void *opaque)
{
StreamBlockJob *s = opaque;
@@ -184,15 +155,7 @@ wait:
ret = error;
if (!block_job_is_cancelled(&s->common) && sector_num == end && ret == 0) {
- const char *base_id = NULL, *base_fmt = NULL;
- if (base) {
- base_id = s->backing_file_id;
- if (base->drv) {
- base_fmt = base->drv->format_name;
- }
- }
- ret = bdrv_change_backing_file(bs, base_id, base_fmt);
- close_unused_images(bs, base, base_id);
+ ret = bdrv_drop_intermediate(bs, bs->backing_hd, base);
}
qemu_vfree(buf);
@@ -237,9 +200,6 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
}
s->base = base;
- if (base_id) {
- pstrcpy(s->backing_file_id, sizeof(s->backing_file_id), base_id);
- }
s->on_error = on_error;
s->common.co = qemu_coroutine_create(stream_run);
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 10/15] commit: Use bdrv_drop_intermediate
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (8 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 09/15] stream: Use bdrv_drop_intermediate and drop close_unused_images Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 11/15] qmp: Add command 'blockdev-backup' Fam Zheng
` (5 subsequent siblings)
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 6a53d79..b4b12d0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -493,14 +493,10 @@ immediate_exit:
if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) {
bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
}
- bdrv_swap(s->target, s->common.bs);
if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
- /* drop the bs loop chain formed by the swap: break the loop then
- * trigger the unref from the top one */
- BlockDriverState *p = s->base->backing_hd;
- s->base->backing_hd = NULL;
- bdrv_op_unblock_all(p, s->base->backing_blocker);
- bdrv_unref(p);
+ ret = bdrv_drop_intermediate(s->common.bs, s->common.bs, s->base);
+ } else {
+ bdrv_swap(s->target, s->common.bs);
}
}
bdrv_unref(s->target);
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 11/15] qmp: Add command 'blockdev-backup'
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (9 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 10/15] commit: Use bdrv_drop_intermediate Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 12/15] block: Allow backup on referenced named BlockDriverState Fam Zheng
` (4 subsequent siblings)
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
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.
Add check and report error for bs == target which became possible but is
an illegal case with introduction of blockdev-backup.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/backup.c | 26 ++++++++++++++++++++++++++
blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
qmp-commands.hx | 44 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 166 insertions(+)
diff --git a/block/backup.c b/block/backup.c
index 15a2e55..ea46340 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -344,6 +344,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);
@@ -362,6 +363,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
assert(target);
assert(cb);
+ if (bs == target) {
+ error_setg(errp, "Source and target cannot be the same");
+ return;
+ }
+
if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
!bdrv_iostatus_is_enabled(bs)) {
@@ -369,6 +375,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'",
@@ -382,6 +406,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 21fc55b..287a2d8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1977,6 +1977,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;
@@ -1993,6 +1995,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;
}
@@ -2055,6 +2058,50 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
return bdrv_named_nodes_list();
}
+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 36cb964..c6b10b9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1953,6 +1953,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.1
+##
+{ '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.
@@ -2161,6 +2195,21 @@
{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
##
+# @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.1
+##
+{ '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 cae890e..15b8ee4 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1077,6 +1077,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.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 12/15] block: Allow backup on referenced named BlockDriverState
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (10 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 11/15] qmp: Add command 'blockdev-backup' Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 13/15] block: Add blockdev-backup to transaction Fam Zheng
` (3 subsequent siblings)
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
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 e0d7984..2aebf45 100644
--- a/block.c
+++ b/block.c
@@ -1120,6 +1120,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);
out:
bdrv_refresh_limits(bs);
}
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 13/15] block: Add blockdev-backup to transaction
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (11 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 12/15] block: Allow backup on referenced named BlockDriverState Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 14/15] qemu-iotests: Test blockdev-backup in 055 Fam Zheng
` (2 subsequent siblings)
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
Signed-off-by: Fam Zheng <famz@redhat.com>
---
blockdev.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 3 +++
2 files changed, 51 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index 287a2d8..14c5be6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1411,6 +1411,49 @@ static void drive_backup_abort(BlkTransactionState *common)
}
}
+typedef struct BlockdevBackupState {
+ BlkTransactionState common;
+ BlockDriverState *bs;
+ BlockJob *job;
+} BlockdevBackupState;
+
+static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
+{
+ BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+ BlockdevBackup *backup;
+ Error *local_err = NULL;
+
+ assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
+ backup = common->action->blockdev_backup;
+
+ qmp_blockdev_backup(backup->device, backup->target,
+ backup->sync,
+ backup->has_speed, backup->speed,
+ backup->has_on_source_error, backup->on_source_error,
+ backup->has_on_target_error, backup->on_target_error,
+ &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ state->bs = NULL;
+ state->job = NULL;
+ return;
+ }
+
+ state->bs = bdrv_find(backup->device);
+ state->job = state->bs->job;
+}
+
+static void blockdev_backup_abort(BlkTransactionState *common)
+{
+ BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+ BlockDriverState *bs = state->bs;
+
+ /* Only cancel if it's the job we started */
+ if (bs && bs->job && bs->job == state->job) {
+ block_job_cancel_sync(bs->job);
+ }
+}
+
static void abort_prepare(BlkTransactionState *common, Error **errp)
{
error_setg(errp, "Transaction aborted using Abort action");
@@ -1433,6 +1476,11 @@ static const BdrvActionOps actions[] = {
.prepare = drive_backup_prepare,
.abort = drive_backup_abort,
},
+ [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
+ .instance_size = sizeof(BlockdevBackupState),
+ .prepare = blockdev_backup_prepare,
+ .abort = blockdev_backup_abort,
+ },
[TRANSACTION_ACTION_KIND_ABORT] = {
.instance_size = sizeof(BlkTransactionState),
.prepare = abort_prepare,
diff --git a/qapi-schema.json b/qapi-schema.json
index c6b10b9..510a8f8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2001,11 +2001,14 @@
#
# A discriminated record of operations that can be performed with
# @transaction.
+#
+# Since 1.1, blockdev-backup since 2.1
##
{ 'union': 'TransactionAction',
'data': {
'blockdev-snapshot-sync': 'BlockdevSnapshot',
'drive-backup': 'DriveBackup',
+ 'blockdev-backup': 'BlockdevBackup',
'abort': 'Abort',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
} }
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 14/15] qemu-iotests: Test blockdev-backup in 055
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (12 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 13/15] block: Add blockdev-backup to transaction Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 15/15] qemu-iotests: Image fleecing test case 089 Fam Zheng
2018-06-29 18:00 ` [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Eric Blake
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
This applies cases on drive-backup on blockdev-backup, except cases with
target format and mode.
Also add a case to check source == target.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
tests/qemu-iotests/055 | 277 ++++++++++++++++++++++++++++++++++++++-------
tests/qemu-iotests/055.out | 4 +-
2 files changed, 236 insertions(+), 45 deletions(-)
diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 451b67d..ab47d59 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -1,8 +1,8 @@
#!/usr/bin/env python
#
-# Tests for drive-backup
+# Tests for drive-backup and blockdev-backup
#
-# Copyright (C) 2013 Red Hat, Inc.
+# Copyright (C) 2013, 2014 Red Hat, Inc.
#
# Based on 041.
#
@@ -27,6 +27,7 @@ from iotests import qemu_img, qemu_io
test_img = os.path.join(iotests.test_dir, 'test.img')
target_img = os.path.join(iotests.test_dir, 'target.img')
+blockdev_target_img = os.path.join(iotests.test_dir, 'blockdev-target.img')
class TestSingleDrive(iotests.QMPTestCase):
image_len = 64 * 1024 * 1024 # MB
@@ -38,34 +39,48 @@ class TestSingleDrive(iotests.QMPTestCase):
qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
+ qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(TestSingleDrive.image_len))
- self.vm = iotests.VM().add_drive(test_img)
+ self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
self.vm.launch()
def tearDown(self):
self.vm.shutdown()
os.remove(test_img)
+ os.remove(blockdev_target_img)
try:
os.remove(target_img)
except OSError:
pass
- def test_cancel(self):
+ def do_test_cancel(self, test_drive_backup):
self.assert_no_active_block_jobs()
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+ if test_drive_backup:
+ result = self.vm.qmp('drive-backup', device='drive0',
+ target=target_img, sync='full')
+ else:
+ result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive1', sync='full')
self.assert_qmp(result, 'return', {})
event = self.cancel_and_wait()
self.assert_qmp(event, 'data/type', 'backup')
- def test_pause(self):
+ def test_cancel(self):
+ self.do_test_cancel(True)
+ self.do_test_cancel(False)
+
+ def do_test_pause(self, test_drive_backup):
self.assert_no_active_block_jobs()
self.vm.pause_drive('drive0')
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+ if test_drive_backup:
+ result = self.vm.qmp('drive-backup', device='drive0',
+ target=target_img, sync='full')
+ else:
+ result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive1', sync='full')
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-job-pause', device='drive0')
@@ -86,14 +101,28 @@ class TestSingleDrive(iotests.QMPTestCase):
self.wait_until_completed()
self.vm.shutdown()
- self.assertTrue(iotests.compare_images(test_img, target_img),
- 'target image does not match source after backup')
+ if test_drive_backup:
+ self.assertTrue(iotests.compare_images(test_img, target_img),
+ 'target image does not match source after backup')
+ else:
+ self.assertTrue(iotests.compare_images(test_img, blockdev_target_img),
+ 'target image does not match source after backup')
+
+ def test_pause_drive_backup(self):
+ self.do_test_pause(True)
+
+ def test_pause_blockdev_backup(self):
+ self.do_test_pause(False)
def test_medium_not_found(self):
result = self.vm.qmp('drive-backup', device='ide1-cd0',
target=target_img, sync='full')
self.assert_qmp(result, 'error/class', 'GenericError')
+ result = self.vm.qmp('blockdev-backup', device='ide1-cd0',
+ target='drive1', sync='full')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
def test_image_not_found(self):
result = self.vm.qmp('drive-backup', device='drive0',
target=target_img, sync='full', mode='existing')
@@ -110,26 +139,56 @@ class TestSingleDrive(iotests.QMPTestCase):
target=target_img, sync='full')
self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+ result = self.vm.qmp('blockdev-backup', device='nonexistent',
+ target='drive0', sync='full')
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='nonexistent', sync='full')
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ result = self.vm.qmp('blockdev-backup', device='nonexistent',
+ target='nonexistent', sync='full')
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ def test_target_is_source(self):
+ result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive0', sync='full')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
class TestSetSpeed(iotests.QMPTestCase):
image_len = 80 * 1024 * 1024 # MB
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSetSpeed.image_len))
qemu_io('-c', 'write -P1 0 512', test_img)
- self.vm = iotests.VM().add_drive(test_img)
+ qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(TestSingleDrive.image_len))
+
+ self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
self.vm.launch()
def tearDown(self):
self.vm.shutdown()
os.remove(test_img)
- os.remove(target_img)
+ try:
+ os.remove(blockdev_target_img)
+ except OSError:
+ pass
+ try:
+ os.remove(target_img)
+ except OSError:
+ pass
- def test_set_speed(self):
+ def do_test_set_speed(self, test_drive_backup):
self.assert_no_active_block_jobs()
self.vm.pause_drive('drive0')
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+ if test_drive_backup:
+ result = self.vm.qmp('drive-backup', device='drive0',
+ target=target_img, sync='full')
+ else:
+ result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive1', sync='full')
self.assert_qmp(result, 'return', {})
# Default speed is 0
@@ -148,10 +207,14 @@ class TestSetSpeed(iotests.QMPTestCase):
event = self.cancel_and_wait(resume=True)
self.assert_qmp(event, 'data/type', 'backup')
- # Check setting speed in drive-backup works
+ # Check setting speed option works
self.vm.pause_drive('drive0')
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full', speed=4*1024*1024)
+ if test_drive_backup:
+ result = self.vm.qmp('drive-backup', device='drive0',
+ target=target_img, sync='full', speed=4*1024*1024)
+ else:
+ result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive1', sync='full', speed=4*1024*1024)
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('query-block-jobs')
@@ -161,18 +224,32 @@ class TestSetSpeed(iotests.QMPTestCase):
event = self.cancel_and_wait(resume=True)
self.assert_qmp(event, 'data/type', 'backup')
- def test_set_speed_invalid(self):
+ def test_set_speed_drive_backup(self):
+ self.do_test_set_speed(True)
+
+ def test_set_speed_blockdev_backup(self):
+ self.do_test_set_speed(False)
+
+ def do_test_set_speed_invalid(self, test_drive_backup):
self.assert_no_active_block_jobs()
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full', speed=-1)
+ if test_drive_backup:
+ result = self.vm.qmp('drive-backup', device='drive0',
+ target=target_img, sync='full', speed=-1)
+ else:
+ result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive1', sync='full', speed=-1)
self.assert_qmp(result, 'error/class', 'GenericError')
self.assert_no_active_block_jobs()
self.vm.pause_drive('drive0')
- result = self.vm.qmp('drive-backup', device='drive0',
- target=target_img, sync='full')
+ if test_drive_backup:
+ result = self.vm.qmp('drive-backup', device='drive0',
+ target=target_img, sync='full')
+ else:
+ result = self.vm.qmp('blockdev-backup', device='drive0',
+ target='drive1', sync='full')
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
@@ -181,6 +258,12 @@ class TestSetSpeed(iotests.QMPTestCase):
event = self.cancel_and_wait(resume=True)
self.assert_qmp(event, 'data/type', 'backup')
+ def test_set_speed_invalid_drive_backup(self):
+ self.do_test_set_speed_invalid(True)
+
+ def test_set_speed_invalid_blockdev_backup(self):
+ self.do_test_set_speed_invalid(False)
+
class TestSingleTransaction(iotests.QMPTestCase):
image_len = 64 * 1024 * 1024 # MB
@@ -190,44 +273,71 @@ class TestSingleTransaction(iotests.QMPTestCase):
qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
+ qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(TestSingleDrive.image_len))
- self.vm = iotests.VM().add_drive(test_img)
+ self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img)
self.vm.launch()
def tearDown(self):
self.vm.shutdown()
os.remove(test_img)
+ os.remove(blockdev_target_img)
try:
os.remove(target_img)
except OSError:
pass
- def test_cancel(self):
+ def do_test_cancel(self, test_drive_backup):
self.assert_no_active_block_jobs()
- result = self.vm.qmp('transaction', actions=[{
- 'type': 'drive-backup',
- 'data': { 'device': 'drive0',
- 'target': target_img,
- 'sync': 'full' },
- }
- ])
+ if test_drive_backup:
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'drive-backup',
+ 'data': { 'device': 'drive0',
+ 'target': target_img,
+ 'sync': 'full' },
+ }
+ ])
+ else:
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'drive0',
+ 'target': 'drive1',
+ 'sync': 'full' },
+ }
+ ])
+
self.assert_qmp(result, 'return', {})
event = self.cancel_and_wait()
self.assert_qmp(event, 'data/type', 'backup')
- def test_pause(self):
+ def test_cancel_drive_backup(self):
+ self.do_test_cancel(True)
+
+ def test_cancel_blockdev_backup(self):
+ self.do_test_cancel(False)
+
+ def do_test_pause(self, test_drive_backup):
self.assert_no_active_block_jobs()
self.vm.pause_drive('drive0')
- result = self.vm.qmp('transaction', actions=[{
- 'type': 'drive-backup',
- 'data': { 'device': 'drive0',
- 'target': target_img,
- 'sync': 'full' },
- }
- ])
+ if test_drive_backup:
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'drive-backup',
+ 'data': { 'device': 'drive0',
+ 'target': target_img,
+ 'sync': 'full' },
+ }
+ ])
+ else:
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'drive0',
+ 'target': 'drive1',
+ 'sync': 'full' },
+ }
+ ])
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-job-pause', device='drive0')
@@ -248,8 +358,18 @@ class TestSingleTransaction(iotests.QMPTestCase):
self.wait_until_completed()
self.vm.shutdown()
- self.assertTrue(iotests.compare_images(test_img, target_img),
- 'target image does not match source after backup')
+ if test_drive_backup:
+ self.assertTrue(iotests.compare_images(test_img, target_img),
+ 'target image does not match source after backup')
+ else:
+ self.assertTrue(iotests.compare_images(test_img, blockdev_target_img),
+ 'target image does not match source after backup')
+
+ def test_pause_drive_backup(self):
+ self.do_test_pause(True)
+
+ def test_pause_blockdev_backup(self):
+ self.do_test_pause(False)
def test_medium_not_found(self):
result = self.vm.qmp('transaction', actions=[{
@@ -260,6 +380,14 @@ class TestSingleTransaction(iotests.QMPTestCase):
}
])
self.assert_qmp(result, 'error/class', 'GenericError')
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'ide1-cd0',
+ 'target': 'drive1',
+ 'sync': 'full' },
+ }
+ ])
+ self.assert_qmp(result, 'error/class', 'GenericError')
def test_image_not_found(self):
result = self.vm.qmp('transaction', actions=[{
@@ -283,6 +411,43 @@ class TestSingleTransaction(iotests.QMPTestCase):
])
self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'nonexistent',
+ 'target': 'drive1',
+ 'sync': 'full' },
+ }
+ ])
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'drive0',
+ 'target': 'nonexistent',
+ 'sync': 'full' },
+ }
+ ])
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'nonexistent',
+ 'target': 'nonexistent',
+ 'sync': 'full' },
+ }
+ ])
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ def test_target_is_source(self):
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'drive0',
+ 'target': 'drive0',
+ 'sync': 'full' },
+ }
+ ])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
def test_abort(self):
result = self.vm.qmp('transaction', actions=[{
'type': 'drive-backup',
@@ -298,5 +463,31 @@ class TestSingleTransaction(iotests.QMPTestCase):
self.assert_qmp(result, 'error/class', 'GenericError')
self.assert_no_active_block_jobs()
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'nonexistent',
+ 'target': 'drive1',
+ 'sync': 'full' },
+ }, {
+ 'type': 'Abort',
+ 'data': {},
+ }
+ ])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_no_active_block_jobs()
+
+ result = self.vm.qmp('transaction', actions=[{
+ 'type': 'blockdev-backup',
+ 'data': { 'device': 'drive0',
+ 'target': 'nonexistent',
+ 'sync': 'full' },
+ }, {
+ 'type': 'Abort',
+ 'data': {},
+ }
+ ])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ self.assert_no_active_block_jobs()
+
if __name__ == '__main__':
iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index 6323079..c6a10f8 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-..............
+.....................
----------------------------------------------------------------------
-Ran 14 tests
+Ran 21 tests
OK
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v20 15/15] qemu-iotests: Image fleecing test case 089
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (13 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 14/15] qemu-iotests: Test blockdev-backup in 055 Fam Zheng
@ 2014-05-20 6:04 ` Fam Zheng
2018-06-29 18:00 ` [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Eric Blake
15 siblings, 0 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-20 6:04 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, jcody, hbrock, rjones, armbru, imain, stefanha, pbonzini
This tests the workflow of creating a lightweight point-in-time snapshot
with blockdev-backup command, and exporting it with built-in NBD server.
It's tested that any post-snapshot writing to the original device
doesn't change data seen in NBD target.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
tests/qemu-iotests/089 | 99 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/089.out | 5 +++
tests/qemu-iotests/group | 1 +
3 files changed, 105 insertions(+)
create mode 100755 tests/qemu-iotests/089
create mode 100644 tests/qemu-iotests/089.out
diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
new file mode 100755
index 0000000..8be32d7
--- /dev/null
+++ b/tests/qemu-iotests/089
@@ -0,0 +1,99 @@
+#!/usr/bin/env python
+#
+# Tests for image fleecing (point in time snapshot export to NBD)
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# Based on 055.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+target_img = os.path.join(iotests.test_dir, 'target.img')
+nbd_sock = os.path.join(iotests.test_dir, 'nbd.sock')
+
+class TestImageFleecing(iotests.QMPTestCase):
+ image_len = 64 * 1024 * 1024 # MB
+
+ def setUp(self):
+ # Write data to the image so we can compare later
+ qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestImageFleecing.image_len))
+ self.patterns = [
+ ("0x5d", "0", "64k"),
+ ("0xd5", "1M", "64k"),
+ ("0xdc", "32M", "64k"),
+ ("0xdc", "67043328", "64k")]
+
+ for p in self.patterns:
+ qemu_io('-c', 'write -P%s %s %s' % p, test_img)
+
+ qemu_img('create', '-f', iotests.imgfmt, target_img, str(TestImageFleecing.image_len))
+
+ self.vm = iotests.VM().add_drive(test_img)
+ self.vm.launch()
+
+ self.overwrite_patterns = [
+ ("0xa0", "0", "64k"),
+ ("0x0a", "1M", "64k"),
+ ("0x55", "32M", "64k"),
+ ("0x56", "67043328", "64k")]
+
+ self.nbd_uri = "nbd+unix:///drive1?socket=%s" % nbd_sock
+
+ def tearDown(self):
+ self.vm.shutdown()
+ os.remove(test_img)
+ os.remove(target_img)
+
+ def verify_patterns(self):
+ for p in self.patterns:
+ self.assertEqual(-1, qemu_io(self.nbd_uri, '-c', 'read -P%s %s %s' % p).find("verification failed"),
+ "Failed to verify pattern: %s %s %s" % p)
+
+ def test_image_fleecing(self):
+ result = self.vm.qmp("blockdev-add", **{"options": {
+ "driver": "qcow2",
+ "id": "drive1",
+ "file": {
+ "driver": "file",
+ "filename": target_img,
+ },
+ "backing": "drive0",
+ }})
+ self.assert_qmp(result, 'return', {})
+ result = self.vm.qmp("nbd-server-start", **{"addr": { "type": "unix", "data": { "path": nbd_sock } } })
+ self.assert_qmp(result, 'return', {})
+ result = self.vm.qmp("blockdev-backup", device="drive0", target="drive1", sync="none")
+ self.assert_qmp(result, 'return', {})
+ result = self.vm.qmp("nbd-server-add", device="drive1")
+ self.assert_qmp(result, 'return', {})
+
+ self.verify_patterns()
+
+ for p in self.overwrite_patterns:
+ self.vm.hmp_qemu_io("drive0", "write -P%s %s %s" % p)
+
+ self.verify_patterns()
+
+ self.cancel_and_wait(resume=True)
+ self.assert_no_active_block_jobs()
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
new file mode 100644
index 0000000..ae1213e
--- /dev/null
+++ b/tests/qemu-iotests/089.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cd3e4d2..2988cfd 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -95,5 +95,6 @@
086 rw auto quick
087 rw auto
088 rw auto
+089 rw auto quick
090 rw auto quick
091 rw auto
--
1.9.2
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller Fam Zheng
@ 2014-05-20 11:43 ` Jeff Cody
2014-05-21 1:36 ` Fam Zheng
2014-05-21 13:20 ` Stefan Hajnoczi
1 sibling, 1 reply; 37+ messages in thread
From: Jeff Cody @ 2014-05-20 11:43 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, hbrock, qemu-devel, rjones, imain, stefanha, pbonzini
On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote:
> 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>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> ---
> blockdev.c | 8 ++++++++
> blockjob.c | 2 +-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 5d950fa..21fc55b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1850,6 +1850,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) {
> @@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device,
> return;
> }
>
> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> + return;
> + }
> +
Is the blocker intended to operate at the device level, i.e. to mark a
whole chain as 'blocked' for one or more operations? Or, is it
intended to block at the singular BDS level (the commit message in
patch 2 implies this meaning)?
More to the point: if a BDS is marked as blocked, does that also imply
all of the images in its backing chain are also considered blocked?
Conversely, if a BDS is *not* marked as blocked, does that mean all of
its backing chain is also unblocked?
If the answer to the two questions above is 'yes', then the
bdrv_op_block/unblock functions should probably operate recursively
down the chain to the bottom-most backing file.
If the answer is 'no', then for some operations like stream and commit
(and probably others), don't we also need to worry about the blocker
state of a lot more images in the chain?
> /* default top_bs is the active layer */
> top_bs = bs;
>
> diff --git a/blockjob.c b/blockjob.c
> index 60e72f5..7d84ca1 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.9.2
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-20 11:43 ` Jeff Cody
@ 2014-05-21 1:36 ` Fam Zheng
2014-05-21 4:34 ` Jeff Cody
0 siblings, 1 reply; 37+ messages in thread
From: Fam Zheng @ 2014-05-21 1:36 UTC (permalink / raw)
To: Jeff Cody
Cc: kwolf, armbru, hbrock, qemu-devel, rjones, imain, stefanha, pbonzini
On Tue, 05/20 07:43, Jeff Cody wrote:
> On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote:
> > 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>
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > Reviewed-by: Jeff Cody <jcody@redhat.com>
> > ---
> > blockdev.c | 8 ++++++++
> > blockjob.c | 2 +-
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 5d950fa..21fc55b 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1850,6 +1850,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) {
> > @@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device,
> > return;
> > }
> >
> > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> > + return;
> > + }
> > +
>
> Is the blocker intended to operate at the device level, i.e. to mark a
> whole chain as 'blocked' for one or more operations? Or, is it
> intended to block at the singular BDS level (the commit message in
> patch 2 implies this meaning)?
Good question! It should be per BDS, that's why we need backing_blocker.
Fam
>
> More to the point: if a BDS is marked as blocked, does that also imply
> all of the images in its backing chain are also considered blocked?
No.
> Conversely, if a BDS is *not* marked as blocked, does that mean all of
> its backing chain is also unblocked?
No. But all the backing_hd is blocked by backing_blocker, so we are safe.
With node-name introduced, some qmp operations are accessible on a BDS in the
middle of a chain, with node-name argument. E.g. @BlockdevSnapshot (Hmm, why
would blockdev-snapshot operate on node-name, when it's called blockdev-*?).
So the question is, what happens if user tries to take some operation on mid,
with the node-name:
base <-- mid <-- active
With this series, we are safe because mid is protected by the backing_blocker
of active, which blocks all the operations on mid, except as commit target and
backup source.
The idea is, we firstly block any operation in the middle of the chain with
backing_blocker, and we relax those that we think is safe and demanded. This
mechanism also protects the node-name based operations.
Fam
>
> If the answer to the two questions above is 'yes', then the
> bdrv_op_block/unblock functions should probably operate recursively
> down the chain to the bottom-most backing file.
>
> If the answer is 'no', then for some operations like stream and commit
> (and probably others), don't we also need to worry about the blocker
> state of a lot more images in the chain?
>
>
> > /* default top_bs is the active layer */
> > top_bs = bs;
> >
> > diff --git a/blockjob.c b/blockjob.c
> > index 60e72f5..7d84ca1 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.9.2
> >
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-21 1:36 ` Fam Zheng
@ 2014-05-21 4:34 ` Jeff Cody
2014-05-21 6:08 ` Fam Zheng
0 siblings, 1 reply; 37+ messages in thread
From: Jeff Cody @ 2014-05-21 4:34 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, hbrock, qemu-devel, rjones, imain, stefanha, pbonzini
On Wed, May 21, 2014 at 09:36:08AM +0800, Fam Zheng wrote:
> On Tue, 05/20 07:43, Jeff Cody wrote:
> > On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote:
> > > 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>
> > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > > Reviewed-by: Jeff Cody <jcody@redhat.com>
> > > ---
> > > blockdev.c | 8 ++++++++
> > > blockjob.c | 2 +-
> > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 5d950fa..21fc55b 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -1850,6 +1850,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) {
> > > @@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device,
> > > return;
> > > }
> > >
> > > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> > > + return;
> > > + }
> > > +
> >
> > Is the blocker intended to operate at the device level, i.e. to mark a
> > whole chain as 'blocked' for one or more operations? Or, is it
> > intended to block at the singular BDS level (the commit message in
> > patch 2 implies this meaning)?
>
> Good question! It should be per BDS, that's why we need backing_blocker.
>
> Fam
>
> >
> > More to the point: if a BDS is marked as blocked, does that also imply
> > all of the images in its backing chain are also considered blocked?
>
> No.
Then why don't we check the blocker values for overlay_bs, top_bs,
base_bs, and intermediate images in qmp_block_commit()? This patch
only checks the active bs blocker.. yet in some commits, the active
layer BDS may not actually be affected at all.
>
> > Conversely, if a BDS is *not* marked as blocked, does that mean all of
> > its backing chain is also unblocked?
>
> No. But all the backing_hd is blocked by backing_blocker, so we are safe.
>
> With node-name introduced, some qmp operations are accessible on a BDS in the
> middle of a chain, with node-name argument. E.g. @BlockdevSnapshot (Hmm, why
> would blockdev-snapshot operate on node-name, when it's called blockdev-*?).
>
Thanks - and that is exactly what prompted my question; with
node-name, we don't necessarily start at the top of the chain, and we
can (and will) reference BDSs individually.
> So the question is, what happens if user tries to take some operation on mid,
> with the node-name:
>
> base <-- mid <-- active
>
> With this series, we are safe because mid is protected by the backing_blocker
> of active, which blocks all the operations on mid, except as commit target and
> backup source.
>
Right, but that commit exception disproves the rule of 'blockers work
on the BDS level'.
It means intermediate images (anything below active) will not be
blocked for commit. And this ends up working OK (I think even with my
block-commit node-name patches), because we still use the 'device' to
lookup the active BDS, and that one BDS will be blocked and we catch
that in this patch. But that may not always be the case, particular
since not all block-commits even involve the active layer.
And this means that we are back to the first part - the active BDS
blocker is effectively for the chain, not the BDS. We rely on the
fact that the active layer BDS will be blocked, to make up for the
fact that the rest of the chain is not blocked for commit.
So to be safe, and to use the blocker as individual BDS blockers,
shouldn't we do a bdrv_op_block_all() in block_job_create()
for every BDS in a chain affected by the block job, and then clear our
exception whitelist for those same BDSs (if they still exist!) when
the block job is complete (or on failure to start the job)? That way,
we are not relying on the active layer blocker acting as a proxy
blocker for the entire chain.
> The idea is, we firstly block any operation in the middle of the chain with
> backing_blocker, and we relax those that we think is safe and demanded. This
> mechanism also protects the node-name based operations.
>
I worry that as-is, what this really means is that the blockers on the
intermediate images don't really do much. Because any operation that
will actually affect an intermediate image will need similar treatment
as commit to relax the rules... so the actual operations you want to
block all end up with exceptions to not be blocked. But I think if we
do what I outlined above, that could negate my fear.
>
> >
> > If the answer to the two questions above is 'yes', then the
> > bdrv_op_block/unblock functions should probably operate recursively
> > down the chain to the bottom-most backing file.
> >
> > If the answer is 'no', then for some operations like stream and commit
> > (and probably others), don't we also need to worry about the blocker
> > state of a lot more images in the chain?
> >
> >
> > > /* default top_bs is the active layer */
> > > top_bs = bs;
> > >
> > > diff --git a/blockjob.c b/blockjob.c
> > > index 60e72f5..7d84ca1 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.9.2
> > >
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-21 4:34 ` Jeff Cody
@ 2014-05-21 6:08 ` Fam Zheng
2014-05-21 13:13 ` Stefan Hajnoczi
2014-05-21 14:04 ` Jeff Cody
0 siblings, 2 replies; 37+ messages in thread
From: Fam Zheng @ 2014-05-21 6:08 UTC (permalink / raw)
To: Jeff Cody
Cc: kwolf, qemu-devel, hbrock, rjones, armbru, imain, stefanha, pbonzini
On Wed, 05/21 00:34, Jeff Cody wrote:
> On Wed, May 21, 2014 at 09:36:08AM +0800, Fam Zheng wrote:
> > On Tue, 05/20 07:43, Jeff Cody wrote:
> > > On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote:
> > > > 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>
> > > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > > > Reviewed-by: Jeff Cody <jcody@redhat.com>
> > > > ---
> > > > blockdev.c | 8 ++++++++
> > > > blockjob.c | 2 +-
> > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/blockdev.c b/blockdev.c
> > > > index 5d950fa..21fc55b 100644
> > > > --- a/blockdev.c
> > > > +++ b/blockdev.c
> > > > @@ -1850,6 +1850,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) {
> > > > @@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device,
> > > > return;
> > > > }
> > > >
> > > > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> > > > + return;
> > > > + }
> > > > +
> > >
> > > Is the blocker intended to operate at the device level, i.e. to mark a
> > > whole chain as 'blocked' for one or more operations? Or, is it
> > > intended to block at the singular BDS level (the commit message in
> > > patch 2 implies this meaning)?
> >
> > Good question! It should be per BDS, that's why we need backing_blocker.
> >
> > Fam
> >
> > >
> > > More to the point: if a BDS is marked as blocked, does that also imply
> > > all of the images in its backing chain are also considered blocked?
> >
> > No.
>
> Then why don't we check the blocker values for overlay_bs, top_bs,
> base_bs, and intermediate images in qmp_block_commit()? This patch
> only checks the active bs blocker.. yet in some commits, the active
> layer BDS may not actually be affected at all.
>
> >
> > > Conversely, if a BDS is *not* marked as blocked, does that mean all of
> > > its backing chain is also unblocked?
> >
> > No. But all the backing_hd is blocked by backing_blocker, so we are safe.
> >
> > With node-name introduced, some qmp operations are accessible on a BDS in the
> > middle of a chain, with node-name argument. E.g. @BlockdevSnapshot (Hmm, why
> > would blockdev-snapshot operate on node-name, when it's called blockdev-*?).
> >
>
> Thanks - and that is exactly what prompted my question; with
> node-name, we don't necessarily start at the top of the chain, and we
> can (and will) reference BDSs individually.
>
>
> > So the question is, what happens if user tries to take some operation on mid,
> > with the node-name:
> >
> > base <-- mid <-- active
> >
> > With this series, we are safe because mid is protected by the backing_blocker
> > of active, which blocks all the operations on mid, except as commit target and
> > backup source.
> >
>
> Right, but that commit exception disproves the rule of 'blockers work
> on the BDS level'.
>
> It means intermediate images (anything below active) will not be
> blocked for commit. And this ends up working OK (I think even with my
> block-commit node-name patches), because we still use the 'device' to
> lookup the active BDS, and that one BDS will be blocked and we catch
> that in this patch. But that may not always be the case, particular
> since not all block-commits even involve the active layer.
>
> And this means that we are back to the first part - the active BDS
> blocker is effectively for the chain, not the BDS. We rely on the
> fact that the active layer BDS will be blocked, to make up for the
> fact that the rest of the chain is not blocked for commit.
Right. It is not practical to apply the semantics of op blocker on a whole
chain, because we need different permissions on active and its backing, and ...
>
> So to be safe, and to use the blocker as individual BDS blockers,
> shouldn't we do a bdrv_op_block_all() in block_job_create()
> for every BDS in a chain affected by the block job, and then clear our
> exception whitelist for those same BDSs (if they still exist!) when
> the block job is complete (or on failure to start the job)? That way,
> we are not relying on the active layer blocker acting as a proxy
> blocker for the entire chain.
... backing_blocker is introduced to save the effort to add blockers
recursively. I think it means we should distinguish COMMIT_SOURCE and
COMMIT_TARGET, as in block-backup, and only allow COMMIT_TARGET on backing_hd.
Could this make us safe? What does it mean to your node-name commit changes?
Thanks,
Fam
>
>
> > The idea is, we firstly block any operation in the middle of the chain with
> > backing_blocker, and we relax those that we think is safe and demanded. This
> > mechanism also protects the node-name based operations.
> >
>
> I worry that as-is, what this really means is that the blockers on the
> intermediate images don't really do much. Because any operation that
> will actually affect an intermediate image will need similar treatment
> as commit to relax the rules... so the actual operations you want to
> block all end up with exceptions to not be blocked. But I think if we
> do what I outlined above, that could negate my fear.
>
> >
> > >
> > > If the answer to the two questions above is 'yes', then the
> > > bdrv_op_block/unblock functions should probably operate recursively
> > > down the chain to the bottom-most backing file.
> > >
> > > If the answer is 'no', then for some operations like stream and commit
> > > (and probably others), don't we also need to worry about the blocker
> > > state of a lot more images in the chain?
> > >
> > >
> > > > /* default top_bs is the active layer */
> > > > top_bs = bs;
> > > >
> > > > diff --git a/blockjob.c b/blockjob.c
> > > > index 60e72f5..7d84ca1 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.9.2
> > > >
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 01/15] block: Add BlockOpType enum
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 01/15] block: Add BlockOpType enum Fam Zheng
@ 2014-05-21 12:28 ` Stefan Hajnoczi
0 siblings, 0 replies; 37+ messages in thread
From: Stefan Hajnoczi @ 2014-05-21 12:28 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, jcody, hbrock, qemu-devel, rjones, imain, pbonzini
On Tue, May 20, 2014 at 02:04:26PM +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>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> ---
> include/block/block.h | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 02/15] block: Introduce op_blockers to BlockDriverState
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 02/15] block: Introduce op_blockers to BlockDriverState Fam Zheng
@ 2014-05-21 12:28 ` Stefan Hajnoczi
0 siblings, 0 replies; 37+ messages in thread
From: Stefan Hajnoczi @ 2014-05-21 12:28 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, jcody, hbrock, qemu-devel, rjones, imain, pbonzini
On Tue, May 20, 2014 at 02:04:27PM +0800, Fam Zheng wrote:
> 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>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> ---
> block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 7 +++++
> include/block/block_int.h | 5 ++++
> 3 files changed, 88 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 03/15] block: Replace in_use with operation blocker
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 03/15] block: Replace in_use with operation blocker Fam Zheng
@ 2014-05-21 12:46 ` Stefan Hajnoczi
0 siblings, 0 replies; 37+ messages in thread
From: Stefan Hajnoczi @ 2014-05-21 12:46 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, jcody, hbrock, qemu-devel, rjones, imain, pbonzini
On Tue, May 20, 2014 at 02:04:28PM +0800, Fam Zheng wrote:
> 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, ...).
>
> There is one exception in block_job_create, where
> bdrv_op_blocker_is_empty() is used, because we don't know the operation
> type here. This doesn't matter because in a few commits away we will drop
> the check and move it to callers that _do_ know the type.
>
> - 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>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> ---
> block-migration.c | 7 +++++--
> block.c | 24 +++++++-----------------
> blockdev.c | 19 +++++++++----------
> 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, 45 insertions(+), 43 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-21 6:08 ` Fam Zheng
@ 2014-05-21 13:13 ` Stefan Hajnoczi
2014-05-21 13:17 ` Jeff Cody
2014-05-21 14:04 ` Jeff Cody
1 sibling, 1 reply; 37+ messages in thread
From: Stefan Hajnoczi @ 2014-05-21 13:13 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, qemu-devel, Jeff Cody, hbrock, armbru, rjones, imain, pbonzini
On Wed, May 21, 2014 at 02:08:46PM +0800, Fam Zheng wrote:
> On Wed, 05/21 00:34, Jeff Cody wrote:
> > On Wed, May 21, 2014 at 09:36:08AM +0800, Fam Zheng wrote:
> > > On Tue, 05/20 07:43, Jeff Cody wrote:
> > > > On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote:
> > > > > 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>
> > > > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > > > > Reviewed-by: Jeff Cody <jcody@redhat.com>
> > > > > ---
> > > > > blockdev.c | 8 ++++++++
> > > > > blockjob.c | 2 +-
> > > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/blockdev.c b/blockdev.c
> > > > > index 5d950fa..21fc55b 100644
> > > > > --- a/blockdev.c
> > > > > +++ b/blockdev.c
> > > > > @@ -1850,6 +1850,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) {
> > > > > @@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device,
> > > > > return;
> > > > > }
> > > > >
> > > > > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> > > > > + return;
> > > > > + }
> > > > > +
> > > >
> > > > Is the blocker intended to operate at the device level, i.e. to mark a
> > > > whole chain as 'blocked' for one or more operations? Or, is it
> > > > intended to block at the singular BDS level (the commit message in
> > > > patch 2 implies this meaning)?
> > >
> > > Good question! It should be per BDS, that's why we need backing_blocker.
> > >
> > > Fam
> > >
> > > >
> > > > More to the point: if a BDS is marked as blocked, does that also imply
> > > > all of the images in its backing chain are also considered blocked?
> > >
> > > No.
> >
> > Then why don't we check the blocker values for overlay_bs, top_bs,
> > base_bs, and intermediate images in qmp_block_commit()? This patch
> > only checks the active bs blocker.. yet in some commits, the active
> > layer BDS may not actually be affected at all.
> >
> > >
> > > > Conversely, if a BDS is *not* marked as blocked, does that mean all of
> > > > its backing chain is also unblocked?
> > >
> > > No. But all the backing_hd is blocked by backing_blocker, so we are safe.
> > >
> > > With node-name introduced, some qmp operations are accessible on a BDS in the
> > > middle of a chain, with node-name argument. E.g. @BlockdevSnapshot (Hmm, why
> > > would blockdev-snapshot operate on node-name, when it's called blockdev-*?).
> > >
> >
> > Thanks - and that is exactly what prompted my question; with
> > node-name, we don't necessarily start at the top of the chain, and we
> > can (and will) reference BDSs individually.
> >
> >
> > > So the question is, what happens if user tries to take some operation on mid,
> > > with the node-name:
> > >
> > > base <-- mid <-- active
> > >
> > > With this series, we are safe because mid is protected by the backing_blocker
> > > of active, which blocks all the operations on mid, except as commit target and
> > > backup source.
> > >
> >
> > Right, but that commit exception disproves the rule of 'blockers work
> > on the BDS level'.
> >
> > It means intermediate images (anything below active) will not be
> > blocked for commit. And this ends up working OK (I think even with my
> > block-commit node-name patches), because we still use the 'device' to
> > lookup the active BDS, and that one BDS will be blocked and we catch
> > that in this patch. But that may not always be the case, particular
> > since not all block-commits even involve the active layer.
> >
> > And this means that we are back to the first part - the active BDS
> > blocker is effectively for the chain, not the BDS. We rely on the
> > fact that the active layer BDS will be blocked, to make up for the
> > fact that the rest of the chain is not blocked for commit.
>
> Right. It is not practical to apply the semantics of op blocker on a whole
> chain, because we need different permissions on active and its backing, and ...
>
> >
> > So to be safe, and to use the blocker as individual BDS blockers,
> > shouldn't we do a bdrv_op_block_all() in block_job_create()
> > for every BDS in a chain affected by the block job, and then clear our
> > exception whitelist for those same BDSs (if they still exist!) when
> > the block job is complete (or on failure to start the job)? That way,
> > we are not relying on the active layer blocker acting as a proxy
> > blocker for the entire chain.
>
> ... backing_blocker is introduced to save the effort to add blockers
> recursively. I think it means we should distinguish COMMIT_SOURCE and
> COMMIT_TARGET, as in block-backup, and only allow COMMIT_TARGET on backing_hd.
>
> Could this make us safe? What does it mean to your node-name commit changes?
We have these node-name issues independent of this patch series.
bdrv_in_use() has the same problem that node-name allows you to
reference nodes that we never called bdrv_set_in_use() on.
I think the discussion is important but should not block this patch
series.
Stefan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-21 13:13 ` Stefan Hajnoczi
@ 2014-05-21 13:17 ` Jeff Cody
0 siblings, 0 replies; 37+ messages in thread
From: Jeff Cody @ 2014-05-21 13:17 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, Fam Zheng, qemu-devel, hbrock, rjones, armbru, imain, pbonzini
On Wed, May 21, 2014 at 03:13:15PM +0200, Stefan Hajnoczi wrote:
> On Wed, May 21, 2014 at 02:08:46PM +0800, Fam Zheng wrote:
> > On Wed, 05/21 00:34, Jeff Cody wrote:
> > > On Wed, May 21, 2014 at 09:36:08AM +0800, Fam Zheng wrote:
> > > > On Tue, 05/20 07:43, Jeff Cody wrote:
> > > > > On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote:
> > > > > > 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>
> > > > > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > > > > > Reviewed-by: Jeff Cody <jcody@redhat.com>
> > > > > > ---
> > > > > > blockdev.c | 8 ++++++++
> > > > > > blockjob.c | 2 +-
> > > > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/blockdev.c b/blockdev.c
> > > > > > index 5d950fa..21fc55b 100644
> > > > > > --- a/blockdev.c
> > > > > > +++ b/blockdev.c
> > > > > > @@ -1850,6 +1850,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) {
> > > > > > @@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device,
> > > > > > return;
> > > > > > }
> > > > > >
> > > > > > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > >
> > > > > Is the blocker intended to operate at the device level, i.e. to mark a
> > > > > whole chain as 'blocked' for one or more operations? Or, is it
> > > > > intended to block at the singular BDS level (the commit message in
> > > > > patch 2 implies this meaning)?
> > > >
> > > > Good question! It should be per BDS, that's why we need backing_blocker.
> > > >
> > > > Fam
> > > >
> > > > >
> > > > > More to the point: if a BDS is marked as blocked, does that also imply
> > > > > all of the images in its backing chain are also considered blocked?
> > > >
> > > > No.
> > >
> > > Then why don't we check the blocker values for overlay_bs, top_bs,
> > > base_bs, and intermediate images in qmp_block_commit()? This patch
> > > only checks the active bs blocker.. yet in some commits, the active
> > > layer BDS may not actually be affected at all.
> > >
> > > >
> > > > > Conversely, if a BDS is *not* marked as blocked, does that mean all of
> > > > > its backing chain is also unblocked?
> > > >
> > > > No. But all the backing_hd is blocked by backing_blocker, so we are safe.
> > > >
> > > > With node-name introduced, some qmp operations are accessible on a BDS in the
> > > > middle of a chain, with node-name argument. E.g. @BlockdevSnapshot (Hmm, why
> > > > would blockdev-snapshot operate on node-name, when it's called blockdev-*?).
> > > >
> > >
> > > Thanks - and that is exactly what prompted my question; with
> > > node-name, we don't necessarily start at the top of the chain, and we
> > > can (and will) reference BDSs individually.
> > >
> > >
> > > > So the question is, what happens if user tries to take some operation on mid,
> > > > with the node-name:
> > > >
> > > > base <-- mid <-- active
> > > >
> > > > With this series, we are safe because mid is protected by the backing_blocker
> > > > of active, which blocks all the operations on mid, except as commit target and
> > > > backup source.
> > > >
> > >
> > > Right, but that commit exception disproves the rule of 'blockers work
> > > on the BDS level'.
> > >
> > > It means intermediate images (anything below active) will not be
> > > blocked for commit. And this ends up working OK (I think even with my
> > > block-commit node-name patches), because we still use the 'device' to
> > > lookup the active BDS, and that one BDS will be blocked and we catch
> > > that in this patch. But that may not always be the case, particular
> > > since not all block-commits even involve the active layer.
> > >
> > > And this means that we are back to the first part - the active BDS
> > > blocker is effectively for the chain, not the BDS. We rely on the
> > > fact that the active layer BDS will be blocked, to make up for the
> > > fact that the rest of the chain is not blocked for commit.
> >
> > Right. It is not practical to apply the semantics of op blocker on a whole
> > chain, because we need different permissions on active and its backing, and ...
> >
> > >
> > > So to be safe, and to use the blocker as individual BDS blockers,
> > > shouldn't we do a bdrv_op_block_all() in block_job_create()
> > > for every BDS in a chain affected by the block job, and then clear our
> > > exception whitelist for those same BDSs (if they still exist!) when
> > > the block job is complete (or on failure to start the job)? That way,
> > > we are not relying on the active layer blocker acting as a proxy
> > > blocker for the entire chain.
> >
> > ... backing_blocker is introduced to save the effort to add blockers
> > recursively. I think it means we should distinguish COMMIT_SOURCE and
> > COMMIT_TARGET, as in block-backup, and only allow COMMIT_TARGET on backing_hd.
> >
> > Could this make us safe? What does it mean to your node-name commit changes?
>
> We have these node-name issues independent of this patch series.
> bdrv_in_use() has the same problem that node-name allows you to
> reference nodes that we never called bdrv_set_in_use() on.
>
> I think the discussion is important but should not block this patch
> series.
>
I agree (this patch and the other op blocker ones before this already
have my reviewed-by).
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller Fam Zheng
2014-05-20 11:43 ` Jeff Cody
@ 2014-05-21 13:20 ` Stefan Hajnoczi
1 sibling, 0 replies; 37+ messages in thread
From: Stefan Hajnoczi @ 2014-05-21 13:20 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, jcody, hbrock, qemu-devel, rjones, imain, pbonzini
On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote:
> 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>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> ---
> blockdev.c | 8 ++++++++
> blockjob.c | 2 +-
> 2 files changed, 9 insertions(+), 1 deletion(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd()
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd() Fam Zheng
@ 2014-05-21 13:56 ` Stefan Hajnoczi
2014-05-21 14:17 ` Jeff Cody
1 sibling, 0 replies; 37+ messages in thread
From: Stefan Hajnoczi @ 2014-05-21 13:56 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, jcody, hbrock, qemu-devel, rjones, imain, pbonzini
On Tue, May 20, 2014 at 02:04:30PM +0800, Fam Zheng wrote:
> 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 | 36 +++++++++++++++++++++++-------------
> include/block/block.h | 1 +
> 2 files changed, 24 insertions(+), 13 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState Fam Zheng
@ 2014-05-21 14:03 ` Stefan Hajnoczi
2014-05-21 14:24 ` Jeff Cody
2014-05-21 14:06 ` Stefan Hajnoczi
1 sibling, 1 reply; 37+ messages in thread
From: Stefan Hajnoczi @ 2014-05-21 14:03 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, jcody, hbrock, qemu-devel, rjones, imain, pbonzini
On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote:
> diff --git a/block/mirror.c b/block/mirror.c
> index 1c38aa8..6a53d79 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -499,6 +499,7 @@ immediate_exit:
> * trigger the unref from the top one */
> BlockDriverState *p = s->base->backing_hd;
> s->base->backing_hd = NULL;
> + bdrv_op_unblock_all(p, s->base->backing_blocker);
> bdrv_unref(p);
> }
> }
Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead
of open coding it.
Stefan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller
2014-05-21 6:08 ` Fam Zheng
2014-05-21 13:13 ` Stefan Hajnoczi
@ 2014-05-21 14:04 ` Jeff Cody
1 sibling, 0 replies; 37+ messages in thread
From: Jeff Cody @ 2014-05-21 14:04 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, qemu-devel, hbrock, rjones, armbru, imain, stefanha, pbonzini
On Wed, May 21, 2014 at 02:08:46PM +0800, Fam Zheng wrote:
> On Wed, 05/21 00:34, Jeff Cody wrote:
> > On Wed, May 21, 2014 at 09:36:08AM +0800, Fam Zheng wrote:
> > > On Tue, 05/20 07:43, Jeff Cody wrote:
> > > > On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote:
> > > > > 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>
> > > > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > > > > Reviewed-by: Jeff Cody <jcody@redhat.com>
> > > > > ---
> > > > > blockdev.c | 8 ++++++++
> > > > > blockjob.c | 2 +-
> > > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/blockdev.c b/blockdev.c
> > > > > index 5d950fa..21fc55b 100644
> > > > > --- a/blockdev.c
> > > > > +++ b/blockdev.c
> > > > > @@ -1850,6 +1850,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) {
> > > > > @@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device,
> > > > > return;
> > > > > }
> > > > >
> > > > > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> > > > > + return;
> > > > > + }
> > > > > +
> > > >
> > > > Is the blocker intended to operate at the device level, i.e. to mark a
> > > > whole chain as 'blocked' for one or more operations? Or, is it
> > > > intended to block at the singular BDS level (the commit message in
> > > > patch 2 implies this meaning)?
> > >
> > > Good question! It should be per BDS, that's why we need backing_blocker.
> > >
> > > Fam
> > >
> > > >
> > > > More to the point: if a BDS is marked as blocked, does that also imply
> > > > all of the images in its backing chain are also considered blocked?
> > >
> > > No.
> >
> > Then why don't we check the blocker values for overlay_bs, top_bs,
> > base_bs, and intermediate images in qmp_block_commit()? This patch
> > only checks the active bs blocker.. yet in some commits, the active
> > layer BDS may not actually be affected at all.
> >
> > >
> > > > Conversely, if a BDS is *not* marked as blocked, does that mean all of
> > > > its backing chain is also unblocked?
> > >
> > > No. But all the backing_hd is blocked by backing_blocker, so we are safe.
> > >
> > > With node-name introduced, some qmp operations are accessible on a BDS in the
> > > middle of a chain, with node-name argument. E.g. @BlockdevSnapshot (Hmm, why
> > > would blockdev-snapshot operate on node-name, when it's called blockdev-*?).
> > >
> >
> > Thanks - and that is exactly what prompted my question; with
> > node-name, we don't necessarily start at the top of the chain, and we
> > can (and will) reference BDSs individually.
> >
> >
> > > So the question is, what happens if user tries to take some operation on mid,
> > > with the node-name:
> > >
> > > base <-- mid <-- active
> > >
> > > With this series, we are safe because mid is protected by the backing_blocker
> > > of active, which blocks all the operations on mid, except as commit target and
> > > backup source.
> > >
> >
> > Right, but that commit exception disproves the rule of 'blockers work
> > on the BDS level'.
> >
> > It means intermediate images (anything below active) will not be
> > blocked for commit. And this ends up working OK (I think even with my
> > block-commit node-name patches), because we still use the 'device' to
> > lookup the active BDS, and that one BDS will be blocked and we catch
> > that in this patch. But that may not always be the case, particular
> > since not all block-commits even involve the active layer.
> >
> > And this means that we are back to the first part - the active BDS
> > blocker is effectively for the chain, not the BDS. We rely on the
> > fact that the active layer BDS will be blocked, to make up for the
> > fact that the rest of the chain is not blocked for commit.
>
> Right. It is not practical to apply the semantics of op blocker on a whole
> chain, because we need different permissions on active and its backing, and ...
>
> >
> > So to be safe, and to use the blocker as individual BDS blockers,
> > shouldn't we do a bdrv_op_block_all() in block_job_create()
> > for every BDS in a chain affected by the block job, and then clear our
> > exception whitelist for those same BDSs (if they still exist!) when
> > the block job is complete (or on failure to start the job)? That way,
> > we are not relying on the active layer blocker acting as a proxy
> > blocker for the entire chain.
>
> ... backing_blocker is introduced to save the effort to add blockers
> recursively. I think it means we should distinguish COMMIT_SOURCE and
> COMMIT_TARGET, as in block-backup, and only allow COMMIT_TARGET on backing_hd.
>
> Could this make us safe? What does it mean to your node-name commit changes?
>
It is still safe with my node-name commit (and stream in v2) series,
because they still reference the chain via 'device', so we can check
the blocker there.
I'm not sure the ultimate solution is to break commit into
COMMIT_TARGET and COMMIT_SOURCE, at least not yet. Commit will stay
safe for a while yet without it, and I think the ultimate solution is
to somehow either have the blocker truly operate on the individual BDS
level, or provide a chain-wide blocker that exists beyond just the
active layer (which doesn't seem unreasonable for certain operations).
Until that happens, what we need to do is make sure that any block job
operation continues to reference the 'device' and top-level BDS, since
we are in this hybrid mode of operation.
>
> >
> >
> > > The idea is, we firstly block any operation in the middle of the chain with
> > > backing_blocker, and we relax those that we think is safe and demanded. This
> > > mechanism also protects the node-name based operations.
> > >
> >
> > I worry that as-is, what this really means is that the blockers on the
> > intermediate images don't really do much. Because any operation that
> > will actually affect an intermediate image will need similar treatment
> > as commit to relax the rules... so the actual operations you want to
> > block all end up with exceptions to not be blocked. But I think if we
> > do what I outlined above, that could negate my fear.
> >
> > >
> > > >
> > > > If the answer to the two questions above is 'yes', then the
> > > > bdrv_op_block/unblock functions should probably operate recursively
> > > > down the chain to the bottom-most backing file.
> > > >
> > > > If the answer is 'no', then for some operations like stream and commit
> > > > (and probably others), don't we also need to worry about the blocker
> > > > state of a lot more images in the chain?
> > > >
> > > >
> > > > > /* default top_bs is the active layer */
> > > > > top_bs = bs;
> > > > >
> > > > > diff --git a/blockjob.c b/blockjob.c
> > > > > index 60e72f5..7d84ca1 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.9.2
> > > > >
> >
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState Fam Zheng
2014-05-21 14:03 ` Stefan Hajnoczi
@ 2014-05-21 14:06 ` Stefan Hajnoczi
2014-05-21 14:49 ` Markus Armbruster
1 sibling, 1 reply; 37+ messages in thread
From: Stefan Hajnoczi @ 2014-05-21 14:06 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, jcody, hbrock, qemu-devel, rjones, imain, pbonzini
On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote:
> This makes use of op_blocker and blocks all the operations except for
> commit target, on each BlockDriverState->backing_hd.
>
> The asserts for op_blocker in bdrv_swap are removed because with this
> change, the target of block commit has at least the backing blocker of
> its child, so the assertion is not true. Callers should do their check.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block.c | 22 ++++++++++++++++++----
> block/mirror.c | 1 +
> include/block/block_int.h | 3 +++
> 3 files changed, 22 insertions(+), 4 deletions(-)
IMO it would be nice to split the series after this patch.
The op_blocker stuff looks solid and the series is at v20. Let's handle
the rest of the changes in separate, smaller series that can be easily
digested.
Stefan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd()
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd() Fam Zheng
2014-05-21 13:56 ` Stefan Hajnoczi
@ 2014-05-21 14:17 ` Jeff Cody
1 sibling, 0 replies; 37+ messages in thread
From: Jeff Cody @ 2014-05-21 14:17 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, hbrock, qemu-devel, rjones, imain, stefanha, pbonzini
On Tue, May 20, 2014 at 02:04:30PM +0800, Fam Zheng wrote:
> 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 | 36 +++++++++++++++++++++++-------------
> include/block/block.h | 1 +
> 2 files changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/block.c b/block.c
> index ef9c1a7..a47118b 100644
> --- a/block.c
> +++ b/block.c
> @@ -1094,6 +1094,21 @@ fail:
> return ret;
> }
>
> +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
> +{
> +
> + bs->backing_hd = backing_hd;
> + if (!backing_hd) {
> + goto out;
> + }
> + bs->open_flags &= ~BDRV_O_NO_BACKING;
> + pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
> + pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> + backing_hd->drv ? backing_hd->drv->format_name : "");
> +out:
> + bdrv_refresh_limits(bs);
> +}
> +
> /*
> * Opens the backing file for a BlockDriverState if not yet open
> *
> @@ -1107,6 +1122,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> char *backing_filename = g_malloc0(PATH_MAX);
> int ret = 0;
> BlockDriver *back_drv = NULL;
> + BlockDriverState *backing_hd;
> Error *local_err = NULL;
>
> if (bs->backing_hd != NULL) {
> @@ -1129,27 +1145,26 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
> }
>
> + backing_hd = bdrv_new("", errp);
> +
> if (bs->backing_format[0] != '\0') {
> back_drv = bdrv_find_format(bs->backing_format);
> }
>
> assert(bs->backing_hd == NULL);
> - ret = bdrv_open(&bs->backing_hd,
> + ret = bdrv_open(&backing_hd,
> *backing_filename ? backing_filename : NULL, NULL, options,
> bdrv_backing_flags(bs->open_flags), back_drv, &local_err);
> if (ret < 0) {
> - bs->backing_hd = NULL;
> + bdrv_unref(backing_hd);
> + backing_hd = NULL;
> 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);
> goto free_exit;
> }
> -
> - if (bs->backing_hd->file) {
> - pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> - bs->backing_hd->file->filename);
> - }
> + bdrv_set_backing_hd(bs, backing_hd);
>
> /* Recalculate the BlockLimits with the backing file */
> bdrv_refresh_limits(bs);
> @@ -2002,12 +2017,7 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>
> /* The contents of 'tmp' will become bs_top, as we are
> * swapping bs_new and bs_top contents. */
> - bs_top->backing_hd = bs_new;
> - bs_top->open_flags &= ~BDRV_O_NO_BACKING;
> - pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file),
> - bs_new->filename);
> - pstrcpy(bs_top->backing_format, sizeof(bs_top->backing_format),
> - bs_new->drv ? bs_new->drv->format_name : "");
> + bdrv_set_backing_hd(bs_top, bs_new);
> }
>
> static void bdrv_delete(BlockDriverState *bs)
> diff --git a/include/block/block.h b/include/block/block.h
> index 60e79a8..032828a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -213,6 +213,7 @@ int bdrv_parse_discard_flags(const char *mode, int *flags);
> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
> bool allow_none, Error **errp);
> +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
> int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
> int bdrv_open(BlockDriverState **pbs, const char *filename,
> --
> 1.9.2
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState
2014-05-21 14:03 ` Stefan Hajnoczi
@ 2014-05-21 14:24 ` Jeff Cody
2014-05-21 14:37 ` Fam Zheng
0 siblings, 1 reply; 37+ messages in thread
From: Jeff Cody @ 2014-05-21 14:24 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, Fam Zheng, armbru, hbrock, rjones, qemu-devel, imain, pbonzini
On Wed, May 21, 2014 at 04:03:03PM +0200, Stefan Hajnoczi wrote:
> On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote:
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 1c38aa8..6a53d79 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -499,6 +499,7 @@ immediate_exit:
> > * trigger the unref from the top one */
> > BlockDriverState *p = s->base->backing_hd;
> > s->base->backing_hd = NULL;
> > + bdrv_op_unblock_all(p, s->base->backing_blocker);
> > bdrv_unref(p);
> > }
> > }
>
> Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead
> of open coding it.
>
Patch 10 gets rid of essentially this whole chunk of code, and
replaces it with bdrv_drop_intermediate(). So it does get cleaned up,
just later in the series.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState
2014-05-21 14:24 ` Jeff Cody
@ 2014-05-21 14:37 ` Fam Zheng
2014-05-22 16:57 ` Jeff Cody
0 siblings, 1 reply; 37+ messages in thread
From: Fam Zheng @ 2014-05-21 14:37 UTC (permalink / raw)
To: Jeff Cody
Cc: kwolf, armbru, hbrock, qemu-devel, rjones, imain,
Stefan Hajnoczi, pbonzini
On Wed, 05/21 10:24, Jeff Cody wrote:
> On Wed, May 21, 2014 at 04:03:03PM +0200, Stefan Hajnoczi wrote:
> > On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote:
> > > diff --git a/block/mirror.c b/block/mirror.c
> > > index 1c38aa8..6a53d79 100644
> > > --- a/block/mirror.c
> > > +++ b/block/mirror.c
> > > @@ -499,6 +499,7 @@ immediate_exit:
> > > * trigger the unref from the top one */
> > > BlockDriverState *p = s->base->backing_hd;
> > > s->base->backing_hd = NULL;
> > > + bdrv_op_unblock_all(p, s->base->backing_blocker);
> > > bdrv_unref(p);
> > > }
> > > }
> >
> > Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead
> > of open coding it.
> >
>
> Patch 10 gets rid of essentially this whole chunk of code, and
> replaces it with bdrv_drop_intermediate(). So it does get cleaned up,
> just later in the series.
Thanks for pointing out, Jeff.
Stefan, if there are other reasons to respin, I'll take your suggestion and
update this, and then split the series, so you can merge 1-6.
Fam
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState
2014-05-21 14:06 ` Stefan Hajnoczi
@ 2014-05-21 14:49 ` Markus Armbruster
0 siblings, 0 replies; 37+ messages in thread
From: Markus Armbruster @ 2014-05-21 14:49 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, Fam Zheng, jcody, hbrock, qemu-devel, rjones, imain, pbonzini
Stefan Hajnoczi <stefanha@redhat.com> writes:
> On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote:
>> This makes use of op_blocker and blocks all the operations except for
>> commit target, on each BlockDriverState->backing_hd.
>>
>> The asserts for op_blocker in bdrv_swap are removed because with this
>> change, the target of block commit has at least the backing blocker of
>> its child, so the assertion is not true. Callers should do their check.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>> block.c | 22 ++++++++++++++++++----
>> block/mirror.c | 1 +
>> include/block/block_int.h | 3 +++
>> 3 files changed, 22 insertions(+), 4 deletions(-)
>
> IMO it would be nice to split the series after this patch.
>
> The op_blocker stuff looks solid and the series is at v20. Let's handle
> the rest of the changes in separate, smaller series that can be easily
> digested.
Yes, please!
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState
2014-05-21 14:37 ` Fam Zheng
@ 2014-05-22 16:57 ` Jeff Cody
0 siblings, 0 replies; 37+ messages in thread
From: Jeff Cody @ 2014-05-22 16:57 UTC (permalink / raw)
To: Fam Zheng
Cc: kwolf, armbru, hbrock, qemu-devel, rjones, imain,
Stefan Hajnoczi, pbonzini
On Wed, May 21, 2014 at 10:37:50PM +0800, Fam Zheng wrote:
> On Wed, 05/21 10:24, Jeff Cody wrote:
> > On Wed, May 21, 2014 at 04:03:03PM +0200, Stefan Hajnoczi wrote:
> > > On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote:
> > > > diff --git a/block/mirror.c b/block/mirror.c
> > > > index 1c38aa8..6a53d79 100644
> > > > --- a/block/mirror.c
> > > > +++ b/block/mirror.c
> > > > @@ -499,6 +499,7 @@ immediate_exit:
> > > > * trigger the unref from the top one */
> > > > BlockDriverState *p = s->base->backing_hd;
> > > > s->base->backing_hd = NULL;
> > > > + bdrv_op_unblock_all(p, s->base->backing_blocker);
> > > > bdrv_unref(p);
> > > > }
> > > > }
> > >
> > > Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead
> > > of open coding it.
> > >
> >
> > Patch 10 gets rid of essentially this whole chunk of code, and
> > replaces it with bdrv_drop_intermediate(). So it does get cleaned up,
> > just later in the series.
>
> Thanks for pointing out, Jeff.
>
> Stefan, if there are other reasons to respin, I'll take your suggestion and
> update this, and then split the series, so you can merge 1-6.
>
Unfortunately, it will need a respin. I've been doing some more
testing, and with just patches 1-6, block-commit asserts.
The culprits are the open coded backing_hd assignments, like above, in
bdrv_drop_intermediate() (there are two, and either will cause an
assert):
ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
base_bs->drv ? base_bs->drv->format_name : "");
if (ret) {
goto exit;
}
>> new_top_bs->backing_hd = base_bs;
And:
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);
^^^^^^^^^^^
This will assert
Without the call to bdrv_set_backing_hd(), the backing_blocker does
not get cleared, and this asserts in bdrv_delete():
assert(bdrv_op_blocker_is_empty(bs));
So all instances of open coded assignment of backing_hd should be
replaced, either in this patch, or (preferably) in a new patch after
patch 5.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (14 preceding siblings ...)
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 15/15] qemu-iotests: Image fleecing test case 089 Fam Zheng
@ 2018-06-29 18:00 ` Eric Blake
2018-06-29 18:34 ` John Snow
15 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2018-06-29 18:00 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: kwolf, pbonzini, rjones, imain, stefanha, hbrock, jcody, armbru,
John Snow, Vladimir Sementsov-Ogievskiy
On 05/20/2014 01:04 AM, Fam Zheng wrote:
> Thanks for Eric's and Markus' reviewing!
Thread necromancy - now that patch 15/15 has had a revival four years
later (two different threads proposing a test 222 for image fleecing) -
I want to double check if anything else in this series still needs
forward porting.
>
> v20: Rebase to master, with minor improvements as listed below:
>
> [02/15] block: Introduce op_blockers to BlockDriverState
> Better error message in bdrv_op_is_blocked. (Markus)
>
> [03/15] block: Replace in_use with operation blocker
> Accurate commit message: why use bdrv_op_blocker_is_empty in
> block_job_create. (Markus)
>
> [06/15] block: Add backing_blocker in BlockDriverState
> Drop error_is_set().
> Call error_free() unconditionally.
>
>
> [08/15] block: Support dropping active in bdrv_drop_intermediate
> Fix list index in function comment. (Eric)
>
> [13/15] block: Add blockdev-backup to transaction
> Drop error_is_set().
>
> [14/15] qemu-iotests: Test blockdev-backup in 055
> Bump year in file header. (Eric)
>
> Fam Zheng (15):
> block: Add BlockOpType 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
Before this point looks to be in master (in some form or another); after
this point did not have any review.
> 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
> commit: Use bdrv_drop_intermediate
> qmp: Add command 'blockdev-backup'
> block: Allow backup on referenced named BlockDriverState
> block: Add blockdev-backup to transaction
> qemu-iotests: Test blockdev-backup in 055
> qemu-iotests: Image fleecing test case 089
>
> block-migration.c | 7 +-
> block.c | 310 +++++++++++++++++++++++++++-------------
> block/backup.c | 26 ++++
> block/commit.c | 2 +-
> block/mirror.c | 9 +-
> block/stream.c | 42 +-----
> blockdev.c | 122 ++++++++++++++--
> blockjob.c | 14 +-
> hw/block/dataplane/virtio-blk.c | 18 ++-
> include/block/block.h | 29 +++-
> include/block/block_int.h | 9 +-
> include/block/blockjob.h | 3 +
> qapi-schema.json | 52 +++++++
> qmp-commands.hx | 44 ++++++
> tests/qemu-iotests/055 | 277 +++++++++++++++++++++++++++++------
> tests/qemu-iotests/055.out | 4 +-
> tests/qemu-iotests/089 | 99 +++++++++++++
> tests/qemu-iotests/089.out | 5 +
> tests/qemu-iotests/group | 1 +
> 19 files changed, 856 insertions(+), 217 deletions(-)
> create mode 100755 tests/qemu-iotests/089
> create mode 100644 tests/qemu-iotests/089.out
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD
2018-06-29 18:00 ` [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Eric Blake
@ 2018-06-29 18:34 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-06-29 18:34 UTC (permalink / raw)
To: Eric Blake, Fam Zheng, qemu-devel
Cc: kwolf, pbonzini, rjones, imain, stefanha, hbrock, jcody, armbru,
Vladimir Sementsov-Ogievskiy
On 06/29/2018 02:00 PM, Eric Blake wrote:
> On 05/20/2014 01:04 AM, Fam Zheng wrote:
>> Thanks for Eric's and Markus' reviewing!
>
> Thread necromancy - now that patch 15/15 has had a revival four years
> later (two different threads proposing a test 222 for image fleecing) -
> I want to double check if anything else in this series still needs
> forward porting.
>
This is pretty intensely old...
>>
>> v20: Rebase to master, with minor improvements as listed below:
>>
>> [02/15] block: Introduce op_blockers to BlockDriverState
>> Better error message in bdrv_op_is_blocked. (Markus)
>>
>> [03/15] block: Replace in_use with operation blocker
>> Accurate commit message: why use bdrv_op_blocker_is_empty in
>> block_job_create. (Markus)
>>
>> [06/15] block: Add backing_blocker in BlockDriverState
>> Drop error_is_set().
>> Call error_free() unconditionally.
>>
>> [08/15] block: Support dropping active in bdrv_drop_intermediate
>> Fix list index in function comment. (Eric)
>>
>> [13/15] block: Add blockdev-backup to transaction
>> Drop error_is_set().
>>
>> [14/15] qemu-iotests: Test blockdev-backup in 055
>> Bump year in file header. (Eric)
>>
>
>> Fam Zheng (15):
>> block: Add BlockOpType 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
>
> Before this point looks to be in master (in some form or another); after
> this point did not have any review.
>
>> block: Parse "backing" option to reference existing BDS
Well, this obviously currently works.
>> block: Support dropping active in bdrv_drop_intermediate
>> stream: Use bdrv_drop_intermediate and drop close_unused_images
>> commit: Use bdrv_drop_intermediate
Not sure about these three.
>> qmp: Add command 'blockdev-backup'
We do have this one.
>> block: Allow backup on referenced named BlockDriverState
With patch 1/2 or 1/3 from either Vlad or myself, we have this, too.
>> block: Add blockdev-backup to transaction
We already have this.
>> qemu-iotests: Test blockdev-backup in 055
>> qemu-iotests: Image fleecing test case 089
These never got added, but we're working on that now.
>>
>> block-migration.c | 7 +-
>> block.c | 310
>> +++++++++++++++++++++++++++-------------
>> block/backup.c | 26 ++++
>> block/commit.c | 2 +-
>> block/mirror.c | 9 +-
>> block/stream.c | 42 +-----
>> blockdev.c | 122 ++++++++++++++--
>> blockjob.c | 14 +-
>> hw/block/dataplane/virtio-blk.c | 18 ++-
>> include/block/block.h | 29 +++-
>> include/block/block_int.h | 9 +-
>> include/block/blockjob.h | 3 +
>> qapi-schema.json | 52 +++++++
>> qmp-commands.hx | 44 ++++++
>> tests/qemu-iotests/055 | 277
>> +++++++++++++++++++++++++++++------
>> tests/qemu-iotests/055.out | 4 +-
>> tests/qemu-iotests/089 | 99 +++++++++++++
>> tests/qemu-iotests/089.out | 5 +
>> tests/qemu-iotests/group | 1 +
>> 19 files changed, 856 insertions(+), 217 deletions(-)
>> create mode 100755 tests/qemu-iotests/089
>> create mode 100644 tests/qemu-iotests/089.out
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2018-06-29 18:35 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-20 6:04 [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 01/15] block: Add BlockOpType enum Fam Zheng
2014-05-21 12:28 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 02/15] block: Introduce op_blockers to BlockDriverState Fam Zheng
2014-05-21 12:28 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 03/15] block: Replace in_use with operation blocker Fam Zheng
2014-05-21 12:46 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller Fam Zheng
2014-05-20 11:43 ` Jeff Cody
2014-05-21 1:36 ` Fam Zheng
2014-05-21 4:34 ` Jeff Cody
2014-05-21 6:08 ` Fam Zheng
2014-05-21 13:13 ` Stefan Hajnoczi
2014-05-21 13:17 ` Jeff Cody
2014-05-21 14:04 ` Jeff Cody
2014-05-21 13:20 ` Stefan Hajnoczi
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd() Fam Zheng
2014-05-21 13:56 ` Stefan Hajnoczi
2014-05-21 14:17 ` Jeff Cody
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState Fam Zheng
2014-05-21 14:03 ` Stefan Hajnoczi
2014-05-21 14:24 ` Jeff Cody
2014-05-21 14:37 ` Fam Zheng
2014-05-22 16:57 ` Jeff Cody
2014-05-21 14:06 ` Stefan Hajnoczi
2014-05-21 14:49 ` Markus Armbruster
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 07/15] block: Parse "backing" option to reference existing BDS Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 08/15] block: Support dropping active in bdrv_drop_intermediate Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 09/15] stream: Use bdrv_drop_intermediate and drop close_unused_images Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 10/15] commit: Use bdrv_drop_intermediate Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 11/15] qmp: Add command 'blockdev-backup' Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 12/15] block: Allow backup on referenced named BlockDriverState Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 13/15] block: Add blockdev-backup to transaction Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 14/15] qemu-iotests: Test blockdev-backup in 055 Fam Zheng
2014-05-20 6:04 ` [Qemu-devel] [PATCH v20 15/15] qemu-iotests: Image fleecing test case 089 Fam Zheng
2018-06-29 18:00 ` [Qemu-devel] [PATCH v20 00/15] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Eric Blake
2018-06-29 18:34 ` John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.