* [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 12:21 ` Kevin Wolf
2013-12-12 12:49 ` Markus Armbruster
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 02/10] block: Introduce op_blockers to BlockDriverState Fam Zheng
` (9 subsequent siblings)
10 siblings, 2 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, 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>
---
qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/qapi-schema.json b/qapi-schema.json
index d6f8615..ef4d6af 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1440,6 +1440,55 @@
'data': ['commit', 'stream', 'mirror', 'backup'] }
##
+# @BlockOperationType
+# Type of a block operation. (since 1.8)
+#
+# @backup-source: As a backup source. See the 'drive-backup' command.
+#
+# @backup-target: As a backup target. See the 'drive-backup' command.
+#
+# @change: See the 'change' command.
+#
+# @commit: See the 'block-commit' command.
+#
+# @dataplane: The virtio-blk dataplane feature.
+#
+# @drive-del: See the 'drive_del' HMP command.
+#
+# @eject: See the 'eject' command.
+#
+# @external-snapshot: See the 'blockdev-snapshot-sync' command.
+#
+# @internal-snapshot: See the 'blockdev-snapshot-internal-sync' command.
+#
+# @internal-snapshot-delete: See the 'blockdev-snapshot-delete-internal-sync' command.
+#
+# @mirror: See the 'drive-mirror' command.
+#
+# @resize: See the 'block-resize' command.
+#
+# @stream: See the 'block-stream' command.
+#
+# Since: 1.8
+##
+{ 'enum': 'BlockOpType',
+ 'data': [
+ 'backup-source',
+ 'backup-target',
+ 'change',
+ 'commit',
+ 'dataplane',
+ 'drive-del',
+ 'eject',
+ 'external-snapshot',
+ 'internal-snapshot',
+ 'internal-snapshot-delete',
+ 'mirror',
+ 'resize',
+ 'stream'
+] }
+
+##
# @BlockJobInfo:
#
# Information about a long-running block device operation.
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum Fam Zheng
@ 2013-12-12 12:21 ` Kevin Wolf
2013-12-12 12:32 ` Fam Zheng
2013-12-12 12:49 ` Markus Armbruster
1 sibling, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2013-12-12 12:21 UTC (permalink / raw)
To: Fam Zheng; +Cc: armbru, qemu-devel, rjones, imain, stefanha, pbonzini
Am 12.12.2013 um 09:23 hat Fam Zheng geschrieben:
> This adds the enum of all the operations that can be taken on a block
> device.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d6f8615..ef4d6af 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1440,6 +1440,55 @@
> 'data': ['commit', 'stream', 'mirror', 'backup'] }
>
> ##
> +# @BlockOperationType
> +# Type of a block operation. (since 1.8)
I don't think this description is accurate any more. You now have a list
of roles that a block device can take, or something like that. Not sure
what the best way to put it is. backup-source isn't an operation anyway,
nor is dataplane.
Also, should have an empty line after @BlockOperationType and 2.0
instead of 1.8.
> +# @backup-source: As a backup source. See the 'drive-backup' command.
> +#
> +# @backup-target: As a backup target. See the 'drive-backup' command.
> +#
> +# @change: See the 'change' command.
> +#
> +# @commit: See the 'block-commit' command.
> +#
> +# @dataplane: The virtio-blk dataplane feature.
> +#
> +# @drive-del: See the 'drive_del' HMP command.
> +#
> +# @eject: See the 'eject' command.
> +#
> +# @external-snapshot: See the 'blockdev-snapshot-sync' command.
> +#
> +# @internal-snapshot: See the 'blockdev-snapshot-internal-sync' command.
> +#
> +# @internal-snapshot-delete: See the 'blockdev-snapshot-delete-internal-sync' command.
> +#
> +# @mirror: See the 'drive-mirror' command.
> +#
> +# @resize: See the 'block-resize' command.
> +#
> +# @stream: See the 'block-stream' command.
> +#
> +# Since: 1.8
Here as well.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum
2013-12-12 12:21 ` Kevin Wolf
@ 2013-12-12 12:32 ` Fam Zheng
0 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 12:32 UTC (permalink / raw)
To: Kevin Wolf, eblake; +Cc: rjones, armbru, qemu-devel, imain, stefanha, pbonzini
On 2013年12月12日 20:21, Kevin Wolf wrote:
> Am 12.12.2013 um 09:23 hat Fam Zheng geschrieben:
>> This adds the enum of all the operations that can be taken on a block
>> device.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>> qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index d6f8615..ef4d6af 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1440,6 +1440,55 @@
>> 'data': ['commit', 'stream', 'mirror', 'backup'] }
>>
>> ##
>> +# @BlockOperationType
>> +# Type of a block operation. (since 1.8)
>
> I don't think this description is accurate any more. You now have a list
> of roles that a block device can take, or something like that. Not sure
> what the best way to put it is. backup-source isn't an operation anyway,
> nor is dataplane.
"Using this as backup source" or "start dataplane on that" still sounds
an operation to me. It's just the 1:1 corresponding with QMP commands
that is loosed, but not totally independent, as the long comments below
show.
>
> Also, should have an empty line after @BlockOperationType and 2.0
> instead of 1.8.
>
OK, will fix. Thanks.
Fam
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum Fam Zheng
2013-12-12 12:21 ` Kevin Wolf
@ 2013-12-12 12:49 ` Markus Armbruster
2013-12-12 12:53 ` Fam Zheng
1 sibling, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2013-12-12 12:49 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
Fam Zheng <famz@redhat.com> writes:
> This adds the enum of all the operations that can be taken on a block
> device.
All operations? All blockable operations? Something else?
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d6f8615..ef4d6af 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1440,6 +1440,55 @@
> 'data': ['commit', 'stream', 'mirror', 'backup'] }
>
> ##
> +# @BlockOperationType
> +# Type of a block operation. (since 1.8)
(since 2.0)
> +#
> +# @backup-source: As a backup source. See the 'drive-backup' command.
> +#
> +# @backup-target: As a backup target. See the 'drive-backup' command.
> +#
> +# @change: See the 'change' command.
> +#
> +# @commit: See the 'block-commit' command.
> +#
> +# @dataplane: The virtio-blk dataplane feature.
> +#
> +# @drive-del: See the 'drive_del' HMP command.
> +#
> +# @eject: See the 'eject' command.
> +#
> +# @external-snapshot: See the 'blockdev-snapshot-sync' command.
> +#
> +# @internal-snapshot: See the 'blockdev-snapshot-internal-sync' command.
> +#
> +# @internal-snapshot-delete: See the 'blockdev-snapshot-delete-internal-sync' command.
> +#
> +# @mirror: See the 'drive-mirror' command.
> +#
> +# @resize: See the 'block-resize' command.
> +#
> +# @stream: See the 'block-stream' command.
> +#
> +# Since: 1.8
Likewise.
> +##
> +{ 'enum': 'BlockOpType',
> + 'data': [
> + 'backup-source',
> + 'backup-target',
> + 'change',
> + 'commit',
> + 'dataplane',
> + 'drive-del',
> + 'eject',
> + 'external-snapshot',
> + 'internal-snapshot',
> + 'internal-snapshot-delete',
> + 'mirror',
> + 'resize',
> + 'stream'
> +] }
> +
> +##
> # @BlockJobInfo:
> #
> # Information about a long-running block device operation.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum
2013-12-12 12:49 ` Markus Armbruster
@ 2013-12-12 12:53 ` Fam Zheng
0 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 12:53 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
On 2013年12月12日 20:49, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
>
>> This adds the enum of all the operations that can be taken on a block
>> device.
>
> All operations? All blockable operations? Something else?
>
All QMP operations those apply on device, plus all operations those
"in_use" is protecting, at least. I will reconsider this enum or update
the comment.
Fam
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v7 02/10] block: Introduce op_blockers to BlockDriverState
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 12:50 ` Markus Armbruster
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS Fam Zheng
` (8 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, 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 bdrv_set_in_use of now).
* A blocker is only referenced by op_blockers, so the lifecycle is
managed by caller, and shouldn't be lost until unblock, so typically
a caller does these:
- Allocate a blocker with error_setg or similar, call bdrv_op_block()
to block some operations.
- Hold the blocker, do his job.
- Unblock operations that it blocked, with the same reason pointer
passed to bdrv_op_unblock().
- Release the blocker with error_free().
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 6 +++++
include/block/block_int.h | 5 +++++
3 files changed, 68 insertions(+)
diff --git a/block.c b/block.c
index 13f001a..c1a3576 100644
--- a/block.c
+++ b/block.c
@@ -4629,6 +4629,63 @@ void bdrv_unref(BlockDriverState *bs)
}
}
+struct BdrvOpBlocker {
+ Error *reason;
+ QLIST_ENTRY(BdrvOpBlocker) list;
+};
+
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
+{
+ BdrvOpBlocker *blocker;
+ assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+ if (!QLIST_EMPTY(&bs->op_blockers[op])) {
+ blocker = QLIST_FIRST(&bs->op_blockers[op]);
+ if (errp) {
+ *errp = error_copy(blocker->reason);
+ }
+ return true;
+ }
+ return false;
+}
+
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+ BdrvOpBlocker *blocker;
+ assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+
+ blocker = g_malloc0(sizeof(BdrvOpBlocker));
+ blocker->reason = reason;
+ QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
+}
+
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+ BdrvOpBlocker *blocker, *next;
+ assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+ QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
+ if (blocker->reason == reason) {
+ QLIST_REMOVE(blocker, list);
+ g_free(blocker);
+ }
+ }
+}
+
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason)
+{
+ int i;
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ bdrv_op_block(bs, i, reason);
+ }
+}
+
+void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
+{
+ int i;
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ bdrv_op_unblock(bs, i, reason);
+ }
+}
+
void bdrv_set_in_use(BlockDriverState *bs, int in_use)
{
assert(bs->in_use != in_use);
diff --git a/include/block/block.h b/include/block/block.h
index 36efaea..18397e0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -442,6 +442,12 @@ 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);
+
#ifdef CONFIG_LINUX_AIO
int raw_get_aio_fd(BlockDriverState *bs);
#else
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8b132d7..458acd6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -252,6 +252,8 @@ typedef struct BlockLimits {
int opt_transfer_length;
} BlockLimits;
+typedef struct BdrvOpBlocker BdrvOpBlocker;
+
/*
* Note: the function bdrv_append() copies and swaps contents of
* BlockDriverStates, so if you add new fields to this struct, please
@@ -333,6 +335,9 @@ struct BlockDriverState {
QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
+ /* operation blockers */
+ QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX];
+
/* long-running background operation */
BlockJob *job;
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 02/10] block: Introduce op_blockers to BlockDriverState
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 02/10] block: Introduce op_blockers to BlockDriverState Fam Zheng
@ 2013-12-12 12:50 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2013-12-12 12:50 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
Fam Zheng <famz@redhat.com> writes:
> 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 bdrv_set_in_use of now).
Suggest "is similar to the existing bdrv_set_in_use()"
>
> * A blocker is only referenced by op_blockers, so the lifecycle is
> managed by caller, and shouldn't be lost until unblock, so typically
> a caller does these:
>
> - Allocate a blocker with error_setg or similar, call bdrv_op_block()
> to block some operations.
> - Hold the blocker, do his job.
> - Unblock operations that it blocked, with the same reason pointer
> passed to bdrv_op_unblock().
> - Release the blocker with error_free().
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 6 +++++
> include/block/block_int.h | 5 +++++
> 3 files changed, 68 insertions(+)
>
> diff --git a/block.c b/block.c
> index 13f001a..c1a3576 100644
> --- a/block.c
> +++ b/block.c
> @@ -4629,6 +4629,63 @@ void bdrv_unref(BlockDriverState *bs)
> }
> }
>
> +struct BdrvOpBlocker {
> + Error *reason;
> + QLIST_ENTRY(BdrvOpBlocker) list;
> +};
> +
> +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
> +{
> + BdrvOpBlocker *blocker;
> + assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
> + if (!QLIST_EMPTY(&bs->op_blockers[op])) {
> + blocker = QLIST_FIRST(&bs->op_blockers[op]);
> + if (errp) {
> + *errp = error_copy(blocker->reason);
> + }
> + return true;
> + }
> + return false;
> +}
Your interface permits registering multiple blockers, but when you're
blocked, you can only find a single blocker (the most recent one, but
that's implementation detail).
If a need for finding all of them should arise, we'll do it. No change
necessary now.
[...]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 01/10] qapi: Add BlockOperationType enum Fam Zheng
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 02/10] block: Introduce op_blockers to BlockDriverState Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 12:52 ` Markus Armbruster
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate Fam Zheng
` (7 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rjones, armbru, imain, stefanha, pbonzini
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block.c | 109 +++++++++++++++++++++++++++++-----------------
block/mirror.c | 2 +-
include/block/block.h | 3 +-
include/block/block_int.h | 3 ++
4 files changed, 76 insertions(+), 41 deletions(-)
diff --git a/block.c b/block.c
index c1a3576..41562fd 100644
--- a/block.c
+++ b/block.c
@@ -961,63 +961,87 @@ fail:
/*
* Opens the backing file for a BlockDriverState if not yet open
*
+ * If backing_bs is not NULL or empty, find the BDS by name and reference it as
+ * the backing_hd, in this case options is ignored.
+ *
* options is a QDict of options to pass to the block drivers, or NULL for an
* empty set of options. The reference to the QDict is transferred to this
* function (even on failure), so if the caller intends to reuse the dictionary,
* it needs to use QINCREF() before calling bdrv_file_open.
*/
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
+int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
+ QDict *options, Error **errp)
{
char backing_filename[PATH_MAX];
int back_flags, ret;
BlockDriver *back_drv = NULL;
Error *local_err = NULL;
- if (bs->backing_hd != NULL) {
- QDECREF(options);
- return 0;
- }
+ if (backing_bs && *backing_bs != '\0') {
+ bs->backing_hd = bdrv_find(backing_bs);
+ if (!bs->backing_hd) {
+ error_setg(errp, "Backing device not found: %s", backing_bs);
+ return -ENOENT;
+ }
+ bdrv_ref(bs->backing_hd);
+ } else {
+ if (bs->backing_hd != NULL) {
+ QDECREF(options);
+ return 0;
+ }
- /* NULL means an empty set of options */
- if (options == NULL) {
- options = qdict_new();
- }
+ /* NULL means an empty set of options */
+ if (options == NULL) {
+ options = qdict_new();
+ }
- bs->open_flags &= ~BDRV_O_NO_BACKING;
- if (qdict_haskey(options, "file.filename")) {
- backing_filename[0] = '\0';
- } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
- QDECREF(options);
- return 0;
- } else {
- bdrv_get_full_backing_filename(bs, backing_filename,
- sizeof(backing_filename));
- }
+ bs->open_flags &= ~BDRV_O_NO_BACKING;
+ if (qdict_haskey(options, "file.filename")) {
+ backing_filename[0] = '\0';
+ } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
+ QDECREF(options);
+ return 0;
+ } else {
+ bdrv_get_full_backing_filename(bs, backing_filename,
+ sizeof(backing_filename));
+ }
- bs->backing_hd = bdrv_new("");
+ bs->backing_hd = bdrv_new("");
- if (bs->backing_format[0] != '\0') {
- back_drv = bdrv_find_format(bs->backing_format);
- }
+ if (bs->backing_format[0] != '\0') {
+ back_drv = bdrv_find_format(bs->backing_format);
+ }
- /* backing files always opened read-only */
- back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
- BDRV_O_COPY_ON_READ);
+ /* backing files always opened read-only */
+ back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
+ BDRV_O_COPY_ON_READ);
- ret = bdrv_open(bs->backing_hd,
- *backing_filename ? backing_filename : NULL, options,
- back_flags, back_drv, &local_err);
- if (ret < 0) {
- bdrv_unref(bs->backing_hd);
- bs->backing_hd = NULL;
- bs->open_flags |= BDRV_O_NO_BACKING;
- error_setg(errp, "Could not open backing file: %s",
- error_get_pretty(local_err));
- error_free(local_err);
- return ret;
+ ret = bdrv_open(bs->backing_hd,
+ *backing_filename ? backing_filename : NULL, options,
+ back_flags, back_drv, &local_err);
+ if (ret < 0) {
+ bdrv_unref(bs->backing_hd);
+ bs->backing_hd = NULL;
+ bs->open_flags |= BDRV_O_NO_BACKING;
+ error_setg(errp, "Could not open backing file: %s",
+ error_get_pretty(local_err));
+ error_free(local_err);
+ return ret;
+ }
}
+ assert(bs->backing_hd);
+ assert(!bs->backing_blocker);
+ error_setg(&bs->backing_blocker,
+ "device is used as backing hd of '%s'",
+ bs->device_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);
pstrcpy(bs->backing_file, sizeof(bs->backing_file),
bs->backing_hd->file->filename);
+ pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+ bs->backing_hd->drv->format_name);
return 0;
}
@@ -1164,9 +1188,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
/* If there is a backing file, use it */
if ((flags & BDRV_O_NO_BACKING) == 0) {
QDict *backing_options;
-
qdict_extract_subqdict(options, &backing_options, "backing.");
- ret = bdrv_open_backing_file(bs, backing_options, &local_err);
+ ret = bdrv_open_backing_file(bs, qdict_get_try_str(options, "backing"),
+ backing_options, &local_err);
+
+ qdict_del(options, "backing");
if (ret < 0) {
goto close_and_fail;
}
@@ -1459,9 +1485,14 @@ void bdrv_close(BlockDriverState *bs)
if (bs->drv) {
if (bs->backing_hd) {
+ assert(bs->backing_blocker);
+ bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
bdrv_unref(bs->backing_hd);
bs->backing_hd = NULL;
}
+ if (bs->backing_blocker) {
+ error_free(bs->backing_blocker);
+ }
bs->drv->bdrv_close(bs);
g_free(bs->opaque);
#ifdef _WIN32
diff --git a/block/mirror.c b/block/mirror.c
index 6dc27ad..b1596d3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -511,7 +511,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
Error *local_err = NULL;
int ret;
- ret = bdrv_open_backing_file(s->target, NULL, &local_err);
+ ret = bdrv_open_backing_file(s->target, NULL, NULL, &local_err);
if (ret < 0) {
char backing_filename[PATH_MAX];
bdrv_get_full_backing_filename(s->target, backing_filename,
diff --git a/include/block/block.h b/include/block/block.h
index 18397e0..24d06f9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -185,7 +185,8 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
int bdrv_parse_discard_flags(const char *mode, int *flags);
int bdrv_file_open(BlockDriverState **pbs, const char *filename,
QDict *options, int flags, Error **errp);
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
+int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
+ QDict *options, Error **errp);
int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
int flags, BlockDriver *drv, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 458acd6..83dcf7d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -342,6 +342,9 @@ struct BlockDriverState {
BlockJob *job;
QDict *options;
+
+ /* For backing reference, block the operations of named backing device */
+ Error *backing_blocker;
};
int get_tmp_filename(char *filename, int size);
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS Fam Zheng
@ 2013-12-12 12:52 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2013-12-12 12:52 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
Fam Zheng <famz@redhat.com> writes:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block.c | 109 +++++++++++++++++++++++++++++-----------------
> block/mirror.c | 2 +-
> include/block/block.h | 3 +-
> include/block/block_int.h | 3 ++
> 4 files changed, 76 insertions(+), 41 deletions(-)
>
> diff --git a/block.c b/block.c
> index c1a3576..41562fd 100644
> --- a/block.c
> +++ b/block.c
> @@ -961,63 +961,87 @@ fail:
> /*
> * Opens the backing file for a BlockDriverState if not yet open
> *
> + * If backing_bs is not NULL or empty, find the BDS by name and reference it as
> + * the backing_hd, in this case options is ignored.
> + *
The name "backing_bs" suggests this is a BlockDriverState, which it's
not. What about calling it "name", just like bdrv_find()'s parameter?
Or "device_name", like the BlockDriverState member?
> * options is a QDict of options to pass to the block drivers, or NULL for an
> * empty set of options. The reference to the QDict is transferred to this
> * function (even on failure), so if the caller intends to reuse the dictionary,
> * it needs to use QINCREF() before calling bdrv_file_open.
> */
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
> + QDict *options, Error **errp)
This function is now too overloaded for my taste.
Possibly cleaner:
* Have a new function to set backing_hd. Takes a device name argument,
or maybe a BDS. I'm leaning towards the latter.
* The new function and old bdrv_open_backing_file() have a common tail.
Factor it out into a helper.
> {
> char backing_filename[PATH_MAX];
> int back_flags, ret;
> BlockDriver *back_drv = NULL;
> Error *local_err = NULL;
>
> - if (bs->backing_hd != NULL) {
> - QDECREF(options);
> - return 0;
> - }
> + if (backing_bs && *backing_bs != '\0') {
> + bs->backing_hd = bdrv_find(backing_bs);
What if bs->backing_hd is already set?
> + if (!bs->backing_hd) {
> + error_setg(errp, "Backing device not found: %s", backing_bs);
> + return -ENOENT;
> + }
> + bdrv_ref(bs->backing_hd);
> + } else {
> + if (bs->backing_hd != NULL) {
> + QDECREF(options);
> + return 0;
> + }
>
> - /* NULL means an empty set of options */
> - if (options == NULL) {
> - options = qdict_new();
> - }
> + /* NULL means an empty set of options */
> + if (options == NULL) {
> + options = qdict_new();
> + }
>
> - bs->open_flags &= ~BDRV_O_NO_BACKING;
> - if (qdict_haskey(options, "file.filename")) {
> - backing_filename[0] = '\0';
> - } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
> - QDECREF(options);
> - return 0;
> - } else {
> - bdrv_get_full_backing_filename(bs, backing_filename,
> - sizeof(backing_filename));
> - }
> + bs->open_flags &= ~BDRV_O_NO_BACKING;
> + if (qdict_haskey(options, "file.filename")) {
> + backing_filename[0] = '\0';
> + } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
> + QDECREF(options);
> + return 0;
> + } else {
> + bdrv_get_full_backing_filename(bs, backing_filename,
> + sizeof(backing_filename));
> + }
>
> - bs->backing_hd = bdrv_new("");
> + bs->backing_hd = bdrv_new("");
>
> - if (bs->backing_format[0] != '\0') {
> - back_drv = bdrv_find_format(bs->backing_format);
> - }
> + if (bs->backing_format[0] != '\0') {
> + back_drv = bdrv_find_format(bs->backing_format);
> + }
>
> - /* backing files always opened read-only */
> - back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> - BDRV_O_COPY_ON_READ);
> + /* backing files always opened read-only */
> + back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> + BDRV_O_COPY_ON_READ);
>
> - ret = bdrv_open(bs->backing_hd,
> - *backing_filename ? backing_filename : NULL, options,
> - back_flags, back_drv, &local_err);
> - if (ret < 0) {
> - bdrv_unref(bs->backing_hd);
> - bs->backing_hd = NULL;
> - bs->open_flags |= BDRV_O_NO_BACKING;
> - error_setg(errp, "Could not open backing file: %s",
> - error_get_pretty(local_err));
> - error_free(local_err);
> - return ret;
> + ret = bdrv_open(bs->backing_hd,
> + *backing_filename ? backing_filename : NULL, options,
> + back_flags, back_drv, &local_err);
> + if (ret < 0) {
> + bdrv_unref(bs->backing_hd);
> + bs->backing_hd = NULL;
> + bs->open_flags |= BDRV_O_NO_BACKING;
> + error_setg(errp, "Could not open backing file: %s",
> + error_get_pretty(local_err));
> + error_free(local_err);
> + return ret;
> + }
> }
> + assert(bs->backing_hd);
> + assert(!bs->backing_blocker);
> + error_setg(&bs->backing_blocker,
> + "device is used as backing hd of '%s'",
> + bs->device_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);
> pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> bs->backing_hd->file->filename);
> + pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> + bs->backing_hd->drv->format_name);
> return 0;
> }
>
This patch hunk is hard to review, because you had to indent the old
code. Same with whitespace differences ignored:
@@ -961,18 +961,30 @@
/*
* Opens the backing file for a BlockDriverState if not yet open
*
+ * If backing_bs is not NULL or empty, find the BDS by name and reference it as
+ * the backing_hd, in this case options is ignored.
+ *
* options is a QDict of options to pass to the block drivers, or NULL for an
* empty set of options. The reference to the QDict is transferred to this
* function (even on failure), so if the caller intends to reuse the dictionary,
* it needs to use QINCREF() before calling bdrv_file_open.
*/
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
+int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
+ QDict *options, Error **errp)
{
char backing_filename[PATH_MAX];
int back_flags, ret;
BlockDriver *back_drv = NULL;
Error *local_err = NULL;
+ if (backing_bs && *backing_bs != '\0') {
+ bs->backing_hd = bdrv_find(backing_bs);
+ if (!bs->backing_hd) {
+ error_setg(errp, "Backing device not found: %s", backing_bs);
+ return -ENOENT;
+ }
+ bdrv_ref(bs->backing_hd);
+ } else {
if (bs->backing_hd != NULL) {
QDECREF(options);
return 0;
@@ -1016,8 +1028,20 @@
error_free(local_err);
return ret;
}
+ }
+ assert(bs->backing_hd);
+ assert(!bs->backing_blocker);
+ error_setg(&bs->backing_blocker,
+ "device is used as backing hd of '%s'",
+ bs->device_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);
pstrcpy(bs->backing_file, sizeof(bs->backing_file),
bs->backing_hd->file->filename);
+ pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+ bs->backing_hd->drv->format_name);
return 0;
}
Looks like you do more than the stated subject in this patch: you also
add a blocker! Separate patch, please.
"Block all" followed by "unblock specific" is a bit awkward. A function
taking a bit set would let you do the masking on bits rather than an
array of lists. Suggestion, not a request, and it can be done in a
followup patch.
> @@ -1164,9 +1188,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> /* If there is a backing file, use it */
> if ((flags & BDRV_O_NO_BACKING) == 0) {
> QDict *backing_options;
> -
> qdict_extract_subqdict(options, &backing_options, "backing.");
> - ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> + ret = bdrv_open_backing_file(bs, qdict_get_try_str(options, "backing"),
> + backing_options, &local_err);
> +
> + qdict_del(options, "backing");
Why do you have to delete "backing"?
> if (ret < 0) {
> goto close_and_fail;
> }
> @@ -1459,9 +1485,14 @@ void bdrv_close(BlockDriverState *bs)
>
> if (bs->drv) {
> if (bs->backing_hd) {
> + assert(bs->backing_blocker);
> + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> bdrv_unref(bs->backing_hd);
> bs->backing_hd = NULL;
> }
> + if (bs->backing_blocker) {
> + error_free(bs->backing_blocker);
> + }
Can bs->backing_blocker legitimately be non-null when bs->backing_hd is
null?
> bs->drv->bdrv_close(bs);
> g_free(bs->opaque);
> #ifdef _WIN32
> diff --git a/block/mirror.c b/block/mirror.c
> index 6dc27ad..b1596d3 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -511,7 +511,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
> Error *local_err = NULL;
> int ret;
>
> - ret = bdrv_open_backing_file(s->target, NULL, &local_err);
> + ret = bdrv_open_backing_file(s->target, NULL, NULL, &local_err);
> if (ret < 0) {
> char backing_filename[PATH_MAX];
> bdrv_get_full_backing_filename(s->target, backing_filename,
> diff --git a/include/block/block.h b/include/block/block.h
> index 18397e0..24d06f9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -185,7 +185,8 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
> int bdrv_parse_discard_flags(const char *mode, int *flags);
> int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> QDict *options, int flags, Error **errp);
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
> + QDict *options, Error **errp);
> int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> int flags, BlockDriver *drv, Error **errp);
> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 458acd6..83dcf7d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -342,6 +342,9 @@ struct BlockDriverState {
> BlockJob *job;
>
> QDict *options;
> +
> + /* For backing reference, block the operations of named backing device */
> + Error *backing_blocker;
> };
>
> int get_tmp_filename(char *filename, int size);
I find the comment confusing :)
What about:
/* The error object in use for blocking operations on backing_hd */
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (2 preceding siblings ...)
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 13:24 ` Kevin Wolf
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 05/10] stream: Use bdrv_drop_intermediate and drop close_unused_images Fam Zheng
` (6 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, 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 need
some improvements in preparation for op blockers.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block.c | 152 +++++++++++++++++++++++++++------------------------------
block/commit.c | 1 +
2 files changed, 74 insertions(+), 79 deletions(-)
diff --git a/block.c b/block.c
index 41562fd..681d3be 100644
--- a/block.c
+++ b/block.c
@@ -2163,114 +2163,108 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
return overlay;
}
-typedef struct BlkIntermediateStates {
- BlockDriverState *bs;
- QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
-} BlkIntermediateStates;
-
+static void bdrv_set_backing_hd(BlockDriverState *bs,
+ BlockDriverState *new_backing)
+{
+ if (bs->backing_hd) {
+ bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
+ }
+ bs->backing_hd = new_backing;
+ if (new_backing) {
+ bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
+ }
+}
/*
- * Drops images above 'base' up to and including 'top', and sets the image
- * above 'top' to have base as its backing file.
+ * Drops images above 'base' up to and including 'top', and sets new 'base'
+ * as backing_hd of top_overlay (the image orignally has 'top' as backing
+ * file). top_overlay may be NULL if 'top' is active, no such update needed.
+ * Requires that the top_overlay to 'top' is opened r/w.
*
- * Requires that the overlay to 'top' is opened r/w, so that the backing file
- * information in 'bs' can be properly updated.
+ * 1) This will convert the following chain:
+ * ... <- base <- ... <- top <- overlay <-... <- active
*
- * E.g., this will convert the following chain:
- * bottom <- base <- intermediate <- top <- active
+ * to
+ *
+ * ... <- base <- overlay <- active
+ *
+ * 2) It is allowed for bottom==base, in which case it converts:
+ *
+ * ... <- base <- ... <- top <- overlay <- ... <- active
*
* to
*
- * bottom <- base <- active
+ * base <- overlay <- active
*
- * It is allowed for bottom==base, in which case it converts:
+ * 2) It also allows active==top, in which case it converts:
*
- * base <- intermediate <- top <- active
+ * ... <- base <- ... <- top (active)
*
* to
*
- * base <- active
+ * base == active == top, i.e. only base and lower remains: *top == *base when
+ * return.
+ *
+ * 3) If base==NULL, it will drop all the BDS below overlay and set its
+ * backing_hd to NULL. I.e.:
*
- * 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);
-
- if (!top->drv || !base->drv) {
- goto exit;
- }
+ BlockDriverState *drop_start, *overlay;
+ int ret = -EINVAL;
- 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;
- }
-
- intermediate = top;
-
- /* now we will go down through the list, and add each BDS we find
- * into our deletion queue, until we hit the 'base'
- */
- while (intermediate) {
- intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
- intermediate_state->bs = intermediate;
- QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
-
- if (intermediate->backing_hd == base) {
- base_bs = intermediate->backing_hd;
- break;
+ } else if (top == active) {
+ assert(base);
+ drop_start = active->backing_hd;
+ bdrv_swap(active, base);
+ base->backing_hd = NULL;
+ bdrv_unref(drop_start);
+ ret = 0;
+ } else {
+ /* If there's an overlay, its backing_hd points to top's BDS now,
+ * the top image is dropped but this BDS structure is kept and swapped
+ * with base, this way we keep the pointers valid after dropping top */
+ overlay = bdrv_find_overlay(active, top);
+ if (!overlay) {
+ goto exit;
+ }
+ if (base) {
+ ret = bdrv_change_backing_file(overlay, base->filename,
+ base->drv->format_name);
+ } else {
+ ret = bdrv_change_backing_file(overlay, "", "");
+ }
+ if (ret) {
+ goto exit;
+ }
+ if (base) {
+ drop_start = top->backing_hd;
+ bdrv_swap(top, base);
+ /* Break the loop formed by bdrv_swap */
+ bdrv_set_backing_hd(base, NULL);
+ } else {
+ bdrv_set_backing_hd(overlay, NULL);
+ drop_start = top;
}
- intermediate = intermediate->backing_hd;
- }
- if (base_bs == NULL) {
- /* something went wrong, we did not end at the base. safely
- * unravel everything, and exit with error */
- goto exit;
- }
-
- /* success - we can delete the intermediate states, and link top->base */
- ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
- base_bs->drv ? base_bs->drv->format_name : "");
- if (ret) {
- goto exit;
- }
- new_top_bs->backing_hd = base_bs;
-
- QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
- /* so that bdrv_close() does not recursively close the chain */
- intermediate_state->bs->backing_hd = NULL;
- bdrv_unref(intermediate_state->bs);
+ bdrv_unref(drop_start);
}
- ret = 0;
-
exit:
- QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
- g_free(intermediate_state);
- }
return ret;
}
-
static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
size_t size)
{
diff --git a/block/commit.c b/block/commit.c
index d4090cb..4d8cd05 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -142,6 +142,7 @@ wait:
if (!block_job_is_cancelled(&s->common) && sector_num == end) {
/* success */
ret = bdrv_drop_intermediate(active, top, base);
+ base = top;
}
exit_free_buf:
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate Fam Zheng
@ 2013-12-12 13:24 ` Kevin Wolf
2013-12-13 3:30 ` Fam Zheng
0 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2013-12-12 13:24 UTC (permalink / raw)
To: Fam Zheng; +Cc: armbru, qemu-devel, rjones, imain, stefanha, pbonzini
Am 12.12.2013 um 09:23 hat Fam Zheng geschrieben:
> 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 need
> some improvements in preparation for op blockers.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block.c | 152 +++++++++++++++++++++++++++------------------------------
> block/commit.c | 1 +
> 2 files changed, 74 insertions(+), 79 deletions(-)
>
> diff --git a/block.c b/block.c
> index 41562fd..681d3be 100644
> --- a/block.c
> +++ b/block.c
> @@ -2163,114 +2163,108 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
> return overlay;
> }
>
> -typedef struct BlkIntermediateStates {
> - BlockDriverState *bs;
> - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
> -} BlkIntermediateStates;
> -
> +static void bdrv_set_backing_hd(BlockDriverState *bs,
> + BlockDriverState *new_backing)
> +{
> + if (bs->backing_hd) {
> + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> + }
> + bs->backing_hd = new_backing;
> + if (new_backing) {
> + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
What about unblocking commit, like you did in patch 3?
Should bdrv_open_backing_file() be using this function?
> + }
> +}
>
> /*
> - * Drops images above 'base' up to and including 'top', and sets the image
> - * above 'top' to have base as its backing file.
> + * Drops images above 'base' up to and including 'top', and sets new 'base'
> + * as backing_hd of top_overlay (the image orignally has 'top' as backing
> + * file). top_overlay may be NULL if 'top' is active, no such update needed.
> + * Requires that the top_overlay to 'top' is opened r/w.
> *
> - * Requires that the overlay to 'top' is opened r/w, so that the backing file
> - * information in 'bs' can be properly updated.
> + * 1) This will convert the following chain:
> + * ... <- base <- ... <- top <- overlay <-... <- active
> *
> - * E.g., this will convert the following chain:
> - * bottom <- base <- intermediate <- top <- active
> + * to
> + *
> + * ... <- base <- overlay <- active
> + *
> + * 2) It is allowed for bottom==base, in which case it converts:
> + *
> + * ... <- base <- ... <- top <- overlay <- ... <- active
> *
> * to
> *
> - * bottom <- base <- active
> + * base <- overlay <- active
> *
> - * It is allowed for bottom==base, in which case it converts:
> + * 2) It also allows active==top, in which case it converts:
> *
> - * base <- intermediate <- top <- active
> + * ... <- base <- ... <- top (active)
> *
> * to
> *
> - * base <- active
> + * base == active == top, i.e. only base and lower remains: *top == *base when
> + * return.
> + *
> + * 3) If base==NULL, it will drop all the BDS below overlay and set its
> + * backing_hd to NULL. I.e.:
> *
> - * 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);
> -
> - if (!top->drv || !base->drv) {
> - goto exit;
> - }
> + BlockDriverState *drop_start, *overlay;
> + int ret = -EINVAL;
>
> - 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;
> - }
> -
> - intermediate = top;
> -
> - /* now we will go down through the list, and add each BDS we find
> - * into our deletion queue, until we hit the 'base'
> - */
> - while (intermediate) {
> - intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
> - intermediate_state->bs = intermediate;
> - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
> -
> - if (intermediate->backing_hd == base) {
> - base_bs = intermediate->backing_hd;
> - break;
> + } else if (top == active) {
> + assert(base);
> + drop_start = active->backing_hd;
> + bdrv_swap(active, base);
> + base->backing_hd = NULL;
> + bdrv_unref(drop_start);
> + ret = 0;
> + } else {
> + /* If there's an overlay, its backing_hd points to top's BDS now,
> + * the top image is dropped but this BDS structure is kept and swapped
> + * with base, this way we keep the pointers valid after dropping top */
> + overlay = bdrv_find_overlay(active, top);
> + if (!overlay) {
> + goto exit;
> + }
> + if (base) {
> + ret = bdrv_change_backing_file(overlay, base->filename,
> + base->drv->format_name);
> + } else {
> + ret = bdrv_change_backing_file(overlay, "", "");
This should be NULL, NULL instead of empty strings.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate
2013-12-12 13:24 ` Kevin Wolf
@ 2013-12-13 3:30 ` Fam Zheng
0 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-13 3:30 UTC (permalink / raw)
To: Kevin Wolf; +Cc: armbru, qemu-devel, rjones, imain, stefanha, pbonzini
On 2013年12月12日 21:24, Kevin Wolf wrote:
> Am 12.12.2013 um 09:23 hat Fam Zheng geschrieben:
>> 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 need
>> some improvements in preparation for op blockers.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>> block.c | 152 +++++++++++++++++++++++++++------------------------------
>> block/commit.c | 1 +
>> 2 files changed, 74 insertions(+), 79 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 41562fd..681d3be 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2163,114 +2163,108 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>> return overlay;
>> }
>>
>> -typedef struct BlkIntermediateStates {
>> - BlockDriverState *bs;
>> - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
>> -} BlkIntermediateStates;
>> -
>> +static void bdrv_set_backing_hd(BlockDriverState *bs,
>> + BlockDriverState *new_backing)
>> +{
>> + if (bs->backing_hd) {
>> + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
>> + }
>> + bs->backing_hd = new_backing;
>> + if (new_backing) {
>> + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
>
> What about unblocking commit, like you did in patch 3?
>
> Should bdrv_open_backing_file() be using this function?
>
Yes, will do that.
>> + }
>> +}
>>
>> /*
>> - * Drops images above 'base' up to and including 'top', and sets the image
>> - * above 'top' to have base as its backing file.
>> + * Drops images above 'base' up to and including 'top', and sets new 'base'
>> + * as backing_hd of top_overlay (the image orignally has 'top' as backing
>> + * file). top_overlay may be NULL if 'top' is active, no such update needed.
>> + * Requires that the top_overlay to 'top' is opened r/w.
>> *
>> - * Requires that the overlay to 'top' is opened r/w, so that the backing file
>> - * information in 'bs' can be properly updated.
>> + * 1) This will convert the following chain:
>> + * ... <- base <- ... <- top <- overlay <-... <- active
>> *
>> - * E.g., this will convert the following chain:
>> - * bottom <- base <- intermediate <- top <- active
>> + * to
>> + *
>> + * ... <- base <- overlay <- active
>> + *
>> + * 2) It is allowed for bottom==base, in which case it converts:
>> + *
>> + * ... <- base <- ... <- top <- overlay <- ... <- active
>> *
>> * to
>> *
>> - * bottom <- base <- active
>> + * base <- overlay <- active
>> *
>> - * It is allowed for bottom==base, in which case it converts:
>> + * 2) It also allows active==top, in which case it converts:
>> *
>> - * base <- intermediate <- top <- active
>> + * ... <- base <- ... <- top (active)
>> *
>> * to
>> *
>> - * base <- active
>> + * base == active == top, i.e. only base and lower remains: *top == *base when
>> + * return.
>> + *
>> + * 3) If base==NULL, it will drop all the BDS below overlay and set its
>> + * backing_hd to NULL. I.e.:
>> *
>> - * 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);
>> -
>> - if (!top->drv || !base->drv) {
>> - goto exit;
>> - }
>> + BlockDriverState *drop_start, *overlay;
>> + int ret = -EINVAL;
>>
>> - 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;
>> - }
>> -
>> - intermediate = top;
>> -
>> - /* now we will go down through the list, and add each BDS we find
>> - * into our deletion queue, until we hit the 'base'
>> - */
>> - while (intermediate) {
>> - intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
>> - intermediate_state->bs = intermediate;
>> - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
>> -
>> - if (intermediate->backing_hd == base) {
>> - base_bs = intermediate->backing_hd;
>> - break;
>> + } else if (top == active) {
>> + assert(base);
>> + drop_start = active->backing_hd;
>> + bdrv_swap(active, base);
>> + base->backing_hd = NULL;
>> + bdrv_unref(drop_start);
>> + ret = 0;
>> + } else {
>> + /* If there's an overlay, its backing_hd points to top's BDS now,
>> + * the top image is dropped but this BDS structure is kept and swapped
>> + * with base, this way we keep the pointers valid after dropping top */
>> + overlay = bdrv_find_overlay(active, top);
>> + if (!overlay) {
>> + goto exit;
>> + }
>> + if (base) {
>> + ret = bdrv_change_backing_file(overlay, base->filename,
>> + base->drv->format_name);
>> + } else {
>> + ret = bdrv_change_backing_file(overlay, "", "");
>
> This should be NULL, NULL instead of empty strings.
>
OK, thanks.
Fam
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v7 05/10] stream: Use bdrv_drop_intermediate and drop close_unused_images
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (3 preceding siblings ...)
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 06/10] block: Replace in_use with operation blocker Fam Zheng
` (5 subsequent siblings)
10 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rjones, armbru, imain, stefanha, pbonzini
This reuses the new bdrv_drop_intermediate.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/stream.c | 28 +---------------------------
1 file changed, 1 insertion(+), 27 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 46bec7d..9cdcf0e 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -51,32 +51,6 @@ static int coroutine_fn stream_populate(BlockDriverState *bs,
return bdrv_co_copy_on_readv(bs, sector_num, nb_sectors, &qiov);
}
-static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
- const char *base_id)
-{
- BlockDriverState *intermediate;
- intermediate = top->backing_hd;
-
- /* Must assign before bdrv_delete() to prevent traversing dangling pointer
- * while we delete backing image instances.
- */
- top->backing_hd = base;
-
- while (intermediate) {
- BlockDriverState *unused;
-
- /* reached base */
- if (intermediate == base) {
- break;
- }
-
- unused = intermediate;
- intermediate = intermediate->backing_hd;
- unused->backing_hd = NULL;
- bdrv_unref(unused);
- }
-}
-
static void coroutine_fn stream_run(void *opaque)
{
StreamBlockJob *s = opaque;
@@ -190,7 +164,7 @@ wait:
}
}
ret = bdrv_change_backing_file(bs, base_id, base_fmt);
- close_unused_images(bs, base, base_id);
+ bdrv_drop_intermediate(bs, bs->backing_hd, base);
}
qemu_vfree(buf);
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [Qemu-devel] [PATCH v7 06/10] block: Replace in_use with operation blocker
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (4 preceding siblings ...)
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 05/10] stream: Use bdrv_drop_intermediate and drop close_unused_images Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 13:46 ` Markus Armbruster
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 07/10] block: Pass error in bdrv_snapshot_create Fam Zheng
` (4 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, 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, ...).
- Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).
Note: there is no block for only specific types for now, i.e. a caller
blocks all or none. So although the checks are type specific, the above
changes can still be seen as identical in logic.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block-migration.c | 7 +++++--
block.c | 32 +++++++++++++++++++-------------
blockdev.c | 29 +++++++++++++++++++----------
blockjob.c | 12 +++++++-----
hw/block/dataplane/virtio-blk.c | 16 ++++++++++------
include/block/block.h | 2 --
include/block/block_int.h | 1 -
include/block/blockjob.h | 3 +++
8 files changed, 63 insertions(+), 39 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index 897fdba..bf9a25f 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -59,6 +59,7 @@ typedef struct BlkMigDevState {
unsigned long *aio_bitmap;
int64_t completed_sectors;
BdrvDirtyBitmap *dirty_bitmap;
+ Error *blocker;
} BlkMigDevState;
typedef struct BlkMigBlock {
@@ -346,7 +347,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
bmds->completed_sectors = 0;
bmds->shared_base = block_mig_state.shared_base;
alloc_aio_bitmap(bmds);
- bdrv_set_in_use(bs, 1);
+ error_setg(&bmds->blocker, "block device is in use by migration");
+ bdrv_op_block_all(bs, bmds->blocker);
bdrv_ref(bs);
block_mig_state.total_sector_sum += sectors;
@@ -584,7 +586,8 @@ static void blk_mig_cleanup(void)
blk_mig_lock();
while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
- bdrv_set_in_use(bmds->bs, 0);
+ bdrv_op_unblock_all(bmds->bs, bmds->blocker);
+ error_free(bmds->blocker);
bdrv_unref(bmds->bs);
g_free(bmds->aio_bitmap);
g_free(bmds);
diff --git a/block.c b/block.c
index 681d3be..f59f398 100644
--- a/block.c
+++ b/block.c
@@ -1652,15 +1652,17 @@ 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 */
pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
bs_src->device_name);
+ memcpy(bs_dest->op_blockers, bs_src->op_blockers,
+ sizeof(bs_dest->op_blockers));
bs_dest->list = bs_src->list;
}
+static bool bdrv_op_blocker_is_empty(BlockDriverState *bs);
/*
* Swap bs contents for two image chains while they are live,
* while keeping required fields on the BlockDriverState that is
@@ -1682,7 +1684,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(bs_new->in_use == 0);
assert(bs_new->io_limits_enabled == false);
assert(!throttle_have_timer(&bs_new->throttle_state));
@@ -1701,7 +1702,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(bs_new->in_use == 0);
assert(bs_new->io_limits_enabled == false);
assert(!throttle_have_timer(&bs_new->throttle_state));
@@ -1738,7 +1738,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));
@@ -1912,6 +1912,7 @@ int bdrv_commit(BlockDriverState *bs)
int ret = 0;
uint8_t *buf;
char filename[PATH_MAX];
+ Error *local_err;
if (!drv)
return -ENOMEDIUM;
@@ -1920,7 +1921,9 @@ 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, &local_err) ||
+ bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, &local_err)) {
+ error_free(local_err);
return -EBUSY;
}
@@ -2935,6 +2938,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
int bdrv_truncate(BlockDriverState *bs, int64_t offset)
{
BlockDriver *drv = bs->drv;
+ Error *local_err;
int ret;
if (!drv)
return -ENOMEDIUM;
@@ -2942,8 +2946,10 @@ 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, &local_err)) {
+ error_free(local_err);
return -EBUSY;
+ }
ret = drv->bdrv_truncate(bs, offset);
if (ret == 0) {
ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
@@ -4711,15 +4717,15 @@ void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
}
}
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
+static bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
{
- assert(bs->in_use != in_use);
- bs->in_use = in_use;
-}
+ bool ret = true;
+ int i;
-int bdrv_in_use(BlockDriverState *bs)
-{
- return bs->in_use;
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ ret = ret && QLIST_EMPTY(&bs->op_blockers[i]);
+ }
+ return ret;
}
void bdrv_iostatus_enable(BlockDriverState *bs)
diff --git a/blockdev.c b/blockdev.c
index 44755e1..369d8da 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1224,8 +1224,8 @@ static void external_snapshot_prepare(BlkTransactionState *common,
return;
}
- if (bdrv_in_use(state->old_bs)) {
- error_set(errp, QERR_DEVICE_IN_USE, device);
+ if (bdrv_op_is_blocked(state->old_bs,
+ BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
return;
}
@@ -1441,10 +1441,10 @@ 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)) {
error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
return;
@@ -1637,14 +1637,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;
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;
}
@@ -1755,6 +1757,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) {
@@ -1795,6 +1801,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;
@@ -1881,8 +1891,7 @@ void qmp_drive_backup(const char *device, const char *target,
}
}
- if (bdrv_in_use(bs)) {
- error_set(errp, QERR_DEVICE_IN_USE, device);
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
return;
}
@@ -2011,11 +2020,11 @@ 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;
}
+
flags = bs->open_flags | BDRV_O_RDWR;
source = bs->backing_hd;
if (!source && sync == MIRROR_SYNC_MODE_TOP) {
diff --git a/blockjob.c b/blockjob.c
index 9e5fd5c..e198cb2 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -41,13 +41,11 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
{
BlockJob *job;
- if (bs->job || bdrv_in_use(bs)) {
+ if (bs->job) {
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);
job->driver = driver;
job->bs = bs;
@@ -64,11 +62,14 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
if (error_is_set(&local_err)) {
bs->job = NULL;
g_free(job);
- bdrv_set_in_use(bs, 0);
error_propagate(errp, local_err);
return NULL;
}
}
+ 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);
+
return job;
}
@@ -79,8 +80,9 @@ void block_job_completed(BlockJob *job, int ret)
assert(bs->job == job);
job->cb(job->opaque, ret);
bs->job = NULL;
+ bdrv_op_unblock_all(bs, job->blocker);
+ error_free(job->blocker);
g_free(job);
- bdrv_set_in_use(bs, 0);
}
void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index f2d7350..0a7b759 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -69,6 +69,9 @@ struct VirtIOBlockDataPlane {
queue */
unsigned int num_reqs;
+
+ /* Operation blocker on BDS */
+ Error *blocker;
};
/* Raise an interrupt to signal guest, if necessary */
@@ -385,6 +388,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
{
VirtIOBlockDataPlane *s;
int fd;
+ Error *local_err = NULL;
*dataplane = NULL;
@@ -406,8 +410,9 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
/* If dataplane is (re-)enabled while the guest is running there could be
* block jobs that can conflict.
*/
- if (bdrv_in_use(blk->conf.bs)) {
- error_report("cannot start dataplane thread while device is in use");
+ if (bdrv_op_is_blocked(blk->conf.bs, BLOCK_OP_TYPE_DATAPLANE, &local_err)) {
+ error_report("%s", error_get_pretty(local_err));
+ error_free(local_err);
return false;
}
@@ -422,9 +427,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
s->vdev = vdev;
s->fd = fd;
s->blk = blk;
-
- /* Prevent block operations that conflict with data plane thread */
- bdrv_set_in_use(blk->conf.bs, 1);
+ error_setg(&s->blocker, "block device is in use by data plane");
+ bdrv_op_block_all(blk->conf.bs, s->blocker);
*dataplane = s;
return true;
@@ -437,7 +441,7 @@ 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);
g_free(s);
}
diff --git a/include/block/block.h b/include/block/block.h
index 24d06f9..0f380c4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -440,8 +440,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 83dcf7d..df3dc13 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -330,7 +330,6 @@ struct BlockDriverState {
char device_name[32];
QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
int refcnt;
- int in_use; /* users other than guest access, eg. block migration */
QTAILQ_ENTRY(BlockDriverState) list;
QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index d76de62..c0a7875 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -106,6 +106,9 @@ struct BlockJob {
/** The completion function that will be called when the job completes. */
BlockDriverCompletionFunc *cb;
+ /** Block other operations when block job is running */
+ Error *blocker;
+
/** The opaque value that is passed to the completion function. */
void *opaque;
};
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 06/10] block: Replace in_use with operation blocker
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 06/10] block: Replace in_use with operation blocker Fam Zheng
@ 2013-12-12 13:46 ` Markus Armbruster
2013-12-13 3:04 ` Fam Zheng
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2013-12-12 13:46 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
Fam Zheng <famz@redhat.com> writes:
> This drops BlockDriverState.in_use with op_blockers:
>
> - Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1).
> - Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0).
> - Check bdrv_op_is_blocked() in place of bdrv_in_use(bs).
> The specific types are used, e.g. in place of starting block backup,
> bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...).
> - Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).
>
> Note: there is no block for only specific types for now, i.e. a caller
> blocks all or none. So although the checks are type specific, the above
> changes can still be seen as identical in logic.
PATCH 3/10 adds a new blocker that doesn't block all or none. It's not
related to existing in_use, though, and therefore doesn't contradict
your "no functional change" claim.
I've always hated in_use. Its meaning is ill-defined, and its use is
confusing. Three cheers for getting rid of it!
> Signed-off-by: Fam Zheng <famz@redhat.com>
>
> ---
> block-migration.c | 7 +++++--
> block.c | 32 +++++++++++++++++++-------------
> blockdev.c | 29 +++++++++++++++++++----------
> blockjob.c | 12 +++++++-----
> hw/block/dataplane/virtio-blk.c | 16 ++++++++++------
> include/block/block.h | 2 --
> include/block/block_int.h | 1 -
> include/block/blockjob.h | 3 +++
> 8 files changed, 63 insertions(+), 39 deletions(-)
>
> diff --git a/block-migration.c b/block-migration.c
> index 897fdba..bf9a25f 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -59,6 +59,7 @@ typedef struct BlkMigDevState {
> unsigned long *aio_bitmap;
> int64_t completed_sectors;
> BdrvDirtyBitmap *dirty_bitmap;
> + Error *blocker;
> } BlkMigDevState;
>
> typedef struct BlkMigBlock {
> @@ -346,7 +347,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
> bmds->completed_sectors = 0;
> bmds->shared_base = block_mig_state.shared_base;
> alloc_aio_bitmap(bmds);
> - bdrv_set_in_use(bs, 1);
> + error_setg(&bmds->blocker, "block device is in use by migration");
> + bdrv_op_block_all(bs, bmds->blocker);
> bdrv_ref(bs);
>
> block_mig_state.total_sector_sum += sectors;
> @@ -584,7 +586,8 @@ static void blk_mig_cleanup(void)
> blk_mig_lock();
> while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
> QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
> - bdrv_set_in_use(bmds->bs, 0);
> + bdrv_op_unblock_all(bmds->bs, bmds->blocker);
> + error_free(bmds->blocker);
> bdrv_unref(bmds->bs);
> g_free(bmds->aio_bitmap);
> g_free(bmds);
> diff --git a/block.c b/block.c
> index 681d3be..f59f398 100644
> --- a/block.c
> +++ b/block.c
> @@ -1652,15 +1652,17 @@ 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 */
> pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
> bs_src->device_name);
> + memcpy(bs_dest->op_blockers, bs_src->op_blockers,
> + sizeof(bs_dest->op_blockers));
> bs_dest->list = bs_src->list;
> }
>
Doesn't the new memcpy() belong to PATCH 02/10?
> +static bool bdrv_op_blocker_is_empty(BlockDriverState *bs);
> /*
> * Swap bs contents for two image chains while they are live,
> * while keeping required fields on the BlockDriverState that is
> @@ -1682,7 +1684,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(bs_new->in_use == 0);
> assert(bs_new->io_limits_enabled == false);
> assert(!throttle_have_timer(&bs_new->throttle_state));
>
Why can you drop the !in_use assertion rather than replacing it by a
bdrv_op_blocker_is_empty() assertion like you do elsewhere?
> @@ -1701,7 +1702,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(bs_new->in_use == 0);
> assert(bs_new->io_limits_enabled == false);
> assert(!throttle_have_timer(&bs_new->throttle_state));
>
Same question.
> @@ -1738,7 +1738,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));
>
> @@ -1912,6 +1912,7 @@ int bdrv_commit(BlockDriverState *bs)
> int ret = 0;
> uint8_t *buf;
> char filename[PATH_MAX];
> + Error *local_err;
Recommend to always initialize local_err = NULL on general principles.
Your code doesn't strictly require it, because you use it only after
bdrv_op_is_blocked() set it. Initializing it is more obviously correct,
and more robust against change.
But: since your code doesn't do anything with local_err, simply drop it,
and pass NULL to brdv_op_is_blocked().
>
> if (!drv)
> return -ENOMEDIUM;
> @@ -1920,7 +1921,9 @@ 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, &local_err) ||
> + bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, &local_err)) {
> + error_free(local_err);
> return -EBUSY;
> }
>
> @@ -2935,6 +2938,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
> int bdrv_truncate(BlockDriverState *bs, int64_t offset)
> {
> BlockDriver *drv = bs->drv;
> + Error *local_err;
> int ret;
> if (!drv)
> return -ENOMEDIUM;
> @@ -2942,8 +2946,10 @@ 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, &local_err)) {
> + error_free(local_err);
> return -EBUSY;
> + }
> ret = drv->bdrv_truncate(bs, offset);
> if (ret == 0) {
> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
Likewise.
> @@ -4711,15 +4717,15 @@ void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
> }
> }
>
> -void bdrv_set_in_use(BlockDriverState *bs, int in_use)
> +static bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
I'd call it bdrv_op_is_any_blocked(), for symmetry with
bdrv_op_is_any_blocked(). Your choice.
> {
> - assert(bs->in_use != in_use);
> - bs->in_use = in_use;
> -}
> + bool ret = true;
> + int i;
>
> -int bdrv_in_use(BlockDriverState *bs)
> -{
> - return bs->in_use;
> + for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
> + ret = ret && QLIST_EMPTY(&bs->op_blockers[i]);
> + }
> + return ret;
I find this code simpler:
int i;
for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
if (!QLIST_EMPTY(&bs->op_blockers[i])) {
return false;
}
}
return true;
Your choice.
> }
>
> void bdrv_iostatus_enable(BlockDriverState *bs)
> diff --git a/blockdev.c b/blockdev.c
> index 44755e1..369d8da 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1224,8 +1224,8 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> return;
> }
>
> - if (bdrv_in_use(state->old_bs)) {
> - error_set(errp, QERR_DEVICE_IN_USE, device);
> + if (bdrv_op_is_blocked(state->old_bs,
> + BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
> return;
> }
>
This improves the error reported back to the caller, contradicting the
commit message's claim "no functional change". I don't mind, I just
want it noted in the commit message.
> @@ -1441,10 +1441,10 @@ 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)) {
> error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
> return;
Likewise.
> @@ -1637,14 +1637,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;
>
> 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);
Likewise.
Caveat, emptor: even I am unsure about which of the many error reporting
interfaces is to be used in which context, but here goes anyway: This
isn't wrong, because do_drive_del() isn't reachable from QMP. Still,
qerror_report_err() would be more robust against change.
Maybe Luiz can give more definite advice.
> return -1;
> }
>
> @@ -1755,6 +1757,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) {
Oh, here you *add* a blocker check! Why shouldn't this be a separate
patch?
> @@ -1795,6 +1801,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;
>
Likewise.
> @@ -1881,8 +1891,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;
> }
>
Another error message improvement.
> @@ -2011,11 +2020,11 @@ 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)) {
Likewise.
> return;
> }
>
> +
Spurious whitespace change, please drop.
> flags = bs->open_flags | BDRV_O_RDWR;
> source = bs->backing_hd;
> if (!source && sync == MIRROR_SYNC_MODE_TOP) {
> diff --git a/blockjob.c b/blockjob.c
> index 9e5fd5c..e198cb2 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -41,13 +41,11 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> {
> BlockJob *job;
>
> - if (bs->job || bdrv_in_use(bs)) {
> + if (bs->job) {
> error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
> return NULL;
> }
Why can you drop bdrv_in_use() without replacement here?
> bdrv_ref(bs);
> - bdrv_set_in_use(bs, 1);
> -
> job = g_malloc0(driver->instance_size);
> job->driver = driver;
> job->bs = bs;
> @@ -64,11 +62,14 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> if (error_is_set(&local_err)) {
> bs->job = NULL;
> g_free(job);
> - bdrv_set_in_use(bs, 0);
> error_propagate(errp, local_err);
> return NULL;
> }
> }
> + 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);
> +
> return job;
> }
>
The replacement for bdrv_set_in_use() is in a different place. Makes
sense to me, but it should be a separate patch, to keep this one as
mechanical as possible.
> @@ -79,8 +80,9 @@ void block_job_completed(BlockJob *job, int ret)
> assert(bs->job == job);
> job->cb(job->opaque, ret);
> bs->job = NULL;
> + bdrv_op_unblock_all(bs, job->blocker);
> + error_free(job->blocker);
> g_free(job);
> - bdrv_set_in_use(bs, 0);
> }
>
> void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index f2d7350..0a7b759 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -69,6 +69,9 @@ struct VirtIOBlockDataPlane {
> queue */
>
> unsigned int num_reqs;
> +
> + /* Operation blocker on BDS */
> + Error *blocker;
> };
>
> /* Raise an interrupt to signal guest, if necessary */
> @@ -385,6 +388,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
> {
> VirtIOBlockDataPlane *s;
> int fd;
> + Error *local_err = NULL;
>
> *dataplane = NULL;
>
> @@ -406,8 +410,9 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
> /* If dataplane is (re-)enabled while the guest is running there could be
> * block jobs that can conflict.
> */
> - if (bdrv_in_use(blk->conf.bs)) {
> - error_report("cannot start dataplane thread while device is in use");
> + if (bdrv_op_is_blocked(blk->conf.bs, BLOCK_OP_TYPE_DATAPLANE, &local_err)) {
> + error_report("%s", error_get_pretty(local_err));
> + error_free(local_err);
> return false;
> }
>
Error message change. The new one tells us why in more detail, but is
doesn't tell us what failed. Have you reproduced the error to make sure
the message still makes sense?
> @@ -422,9 +427,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
> s->vdev = vdev;
> s->fd = fd;
> s->blk = blk;
> -
> - /* Prevent block operations that conflict with data plane thread */
> - bdrv_set_in_use(blk->conf.bs, 1);
> + error_setg(&s->blocker, "block device is in use by data plane");
> + bdrv_op_block_all(blk->conf.bs, s->blocker);
>
> *dataplane = s;
> return true;
> @@ -437,7 +441,7 @@ 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);
Don't you want to error_free(s->blocker)?
> g_free(s);
> }
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 24d06f9..0f380c4 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -440,8 +440,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 83dcf7d..df3dc13 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -330,7 +330,6 @@ struct BlockDriverState {
> char device_name[32];
> QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
> int refcnt;
> - int in_use; /* users other than guest access, eg. block migration */
> QTAILQ_ENTRY(BlockDriverState) list;
>
> QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
Good riddance!
> 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;
> };
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 06/10] block: Replace in_use with operation blocker
2013-12-12 13:46 ` Markus Armbruster
@ 2013-12-13 3:04 ` Fam Zheng
0 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-13 3:04 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
On 2013年12月12日 21:46, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
>
>> This drops BlockDriverState.in_use with op_blockers:
>>
>> - Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1).
>> - Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0).
>> - Check bdrv_op_is_blocked() in place of bdrv_in_use(bs).
>> The specific types are used, e.g. in place of starting block backup,
>> bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...).
>> - Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).
>>
>> Note: there is no block for only specific types for now, i.e. a caller
>> blocks all or none. So although the checks are type specific, the above
>> changes can still be seen as identical in logic.
>
> PATCH 3/10 adds a new blocker that doesn't block all or none. It's not
> related to existing in_use, though, and therefore doesn't contradict
> your "no functional change" claim.
>
> I've always hated in_use. Its meaning is ill-defined, and its use is
> confusing. Three cheers for getting rid of it!
>
Markus,
Thank you very much for the thorough and nice review on this. The commit
message apparently went out dated, because I reordered the patch and did
numbers of rebase during my self reviewing and testing.
I meant to make it mechanism, and will fix toward that as you suggested.
So I'll put it back to the first part of series in the next revision, to
avoid preceding patches introducing blocker (like you pointed out).
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>
>> ---
>> block-migration.c | 7 +++++--
>> block.c | 32 +++++++++++++++++++-------------
>> blockdev.c | 29 +++++++++++++++++++----------
>> blockjob.c | 12 +++++++-----
>> hw/block/dataplane/virtio-blk.c | 16 ++++++++++------
>> include/block/block.h | 2 --
>> include/block/block_int.h | 1 -
>> include/block/blockjob.h | 3 +++
>> 8 files changed, 63 insertions(+), 39 deletions(-)
>>
>> diff --git a/block-migration.c b/block-migration.c
>> index 897fdba..bf9a25f 100644
>> --- a/block-migration.c
>> +++ b/block-migration.c
>> @@ -59,6 +59,7 @@ typedef struct BlkMigDevState {
>> unsigned long *aio_bitmap;
>> int64_t completed_sectors;
>> BdrvDirtyBitmap *dirty_bitmap;
>> + Error *blocker;
>> } BlkMigDevState;
>>
>> typedef struct BlkMigBlock {
>> @@ -346,7 +347,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
>> bmds->completed_sectors = 0;
>> bmds->shared_base = block_mig_state.shared_base;
>> alloc_aio_bitmap(bmds);
>> - bdrv_set_in_use(bs, 1);
>> + error_setg(&bmds->blocker, "block device is in use by migration");
>> + bdrv_op_block_all(bs, bmds->blocker);
>> bdrv_ref(bs);
>>
>> block_mig_state.total_sector_sum += sectors;
>> @@ -584,7 +586,8 @@ static void blk_mig_cleanup(void)
>> blk_mig_lock();
>> while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
>> QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
>> - bdrv_set_in_use(bmds->bs, 0);
>> + bdrv_op_unblock_all(bmds->bs, bmds->blocker);
>> + error_free(bmds->blocker);
>> bdrv_unref(bmds->bs);
>> g_free(bmds->aio_bitmap);
>> g_free(bmds);
>> diff --git a/block.c b/block.c
>> index 681d3be..f59f398 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1652,15 +1652,17 @@ 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 */
>> pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
>> bs_src->device_name);
>> + memcpy(bs_dest->op_blockers, bs_src->op_blockers,
>> + sizeof(bs_dest->op_blockers));
>> bs_dest->list = bs_src->list;
>> }
>>
>
> Doesn't the new memcpy() belong to PATCH 02/10?
>
Yes, thanks.
>> +static bool bdrv_op_blocker_is_empty(BlockDriverState *bs);
>> /*
>> * Swap bs contents for two image chains while they are live,
>> * while keeping required fields on the BlockDriverState that is
>> @@ -1682,7 +1684,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(bs_new->in_use == 0);
>> assert(bs_new->io_limits_enabled == false);
>> assert(!throttle_have_timer(&bs_new->throttle_state));
>>
>
> Why can you drop the !in_use assertion rather than replacing it by a
> bdrv_op_blocker_is_empty() assertion like you do elsewhere?
>
I shouldn't, will fix.
>> @@ -1701,7 +1702,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(bs_new->in_use == 0);
>> assert(bs_new->io_limits_enabled == false);
>> assert(!throttle_have_timer(&bs_new->throttle_state));
>>
>
> Same question.
Will fix too.
>
>> @@ -1738,7 +1738,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));
>>
>> @@ -1912,6 +1912,7 @@ int bdrv_commit(BlockDriverState *bs)
>> int ret = 0;
>> uint8_t *buf;
>> char filename[PATH_MAX];
>> + Error *local_err;
>
> Recommend to always initialize local_err = NULL on general principles.
>
> Your code doesn't strictly require it, because you use it only after
> bdrv_op_is_blocked() set it. Initializing it is more obviously correct,
> and more robust against change.
>
> But: since your code doesn't do anything with local_err, simply drop it,
> and pass NULL to brdv_op_is_blocked().
>
Good suggestion, thanks.
>>
>> if (!drv)
>> return -ENOMEDIUM;
>> @@ -1920,7 +1921,9 @@ 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, &local_err) ||
>> + bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, &local_err)) {
>> + error_free(local_err);
>> return -EBUSY;
>> }
>>
>> @@ -2935,6 +2938,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
>> int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>> {
>> BlockDriver *drv = bs->drv;
>> + Error *local_err;
>> int ret;
>> if (!drv)
>> return -ENOMEDIUM;
>> @@ -2942,8 +2946,10 @@ 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, &local_err)) {
>> + error_free(local_err);
>> return -EBUSY;
>> + }
>> ret = drv->bdrv_truncate(bs, offset);
>> if (ret == 0) {
>> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>
> Likewise.
>
Yes.
>> @@ -4711,15 +4717,15 @@ void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
>> }
>> }
>>
>> -void bdrv_set_in_use(BlockDriverState *bs, int in_use)
>> +static bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
>
> I'd call it bdrv_op_is_any_blocked(), for symmetry with
> bdrv_op_is_any_blocked(). Your choice.
>
I'd like to keep it as is for now. Renaming is easy while there are
non-trivial fixes needed for the series.
>> {
>> - assert(bs->in_use != in_use);
>> - bs->in_use = in_use;
>> -}
>> + bool ret = true;
>> + int i;
>>
>> -int bdrv_in_use(BlockDriverState *bs)
>> -{
>> - return bs->in_use;
>> + for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
>> + ret = ret && QLIST_EMPTY(&bs->op_blockers[i]);
>> + }
>> + return ret;
>
> I find this code simpler:
>
> int i;
>
> for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
> if (!QLIST_EMPTY(&bs->op_blockers[i])) {
> return false;
> }
> }
> return true;
>
Taken, thanks.
>
>> }
>>
>> void bdrv_iostatus_enable(BlockDriverState *bs)
>> diff --git a/blockdev.c b/blockdev.c
>> index 44755e1..369d8da 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1224,8 +1224,8 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>> return;
>> }
>>
>> - if (bdrv_in_use(state->old_bs)) {
>> - error_set(errp, QERR_DEVICE_IN_USE, device);
>> + if (bdrv_op_is_blocked(state->old_bs,
>> + BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
>> return;
>> }
>>
>
> This improves the error reported back to the caller, contradicting the
> commit message's claim "no functional change". I don't mind, I just
> want it noted in the commit message.
>
OK.
>> @@ -1441,10 +1441,10 @@ 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)) {
>> error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
>> return;
>
> Likewise.
>
>> @@ -1637,14 +1637,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;
>>
>> 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);
>
> Likewise.
>
> Caveat, emptor: even I am unsure about which of the many error reporting
> interfaces is to be used in which context, but here goes anyway: This
> isn't wrong, because do_drive_del() isn't reachable from QMP. Still,
> qerror_report_err() would be more robust against change.
>
> Maybe Luiz can give more definite advice.
>
I'll keep the original error message and drop local_err.
>> return -1;
>> }
>>
>> @@ -1755,6 +1757,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) {
>
> Oh, here you *add* a blocker check! Why shouldn't this be a separate
> patch?
>
>> @@ -1795,6 +1801,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;
>>
>
> Likewise.
>
These two are part of replacement for in_use check in
block_job_create(), where checking for bdrv_op_blocker_is_empty()
doesn't sound perfect, but works. Let's convert to that weird check
intermediately and improve through a separate patch.
>> @@ -1881,8 +1891,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;
>> }
>>
>
> Another error message improvement.
>
>> @@ -2011,11 +2020,11 @@ 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)) {
>
> Likewise.
>
>> return;
>> }
>>
>> +
>
> Spurious whitespace change, please drop.
>
OK.
>> flags = bs->open_flags | BDRV_O_RDWR;
>> source = bs->backing_hd;
>> if (!source && sync == MIRROR_SYNC_MODE_TOP) {
>> diff --git a/blockjob.c b/blockjob.c
>> index 9e5fd5c..e198cb2 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -41,13 +41,11 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>> {
>> BlockJob *job;
>>
>> - if (bs->job || bdrv_in_use(bs)) {
>> + if (bs->job) {
>> error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>> return NULL;
>> }
>
> Why can you drop bdrv_in_use() without replacement here?
>
As explained above.
>> bdrv_ref(bs);
>> - bdrv_set_in_use(bs, 1);
>> -
>> job = g_malloc0(driver->instance_size);
>> job->driver = driver;
>> job->bs = bs;
>> @@ -64,11 +62,14 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>> if (error_is_set(&local_err)) {
>> bs->job = NULL;
>> g_free(job);
>> - bdrv_set_in_use(bs, 0);
>> error_propagate(errp, local_err);
>> return NULL;
>> }
>> }
>> + 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);
>> +
>> return job;
>> }
>>
>
> The replacement for bdrv_set_in_use() is in a different place. Makes
> sense to me, but it should be a separate patch, to keep this one as
> mechanical as possible.
>
OK, makes sense.
>> @@ -79,8 +80,9 @@ void block_job_completed(BlockJob *job, int ret)
>> assert(bs->job == job);
>> job->cb(job->opaque, ret);
>> bs->job = NULL;
>> + bdrv_op_unblock_all(bs, job->blocker);
>> + error_free(job->blocker);
>> g_free(job);
>> - bdrv_set_in_use(bs, 0);
>> }
>>
>> void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
>> index f2d7350..0a7b759 100644
>> --- a/hw/block/dataplane/virtio-blk.c
>> +++ b/hw/block/dataplane/virtio-blk.c
>> @@ -69,6 +69,9 @@ struct VirtIOBlockDataPlane {
>> queue */
>>
>> unsigned int num_reqs;
>> +
>> + /* Operation blocker on BDS */
>> + Error *blocker;
>> };
>>
>> /* Raise an interrupt to signal guest, if necessary */
>> @@ -385,6 +388,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>> {
>> VirtIOBlockDataPlane *s;
>> int fd;
>> + Error *local_err = NULL;
>>
>> *dataplane = NULL;
>>
>> @@ -406,8 +410,9 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>> /* If dataplane is (re-)enabled while the guest is running there could be
>> * block jobs that can conflict.
>> */
>> - if (bdrv_in_use(blk->conf.bs)) {
>> - error_report("cannot start dataplane thread while device is in use");
>> + if (bdrv_op_is_blocked(blk->conf.bs, BLOCK_OP_TYPE_DATAPLANE, &local_err)) {
>> + error_report("%s", error_get_pretty(local_err));
>> + error_free(local_err);
>> return false;
>> }
>>
>
> Error message change. The new one tells us why in more detail, but is
> doesn't tell us what failed. Have you reproduced the error to make sure
> the message still makes sense?
>
No, I'll concatenate the original test with local_err.
>> @@ -422,9 +427,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>> s->vdev = vdev;
>> s->fd = fd;
>> s->blk = blk;
>> -
>> - /* Prevent block operations that conflict with data plane thread */
>> - bdrv_set_in_use(blk->conf.bs, 1);
>> + error_setg(&s->blocker, "block device is in use by data plane");
>> + bdrv_op_block_all(blk->conf.bs, s->blocker);
>>
>> *dataplane = s;
>> return true;
>> @@ -437,7 +441,7 @@ 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);
>
> Don't you want to error_free(s->blocker)?
>
Good catch. Thanks.
Fam
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v7 07/10] block: Pass error in bdrv_snapshot_create
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (5 preceding siblings ...)
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 06/10] block: Replace in_use with operation blocker Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 13:52 ` Markus Armbruster
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 08/10] block: Add checks of blocker in block operations Fam Zheng
` (3 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rjones, armbru, imain, stefanha, pbonzini
This allows descent error information to be reported.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/snapshot.c | 5 +++--
blockdev.c | 2 +-
include/block/snapshot.h | 3 ++-
qemu-img.c | 2 +-
savevm.c | 2 +-
5 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 9047f8d..02cfb07 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -157,7 +157,8 @@ int bdrv_can_snapshot(BlockDriverState *bs)
}
int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info)
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
if (!drv) {
@@ -167,7 +168,7 @@ int bdrv_snapshot_create(BlockDriverState *bs,
return drv->bdrv_snapshot_create(bs, sn_info);
}
if (bs->file) {
- return bdrv_snapshot_create(bs->file, sn_info);
+ return bdrv_snapshot_create(bs->file, sn_info, errp);
}
return -ENOTSUP;
}
diff --git a/blockdev.c b/blockdev.c
index 369d8da..404159e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1138,7 +1138,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
sn->date_nsec = tv.tv_usec * 1000;
sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
- ret1 = bdrv_snapshot_create(bs, sn);
+ ret1 = bdrv_snapshot_create(bs, sn, errp);
if (ret1 < 0) {
error_setg_errno(errp, -ret1,
"Failed to create snapshot '%s' on device '%s'",
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 770d9bb..5fe5834 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -56,7 +56,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
Error **errp);
int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info);
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
int bdrv_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id);
int bdrv_snapshot_delete(BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index 7dfe982..54b1d23 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2151,7 +2151,7 @@ static int img_snapshot(int argc, char **argv)
sn.date_sec = tv.tv_sec;
sn.date_nsec = tv.tv_usec * 1000;
- ret = bdrv_snapshot_create(bs, &sn);
+ ret = bdrv_snapshot_create(bs, &sn, NULL);
if (ret) {
error_report("Could not create snapshot '%s': %d (%s)",
snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index 3f912dd..75e5093 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2439,7 +2439,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
if (bdrv_can_snapshot(bs1)) {
/* Write VM state size only to the image that contains the state */
sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
- ret = bdrv_snapshot_create(bs1, sn);
+ ret = bdrv_snapshot_create(bs1, sn, NULL);
if (ret < 0) {
monitor_printf(mon, "Error while creating snapshot on '%s'\n",
bdrv_get_device_name(bs1));
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 07/10] block: Pass error in bdrv_snapshot_create
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 07/10] block: Pass error in bdrv_snapshot_create Fam Zheng
@ 2013-12-12 13:52 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2013-12-12 13:52 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
Fam Zheng <famz@redhat.com> writes:
> This allows descent error information to be reported.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/snapshot.c | 5 +++--
> blockdev.c | 2 +-
> include/block/snapshot.h | 3 ++-
> qemu-img.c | 2 +-
> savevm.c | 2 +-
> 5 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 9047f8d..02cfb07 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -157,7 +157,8 @@ int bdrv_can_snapshot(BlockDriverState *bs)
> }
>
> int bdrv_snapshot_create(BlockDriverState *bs,
> - QEMUSnapshotInfo *sn_info)
> + QEMUSnapshotInfo *sn_info,
> + Error **errp)
> {
> BlockDriver *drv = bs->drv;
> if (!drv) {
> @@ -167,7 +168,7 @@ int bdrv_snapshot_create(BlockDriverState *bs,
> return drv->bdrv_snapshot_create(bs, sn_info);
> }
> if (bs->file) {
> - return bdrv_snapshot_create(bs->file, sn_info);
> + return bdrv_snapshot_create(bs->file, sn_info, errp);
> }
> return -ENOTSUP;
> }
> diff --git a/blockdev.c b/blockdev.c
> index 369d8da..404159e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1138,7 +1138,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
> sn->date_nsec = tv.tv_usec * 1000;
> sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>
> - ret1 = bdrv_snapshot_create(bs, sn);
> + ret1 = bdrv_snapshot_create(bs, sn, errp);
> if (ret1 < 0) {
> error_setg_errno(errp, -ret1,
> "Failed to create snapshot '%s' on device '%s'",
I'm afraid this will fail the assertion in error_set_errno() as soon as
bdrv_snapshot_create() actually uses errp, and errp isn't null. Do you
defuse this time bomb later in your series?
[...]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v7 08/10] block: Add checks of blocker in block operations
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (6 preceding siblings ...)
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 07/10] block: Pass error in bdrv_snapshot_create Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 13:56 ` Markus Armbruster
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 09/10] qmp: add command 'blockdev-backup' Fam Zheng
` (2 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rjones, armbru, imain, stefanha, pbonzini
Before operate on a BlockDriverState, respective types are checked
against bs->op_blockers and it will error out if there's a blocker.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/snapshot.c | 11 +++++++++++
blockdev.c | 8 ++++++++
2 files changed, 19 insertions(+)
diff --git a/block/snapshot.c b/block/snapshot.c
index 02cfb07..0dc8f40 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -164,6 +164,11 @@ int bdrv_snapshot_create(BlockDriverState *bs,
if (!drv) {
return -ENOMEDIUM;
}
+
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
+ return -EPERM;
+ }
+
if (drv->bdrv_snapshot_create) {
return drv->bdrv_snapshot_create(bs, sn_info);
}
@@ -233,6 +238,12 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
return -ENOMEDIUM;
}
+
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
+ errp)) {
+ return -EPERM;
+ }
+
if (!snapshot_id && !name) {
error_setg(errp, "snapshot_id and name are both NULL");
return -EINVAL;
diff --git a/blockdev.c b/blockdev.c
index 404159e..5e5d1b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1536,6 +1536,10 @@ void qmp_change_blockdev(const char *device, const char *filename,
return;
}
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
+ return;
+ }
+
if (format) {
drv = bdrv_find_whitelisted_format(format, bs->read_only);
if (!drv) {
@@ -1684,6 +1688,10 @@ void qmp_block_resize(const char *device, int64_t size, Error **errp)
return;
}
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, errp)) {
+ return;
+ }
+
if (size < 0) {
error_set(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
return;
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 08/10] block: Add checks of blocker in block operations
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 08/10] block: Add checks of blocker in block operations Fam Zheng
@ 2013-12-12 13:56 ` Markus Armbruster
2013-12-13 3:29 ` Fam Zheng
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2013-12-12 13:56 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
Fam Zheng <famz@redhat.com> writes:
> Before operate on a BlockDriverState, respective types are checked
> against bs->op_blockers and it will error out if there's a blocker.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
So this patch adds protection against "two of the same kind
simultaneously". How could we check it's complete?
Have we pondered the more general problem of which "operations"
(whatever that is) exclude each other?
[...]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v7 08/10] block: Add checks of blocker in block operations
2013-12-12 13:56 ` Markus Armbruster
@ 2013-12-13 3:29 ` Fam Zheng
0 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-13 3:29 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, rjones, imain, stefanha, pbonzini
On 2013年12月12日 21:56, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
>
>> Before operate on a BlockDriverState, respective types are checked
>> against bs->op_blockers and it will error out if there's a blocker.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>
> So this patch adds protection against "two of the same kind
> simultaneously". How could we check it's complete?
>
> Have we pondered the more general problem of which "operations"
> (whatever that is) exclude each other?
>
Good point. For what we want now, I think these extra checks are not
required. I think these could be added in a separate series if any.
Planning to drop it for next revision but the discussion is still open.
Thanks,
Fam
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v7 09/10] qmp: add command 'blockdev-backup'
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (7 preceding siblings ...)
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 08/10] block: Add checks of blocker in block operations Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 10/10] block: Allow backup on referenced named BlockDriverState Fam Zheng
2013-12-13 2:40 ` [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Ian Main
10 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, 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.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/backup.c | 21 +++++++++++++++++++++
blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
qmp-commands.hx | 44 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 161 insertions(+)
diff --git a/block/backup.c b/block/backup.c
index 0198514..c8fe1a9 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -339,6 +339,7 @@ static void coroutine_fn backup_run(void *opaque)
hbitmap_free(job->bitmap);
bdrv_iostatus_disable(target);
+ bdrv_op_unblock_all(target, job->common.blocker);
bdrv_unref(target);
block_job_completed(&job->common, ret);
@@ -364,6 +365,24 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
return;
}
+ if (!bdrv_is_inserted(bs)) {
+ error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bs->device_name);
+ return;
+ }
+
+ if (!bdrv_is_inserted(target)) {
+ error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target->device_name);
+ return;
+ }
+
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
+ return;
+ }
+
+ if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
+ return;
+ }
+
len = bdrv_getlength(bs);
if (len < 0) {
error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -377,6 +396,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
return;
}
+ bdrv_op_block_all(target, job->common.blocker);
+
job->on_source_error = on_source_error;
job->on_target_error = on_target_error;
job->target = target;
diff --git a/blockdev.c b/blockdev.c
index 5e5d1b0..7c3d3da 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1883,6 +1883,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;
@@ -1899,6 +1901,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;
}
@@ -1957,6 +1960,50 @@ void qmp_drive_backup(const char *device, const char *target,
}
}
+void qmp_blockdev_backup(const char *device, const char *target,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BlockDriverState *target_bs;
+ Error *local_err = NULL;
+
+ if (!has_speed) {
+ speed = 0;
+ }
+ if (!has_on_source_error) {
+ on_source_error = BLOCKDEV_ON_ERROR_REPORT;
+ }
+ if (!has_on_target_error) {
+ on_target_error = BLOCKDEV_ON_ERROR_REPORT;
+ }
+
+ bs = bdrv_find(device);
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ return;
+ }
+
+ target_bs = bdrv_find(target);
+ if (!target_bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+ return;
+ }
+
+ bdrv_ref(target_bs);
+ backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+ block_job_cb, bs, &local_err);
+ if (local_err != NULL) {
+ bdrv_unref(target_bs);
+ error_propagate(errp, local_err);
+ }
+}
+
#define DEFAULT_MIRROR_BUF_SIZE (10 << 20)
void qmp_drive_mirror(const char *device, const char *target,
diff --git a/qapi-schema.json b/qapi-schema.json
index ef4d6af..0348431 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1868,6 +1868,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: 1.8
+##
+{ '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.
@@ -2057,6 +2091,21 @@
{ 'command': 'drive-backup', 'data': 'DriveBackup' }
##
+# @blockdev-backup
+#
+# Block device version for drive-backup command. Use existing device as target
+# of backup.
+#
+# For the arguments, see the documentation of BlockdevBackup.
+#
+# Returns: Nothing on success.
+# If @device or @target is not a valid block device, DeviceNotFound.
+#
+# Since 1.8
+##
+{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
+
+##
# @drive-mirror
#
# Start mirroring a block device's writes to a new destination.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fba15cd..32e2b10 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -963,6 +963,50 @@ Example:
"sync": "full",
"target": "backup.img" } }
<- { "return": {} }
+
+EQMP
+
+ {
+ .name = "blockdev-backup",
+ .args_type = "sync:s,device:B,target:B,speed:i?,"
+ "on-source-error:s?,on-target-error:s?",
+ .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
+ },
+
+SQMP
+blockdev-backup
+------------
+
+The device version of drive-backup: this command takes a existing named device
+as backup target.
+
+Arguments:
+
+- "device": the name of the device which should be copied.
+ (json-string)
+- "target": the target of the new image. If the file exists, or if it is a
+ device, the existing file/device will be used as the new
+ destination. If it does not exist, a new file will be created.
+ (json-string)
+- "sync": what parts of the disk image should be copied to the destination;
+ possibilities include "full" for all the disk, "top" for only the
+ sectors allocated in the topmost image, or "none" to only replicate
+ new I/O (MirrorSyncMode).
+- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "on-source-error": the action to take on an error on the source, default
+ 'report'. 'stop' and 'enospc' can only be used
+ if the block device supports io-status.
+ (BlockdevOnError, optional)
+- "on-target-error": the action to take on an error on the target, default
+ 'report' (no limitations, since this applies to
+ a different block device than device).
+ (BlockdevOnError, optional)
+
+Example:
+-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
+ "target": "tgt-id" } }
+<- { "return": {} }
+
EQMP
{
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [Qemu-devel] [PATCH v7 10/10] block: Allow backup on referenced named BlockDriverState
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (8 preceding siblings ...)
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 09/10] qmp: add command 'blockdev-backup' Fam Zheng
@ 2013-12-12 8:23 ` Fam Zheng
2013-12-13 2:40 ` [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Ian Main
10 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-12 8:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, 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 f59f398..2525486 100644
--- a/block.c
+++ b/block.c
@@ -1038,6 +1038,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
/* Otherwise we won't be able to commit due to check in bdrv_commit */
bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
bs->backing_blocker);
+ bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_BACKUP_SOURCE,
+ bs->backing_blocker);
pstrcpy(bs->backing_file, sizeof(bs->backing_file),
bs->backing_hd->file->filename);
pstrcpy(bs->backing_format, sizeof(bs->backing_format),
--
1.8.5.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD
2013-12-12 8:23 [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Fam Zheng
` (9 preceding siblings ...)
2013-12-12 8:23 ` [Qemu-devel] [PATCH v7 10/10] block: Allow backup on referenced named BlockDriverState Fam Zheng
@ 2013-12-13 2:40 ` Ian Main
2013-12-13 3:05 ` Fam Zheng
10 siblings, 1 reply; 26+ messages in thread
From: Ian Main @ 2013-12-13 2:40 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, armbru, qemu-devel, rjones, stefanha, pbonzini
On Thu, Dec 12, 2013 at 04:23:36PM +0800, Fam Zheng wrote:
> This series adds for point-in-time snapshot NBD exporting based on
> blockdev-backup (variant of drive-backup with existing device as target).
>
> We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
> export it through built in NBD server. The steps are as below:
>
> 1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>
>
> (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
> providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
> used r/w by guest. Whether or not setting backing file in the image file
> doesn't matter, as we are going to override the backing hd in the next
> step)
>
> 2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2
>
> (where source-drive is the running BlockDriverState name for
> RUNNING-VM.img. This patch implements "backing=" option to override
> backing_hd for added drive)
>
> 3. (QMP) blockdev-backup device=source-drive sync=none target=target0
>
> (this is the QMP command introduced by this series, which use a named
> device as target of drive-backup)
>
> 4. (QMP) nbd-server-add device=target0
>
> When image fleecing done:
>
> 1. (QMP) block-job-complete device=source-drive
If you do another revision, this should be block-job-cancel.
Ian
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD
2013-12-13 2:40 ` [Qemu-devel] [PATCH v7 00/10] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD Ian Main
@ 2013-12-13 3:05 ` Fam Zheng
0 siblings, 0 replies; 26+ messages in thread
From: Fam Zheng @ 2013-12-13 3:05 UTC (permalink / raw)
To: Ian Main; +Cc: kwolf, armbru, qemu-devel, rjones, stefanha, pbonzini
On 2013年12月13日 10:40, Ian Main wrote:
> On Thu, Dec 12, 2013 at 04:23:36PM +0800, Fam Zheng wrote:
>> This series adds for point-in-time snapshot NBD exporting based on
>> blockdev-backup (variant of drive-backup with existing device as target).
>>
>> We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
>> export it through built in NBD server. The steps are as below:
>>
>> 1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>
>>
>> (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
>> providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
>> used r/w by guest. Whether or not setting backing file in the image file
>> doesn't matter, as we are going to override the backing hd in the next
>> step)
>>
>> 2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2
>>
>> (where source-drive is the running BlockDriverState name for
>> RUNNING-VM.img. This patch implements "backing=" option to override
>> backing_hd for added drive)
>>
>> 3. (QMP) blockdev-backup device=source-drive sync=none target=target0
>>
>> (this is the QMP command introduced by this series, which use a named
>> device as target of drive-backup)
>>
>> 4. (QMP) nbd-server-add device=target0
>>
>> When image fleecing done:
>>
>> 1. (QMP) block-job-complete device=source-drive
>
> If you do another revision, this should be block-job-cancel.
>
OK, thanks for reminding.
Fam
^ permalink raw reply [flat|nested] 26+ messages in thread